The advanced grading code and interfaces are looking absolutely fantastic.
I've been looking over this for the past two days now and have made a LOT of notes so please bear with me there is a lot mentioned below. 99% of it is absolutely trivial and can easily be polished off anytime (after integration, after release what ever) although before release would be great as I'm sure there will me many people wanting to see this integrated with other modules.
Also just noting that I have gone over as much of this new code as possible, I have undoubtedly missed things (hopefully not lots ) and there are still several areas of code I want to come back to further investigate and test. However at this point I am more than happy to give this my +1 pending response about the below notes.
First up some VERY generic notes about coding style things that I've noticed wrong in several places throughout the code:
1. There should be no space between the opening PHP tag and the start of the boilerplate.
2. All classes should include an @copyright and @license tag in their phpdoc block.
3. All methods need to declare their visibility.
4. All TODO's in code require an MDL issue number or if no longer pertinent need to be removed. `git diff origin/master | egrep '^+.*TODO' | egrep --color=auto 'TODO'`
5. All functions, methods, and classes need phpdoc blocks.
6. Variable names should not contain underscore `git diff origin/master | egrep '^+.*\$\w+_\w+' | egrep --color=auto '\$\w+_\w+'`
Next questions I have about the code, db etc
7. There were a couple of database fields that look like they could be renamed to make them clearer. I don't think this will happen given we want to integrate this yesterday but just incase someone else has thought of this and the decision is teetering. I suppose my thinking is that I like to be able to easily see what/where a FK relates to, if recording a user's id I like to think it was userid as its very obvious where it relates to. Exception of course if relation is going to exist more than once or if there is something more descriptive. In the case of the following I'm imagining its set by terminology.
7.1 grading_instances.formid => grading_instances.defintionid given that is what it relates to.
7.2 grading_instances.raterid => grading_instances. userid.
7.3 gradingform_rubric_criteria.formid => gradingform_rubric_criteria.definitionid
7.4 gradingform_rubric_fillings.forminstanceid => gradingform_rubric_fillings.instanceid
8. I am not sure about this one but should contextlevel for moodle/grade:managegradingforms be CONTEXT_MODULE? (Perhaps look into the context level set for all new caps)
9. I see that the rubric editor element is displaying it's own errors when it fails validation, is there any reason it is doing this and not using the forms normal error display ( I see it is setting a dummy object so that the form is aware of any errors).
10. There is one thing that catches my eye every time I see it that I am unsure about. I'm raising this too see whether it was done for a specific reason or whether its just how things turned out or what, its the way in which the grading manager is used in several places within the code. When fetching the grading manager (get_grading_manager) you can provide it with a context/areaid and/or component and/or area. At the end of it really the manager records the context, component, and area. What gets me is that in several places within the code the grading manager is used within iteration of a collection of areas and as part of each iteration the area is set against the grading manager so that further calls made to the grading manager use the area for the iteration. What I don't like about this is that it looks to me like area should in this case be an argument, even an optional argument perhaps to override the area property. The problem with what is happening at the moment is that code after the iteration cannot be sure what the state of the grading manager is any more - there's no telling what the area is. I see a potential problem in the future because of that. Reading over the grading_manager as a whole I get the feeling that half of it wants to be a helper class with static methods without state, and half of it wants to be maintain state and provide assistance when working with a specific integration. Anyway I'm very keen to hear about this class/instance if someone can help me with that.
Now for notes about specific files
11. grade/grading/lib.php -> line 32 incorrect php doc - a description of what can be passed here would also greatly help.
> line 3434 no need to add $this>page->activity name to the get_grading_manager call.
13. lib/navigationlib.php -> Tracing the navigation addition through it's call process it looks like everything relies on the page's context being CONTEXT_MODULE. Needs testing generating module navigation from a course context to make sure things don't go BOOM.
14. lib/form/grading.php -> passing grading instance with the attributes is required, either it should be turned into a separate argument for the element's instantiation, or it should be checked on instantiation and a coding exception or equivalent thrown if it's not present. In both cases it should be documented.
15. mod/assignment/lib.php -> validate_and_preprocess_feedback injects data into $_POST, I have no doubt it works but I don't think this is good code, and it isn't a good example of how to collect + use the resulting grade from an advanced grading interface.
16. mod/assignment/lib.php -> private $advancegradinginstance should be declared at the top of the class and
17.1 gradingform_controller::create_instance unneeded global
17.2 gradingform_controller ::get_or_create_instance, the only optional argument is the first argument? Reading this function through I feel that calling code need to be smarter (if it has an instanced then it's get, if not its create) or this function should not require the $instanced and should fetch from the database and failing that create a new instance. In line with this is the implementation of gradingform_rubric_controller::get_or_create_instance which expects either instanced or raterid+itemid, but if for instance everything is set to null it still tries to create a new instance.
17.3 gradingform_controller::sql_search_where unneeded global
17.4 gradingform_controller::set_grade_range phpdocs missing @param
17.5 gradingform_instance constants need some sort of phpdoc
18. grade/grading/form/rubric/lib.php -> constants in gradingform_rubric_controller should be using /** syntax so that phpdocs picks them up.
19. grade/grading/form/rubric/lib.php -> The SQL loading the definition (gradingform_rubric_controller::load_definition) is it really worthwhile having the the definition, criteria, and levels all fetched in that single statement? Surely in a rubric that is rich with content, lots of criteria, and lots of levels that is going to result in a lot of duplicate information passing over the pipes. Also readability perhs
20. grade/grading/form/rubric/renderer.php -> get_css_class_suffix variable names should be meaningful
21. grade/grading/form/rubric/renderer.php -> display_instances meaningful variable names again
22. grade/grading/form/rubric/rubriceditor.php -> I don't think that constructor is needed is it?
23. lang/en/grading.php -> typo in gradingmethod_help (by the looks anyway) "To disable advance grading" => "To disable advanced grading"
24. pix directory
24.1 Can the BIG_ICONS file be removed, is that required or can the purpose of the b directory be documented in docs somewhere instead ( I think that is where they are documented correct)
24.2 These new icons, where are they from? do we need to give any body credit for them?
25 There is a bug somewhere within gradingform_rubric_controller::update_defintion. When creating a new rubric if I add a criterion and set a score of 999999999999999999999999999999999999 then I get a fatal error. However if I update a criterion with the same score it works perfectly.
User interface notes:
26 When creating a new rubric there is a label `Current rubric status` with no value or field. A little confusing and I don't really get what it is there for.
27 Using the standard theme when creating a rubric the `Add criterion` button's background colour, and top + left border colours are the same as the background colour, this makes the button a lot less obvious as a button. As a developer I spotted it straight away but I remember the enrol UI hitting usability problems in a review because of this. Worth touching it up a little I think.
28 When creating a rubric having added a criterion the cursor needs to be changed when mousing over the `Click to edit level` links so that it is visually obvious that there is an associated action.
29 When creating a rubric, and having added a criterion if I click any of the delete level icons and then click the close button in the top right of the confirmation box nothing happens. (Same for deleting a criterion)
30 When creating a rubric and having added a criterion if I add a small text for the first, second and third levels the delete level icon moves overtop of the number of points text. Still clickable so very minor.
31 When grading a user if I submit the form the advanced grading form without choosing a level for each criterion when the page reloads to require me to I notice the following three things:
31.1 The error message I get 'Please choose something for each criterion' could be improved
31.2 The level selection boxes increase in height without actually having any more content to fill them, it seems to be relate to the height of the browser window. (Noting I have minimal text for the criterion and levels).
31.3 The remark text area is really small by default (10 cols, 5 rows).
Some final parting notes:
32. Thanks for the small set of unit tests - not sure how practical it would be but certainly if at some point more could be developed that would be of huge benefit - especially when reviewing new advanced grading methods and integrations as it really helps to build understanding.
33. There are commits that have been made without any MDL issue numbers in them. I've had a chat to David about this and we've come to the conclusion that trying running an interactive rebase to add commit messages is going to be undue risk as we would have to re-resolve all of the conflicts that have occurred each time the weekly release was merged back into this. I think certainly in future all commit messages NEED to contain at least one tracker issue. It's easy to find out what's going on by inspecting the history in git, its not nearly so easy in tracker.
I honestly think that this can go in at this point providing there is a subtask or new issue created to clean up at least a few of the things noted above before release.
The abuse of $_POST I think is top of the list and then really it would be great if the code could be tidied up per the coding style in docs.
I'll wait for feedback to find out whether there are things that either of you would prefer to clean up before this gets integrated.
All in all David + Marina this is awesome stuff!
I think I was most impressed by the interfaces btw. They still a little polishing here and there but MAN they look good!
The code I think will continue to evolve, certainly as more advanced grading methods are developed there will be a few things that can be optimised. Martin tells me there wasn't time to get everything desired done so no doubt you are aware of some of those places anyway.
It will be interesting to see where things get taken in regards to advanced grading. I'm sure as people start to see it in action within assignment they will being crying out to see it integrated into more and more modules.