Moodle
  1. Moodle
  2. MDL-31386

Restoring a matched question in a lesson leads to answers being processed in incorrect order

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Backup, Lesson
    • Labels:
    • Testing Instructions:
      Hide

      Preparation

      • Create a new course
      • Create a new lesson within that course
      • Choose Edit
      • Create a new 'Question Page'
      • Choose 'Matching' as the type
      • Set some answers for each field

      Backup

      • Create a new backup of the course

      Restore

      • Restore into the new course
      • Confirm that the question definition is correct for the restored question
      Show
      Preparation Create a new course Create a new lesson within that course Choose Edit Create a new 'Question Page' Choose 'Matching' as the type Set some answers for each field Backup Create a new backup of the course Restore Restore into the new course Confirm that the question definition is correct for the restored question
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31386-master-1
    • Rank:
      37903

      Description

      When using a matched answer question in a lesson, the fields (Correct Answer, Wrong Answer, Answer 1, Answer n) are determined by the order of the answers in the database. That's to say that when they're retrieved, they are sorted by id ascending. As a result you get:

      • id 0: Correct Answer: "Correct Answer text"
      • id 1: Wrong Answer: "Wrong Answer text"
      • id 2: Answer 1: "Answer 1 text"
      • id 3: Answer 2: "Answer 2 text"
      • id 4: Answer 3: "Answer 3 text"

      When backing up a course with a lesson, these are ordered n reverse:

      <answer id="5">
      </answer>
      <answer id="4">
      </answer>
      <answer id="3">
      </answer>
      <answer id="2">
      </answer>
      <answer id="1">
      </answer>
      

      As a result, when the restore takes place, the questions are taken out of order and reversed:

      • id 0: Correct Answer: "Answer 3 text"
      • id 1: Wrong Answer: "Answer 2 text"
      • id 2: Answer 1: "Answer 1 text"
      • id 3: Answer 2: "Wrong Answer text"
      • id 4: Answer 3: "Correct Answer text"

      And you end up with the

      I can't decide whether the issue lies with:

      • the backup process reversing the order of the answers; or
      • the restore process not ordering the answers by the id in the XML structure.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Initially I'll just look to make sure that the answers are exported in the correct order.

          As a broader task, we might want to look at ordering by id when restoring if an id field is present in the xml.

          Show
          Andrew Nicols added a comment - Initially I'll just look to make sure that the answers are exported in the correct order. As a broader task, we might want to look at ordering by id when restoring if an id field is present in the xml.
          Hide
          Andrew Nicols added a comment -

          This patch does cherry-pick cleanly to:

          • MOODLE_21_STABLE
          • MOODLE_22_STABLE

          but I've not tested the functionality of all of the backups yet on these versions.

          Show
          Andrew Nicols added a comment - This patch does cherry-pick cleanly to: MOODLE_21_STABLE MOODLE_22_STABLE but I've not tested the functionality of all of the backups yet on these versions.
          Hide
          Andrew Nicols added a comment -

          It is difficult to test the failure scenario as it relies upon the natural return order of the database not being in ascending ID order.

          Show
          Andrew Nicols added a comment - It is difficult to test the failure scenario as it relies upon the natural return order of the database not being in ascending ID order.
          Hide
          Andrew Nicols added a comment -

          For master, this should be fixed by MDL-31390 but for MOODLE_22_STABLE and MOODLE_21_STABLE we'll probably need a specific fix for lessions

          Show
          Andrew Nicols added a comment - For master, this should be fixed by MDL-31390 but for MOODLE_22_STABLE and MOODLE_21_STABLE we'll probably need a specific fix for lessions
          Hide
          Andrew Nicols added a comment -

          The attached patch (although named master) can be applied to all branches.
          I haven't listed master against it as it would be better fixed by MDL-31390 if this is integrated.

          Show
          Andrew Nicols added a comment - The attached patch (although named master) can be applied to all branches. I haven't listed master against it as it would be better fixed by MDL-31390 if this is integrated.
          Hide
          Andrew Nicols added a comment -

          Also to note, this fix will not affect restoration of older backups. These will still be broken!

          Show
          Andrew Nicols added a comment - Also to note, this fix will not affect restoration of older backups. These will still be broken!
          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that and sharing a solution.

          Show
          Michael de Raadt added a comment - Thanks for spotting that and sharing a solution.
          Hide
          Andrew Nicols added a comment - - edited

          Since Eloy has suggested that MDL-31390 is unlikely to make the cut, this should probably be applied to all branches.

          Show
          Andrew Nicols added a comment - - edited Since Eloy has suggested that MDL-31390 is unlikely to make the cut, this should probably be applied to all branches.
          Hide
          Adrian Greeve added a comment -

          I'm having trouble replicating the problem. I noticed that you said that it's difficult to test the failure scenario. In what situation would the natural return order not be in ascending ID order?
          I have to say that the code itself looks fine

          Show
          Adrian Greeve added a comment - I'm having trouble replicating the problem. I noticed that you said that it's difficult to test the failure scenario. In what situation would the natural return order not be in ascending ID order? I have to say that the code itself looks fine
          Hide
          Andrew Nicols added a comment -

          Hi Adrian,

          This will completely depend on your database engine and server and can be very difficult to do without manually fettling things. I've just tried and failed repeatedly to get it to do so using the UI. The only way I could repeatedly change the natural sort order was to update the table directly. That said, there's absolutely nothing to stop this from happening in the wild – it purely depends on the database server, memory allocation, size of table, fragmentation on disk, etc. On my small sample dataset of 6 rows, it's difficult to force it. In the end, I did so with:

          UPDATE mdl_lesson_answers set timemodified = timemodified + 1 where id IN (SELECT MIN(id) FROM mdl_lesson_answers GROUP BY lessonid);
          

          On my dev system, I ended up with a table like:

          moodle-master=# UPDATE mdl_lesson_answers set timemodified = timemodified + 1 where id IN (SELECT MIN(id) FROM mdl_lesson_answers GROUP BY lessonid);
          UPDATE 1
          moodle-master=# select * from mdl_lesson_answers;
           id | lessonid | pageid | jumpto | grade | score | flags | timecreated | timemodified |  answer  | answerformat | response | responseformat 
          ----+----------+--------+--------+-------+-------+-------+-------------+--------------+----------+--------------+----------+----------------
           11 |        1 |      3 |    -40 |     0 |     5 |     0 |  1328691306 |            0 | <p>w</p> |            1 |          |              0
           25 |        1 |      3 |      0 |     0 |     0 |     0 |           0 |            0 | example  |            0 | response |              0
           26 |        1 |      3 |      0 |     0 |     0 |     0 |           0 |            0 | example  |            0 | response |              0
           27 |        1 |      3 |      0 |     0 |     0 |     0 |           0 |            0 | example  |            0 | response |              0
           23 |        1 |      3 |      0 |     0 |     0 |     0 |  1328691306 |            1 | a        |            0 | a        |              0
           10 |        1 |      3 |     -1 |     0 |   111 |     0 |  1328691306 |            1 | <p>c</p> |            1 |          |              0
          (6 rows)
          

          I suspect that if the database is busy, then this is more likely to occur naturally than in a single-user dev environment.

          Show
          Andrew Nicols added a comment - Hi Adrian, This will completely depend on your database engine and server and can be very difficult to do without manually fettling things. I've just tried and failed repeatedly to get it to do so using the UI. The only way I could repeatedly change the natural sort order was to update the table directly. That said, there's absolutely nothing to stop this from happening in the wild – it purely depends on the database server, memory allocation, size of table, fragmentation on disk, etc. On my small sample dataset of 6 rows, it's difficult to force it. In the end, I did so with: UPDATE mdl_lesson_answers set timemodified = timemodified + 1 where id IN (SELECT MIN(id) FROM mdl_lesson_answers GROUP BY lessonid); On my dev system, I ended up with a table like: moodle-master=# UPDATE mdl_lesson_answers set timemodified = timemodified + 1 where id IN (SELECT MIN(id) FROM mdl_lesson_answers GROUP BY lessonid); UPDATE 1 moodle-master=# select * from mdl_lesson_answers; id | lessonid | pageid | jumpto | grade | score | flags | timecreated | timemodified | answer | answerformat | response | responseformat ----+----------+--------+--------+-------+-------+-------+-------------+--------------+----------+--------------+----------+---------------- 11 | 1 | 3 | -40 | 0 | 5 | 0 | 1328691306 | 0 | <p>w</p> | 1 | | 0 25 | 1 | 3 | 0 | 0 | 0 | 0 | 0 | 0 | example | 0 | response | 0 26 | 1 | 3 | 0 | 0 | 0 | 0 | 0 | 0 | example | 0 | response | 0 27 | 1 | 3 | 0 | 0 | 0 | 0 | 0 | 0 | example | 0 | response | 0 23 | 1 | 3 | 0 | 0 | 0 | 0 | 1328691306 | 1 | a | 0 | a | 0 10 | 1 | 3 | -1 | 0 | 111 | 0 | 1328691306 | 1 | <p>c</p> | 1 | | 0 (6 rows) I suspect that if the database is busy, then this is more likely to occur naturally than in a single-user dev environment.
          Hide
          Adrian Greeve added a comment -

          I've had a talk with the other developers here and we all agree you can't trust the id to be in order. The solution looks good and I've tested it out on Master. I'm all for going ahead with this fix.

          Thanks for your help and the fix Andrew.

          Show
          Adrian Greeve added a comment - I've had a talk with the other developers here and we all agree you can't trust the id to be in order. The solution looks good and I've tested it out on Master. I'm all for going ahead with this fix. Thanks for your help and the fix Andrew.
          Hide
          Tim Hunt added a comment -

          Submitting for integration, since Andrew seems to have a trustworthy face, and he asked in dev chat.

          Show
          Tim Hunt added a comment - Submitting for integration, since Andrew seems to have a trustworthy face, and he asked in dev chat.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've made Eloy the integrator of this issue.
          I've had a good look at this and I'm happy with the code, however Eloy is the backup expert and I think he should give this the final nod.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've made Eloy the integrator of this issue. I've had a good look at this and I'm happy with the code, however Eloy is the backup expert and I think he should give this the final nod. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          While I hate this sort of "order dependencies", I think this is the quicker and easier way to guarantee consistency with the existing dependency.

          And cannot imagine it breaking anything... so integrated (21, 22 and master). Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - While I hate this sort of "order dependencies", I think this is the quicker and easier way to guarantee consistency with the existing dependency. And cannot imagine it breaking anything... so integrated (21, 22 and master). Thanks!
          Hide
          Ankit Agarwal added a comment -

          Works as described
          Passing!
          Thanks

          Show
          Ankit Agarwal added a comment - Works as described Passing! Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

          Closing as fixed, heading to zzzZZZzzz, niao

          Show
          Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: