Moodle
  1. Moodle
  2. MDL-31314

Restoring backups from 1.9 does not set grade_categories.depth

    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, Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      1. Restore the attached backup-moodle2-course-g100-20120126-1012-19export.mbz

      2. Go to the gradebook, edit categories page, and verify that the grade category has move and delete icons.

      3. Verify in the database that the grade_category is present, and that the depth and path fields are correct.

      Show
      1. Restore the attached backup-moodle2-course-g100-20120126-1012-19export.mbz 2. Go to the gradebook, edit categories page, and verify that the grade category has move and delete icons. 3. Verify in the database that the grade_category is present, and that the depth and path fields are correct.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      37783

      Description

      Moodle 1.9 did not write grade_categories.depth and grade_categories.path to the backup file because both of these can be re-constructed from grade_categories.parent.

      This is not totally a problem, because 2.x restore code computes grade_categories.path from parent on lines 375 - 400 of backup/moodle2/restore_stepslib.php

      However, it does not re-compute depth at the same time. It relies on the correct value being in the backup file which was not the case for a custom "Convert courses from 1.9 -> 2.x" system we developed at the OU>

      As a results, you get grade_categories in the DB with depth = 0.

      Surprisingly, this does not break very much. We only noticed it at the OU because the move icon is only shown on categories where depth > 0, and someone noticed that they could not move any categories in their course that came from 1.9.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Please can someone review this.

          If it is OK, the fix needs to be back-ported to 2.1 and 2.2 branches.

          Show
          Tim Hunt added a comment - Please can someone review this. If it is OK, the fix needs to be back-ported to 2.1 and 2.2 branches.
          Hide
          Tim Hunt added a comment -

          Sorry, I have not tested this myself yet. I really did not need this dumped on me today on top of everything else, so if anyone else feels grateful for me for finding the problem and making the patch, please can they test it for me. Otherwise, I will try to get back to it later.

          Show
          Tim Hunt added a comment - Sorry, I have not tested this myself yet. I really did not need this dumped on me today on top of everything else, so if anyone else feels grateful for me for finding the problem and making the patch, please can they test it for me. Otherwise, I will try to get back to it later.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It looks perfect!

          I was going to test it with the reference course backup (
          backup-refcourse-20110701-2127.zip from MDL-27495), and just detected that the recent cleanup of upgrade code (MDL-30610) and its 1st regression fix (MDL-31124) have been insufficient to be able to restore that course.

          Fixing it now... will be back here after that.

          Show
          Eloy Lafuente (stronk7) added a comment - It looks perfect! I was going to test it with the reference course backup ( backup-refcourse-20110701-2127.zip from MDL-27495 ), and just detected that the recent cleanup of upgrade code ( MDL-30610 ) and its 1st regression fix ( MDL-31124 ) have been insufficient to be able to restore that course. Fixing it now... will be back here after that.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, MDL-31333 sent to integration (it enables restoring backup-refcourse-20110701-2127.zip from MDL-27495).

          Strange, i've restored if and I get, in DB:

          id	parent	depth	path
          4	NULL	1	/4/
          5	4	2	/4/5/
          6	4	2	/4/6/
          7	4	2	/4/7/
          

          without applying your patch... re-trying...

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, MDL-31333 sent to integration (it enables restoring backup-refcourse-20110701-2127.zip from MDL-27495 ). Strange, i've restored if and I get, in DB: id parent depth path 4 NULL 1 /4/ 5 4 2 /4/5/ 6 4 2 /4/6/ 7 4 2 /4/7/ without applying your patch... re-trying...
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Wow, confirmed. I've restored two more times and depths are there without applying the patch (and before visiting the course or the grader report, just in case). Trying to trace it down...

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Wow, confirmed. I've restored two more times and depths are there without applying the patch (and before visiting the course or the grader report, just in case). Trying to trace it down...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oh, confirmed. The 1.9 => 2.x conversion is supposedly handling depth and path perfectly, see the write_grade_categories() @ backup/converter/moodle1/handlerlib.php:

          https://github.com/stronk7/moodle/blob/MOODLE_21_STABLE/backup/converter/moodle1/handlerlib.php#L1590

          It was implemented by MDL-28006, so seems that should be working since 2.1 release... uhm... we need to look a bit more carefully what is happening there (surely being able to get one 1.9 backup reproducing the problem would help).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, confirmed. The 1.9 => 2.x conversion is supposedly handling depth and path perfectly, see the write_grade_categories() @ backup/converter/moodle1/handlerlib.php: https://github.com/stronk7/moodle/blob/MOODLE_21_STABLE/backup/converter/moodle1/handlerlib.php#L1590 It was implemented by MDL-28006 , so seems that should be working since 2.1 release... uhm... we need to look a bit more carefully what is happening there (surely being able to get one 1.9 backup reproducing the problem would help). Ciao
          Hide
          Tim Hunt added a comment -

          Note that I did not find this using the Moodle core 'restore from 1.9' feature. This is only happening with the OU, 'convert one of our Moodle 1.9 sites will all our custom hacks to one of our Moodle 2.1 sites with a different set of custom hacks' feature.

          So, in once sense, you do not need to add my fix to Moodle core. I could, if required, re-code it so that the code lives inside our local/convertsite plugin.

          However, it does not seem unreasonable to me that if restore_stepslib is recomputing path, that it also re-computes depth.

          A number of people have writted their own 1.9 -> 2.x converters, and I wonder how many of them knew the subtlety that you have to fill-in the grade-category depth? Especially since depth = 0 zero only causes very subtle breakage.

          So, is it OK to apply my fix to core?

          Show
          Tim Hunt added a comment - Note that I did not find this using the Moodle core 'restore from 1.9' feature. This is only happening with the OU, 'convert one of our Moodle 1.9 sites will all our custom hacks to one of our Moodle 2.1 sites with a different set of custom hacks' feature. So, in once sense, you do not need to add my fix to Moodle core. I could, if required, re-code it so that the code lives inside our local/convertsite plugin. However, it does not seem unreasonable to me that if restore_stepslib is recomputing path, that it also re-computes depth. A number of people have writted their own 1.9 -> 2.x converters, and I wonder how many of them knew the subtlety that you have to fill-in the grade-category depth? Especially since depth = 0 zero only causes very subtle breakage. So, is it OK to apply my fix to core?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Grrr, you could have specified that the issue was not happening using the 'core' feature. That would have saved some time here. Also, then, the testing instructions are 100% incorrect (unless they include one step to "hack" the XML and delete path & depth, or you provide one 2.0 backup without them).

          So yes, I think this fix can land, to make the process safer for some "custom conversion solutions" (I don't want to discuss who has the responsibility of doing that ok). But please, amend / fix / provide a way to test this.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Grrr, you could have specified that the issue was not happening using the 'core' feature. That would have saved some time here. Also, then, the testing instructions are 100% incorrect (unless they include one step to "hack" the XML and delete path & depth, or you provide one 2.0 backup without them). So yes, I think this fix can land, to make the process safer for some "custom conversion solutions" (I don't want to discuss who has the responsibility of doing that ok). But please, amend / fix / provide a way to test this. Ciao
          Hide
          Tim Hunt added a comment -

          Yes, sorry. I should have made it clearer. I will provide a minimal text XML from our conversion process for use in the test instructions.

          Show
          Tim Hunt added a comment - Yes, sorry. I should have made it clearer. I will provide a minimal text XML from our conversion process for use in the test instructions.
          Hide
          Tim Hunt added a comment -

          Submitting for integration now.

          Strangely, when I follow the test instructions, I get some notices in the gradebook (e.g. missing ->sortorder) however, once you go to the edit categories screen, and then click reload, the errors go away. The same errors happen with and without my fix, and I don't have time to investigate them now, so I hope it is OK to apply my fix, and leave the notices for later.

          Show
          Tim Hunt added a comment - Submitting for integration now. Strangely, when I follow the test instructions, I get some notices in the gradebook (e.g. missing ->sortorder) however, once you go to the edit categories screen, and then click reload, the errors go away. The same errors happen with and without my fix, and I don't have time to investigate them now, so I hope it is OK to apply my fix, and leave the notices for later.
          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
          Tim Hunt added a comment -

          All three branches re-based.

          Show
          Tim Hunt added a comment - All three branches re-based.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! (although I'm not sure if this is something restore should fix that way or the "exporter" tool instead. Who cares anyway).

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (although I'm not sure if this is something restore should fix that way or the "exporter" tool instead. Who cares anyway).
          Hide
          Adrian Greeve added a comment -

          I tested this in version 2.1, 2.2 and master. I did notice the errors that were displayed when first loading the page. The table seemed to be a little bit out of alignment as well, but a refresh did make those disappear.
          Test passed. Thanks Tim.

          Show
          Adrian Greeve added a comment - I tested this in version 2.1, 2.2 and master. I did notice the errors that were displayed when first loading the page. The table seemed to be a little bit out of alignment as well, but a refresh did make those disappear. Test passed. Thanks Tim.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: