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.