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:

      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.

        Gliffy Diagrams

          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: