Moodle
  1. Moodle
  2. MDL-31589

Restoring a lesson backup can lead to broken data

    Details

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

      Prepare a backup (or use the one I've attached)

      • Create a new lesson
      • Create a Matching question within the lesson with fields as follows:
        • Correct response: correct
        • Wrong resonse: wrong
        • Matching pair 1 match: pair 1 match
        • Matching pair 1 answer: pair 1 answer
        • Matching pair 2 match: pair 2 match
        • Matching pair 2 answer: pair 2 answer
        • Matching pair 3 match: pair 3 match
        • Matching pair 3 answer: pair 3 answer
      • Backup the activity
      • Download and unzip the backup
      • Open the activities/lesson_X/lesson.xml fild
      • Find the answers and reverse the order

      Testing

      • restore the backup
      • open the lesson
      • edit the question
      • The answers should not be mixed up
      Show
      Prepare a backup (or use the one I've attached) Create a new lesson Create a Matching question within the lesson with fields as follows: Correct response: correct Wrong resonse: wrong Matching pair 1 match: pair 1 match Matching pair 1 answer: pair 1 answer Matching pair 2 match: pair 2 match Matching pair 2 answer: pair 2 answer Matching pair 3 match: pair 3 match Matching pair 3 answer: pair 3 answer Backup the activity Download and unzip the backup Open the activities/lesson_X/lesson.xml fild Find the answers and reverse the order Testing restore the backup open the lesson edit the question The answers should not be mixed up
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31589-master-2
    • Rank:
      38152

      Description

      As raised in MDL-31386, backups of lessons can contain incorrectly ordered data. Although MDL-31386 suggests a fix to the generated backups, any older backups will still be broken.

      We should fix the restoration process to process the elements in order somehow.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Raising the priority to critical as it will cause data corruption in restores

          Show
          Andrew Nicols added a comment - Raising the priority to critical as it will cause data corruption in restores
          Hide
          Andrew Nicols added a comment -

          I've had a look at this but I'm not overly familiar with the backup and restore code

          I can't see any way to get the XML parser to process the data in order - it currently grabs each tag in order.
          As a (slightly hacky) solution we could store the data in an instance variable with the original ID, and handle the actual insert in the after_execute() function after ordering the data by the old IDs, and then set the mappings again. As I say, this seems hacky.

          I'm hoping that someone else has a better solution but I'll work on this one in the mean time.

          I suspect that there may well be other areas in Moodle which hit this kind of bug.

          Show
          Andrew Nicols added a comment - I've had a look at this but I'm not overly familiar with the backup and restore code I can't see any way to get the XML parser to process the data in order - it currently grabs each tag in order. As a (slightly hacky) solution we could store the data in an instance variable with the original ID, and handle the actual insert in the after_execute() function after ordering the data by the old IDs, and then set the mappings again. As I say, this seems hacky. I'm hoping that someone else has a better solution but I'll work on this one in the mean time. I suspect that there may well be other areas in Moodle which hit this kind of bug.
          Hide
          Andrew Nicols added a comment -

          The suggested commit stores answers in an instance variable until execution is complete, at which point it actually inserts them in the after_execute() function.

          I believe that this should suffice in this case - the only other place which requires the lesson_answer mappings is later on in the same function.

          I've added an additional backup attachment which has multiple questions in the same lesson to ensure that the jumpto links don't break

          Show
          Andrew Nicols added a comment - The suggested commit stores answers in an instance variable until execution is complete, at which point it actually inserts them in the after_execute() function. I believe that this should suffice in this case - the only other place which requires the lesson_answer mappings is later on in the same function. I've added an additional backup attachment which has multiple questions in the same lesson to ensure that the jumpto links don't break
          Hide
          Andrew Nicols added a comment -

          I'm going to submit this for peer review as the patch I've suggested may actually be the best solution in this case.

          This should cherry-pick cleanly to:

          • master
          • MOODLE_22_STABLE
          • MOODLE_21_STABLE
          Show
          Andrew Nicols added a comment - I'm going to submit this for peer review as the patch I've suggested may actually be the best solution in this case. This should cherry-pick cleanly to: master MOODLE_22_STABLE MOODLE_21_STABLE
          Hide
          Rajesh Taneja added a comment -

          Hello Andrew,

          Code looks good, few things you might want to consider before pushing it for integration review.

          1. Instead of asort, you should use ksort as sorting by id (which is array key) is required.
          2. Remove extra line, added on 209
          Show
          Rajesh Taneja added a comment - Hello Andrew, Code looks good, few things you might want to consider before pushing it for integration review. Instead of asort, you should use ksort as sorting by id (which is array key) is required. Remove extra line, added on 209
          Hide
          Andrew Nicols added a comment -

          Changes made as per suggestions

          Show
          Andrew Nicols added a comment - Changes made as per suggestions
          Hide
          Andrew Nicols added a comment - - edited

          Discovered whilst testing that the original version of this sample backup attachment for testing was somehow broken

          Show
          Andrew Nicols added a comment - - edited Discovered whilst testing that the original version of this sample backup attachment for testing was somehow broken
          Hide
          Rajesh Taneja added a comment -

          Thanks for updating this, Andrew
          Pushing it for integration review.

          Show
          Rajesh Taneja added a comment - Thanks for updating this, Andrew Pushing it for integration review.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now
          Hide
          Andrew Davis added a comment - - edited

          Seems to work fine. Passing.

          The only niggle was, Andrew and Rajesh, make sure the testing instructions are not bug reproduction instructions. They shouldnt reference the incorrect behaviour as Im assuming that "The answers are all mixed up" is what I should NOT see in testing. If I have misunderstood make sure to let me know.

          Show
          Andrew Davis added a comment - - edited Seems to work fine. Passing. The only niggle was, Andrew and Rajesh, make sure the testing instructions are not bug reproduction instructions. They shouldnt reference the incorrect behaviour as Im assuming that "The answers are all mixed up" is what I should NOT see in testing. If I have misunderstood make sure to let me know.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew, You got it right, the answers should not be mixed up.
          Will update the testing instructions.

          Show
          Rajesh Taneja added a comment - Thanks Andrew, You got it right, the answers should not be mixed up. Will update the testing instructions.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some changes to Moodle should be milestones in the project by themselves.

          This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: