Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Backup
    • Labels:
      None
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32820

      Description

      Analogous to MDL-22139 (backup gradebook), if all the activities in course have been restored, restore the gradebook (categories, calculations). Observing the "users" setting and so on.

      Ciao

      1. 20100816gradebookbackup.patch
        2 kB
        Andrew Davis
      2. 20100819gradebookbackup.patch
        7 kB
        Andrew Davis
      3. 20100820gradebookbackup.patch
        8 kB
        Andrew Davis
      4. 20100820gradebookbackup2.patch
        9 kB
        Andrew Davis
      5. 20100826gradebookbackup3.patch
        13 kB
        Andrew Davis
      6. 20100903.patch
        3 kB
        Andrew Davis

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          As commented in MDL-23479, all the activity grade_items/grade_grades/grade_letters are already restored.

          Not it's the turn of restoring the gradebook only if:

          1) the backup file includes the gradebook information.
          2) all the activities has been selected for restore.

          If both are true, then gradebook categories/calculated items and course overridden grades can be restored safely.

          Note that, for each activity, I've left the original categoryid in the parentid field of get_mapping('grade_item'). That way you can find the gradeitems which categoryid must be changed.

          It must be one step in the restore_final_task, as far as requires all the activities already in place.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - As commented in MDL-23479 , all the activity grade_items/grade_grades/grade_letters are already restored. Not it's the turn of restoring the gradebook only if: 1) the backup file includes the gradebook information. 2) all the activities has been selected for restore. If both are true, then gradebook categories/calculated items and course overridden grades can be restored safely. Note that, for each activity, I've left the original categoryid in the parentid field of get_mapping('grade_item'). That way you can find the gradeitems which categoryid must be changed. It must be one step in the restore_final_task, as far as requires all the activities already in place. Ciao
          Hide
          Andrew Davis added a comment -

          Attaching a very early patch just to check I'm on the right track. Should I be inheriting from restore_execution_step or restore_activity_structure_step?

          Show
          Andrew Davis added a comment - Attaching a very early patch just to check I'm on the right track. Should I be inheriting from restore_execution_step or restore_activity_structure_step?
          Hide
          Andrew Davis added a comment -

          Attaching a newer patch. Awaiting Eloy's return to answer questions throughout the patch and about whether I'm actually going about this the right way

          Show
          Andrew Davis added a comment - Attaching a newer patch. Awaiting Eloy's return to answer questions throughout the patch and about whether I'm actually going about this the right way
          Hide
          Andrew Davis added a comment -

          Attaching yet another patch. The restore now at least runs and puts stuff in the db. there are however problems that Im working on now.

          Note that I had to make some changes in the gradebook backup code so existing backup files won't work. That doesnt actually matter though as some of the activities havent got working backups yet so gradebook wont be getting backed up anyway.

          Show
          Andrew Davis added a comment - Attaching yet another patch. The restore now at least runs and puts stuff in the db. there are however problems that Im working on now. Note that I had to make some changes in the gradebook backup code so existing backup files won't work. That doesnt actually matter though as some of the activities havent got working backups yet so gradebook wont be getting backed up anyway.
          Hide
          Andrew Davis added a comment - - edited

          Another patch. restore completes without error and categories even get created however they don't seem to wind up with items in them.

          Was there a reason I didnt put grade items within grade categories in the backup?

          Show
          Andrew Davis added a comment - - edited Another patch. restore completes without error and categories even get created however they don't seem to wind up with items in them. Was there a reason I didnt put grade items within grade categories in the backup?
          Hide
          Martin Dougiamas added a comment -

          Eloy can you review this please?

          Show
          Martin Dougiamas added a comment - Eloy can you review this please?
          Hide
          Andrew Davis added a comment -

          Hopefully Ill get this finished today. Just attaching another intermediary patch for backup purposes and on the off chance Eloy happens by and suddenly has heaps of spare time

          Show
          Andrew Davis added a comment - Hopefully Ill get this finished today. Just attaching another intermediary patch for backup purposes and on the off chance Eloy happens by and suddenly has heaps of spare time
          Hide
          Andrew Davis added a comment - - edited

          Done some preliminary commits. 2 remaining issues that I know of:

          1) in /backup/moodle2/restore_final_task.class.php how do I test whether or not the gradebook was included in the backup? Ive commented out the gradebook task in the version I committed as otherwise it causes an error on restore. You'll need to go to line 47 and uncomment the task to see the gradebook restore work.

          2) activity grade items dont seem to be going in their correct categories. havent really looked into how I hook the gradebook restore up to the activity restores.

          Show
          Andrew Davis added a comment - - edited Done some preliminary commits. 2 remaining issues that I know of: 1) in /backup/moodle2/restore_final_task.class.php how do I test whether or not the gradebook was included in the backup? Ive commented out the gradebook task in the version I committed as otherwise it causes an error on restore. You'll need to go to line 47 and uncomment the task to see the gradebook restore work. 2) activity grade items dont seem to be going in their correct categories. havent really looked into how I hook the gradebook restore up to the activity restores.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Perfect to have that code in CVS and disabled, it makes things easier for testing! +1

          After a big "sorry" by my delay... here there are some points:

          1) about the tests... they must be achieved by using the execute_condition() method available in the restore_structure_step class. it returns true/false to decide if the step will be really executed. See restore_userscompletion_structure_step for one working example with some comments.

          Also about the tests, I think at least these must be achieved:

          • The file exists (that means the backup process included all the activities in the course originally (or the file wouldn't be there).
          • There is not any gradebook category in the target course (no problem if there are activity grade items)
          • All the activities in the backup file are being restored (I think I can annotate this is some place for easier checking by you).
          • Something that surely I will be missing

          2) about the grade items not "falling" in the correct categories... the key here is that activity grade items are created BEFORE the categories (on each activity task, before the final task). But as I commented, I saved in the "parentitemid" column of the backup_ids table the original category->id where each grade_item was pointing to originally. So I think that all you need to do is, after creating all the categories, iterate over all the grade_item records in backup_ids and update the corresponding grade_items by mapping "parentitemid" (the old id) to the new ones.

          Grr, let me know if the 2) above is clear enough... I'm really tired now to find better explanation. It is clear in my mind but surely not in the explanation above.

          I'll try to look at code tomorrow (haven't done that yet, grrr). Ciao and thanks for your patience!

          Show
          Eloy Lafuente (stronk7) added a comment - Perfect to have that code in CVS and disabled, it makes things easier for testing! +1 After a big "sorry" by my delay... here there are some points: 1) about the tests... they must be achieved by using the execute_condition() method available in the restore_structure_step class. it returns true/false to decide if the step will be really executed. See restore_userscompletion_structure_step for one working example with some comments. Also about the tests, I think at least these must be achieved: The file exists (that means the backup process included all the activities in the course originally (or the file wouldn't be there). There is not any gradebook category in the target course (no problem if there are activity grade items) All the activities in the backup file are being restored (I think I can annotate this is some place for easier checking by you). Something that surely I will be missing 2) about the grade items not "falling" in the correct categories... the key here is that activity grade items are created BEFORE the categories (on each activity task, before the final task). But as I commented, I saved in the "parentitemid" column of the backup_ids table the original category->id where each grade_item was pointing to originally. So I think that all you need to do is, after creating all the categories, iterate over all the grade_item records in backup_ids and update the corresponding grade_items by mapping "parentitemid" (the old id) to the new ones. Grr, let me know if the 2) above is clear enough... I'm really tired now to find better explanation. It is clear in my mind but surely not in the explanation above. I'll try to look at code tomorrow (haven't done that yet, grrr). Ciao and thanks for your patience!
          Hide
          Andrew Davis added a comment -

          1) I've committed a very basic implementation of execute_condition() that just checks whether the gradebook file exists within the backup.

          "All the activities in the backup file are being restored (I think I can annotate this is some place for easier checking by you)."

          I'm not sure about this. Im not sure that excluding one activity should mean that all your categories arent included.

          2) Found after_execute() which is presumably how I do this. I'm having a go at writing an implementation now.

          Show
          Andrew Davis added a comment - 1) I've committed a very basic implementation of execute_condition() that just checks whether the gradebook file exists within the backup. "All the activities in the backup file are being restored (I think I can annotate this is some place for easier checking by you)." I'm not sure about this. Im not sure that excluding one activity should mean that all your categories arent included. 2) Found after_execute() which is presumably how I do this. I'm having a go at writing an implementation now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          1) I've committed a very basic implementation of execute_condition() that just checks whether the gradebook file exists within the backup.

          "All the activities in the backup file are being restored (I think I can annotate this is some place for easier checking by you)."

          I'm not sure about this. Im not sure that excluding one activity should mean that all your categories arent included.

          Well, I don't know too much about the rationale about this, but I can guarantee you that 1.9 restore has been, always, doing that. I guess it is because of calculation gradeitem dependencies or so, I mean, if the calculation formula points to one gradeitem belonging to a non-restored activity, then the calculation cannot be restored (will fail). The correct way to do it should be to look for all those dependencies. The "quick" way is the used in 1.9: avoid restoring them if any activity is missing.

          I guess that's the cause for such "extreme" condition. Note it is just a guess due to my limited knowledge of gradebook internals. You just need which approach to follow (the quick or the correct).

          Clearly I think the correct is the correct lol. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - 1) I've committed a very basic implementation of execute_condition() that just checks whether the gradebook file exists within the backup. "All the activities in the backup file are being restored (I think I can annotate this is some place for easier checking by you)." I'm not sure about this. Im not sure that excluding one activity should mean that all your categories arent included. Well, I don't know too much about the rationale about this, but I can guarantee you that 1.9 restore has been, always, doing that. I guess it is because of calculation gradeitem dependencies or so, I mean, if the calculation formula points to one gradeitem belonging to a non-restored activity, then the calculation cannot be restored (will fail). The correct way to do it should be to look for all those dependencies. The "quick" way is the used in 1.9: avoid restoring them if any activity is missing. I guess that's the cause for such "extreme" condition. Note it is just a guess due to my limited knowledge of gradebook internals. You just need which approach to follow (the quick or the correct). Clearly I think the correct is the correct lol. Ciao
          Hide
          Andrew Davis added a comment -

          I've done another commit or two. I'm having trouble going through the activity grade items and correcting their categories. This was always returning null.

          $grade_item = restore_dbops::get_backup_ids_record($this->get_restoreid(), 'grade_item', $grade_item_backup->itemid)->info;

          line 256 of /backup/moodle2/restore_stepslib.php I don't think I can simply use $grade_item_backup->newitemid and get the grade_item from the database as it appears that the grade_categories in the imported items have been "corrected" and set to the course grade_item.

          The output from that area of the code is below. How do I get the original category id for the restored grade_items???

          grade_item_backup==

          object(stdClass)[4672]
          public 'id' => string '134' (length=3)
          public 'backupid' => string '1881e21f2fdad9efbaf2d575582cc12c' (length=32)
          public 'itemname' => string 'grade_item' (length=10)
          public 'itemid' => string '1' (length=1)
          public 'newitemid' => string '601' (length=3)
          public 'parentitemid' => null
          public 'info' => null

          string 'trying to get grade_item from backup with id==1' (length=47)

          string 'grade_item object from backup==' (length=31)

          null

          backup_ids_temp contains a grade_item that doesnt have the serialized grade_item object in ->info

          Show
          Andrew Davis added a comment - I've done another commit or two. I'm having trouble going through the activity grade items and correcting their categories. This was always returning null. $grade_item = restore_dbops::get_backup_ids_record($this->get_restoreid(), 'grade_item', $grade_item_backup->itemid)->info; line 256 of /backup/moodle2/restore_stepslib.php I don't think I can simply use $grade_item_backup->newitemid and get the grade_item from the database as it appears that the grade_categories in the imported items have been "corrected" and set to the course grade_item. The output from that area of the code is below. How do I get the original category id for the restored grade_items??? grade_item_backup== object(stdClass) [4672] public 'id' => string '134' (length=3) public 'backupid' => string '1881e21f2fdad9efbaf2d575582cc12c' (length=32) public 'itemname' => string 'grade_item' (length=10) public 'itemid' => string '1' (length=1) public 'newitemid' => string '601' (length=3) public 'parentitemid' => null public 'info' => null string 'trying to get grade_item from backup with id==1' (length=47) string 'grade_item object from backup==' (length=31) null backup_ids_temp contains a grade_item that doesnt have the serialized grade_item object in ->info
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Andrew,

          the original category for those activity grade items is stored in the "parentitemid" column. You don't need to access to the "info" column at all, that should be always empty for those records.

          At the same time, I've reviewed the code and there was one problem saving the paretitemid. I've already fixed it and now (tested) the ids will be there.

          Ciao

          PS: Will be looking to your code today...

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Andrew, the original category for those activity grade items is stored in the "parentitemid" column. You don't need to access to the "info" column at all, that should be always empty for those records. At the same time, I've reviewed the code and there was one problem saving the paretitemid. I've already fixed it and now (tested) the ids will be there. Ciao PS: Will be looking to your code today...
          Hide
          Martin Dougiamas added a comment -

          How close is this to being finished?

          Show
          Martin Dougiamas added a comment - How close is this to being finished?
          Hide
          Andrew Davis added a comment -

          Having another go at this based on Eloy's last comment then I'll let you know :|

          Show
          Andrew Davis added a comment - Having another go at this based on Eloy's last comment then I'll let you know :|
          Hide
          Andrew Davis added a comment -

          Ok, just done two more commits. Gradebook restore is now enabled. restore_gradebook_step::execute_condition() should prevent gradebook restore running if you are restoring a backup that doesnt contain gradebook info (it isnt included unless 100% of activities are in the backup).

          Grade categories, grade items and grade grades are now all restored 2 things left to do:

          grade item sortorder is being lost. Probably due to activity grade items being dealt with separately from course, category and manual grade items. Doesnt cause an error but means your grade items are displayed in a different order on the categories and items screen which is a bit off putting.

          restore_gradebook_step::execute_condition() is only testing that the gradebook file exists in the backup. It doesnt test whether all the activities it needs have actually been restored.

          Show
          Andrew Davis added a comment - Ok, just done two more commits. Gradebook restore is now enabled. restore_gradebook_step::execute_condition() should prevent gradebook restore running if you are restoring a backup that doesnt contain gradebook info (it isnt included unless 100% of activities are in the backup). Grade categories, grade items and grade grades are now all restored 2 things left to do: grade item sortorder is being lost. Probably due to activity grade items being dealt with separately from course, category and manual grade items. Doesnt cause an error but means your grade items are displayed in a different order on the categories and items screen which is a bit off putting. restore_gradebook_step::execute_condition() is only testing that the gradebook file exists in the backup. It doesnt test whether all the activities it needs have actually been restored.
          Hide
          Andrew Davis added a comment -

          Eloy, I'm going to need some tips from you as to how I do those 2 things.

          Show
          Andrew Davis added a comment - Eloy, I'm going to need some tips from you as to how I do those 2 things.
          Hide
          Martin Dougiamas added a comment -

          Eloooooooooooooooyyyyyyyyy!!!!

          Show
          Martin Dougiamas added a comment - Eloooooooooooooooyyyyyyyyy!!!!
          Hide
          Andrew Davis added a comment - - edited

          Attaching a patch that prevents the gradebook restore from happening unless all activities are being restored. Want this checked by Eloy before committing as I've made some changes to the core restore code.

          Looking into sortorder problem. The grade items that are restored as part of the gradebook are retaining their sort order. The activity grade items seem to be getting a new random sort order. The correct sort order is in the backup file itself but they're being assigned a new one on restore.

          Show
          Andrew Davis added a comment - - edited Attaching a patch that prevents the gradebook restore from happening unless all activities are being restored. Want this checked by Eloy before committing as I've made some changes to the core restore code. Looking into sortorder problem. The grade items that are restored as part of the gradebook are retaining their sort order. The activity grade items seem to be getting a new random sort order. The correct sort order is in the backup file itself but they're being assigned a new one on restore.
          Hide
          Andrew Davis added a comment -

          I've committed changes that preserve grade_item->sortorder. When the grade items were inserted they were automatically being assigned a new sortorder. I just added something after the insert that reinstates the old sortorder and updates the grade item. That does mean we're now doing 2 writes for each instance of grade item but in the time I have I do not see another way.

          Show
          Andrew Davis added a comment - I've committed changes that preserve grade_item->sortorder. When the grade items were inserted they were automatically being assigned a new sortorder. I just added something after the insert that reinstates the old sortorder and updates the grade item. That does mean we're now doing 2 writes for each instance of grade item but in the time I have I do not see another way.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Andrew, here it's the whole review... really near-near perfect, congrats! Some things to think about (numbered for reference):

          • In backup_gradebook_structure_step:

          1) do we need $grade_category->courseid ?
          2) do we need $letter->contextid ?
          3) we are annotating scales and outcomes, perfect... perhaps we need to annotate also $grade_grades->userid ? I think that in 99.99 of times individual activities already have annotated all the users but... to be 100% correct we must annotate any "userid" to avoid "orphans" in backup files.

          • In restore_gradebook_step (I'd name it restore_gradebook_structure_step, but that is not important):

          4) You are using raw sql inserts / updates instead of the gradebook API. Is there any reason for that? Note that code in 1.9 and also the 2.0 restore of activity grades (restore_activity_grades_structure_step) is using the gradebook API for creating grade items and friends. Not a big concern... for now, just want to know if there is any reason.

          5) Just detected that "settings" like hide/show and other options/preferences aren't included at all, both in backup and restore. Should we? Where are they stored?

          6) When looking for the "course" category, you've this line: $category->fullname = '?' in the params... is that correct? That category always have "?" in fullname? Sounds a bit... crazy. Isn't enough to look for category having the parent = null? Or there are other categories that can have the parent = null ?

          7) in the after_execute() method you are reassigning all the grade_item->categoryid to the new ones. Perfect. Only then, when everything has been restored, that can be done.

          8) Following the same approach than in 7), shouldn't the grade_category->parent reassignments also be done in the after_execute() method? I've seen that in process_grade_category() you are calculating the parents and the paths when not all the categories have been restored yet. I think we should, initially, create all the grade categories being direct child of the course category and later, in after_execute(), once all them have been restored... rebuild the tree (in a very similar way like we are doing with grade items).

          9) There are some columns in grade_grades, see for example the "overridden" one.. that also use to have one timestamp when used. For activities restore I've it being rolled and also one comment saying "// TODO: Ask, all the rest of locktime/exported... work with time... to be rolled?". Plz, do the same in gradebook restore (roll the "overridden" column and put the TODO) Not critical but to have it detected.

          10) About the grade_items->sortorder, your fix looks perfect. Perhaps the API should accept the sortorder directly but we can live with the double insert/update for now.

          11) Already implemented: About the patch proposed to detect if all the activities are being restored... yes, we'll need to iterate over all the activity tasks and so on... but I don't think the way is 100% correct (conceptually one task must not know about the controller or about the rest of tasks - that's the reason for not having those get_plan() and get_controller() methods available for tasks. Also, relying in old_moduleid doesn't seem correct. But better look for all the "xxxx_included" activity settings in order to decide. Let me check the better way to do so.

          12) So the only remaining condition to check is that the course hasn't any grade category yet (following the 1.9 approach), can you add that test? I can imagine problems if we try to restore the same course twice (dupe cats...) or so.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Andrew, here it's the whole review... really near-near perfect, congrats! Some things to think about (numbered for reference): In backup_gradebook_structure_step: 1) do we need $grade_category->courseid ? 2) do we need $letter->contextid ? 3) we are annotating scales and outcomes, perfect... perhaps we need to annotate also $grade_grades->userid ? I think that in 99.99 of times individual activities already have annotated all the users but... to be 100% correct we must annotate any "userid" to avoid "orphans" in backup files. In restore_gradebook_step (I'd name it restore_gradebook_structure_step, but that is not important): 4) You are using raw sql inserts / updates instead of the gradebook API. Is there any reason for that? Note that code in 1.9 and also the 2.0 restore of activity grades (restore_activity_grades_structure_step) is using the gradebook API for creating grade items and friends. Not a big concern... for now, just want to know if there is any reason. 5) Just detected that "settings" like hide/show and other options/preferences aren't included at all, both in backup and restore. Should we? Where are they stored? 6) When looking for the "course" category, you've this line: $category->fullname = '?' in the params... is that correct? That category always have "?" in fullname? Sounds a bit... crazy. Isn't enough to look for category having the parent = null? Or there are other categories that can have the parent = null ? 7) in the after_execute() method you are reassigning all the grade_item->categoryid to the new ones. Perfect. Only then, when everything has been restored, that can be done. 8) Following the same approach than in 7), shouldn't the grade_category->parent reassignments also be done in the after_execute() method? I've seen that in process_grade_category() you are calculating the parents and the paths when not all the categories have been restored yet. I think we should, initially, create all the grade categories being direct child of the course category and later, in after_execute(), once all them have been restored... rebuild the tree (in a very similar way like we are doing with grade items). 9) There are some columns in grade_grades, see for example the "overridden" one.. that also use to have one timestamp when used. For activities restore I've it being rolled and also one comment saying "// TODO: Ask, all the rest of locktime/exported... work with time... to be rolled?". Plz, do the same in gradebook restore (roll the "overridden" column and put the TODO) Not critical but to have it detected. 10) About the grade_items->sortorder, your fix looks perfect. Perhaps the API should accept the sortorder directly but we can live with the double insert/update for now. 11) Already implemented: About the patch proposed to detect if all the activities are being restored... yes, we'll need to iterate over all the activity tasks and so on... but I don't think the way is 100% correct (conceptually one task must not know about the controller or about the rest of tasks - that's the reason for not having those get_plan() and get_controller() methods available for tasks. Also, relying in old_moduleid doesn't seem correct. But better look for all the "xxxx_included" activity settings in order to decide. Let me check the better way to do so. 12) So the only remaining condition to check is that the course hasn't any grade category yet (following the 1.9 approach), can you add that test? I can imagine problems if we try to restore the same course twice (dupe cats...) or so. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've implemented 11 (it is in CVS now). It checks both for missing modules (not available in target site) and excluded activities via settings.

          So we only need to add the check commented in 12) to have de final decision about to restore gradebook or no.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I've implemented 11 (it is in CVS now). It checks both for missing modules (not available in target site) and excluded activities via settings. So we only need to add the check commented in 12) to have de final decision about to restore gradebook or no. Ciao
          Hide
          Andrew Davis added a comment -

          I've worked my way through that list and have committed an updated version of the gradebook backup/restore.

          1. No we dont need grade_category->courseid. removed.
          2. Don't think we need grade_letter->contextid but not sure how to get the context id. I did $this->task->get_contextid() but got the error "undefined method restore_final_task::get_contextid()" get_contextid() exists on restore_activity_task but not restore_final_task().
          3. Now annotating grade_grade->userid
          Ive renamed restore_gradebook_step to restore_gradebook_structure_step. if we're going to change it to be consistent now is as good a time as any.
          4. Why am i using SQL to do the inserts rather than the gradebook API? We're using SQL to get the data out when doing the backup so it seemed natural to use SQL to insert the data during restore. Plus by manually inserting rows I can guarantee nothing is being done automatically that may subtly alter the data in an unanticipated way. Maybe I'm being paranoid.
          5. "settings like hide/show and other options/preferences" If i hide a category total backup, restore the category total is hidden in the restored course. What specific settings do you mean? The hidden flag anyway is stored in the grade_items and grade_categories tables.
          6. re $category->fullname = '?' Im wasnt entirely sure that having a null parent was a 100% guarantee that it was a course level category. When course categories are created they have a hard coded fullname of '?' but just had another look and you can actually edit it. I have removed the ? condition.
          7. nothing to do here I believe.
          8. Ive shifted the correction of grade category parent and path to after_execute() as you suggested. Annoyingly we need to make two passes over the grade categories as the code that figures out the paths loads parent categories from the DB.
          9. Ive added the todo and the adjustment of the overriden column
          10. re grade_item->sortorder altering the grade_item insert method to check for a sortorder before getting a new one is easy enough. I'm just wary of doing it just before release.
          11. you've already fixed this
          12. Ive added the test in execute_condition to look for grade categories although Im not entirely sure whether we should outright prevent the gradebook restore in the presence of existing grade categories. Better safe than sorry I guess.

          I think only points 2 and 5 require your attention really

          Show
          Andrew Davis added a comment - I've worked my way through that list and have committed an updated version of the gradebook backup/restore. 1. No we dont need grade_category->courseid. removed. 2. Don't think we need grade_letter->contextid but not sure how to get the context id. I did $this->task->get_contextid() but got the error "undefined method restore_final_task::get_contextid()" get_contextid() exists on restore_activity_task but not restore_final_task(). 3. Now annotating grade_grade->userid Ive renamed restore_gradebook_step to restore_gradebook_structure_step. if we're going to change it to be consistent now is as good a time as any. 4. Why am i using SQL to do the inserts rather than the gradebook API? We're using SQL to get the data out when doing the backup so it seemed natural to use SQL to insert the data during restore. Plus by manually inserting rows I can guarantee nothing is being done automatically that may subtly alter the data in an unanticipated way. Maybe I'm being paranoid. 5. "settings like hide/show and other options/preferences" If i hide a category total backup, restore the category total is hidden in the restored course. What specific settings do you mean? The hidden flag anyway is stored in the grade_items and grade_categories tables. 6. re $category->fullname = '?' Im wasnt entirely sure that having a null parent was a 100% guarantee that it was a course level category. When course categories are created they have a hard coded fullname of '?' but just had another look and you can actually edit it. I have removed the ? condition. 7. nothing to do here I believe. 8. Ive shifted the correction of grade category parent and path to after_execute() as you suggested. Annoyingly we need to make two passes over the grade categories as the code that figures out the paths loads parent categories from the DB. 9. Ive added the todo and the adjustment of the overriden column 10. re grade_item->sortorder altering the grade_item insert method to check for a sortorder before getting a new one is easy enough. I'm just wary of doing it just before release. 11. you've already fixed this 12. Ive added the test in execute_condition to look for grade categories although Im not entirely sure whether we should outright prevent the gradebook restore in the presence of existing grade categories. Better safe than sorry I guess. I think only points 2 and 5 require your attention really
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Agree with everything! Cool!

          Side note: in 7) taking rid of $category->fullname caused my test course restore to start working ok, as far as I had the name changed manually here. Cool!

          About:

          2) Yes, only block/activity/course tasks have the get_contextid() method. The final task haven't it (it is, by definition, context independent). In any case, given that you always know the courseid, I guess something like this will do the trick:

          $data->contextid = get_context_instance(CONTEXT_COURSE, $this->get_courseid())->id;
          

          5) I'm talking about the "course settings" for example, things like "number of decimal points, show percentage, hide totals..." or also "grader report preferences", things like "show calculations, enable ajax..." where are those settings being stored?

          Show
          Eloy Lafuente (stronk7) added a comment - Agree with everything! Cool! Side note: in 7) taking rid of $category->fullname caused my test course restore to start working ok, as far as I had the name changed manually here. Cool! About: 2) Yes, only block/activity/course tasks have the get_contextid() method. The final task haven't it (it is, by definition, context independent). In any case, given that you always know the courseid, I guess something like this will do the trick: $data->contextid = get_context_instance(CONTEXT_COURSE, $ this ->get_courseid())->id; 5) I'm talking about the "course settings" for example, things like "number of decimal points, show percentage, hide totals..." or also "grader report preferences", things like "show calculations, enable ajax..." where are those settings being stored?
          Hide
          Andrew Davis added a comment - - edited

          Committed the grade letter context id fix. Somehow didnt think of that. I forget that backup/restore is actually part of Moodle.

          Grader report preferences are stored in user_preferences and are already being backed up by backup_users_structure_step.

          Course settings are stored in grade_settings and are not currently being backed up. Fixing that now. Good catch

          Show
          Andrew Davis added a comment - - edited Committed the grade letter context id fix. Somehow didnt think of that. I forget that backup/restore is actually part of Moodle. Grader report preferences are stored in user_preferences and are already being backed up by backup_users_structure_step. Course settings are stored in grade_settings and are not currently being backed up. Fixing that now. Good catch
          Hide
          Andrew Davis added a comment - - edited

          Committed code that includes grade_settings in the backup and restore process. Does that mean we're done???

          Show
          Andrew Davis added a comment - - edited Committed code that includes grade_settings in the backup and restore process. Does that mean we're done???
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, I think so! Let's see how this evolve in the future, with the conditions hopefully becoming more relaxed if possible or so. But for now, I think it's done. Congrats!

          Last note: as commented via chat... I'd recommend to review the restore code... just to check that we aren't setting "too much unnecessary mappings". Anything not having FKs pointing to it doesn't need to stored the mappings at all (each mapping is one insert operation so better if we can save as many as possible).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, I think so! Let's see how this evolve in the future, with the conditions hopefully becoming more relaxed if possible or so. But for now, I think it's done. Congrats! Last note: as commented via chat... I'd recommend to review the restore code... just to check that we aren't setting "too much unnecessary mappings". Anything not having FKs pointing to it doesn't need to stored the mappings at all (each mapping is one insert operation so better if we can save as many as possible). Ciao
          Hide
          Andrew Davis added a comment -

          I've committed out the annotating of the old/new IDs for grade_grades and grade_settings. Don't think anything has a FK pointing to them.

          Thanks for all of your help

          Show
          Andrew Davis added a comment - I've committed out the annotating of the old/new IDs for grade_grades and grade_settings. Don't think anything has a FK pointing to them. Thanks for all of your help
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Congrats Andrew!

          Show
          Eloy Lafuente (stronk7) added a comment - Congrats Andrew!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: