Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31386

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh 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
              dobedobedoh Andrew Nicols added a comment -

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

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

              Thanks for spotting that and sharing a solution.

              Show
              salvetore Michael de Raadt added a comment - Thanks for spotting that and sharing a solution.
              Hide
              dobedobedoh 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
              dobedobedoh 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
              abgreeve 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
              abgreeve 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
              dobedobedoh 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
              dobedobedoh 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
              abgreeve 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
              abgreeve 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
              timhunt Tim Hunt added a comment -

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

              Show
              timhunt Tim Hunt added a comment - Submitting for integration, since Andrew seems to have a trustworthy face, and he asked in dev chat.
              Hide
              samhemelryk 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
              samhemelryk 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
              stronk7 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
              stronk7 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_frenz Ankit Agarwal added a comment -

              Works as described
              Passing!
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Works as described Passing! Thanks
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    12/Mar/12