Moodle
  1. Moodle
  2. MDL-34118

backup/restore_questions_activity_structure_step cannot cope with different types of usage.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: Backup, Questions
    • Labels:
      None
    • Testing Instructions:
      Hide

      1. You need a quiz with user data.

      2. Back it up and restore it. (Either the whole course, or just the one activity, or, if possible test both.)

      3. Verify that all the attempt data is correctly transferred. (Side-by-side compare of the quiz grades and responses reports in two browser windows is probably an effective way to do that.)

      That just tests that there are no regressions.

      There is no way to actually test that using a prefix works. The offlinequiz module that this was developed for has not been released yet because it is still being tested. Jurgen (offlinequiz developer) has been testing this, and confirms that it works for them.

      Show
      1. You need a quiz with user data. 2. Back it up and restore it. (Either the whole course, or just the one activity, or, if possible test both.) 3. Verify that all the attempt data is correctly transferred. (Side-by-side compare of the quiz grades and responses reports in two browser windows is probably an effective way to do that.) That just tests that there are no regressions. There is no way to actually test that using a prefix works. The offlinequiz module that this was developed for has not been released yet because it is still being tested. Jurgen (offlinequiz developer) has been testing this, and confirms that it works for them.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      42434

      Description

      Suppose an activity uses question_usages in two different ways. For example, there might be template usages that are part of the activity settings, and then actual usages that are part of the user data.

      This is a problem, because different XML elements in the backup file that contain other elements cannot have the same name. In other words, a structure like

      modname
          templates
              template
                  question_usage
                      ...
          attempts
              attempt
                  question_usage
                      ...
      

      in not allowed. Some of the elements would need to be renamed, like

      modname
          templates
              template
                  template_question_usage
                      ...
          attempts
              attempt
                  question_usage
                      ...
      

      Fortunately, I think it is possible to do this. We just need to add an optional $nameprefix argument to backup_questions_activity_structure_step::add_question_usages and restore_questions_activity_structure_step::add_question_usages.

      I am about to put a patch up for review.

        Activity

        Hide
        Tim Hunt added a comment -

        I think this is all it takes. I need to test it a bit.

        Jurgen, could you test to see if it solves your problem.

        Show
        Tim Hunt added a comment - I think this is all it takes. I need to test it a bit. Jurgen, could you test to see if it solves your problem.
        Hide
        Tim Hunt added a comment -

        Sorry, I just edited the wrong bug.

        Show
        Tim Hunt added a comment - Sorry, I just edited the wrong bug.
        Hide
        Tim Hunt added a comment -

        OK, there is a problem with my proposed fix. Line 437 of backup/util/plan/restore_structure_step.class.php insists that the names of all restore_path_elements are unique, but I am intentionally trying to create multiple restore_path_elements with the same name (even thought they have different $nameprefixes in the path).

        Do you have a suggestion for how to get round that Eloy?

        Show
        Tim Hunt added a comment - OK, there is a problem with my proposed fix. Line 437 of backup/util/plan/restore_structure_step.class.php insists that the names of all restore_path_elements are unique, but I am intentionally trying to create multiple restore_path_elements with the same name (even thought they have different $nameprefixes in the path). Do you have a suggestion for how to get round that Eloy?
        Hide
        Tim Hunt added a comment - - edited

        OK, the only way I can think of to make this work is to add the prefix to the names too, and require people making subclasses to create some new methods like

        function process_group_question_attempt($data) {
            $this->process_question_attempt($data);
        }
        

        to correctly handle the new names.

        I have done this as an extra commit. I am hoping Jurgen can test it before we submit for integration.

        Show
        Tim Hunt added a comment - - edited OK, the only way I can think of to make this work is to add the prefix to the names too, and require people making subclasses to create some new methods like function process_group_question_attempt($data) { $ this ->process_question_attempt($data); } to correctly handle the new names. I have done this as an extra commit. I am hoping Jurgen can test it before we submit for integration.
        Hide
        Tim Hunt added a comment -

        I have just incorporated some final changes from Jurgen and rebased onto the latest master.

        Jurgen reports that this is now working with his offline quiz module.

        I guess that we really ought to wait for peer review from Eloy before this is sent for integration, but I think that this is now ready.

        Show
        Tim Hunt added a comment - I have just incorporated some final changes from Jurgen and rebased onto the latest master. Jurgen reports that this is now working with his offline quiz module. I guess that we really ought to wait for peer review from Eloy before this is sent for integration, but I think that this is now ready.
        Hide
        Tim Hunt added a comment -

        Eloy, I would quite like to get this done. Are you going to have time to peer-review it, or should I just force the issue by submitting this for integration?

        Show
        Tim Hunt added a comment - Eloy, I would quite like to get this done. Are you going to have time to peer-review it, or should I just force the issue by submitting this for integration?
        Hide
        Tim Hunt added a comment -

        To INTEGRATORS,

        The question is, which branches to add this to. I think it is a master-only type of change.

        On the other hand, I think the University of Vienna, who are the ones who need this, are still on 2.2, so it would be good for them if it was back-ported. This change is completely backwards-compatible. Sill, UV can just back-port the commits if they need them.

        However, I cherry-picked to other branches, so you can make the decision.

        Show
        Tim Hunt added a comment - To INTEGRATORS, The question is, which branches to add this to. I think it is a master-only type of change. On the other hand, I think the University of Vienna, who are the ones who need this, are still on 2.2, so it would be good for them if it was back-ported. This change is completely backwards-compatible. Sill, UV can just back-port the commits if they need them. However, I cherry-picked to other branches, so you can make the decision.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I really would consider this one new feature/improvement, so I think it's correct to aim it to 2.4 only.

        This gets my +1 after discussing a bit @ HQ chat, and once the "_commons" refactor is done.

        Thanks and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I really would consider this one new feature/improvement, so I think it's correct to aim it to 2.4 only. This gets my +1 after discussing a bit @ HQ chat, and once the "_commons" refactor is done. Thanks and ciao
        Hide
        Tim Hunt added a comment -

        OK, 2.4-only it is. Since we have changed the API a bit that is probably better for University of Vienna. It means they only have to change their offlinequiz module when they upgrade.

        Show
        Tim Hunt added a comment - OK, 2.4-only it is. Since we have changed the API a bit that is probably better for University of Vienna. It means they only have to change their offlinequiz module when they upgrade.
        Hide
        Dan Poltawski added a comment -

        Integrated to 2.4 only. Thanks!

        Show
        Dan Poltawski added a comment - Integrated to 2.4 only. Thanks!
        Hide
        Jason Fowler added a comment -

        I can't see ANY attempts in the imported quiz, and the description and summary don't provide enough information for me to test this in any other manner

        Show
        Jason Fowler added a comment - I can't see ANY attempts in the imported quiz, and the description and summary don't provide enough information for me to test this in any other manner
        Hide
        Jason Fowler added a comment -

        after random experimenting I found a way to get the data to show - even if a teacher is enrolled in the course, they can't view their own attempts at a quiz ...

        Show
        Jason Fowler added a comment - after random experimenting I found a way to get the data to show - even if a teacher is enrolled in the course, they can't view their own attempts at a quiz ...
        Hide
        Tim Hunt added a comment -

        Jason, Teachers do not attempt the quiz, they can only preview it. Previews are not really attempts, they are not shown in the quiz reports, and they are likely to get deleted at any time (e.g. when you edit the quiz).

        So, to test something like this, you need to make some real attempts with a student log-in.

        Show
        Tim Hunt added a comment - Jason, Teachers do not attempt the quiz, they can only preview it. Previews are not really attempts, they are not shown in the quiz reports, and they are likely to get deleted at any time (e.g. when you edit the quiz). So, to test something like this, you need to make some real attempts with a student log-in.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        YEAR!*

        CAF*, TOT!*

        • Your effort amazingly resulted. (unbelievable :-P)
        • Closing as fixed.
        • Tons of thanks.
        Show
        Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: