1. Teachers can set the advanced grading method to Rubric in the assignment module's setting form when creating/editing the module.
2. The 'Advance grading' management page is available via the activity settings block.
3. Teachers can define the rubric from scratch, providing the rubric name, description and the rubric's criteria and levels for each criterion.
4. Teachers can re-use rubrics defined by their own in other activities.
5. Managers (or generally users with the permission) can save a rubric as a shared template if it is marked as ready for usage.
6. Teachers can re-use shared templates in their courses.
7. Teachers can use the rubric for the assignment module submissions assessment if it is marked as ready for usage.

During the QA phase, the following should be tested:
1. Teachers can set the advanced grading method to Rubric in the assignment module's setting form when creating/editing the module.
2. The 'Advance grading' management page is available via the activity settings block.
3. Teachers can define the rubric from scratch, providing the rubric name, description and the rubric's criteria and levels for each criterion.
4. Teachers can re-use rubrics defined by their own in other activities.
5. Managers (or generally users with the permission) can save a rubric as a shared template if it is marked as ready for usage.
6. Teachers can re-use shared templates in their courses.
7. Teachers can use the rubric for the assignment module submissions assessment if it is marked as ready for usage.

Mark Drechsler
added a comment - 12/Sep/11 8:00 PM Do we have a likely ETA for when some early code releases might be available for this in the lead up to 2.2? Keen to help out with some functional QA when the time is right, the spec looks great

David Mudrák
added a comment - 19/Sep/11 9:45 PM Me and Marina are going to start work on this. We plan to have it finished in 6 weeks so I would say the first testable previews might be available near the end of October (2011 )

Mark Drechsler
added a comment - 24/Sep/11 8:48 PM Awesome David!
Look forward to watching the developments unfold in the lead up to 2.2.
Please let me know if you need any help testing out preview code.
Mark.

David, I read your spec and respect very much the job you've done! It's great!
I have several questions to you:

As I understand, there will be a new API to allow different methods of grading. They can be installed as a new plugin type.

1. Are we transferring the existing 'simple grading' into such plugin as well?

2. What if some other user-contributed plugins allow possibility of 'Quick grading'? Maybe we should not disable it at all, but instead ask the plugin?

3. Did you think of how to manage the changing of the grading method after the assignment was created, especially in the case where some students have been graded? Is it possible that within one assignment some students are graded with one method, and some - with other?

4. I personally think that table prefixes grading_form_ and gradeform_ next to each other are a bit confusing

5. Is it going to be provided in API that there could be multiple graders (f.e. several teachers)? We don't have to implement it right now, but just keep in mind for the future.

Marina Glancy
added a comment - 28/Sep/11 9:51 AM David, I read your spec and respect very much the job you've done! It's great!
I have several questions to you:
As I understand, there will be a new API to allow different methods of grading. They can be installed as a new plugin type.
1. Are we transferring the existing 'simple grading' into such plugin as well?
2. What if some other user-contributed plugins allow possibility of 'Quick grading'? Maybe we should not disable it at all, but instead ask the plugin?
3. Did you think of how to manage the changing of the grading method after the assignment was created, especially in the case where some students have been graded? Is it possible that within one assignment some students are graded with one method, and some - with other?
4. I personally think that table prefixes grading_form_ and gradeform_ next to each other are a bit confusing
5. Is it going to be provided in API that there could be multiple graders (f.e. several teachers)? We don't have to implement it right now, but just keep in mind for the future.

re 1. Definitely not for 2.2. The design shoudl allow us to do so in the future though. The "Simple grading" just means "do not use advanced grading methods" machinery at all. The processing of current grades and scales is everywhere. I wanted to keep this project as a pure optional extension not touching current working code much. My prototypes in the Assignment showed me that it should be basically some if ()

{ ... }

sections injected.

re 2. I would not waste time with it now. It is Assignment specific and who knows how Assignement will look like. It should be doable of course.

re 3. Yes, it is possible that some students are graded with rubric, some with simple and some with another plugin. The only limitation is that inside one gradable area (like one assignment), only one instance per module is allowed. So there can't be one half of students with Rubric1 and the second with Rubric2

re 4. Renamed to grading_ prefix, thanks!

re 5. Of course, each itemid can be assessed by multiple raters. In grading_instances, each would have their own record. Just the combinations of formid+raterid+itemid is unique. So one submission can't be rated multiple times by the same rater. If a module wants to implement two phase assessment, they will declare two gradable areas like "preassessment" and "postassessment" for example

David Mudrák
added a comment - 29/Sep/11 3:55 AM re 1. Definitely not for 2.2. The design shoudl allow us to do so in the future though. The "Simple grading" just means "do not use advanced grading methods" machinery at all. The processing of current grades and scales is everywhere. I wanted to keep this project as a pure optional extension not touching current working code much. My prototypes in the Assignment showed me that it should be basically some if ()
{ ... }
sections injected.
re 2. I would not waste time with it now. It is Assignment specific and who knows how Assignement will look like. It should be doable of course.
re 3. Yes, it is possible that some students are graded with rubric, some with simple and some with another plugin. The only limitation is that inside one gradable area (like one assignment), only one instance per module is allowed. So there can't be one half of students with Rubric1 and the second with Rubric2
re 4. Renamed to grading_ prefix, thanks!
re 5. Of course, each itemid can be assessed by multiple raters. In grading_instances, each would have their own record. Just the combinations of formid+raterid+itemid is unique. So one submission can't be rated multiple times by the same rater. If a module wants to implement two phase assessment, they will declare two gradable areas like "preassessment" and "postassessment" for example

Mark, thanks for your interest. We are still intensively working on this. However, I would like to catch as many issues as possible before we submit this for the final integration into the main repository (which should happen this Friday hopefully). So, if you would like to pre-test and give us initial feedback on this unfinished work in progress, please install 2.2dev Moodle from https://github.com/mudrd8mz/moodle/tree/rubric (sorry, there are no tarballs available yet). We plan to set-up a public testing site later week should you have difficulties to checkout the 'rubric' branch from my repo.

David Mudrák
added a comment - 01/Nov/11 9:04 PM Mark, thanks for your interest. We are still intensively working on this. However, I would like to catch as many issues as possible before we submit this for the final integration into the main repository (which should happen this Friday hopefully). So, if you would like to pre-test and give us initial feedback on this unfinished work in progress, please install 2.2dev Moodle from https://github.com/mudrd8mz/moodle/tree/rubric (sorry, there are no tarballs available yet). We plan to set-up a public testing site later week should you have difficulties to checkout the 'rubric' branch from my repo.

Got it all set up, getting the Consulting team to give it a bit of a thrashing and add any feedback here.

First a few from from me:

1. the handling of decimals. Check out https://skitch.com/markdrechsler/gfemh/rubric-course-feedback-student-student - I've set up a form with decimals, which all worked fine, but it has rounded the end result before writing to the gradebook. Should either give more of a warning that it is rounding the end result, or (ideally) allow for the decimal grade to be sent to the Gradebook. Or I guess remove the ability to have decimal points in the form at all.
2. When I am adding a new activity it doesn't show the 'Grading Method' drop down - but if I save the activity and then edit it again then it does show the 'grading method' drop down. Should be there from the start?
3. There's nothing which shows clearly that a rubric form is in draft form. If you save it as a draft and then try to mark the assignment then there should probably be a warning that lets markers know that the rubric form isn't ready yet, and if they have appropriate permissions then take them to the form to prompt them to edit/release it.
4. I take it that there's no way to view a list of grading forms used in a course, or to upload templates to an area before adding the activity? Just checking.

I'll add more if I come across them - overall it looks awesome though - great work yet again David - I seriously owe you a beer or two one day

Mark Drechsler
added a comment - 02/Nov/11 8:00 AM Hey David,
Got it all set up, getting the Consulting team to give it a bit of a thrashing and add any feedback here.
First a few from from me:
1. the handling of decimals. Check out https://skitch.com/markdrechsler/gfemh/rubric-course-feedback-student-student - I've set up a form with decimals, which all worked fine, but it has rounded the end result before writing to the gradebook. Should either give more of a warning that it is rounding the end result, or (ideally) allow for the decimal grade to be sent to the Gradebook. Or I guess remove the ability to have decimal points in the form at all.
2. When I am adding a new activity it doesn't show the 'Grading Method' drop down - but if I save the activity and then edit it again then it does show the 'grading method' drop down. Should be there from the start?
3. There's nothing which shows clearly that a rubric form is in draft form. If you save it as a draft and then try to mark the assignment then there should probably be a warning that lets markers know that the rubric form isn't ready yet, and if they have appropriate permissions then take them to the form to prompt them to edit/release it.
4. I take it that there's no way to view a list of grading forms used in a course, or to upload templates to an area before adding the activity? Just checking.
I'll add more if I come across them - overall it looks awesome though - great work yet again David - I seriously owe you a beer or two one day
Mark.

Few more:
5. If you grade an assessment using a grading form, then revert the form to 'draft', there is no warnings or stopping the user from doing this, nor is there anywhere showing that the form has been reverted to a draft. I think it just needs more in the way of letting teachers know what is going on re managing their drafts and 'live' versions of forms.
6. Export and import of grading forms using XML - possible?
7. If a grading form is shared, then does it have to be shared site-wide, or can it be restricted to (for example) a category?
8. Did some testing by altering Gradebook marks after having marked using a grading form. Was allowed to update the grade at the activity level - should that have been possible or should it have been locked? Also re-marked the same activity (which was locked in the GB) and got the following error: http://img.skitch.com/20111102-e86busd2w8ne55p4k54i2skw5w.jpg
9. Templates - is it possible to set up pre-made grading form templates available site-wide a-la database templates?

Bit of a mish-mash of bugs, suggestions and comments here - do with as you wish

Mark Drechsler
added a comment - 02/Nov/11 8:46 AM Few more:
5. If you grade an assessment using a grading form, then revert the form to 'draft', there is no warnings or stopping the user from doing this, nor is there anywhere showing that the form has been reverted to a draft. I think it just needs more in the way of letting teachers know what is going on re managing their drafts and 'live' versions of forms.
6. Export and import of grading forms using XML - possible?
7. If a grading form is shared, then does it have to be shared site-wide, or can it be restricted to (for example) a category?
8. Did some testing by altering Gradebook marks after having marked using a grading form. Was allowed to update the grade at the activity level - should that have been possible or should it have been locked? Also re-marked the same activity (which was locked in the GB) and got the following error: http://img.skitch.com/20111102-e86busd2w8ne55p4k54i2skw5w.jpg
9. Templates - is it possible to set up pre-made grading form templates available site-wide a-la database templates?
Bit of a mish-mash of bugs, suggestions and comments here - do with as you wish

I have a couple of comments (numbering following on from Mark's comments):

10. (student) There is no way for the student to view the rubric. It is desirable that the teacher can select to show the grading form to the student or not. In my opinion it is desirable to have the form shown to the student as the default so that they have a clear picture of what they will be graded on.

11. (teacher or admin) I wasn't able to switch to a different rubric or start from scratch once I had selected one.

12. (teacher or admin) When creating a rubric from scratch, as each criteria would most often have the same number of levels it would be useful for the 2nd and additional criteria to be populated automatically with the same number of levels as the first - with the levels still customizable (add/remove).

13. (admin) I wasn't able to locate the area in site admin to manage the rubrics, but perhaps this isn't in yet.

Kim Edgar
added a comment - 02/Nov/11 9:13 AM - edited Hi
I'm part of the consulting team at NetSpot. First of all, great job!
I have a couple of comments (numbering following on from Mark's comments):
10. (student) There is no way for the student to view the rubric. It is desirable that the teacher can select to show the grading form to the student or not. In my opinion it is desirable to have the form shown to the student as the default so that they have a clear picture of what they will be graded on.
11. (teacher or admin) I wasn't able to switch to a different rubric or start from scratch once I had selected one.
12. (teacher or admin) When creating a rubric from scratch, as each criteria would most often have the same number of levels it would be useful for the 2nd and additional criteria to be populated automatically with the same number of levels as the first - with the levels still customizable (add/remove).
13. (admin) I wasn't able to locate the area in site admin to manage the rubrics, but perhaps this isn't in yet.
Thanks
Kim

Thanks NetSpot for their initial review. I am going to comment quickly. But firstly let me say that kudos should go mostly to Marina Glancy who was actually working on the rubric plugin and its integration with the assignment. Hence she is a better person to comment on rubric specific issues. I was just digging some background basement for it.

re 1. There was a bug in calculating the grade from the rubric score, maybe it is related.
re 2. Yeah, that is known issue. The problem is that when the new assignment is being created, we do not know its context yet. And there was an idea that the list of available plugins (rubric and friends) should be context-specific. These two requests as mutually exclusive.
re 3. That was just fixed yesterday IIRC
re 4. Not yet, some sort of general grading forms management page will be added in the future.
re 5. Correct, this is to be added yet. When teacher tries to edit some form that was already marked as ready to use, there will be a warning. When some assessments were actually already done using that form, even bigger warning will be displayed and all such assessments will be marked with a "Needs review" flag. The teacher can (and should) then go and review/resave all such assessments.
re 6. Not yet. And backup/restore is missing still, too.
re 7. Yes, site-wide only. And probably will stay like that, with some possibility to organize the templates using tags/labels. But we will probably not follow the Question bank's approach. I believe that once there is a feature to import/export forms via XML files and another one to re-use own forms even without publishing them as a template, almost everyone's needs should be satisfied (yeah, naive, I know...)
re 8. Advanced grading method plugins are designed just as a tools that calculate the final grade and give the result back to the module. So it all depends on how the activity module deals with these situations. From my point of view, the teacher should always have the right of veta when it comes to grades. That error should be explored in a separate issue, please report it.
re 9. Well, you can define a grading form and save it as a template. Then you can create new grading forms from that template in other activities.
re 10. Good point. Again, this is the situation where activity modules (we call them "clients" in this context) decide how they integrate grading forms. In case of the assignment, it makes sense to have an option to display the form to the students. And I actually believe it worked like that?
re 11. Delete the current one first.
re 12. Yes, that is reported usability issue.
re 13. see re 4.

David Mudrák
added a comment - 02/Nov/11 8:41 PM Thanks NetSpot for their initial review. I am going to comment quickly. But firstly let me say that kudos should go mostly to Marina Glancy who was actually working on the rubric plugin and its integration with the assignment. Hence she is a better person to comment on rubric specific issues. I was just digging some background basement for it.
re 1. There was a bug in calculating the grade from the rubric score, maybe it is related.
re 2. Yeah, that is known issue. The problem is that when the new assignment is being created, we do not know its context yet. And there was an idea that the list of available plugins (rubric and friends) should be context-specific. These two requests as mutually exclusive.
re 3. That was just fixed yesterday IIRC
re 4. Not yet, some sort of general grading forms management page will be added in the future.
re 5. Correct, this is to be added yet. When teacher tries to edit some form that was already marked as ready to use, there will be a warning. When some assessments were actually already done using that form, even bigger warning will be displayed and all such assessments will be marked with a "Needs review" flag. The teacher can (and should) then go and review/resave all such assessments.
re 6. Not yet. And backup/restore is missing still, too.
re 7. Yes, site-wide only. And probably will stay like that, with some possibility to organize the templates using tags/labels. But we will probably not follow the Question bank's approach. I believe that once there is a feature to import/export forms via XML files and another one to re-use own forms even without publishing them as a template, almost everyone's needs should be satisfied (yeah, naive, I know...)
re 8. Advanced grading method plugins are designed just as a tools that calculate the final grade and give the result back to the module. So it all depends on how the activity module deals with these situations. From my point of view, the teacher should always have the right of veta when it comes to grades. That error should be explored in a separate issue, please report it.
re 9. Well, you can define a grading form and save it as a template. Then you can create new grading forms from that template in other activities.
re 10. Good point. Again, this is the situation where activity modules (we call them "clients" in this context) decide how they integrate grading forms. In case of the assignment, it makes sense to have an option to display the form to the students. And I actually believe it worked like that?
re 11. Delete the current one first.
re 12. Yes, that is reported usability issue.
re 13. see re 4.

Hi Mark, Kim.
The most of your notes are already work-in-progress. Actually I don't know at which point you checked out code for testing.
I'll go through the list too and answer if I know about something more than David:
1. This is a good spotting, thanks.
5. I'm working on warnings at the moment, but 'Need review' flag in assignment grading list unfortunately is not likely to appear in 2.2
8. It should be possible to override grade and the rubric will probably become unavailable to student in this case. I'll work on the error message
10. This is strange. Student is able to view his evaluation everywhere - clicking on the assignment, following a link from gradebook, and now even the user Complete report (under activities reports) is fixed and it shows all the grades in the course correctly and with advanced grading feedback (filled rubric in this case). Can you please double check it
12. good point, added it to MDL-30023

Marina Glancy
added a comment - 03/Nov/11 10:07 AM Hi Mark, Kim.
The most of your notes are already work-in-progress. Actually I don't know at which point you checked out code for testing.
I'll go through the list too and answer if I know about something more than David:
1. This is a good spotting, thanks.
5. I'm working on warnings at the moment, but 'Need review' flag in assignment grading list unfortunately is not likely to appear in 2.2
8. It should be possible to override grade and the rubric will probably become unavailable to student in this case. I'll work on the error message
10. This is strange. Student is able to view his evaluation everywhere - clicking on the assignment, following a link from gradebook, and now even the user Complete report (under activities reports) is fixed and it shows all the grades in the course correctly and with advanced grading feedback (filled rubric in this case). Can you please double check it
12. good point, added it to MDL-30023

Hi, just had a second thought on #1.
When you edit assignment settings you can select a scale. It does not have to be numerical 0-100 scale, it could be some custom scale.
If you have simple grading you have a dropdown with the available options. Those options may have text labels (when you use a scale) and also may look like numbers. So the result of rubric mapping is rounded to the available options (integers between 0 and 100 in the default case)

Marina Glancy
added a comment - 03/Nov/11 4:31 PM Hi, just had a second thought on #1.
When you edit assignment settings you can select a scale. It does not have to be numerical 0-100 scale, it could be some custom scale.
If you have simple grading you have a dropdown with the available options. Those options may have text labels (when you use a scale) and also may look like numbers. So the result of rubric mapping is rounded to the available options (integers between 0 and 100 in the default case)

I gave a bit of a sneak peak to our client community and Alan Arnold from University of Canberra has provided the following feedback:

1. The levels (columns) in the rubric need (optional?) headings. It would be common practice for staff to use scales like HD, DN, CR, PS, FL etc or Competent/NotYetCompetent etc - and these should be the headings
2. Having the ability to give a mark within a numerical ranges for each of these levels is also essential for many of our staff. These ranges could be displayed together with the level headings. This would need a new column for the marks for each criterion, auto-totalling at the bottom > this total to go to gradebook

Kim Edgar
added a comment - 04/Nov/11 10:28 AM Hi David and Marina
I gave a bit of a sneak peak to our client community and Alan Arnold from University of Canberra has provided the following feedback:
1. The levels (columns) in the rubric need (optional?) headings. It would be common practice for staff to use scales like HD, DN, CR, PS, FL etc or Competent/NotYetCompetent etc - and these should be the headings
2. Having the ability to give a mark within a numerical ranges for each of these levels is also essential for many of our staff. These ranges could be displayed together with the level headings. This would need a new column for the marks for each criterion, auto-totalling at the bottom > this total to go to gradebook
Thanks
Kim

Thanks for your great work so far on advanced grading and rubrics in particular. We're certainly looking forward to providing this new functionality to our teaching staff.

Here's a suggestion from a lecturer who uses Excel-based rubrics in her own practice. She collaboratively constructs the criteria and level descriptions with her students and tutors, to give them some ownership of the assessment process. She (and I'm sure others) would like the rubric creation process to be capability/permission-based. So, for example, if there was a "rubric-cell-edit" capability, this could be given to folks with a teacher role and not to students by default, but, if needed, the teacher would also have the abilty to change this permission for students via the usual permision-override - thus allowing them to collaboratively build the rubric. At some point the rubric-edit capability would need to revert to teacher-only. In our teacher's case, she simply (?)locks the appropriate Excel cells, then distributes the completed rubric to her tutors to use for grading.

I guess one could extend this suggestion so as to make every step of the rubric creation/grading process permission-based. That way, we don't unreasonably exclude the possiblity of new teaching practices.

Of course, this may the way you're thinking already, in which case - thanks again

Alan Arnold
added a comment - 04/Nov/11 11:44 AM - edited Dear David & Marina
Thanks for your great work so far on advanced grading and rubrics in particular. We're certainly looking forward to providing this new functionality to our teaching staff.
Here's a suggestion from a lecturer who uses Excel-based rubrics in her own practice. She collaboratively constructs the criteria and level descriptions with her students and tutors, to give them some ownership of the assessment process. She (and I'm sure others) would like the rubric creation process to be capability/permission-based. So, for example, if there was a "rubric-cell-edit" capability, this could be given to folks with a teacher role and not to students by default, but, if needed, the teacher would also have the abilty to change this permission for students via the usual permision-override - thus allowing them to collaboratively build the rubric. At some point the rubric-edit capability would need to revert to teacher-only. In our teacher's case, she simply (?)locks the appropriate Excel cells, then distributes the completed rubric to her tutors to use for grading.
I guess one could extend this suggestion so as to make every step of the rubric creation/grading process permission-based. That way, we don't unreasonably exclude the possiblity of new teaching practices.
Of course, this may the way you're thinking already, in which case - thanks again
Alan

1) Why not put the "headings" in the actual text for each cell?
2) What do you mean by ranges? If you mean that the teacher manually types a number between 0 and 20 (say) for each criterion then I think that is not a rubric, and would have to be developed separately as another type of advanced grading (fortunately this is easy to do now with our plugin structure but is not part of our scope for 2.2).

Alan,

We are not thinking of collaborative editing of rubrics inside Moodle yet. I'd like to know how you think locking/editing would actually work in detail, if you have multiple people editing the same rubric at the same time ... it sounds very complex to me. It might be easier to use an open Google Doc (or some other tool) to do this and then transfer the results into Moodle when done.

Martin Dougiamas
added a comment - 04/Nov/11 12:19 PM Kim,
1) Why not put the "headings" in the actual text for each cell?
2) What do you mean by ranges? If you mean that the teacher manually types a number between 0 and 20 (say) for each criterion then I think that is not a rubric, and would have to be developed separately as another type of advanced grading (fortunately this is easy to do now with our plugin structure but is not part of our scope for 2.2).
Alan,
We are not thinking of collaborative editing of rubrics inside Moodle yet. I'd like to know how you think locking/editing would actually work in detail, if you have multiple people editing the same rubric at the same time ... it sounds very complex to me. It might be easier to use an open Google Doc (or some other tool) to do this and then transfer the results into Moodle when done.

1) Columns heading are not an option at the moment as each level can have different number of levels.
2) I do not understand ranges in the context if rubric, too. Please can you provide a mockup and/or detailed description?

Re collaborative editor - I can see it might happen as a custom activity module actually. The product of the collaborative editing could be then saved as a rubric template. And yes, I like the idea of the ability to import spreadsheet structures (from Excel, Google Docs etc) as a rubric.

David Mudrák
added a comment - 04/Nov/11 3:41 PM I agree with martin
1) Columns heading are not an option at the moment as each level can have different number of levels.
2) I do not understand ranges in the context if rubric, too. Please can you provide a mockup and/or detailed description?
Re collaborative editor - I can see it might happen as a custom activity module actually. The product of the collaborative editing could be then saved as a rubric template. And yes, I like the idea of the ability to import spreadsheet structures (from Excel, Google Docs etc) as a rubric.

David Mudrák
added a comment - 09/Nov/11 7:12 AM For your information, the preview of the rubrics support in Moodle 2.2 is available for pre-testing at http://creeper.moodle.net/
Sorry there is no user documentation yet but that is on our todo list.

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.
12. lib/navigationlib.php > 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. grade/grading/form/lib.php
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?

Noted bugs:
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.

Summary:
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.

Sam Hemelryk
added a comment - 11/Nov/11 12:55 PM Hi guys,
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.
12. lib/navigationlib.php > 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. grade/grading/form/lib.php
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?
Noted bugs:
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.
Summary:
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.
Cheers
Sam

Just adding in here guys that I don't expect anyone [ever] to go through and tidy up everything I have noted.
As I was reading through everything several times to understand it I noted down every little trivial thing I spotted.
The reason for noting them down is that a. you know I read it and b. hopefully in the future there will be less popping up.

Of course things like adding phpdocs to things that don't have them should certainly happen and TODO's should certainly have MDL's so that we don't forget things... anyway I'll let you all be the judge of that!

Sam Hemelryk
added a comment - 11/Nov/11 1:13 PM Just adding in here guys that I don't expect anyone [ever] to go through and tidy up everything I have noted.
As I was reading through everything several times to understand it I noted down every little trivial thing I spotted.
The reason for noting them down is that a. you know I read it and b. hopefully in the future there will be less popping up.
Of course things like adding phpdocs to things that don't have them should certainly happen and TODO's should certainly have MDL's so that we don't forget things... anyway I'll let you all be the judge of that!

I'll comment on couple of your comments and explain why I did that way (and why I can't change it):

1. abuse of $_POST is my part and I truly did not find a better solution, and that's why

Unfortunately, assignment grading does not use moodleforms correctly. It creates an instance of moodleform just to display a form. It does not call any methods like is_cancelled, is_submitted, does not check validation and, the worst thing, does not get data from moodleform but takes it directly from $_POST
look at the function process_feedback() the base class https://github.com/moodle/moodle/blob/master/mod/assignment/lib.php
note: $feedback = data_submitted()
and even worse - the assignment modules override (mostly by copying) this function
if they don't find ['xgrade'], they will die
I can change of course the base class and our standard assignment types that misuse this, but I can't change 3rd party assignment types

(13:29:43) samhemelryk: Year thats cool aye, I was pretty certain that was the case (hehe talked with Eloy and Martin about it and they both figured that would be the reason as well)

And I will add comments into the code about it

2. Re: 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).

(13:37:18) Marina: The form would mark the whole rubriceditor element with class 'error' and make it all red(13:37:28) samhemelryk: Ewww yuck!
That is a good reason for it to handle it's own display!(13:38:02) Marina: and I want to highlight with red just the particular parts of it (13:38:08) samhemelryk: (and I actually remember reading that in dev chat at some point sorry I should have remembered that)
Indeed - is it a bug within mforms that is leading to that? I would think that only the element containing the error message should be coloured red(13:38:59) Marina: rubriceditor - is a single element from mform point of view
and if it was not single - I would not be able to display it nicely
because forms do not support renderers

3. Re: Renaming the database fields.
It's up to David, I would not mind renaming except for7.2 grading_instances.raterid => grading_instances.userid.
I think that userid will be very confusing because it sounds like the person being graded, which we don't have in this table because knowing itemid we can find the grade and the userid of the student being graded. And raterid is the id of the grader (teacher)

Marina Glancy
added a comment - 11/Nov/11 1:55 PM wow, Sam, that's a real REVIEW!
I'll comment on couple of your comments and explain why I did that way (and why I can't change it):
1. abuse of $_POST is my part and I truly did not find a better solution, and that's why
Unfortunately, assignment grading does not use moodleforms correctly. It creates an instance of moodleform just to display a form. It does not call any methods like is_cancelled, is_submitted, does not check validation and, the worst thing, does not get data from moodleform but takes it directly from $_POST
look at the function process_feedback() the base class
https://github.com/moodle/moodle/blob/master/mod/assignment/lib.php
note: $feedback = data_submitted()
and even worse - the assignment modules override (mostly by copying) this function
if they don't find ['xgrade'] , they will die
I can change of course the base class and our standard assignment types that misuse this, but I can't change 3rd party assignment types
(13:29:43) samhemelryk: Year thats cool aye, I was pretty certain that was the case (hehe talked with Eloy and Martin about it and they both figured that would be the reason as well)
And I will add comments into the code about it
2. Re: 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).
(13:37:18) Marina: The form would mark the whole rubriceditor element with class 'error' and make it all red
(13:37:28) samhemelryk: Ewww yuck!
That is a good reason for it to handle it's own display!
(13:38:02) Marina: and I want to highlight with red just the particular parts of it
(13:38:08) samhemelryk: (and I actually remember reading that in dev chat at some point sorry I should have remembered that)
Indeed - is it a bug within mforms that is leading to that? I would think that only the element containing the error message should be coloured red
(13:38:59) Marina: rubriceditor - is a single element from mform point of view
and if it was not single - I would not be able to display it nicely
because forms do not support renderers
3. Re: Renaming the database fields.
It's up to David, I would not mind renaming except for
7.2 grading_instances.raterid => grading_instances.userid.
I think that userid will be very confusing because it sounds like the person being graded, which we don't have in this table because knowing itemid we can find the grade and the userid of the student being graded. And raterid is the id of the grader (teacher)

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.

Marina Glancy
added a comment - 11/Nov/11 2:16 PM 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.
Sorry Sam, did not understand this.
First, it's documented in class description:
https://github.com/mudrd8mz/moodle/blob/rubric/lib/form/grading.php#L33
[code]
/**
HTML class for a grading element. This is a wrapper for advanced grading plugins.
When adding the 'grading' element to the form, developer must pass an object of
class gradingform_instance as $attributes ['gradinginstance'] . Otherwise an exception will be
thrown.
This object is responsible for implementing functions to render element html and validate it
[code]
Second, it is validated but not in the constructor, because in mform it is called twice - first time without arguments and the second time with the arguments. So if we want to validate the element arguments we need to do it in onQuickFormEvent what I do here:
https://github.com/mudrd8mz/moodle/blob/rubric/lib/form/grading.php#L119
What exactly you don't agree with?
PS Though I'll update phpdoc for function onQuickFormEvent

Sam, I guess my comments on your comments will be about the same length

Re 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.

The main idea of this function is to move smarter logic inside an advanced grading plugin. In the base class it is as simple as you described: if instanceid is specified => get, otherwise => create.
For rubric this function is overridden and it's not that simple any more. Even if instanceid is not specified, we try to find the last unfinished attempt of this rater to grade this item (if it was later than the last saved attempt). So even when instanceid is not specified we still do not create but rather get instance (sometimes).

This is not used at the moment because there is no AJAX saving but it is a part of the design.

Marina Glancy
added a comment - 11/Nov/11 2:31 PM Sam, I guess my comments on your comments will be about the same length
Re 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.
The main idea of this function is to move smarter logic inside an advanced grading plugin. In the base class it is as simple as you described: if instanceid is specified => get, otherwise => create.
For rubric this function is overridden and it's not that simple any more. Even if instanceid is not specified, we try to find the last unfinished attempt of this rater to grade this item (if it was later than the last saved attempt). So even when instanceid is not specified we still do not create but rather get instance (sometimes).
This is not used at the moment because there is no AJAX saving but it is a part of the design.

Marina can work on the comments/docs today and send pull request to David for his breakfast.

David to continue with anything else he feels he can do today on it. While I can see some small benefit in renaming the fields I don't think it's worth the cost of changing them. Up to you, David. Do what you can.

Eloy can integrate David's final version later today.

QA can start work on it starting Monday.

Next week we should meet and discuss what other things from Sam's list can be fixed before 2.2 release and work on those as new commits.

Martin Dougiamas
added a comment - 11/Nov/11 2:53 PM - edited Sam, very thorough review! Awesome!
After looking through this I think what we can do is:
Marina can work on the comments/docs today and send pull request to David for his breakfast.
David to continue with anything else he feels he can do today on it. While I can see some small benefit in renaming the fields I don't think it's worth the cost of changing them. Up to you, David. Do what you can.
Eloy can integrate David's final version later today.
QA can start work on it starting Monday.
Next week we should meet and discuss what other things from Sam's list can be fixed before 2.2 release and work on those as new commits.
OK?

Sam - I am very impressed with your review. Thanks a lot for the valuable feedback. I just committed some trivial changes and fixes (going to push them later today to github) and here are some quick notes:

Re 1 Yes, I am aware that some respected core developers enforced such rule. I was just on that side of the barricade that did not agree with that. So most probably I forgot to update my local template for new Moodle files.

Re 7 I can see the point of renaming 7.1, 7.3 and 7.4 and will try to do it. With regards to 7.2 (raterid to userid), the field was actually called userid in early DB scheme designs. It was then renamed from userid to raterid as userid was confusing and used to be misinterpretted as the user being graded.

Re 8 Well spotted! I discussed with Petr again and it turns out my assumptions on the module vs course level caps were not correct. Going to change the context level to CONTEXT_COURSE and put extra caps setting into the Assignment module

Re 10 Right. I just added more documentation for that factory function. The grading manager was designed as a handy access point to grading controllers. It is basically an object that "watches" or "is focused at" a given area and is able to invoke its current active grading controller, get the list of available grading methods etc. So it has status (the identification of the currently watched area) that can be changed - eg to iterate over a set of area in the same context by simply setting the area name. It is a pattern that was successfully used in the moodle1 converters subsystem.

There are two basic ways how to call get_grading_manager(). Either by passing $areaid if it is known (as that is all the function needs to know to load the area record) or providing context, component and areaname. The current implementation is sort of an attempt to mimic function overloading in PHP (http://en.wikipedia.org/wiki/Function_overloading). The explicit way would be to have two factory functions: get_grading_manager_by_areaid($areaid) and get_grading_manager_by_location($context, $component, $areaname).

Re 11 Fixed as a part of 10

Re 12 I disagree - the manager needs to know the name of the component in the given context. For example in Workshop, the activity's context is not enough if the areas are actually provided by the subplugins. So we will need to pass the activity's context and the name of the component (which can be both the module or any of its submodules). That is whay we actually have the component field here. One context can be occupied by multiple components.

Re 19 I believe that whenever we can do a job in one query, we must do it in a one query. There's no other option. It does not matter whether a rubric is small or not. Every single query matters. Please ellaborate what's wrong with the readablity here - I can't see any coding style violation here.

Re 23 Fixed, thanks

Re 24.1 I just followed the convention of these files from other /pix directories - see /pix/t/TINY-ICON, /pix/s/SMILEYS etc

Re 24.2 Sure, the origin of the files is recorded in the commit message. They come from the Tango Desktop Project and are released to Public Domain. I am very sensitive wrt licenses so I was careful not to violate anything.

Re 28 Marina, let me suggest to use CSS

{cursor: text;}

Re 32 I definitely agree. For me, the most important unit test would be those that actually check the grade calculation works as expected. Really, grades are critical part of any LMS (it's like money in e-banking systems) and we must pay extra attention to them. Not to talk about the fact that TDD would
lead to better APIs.

David Mudrák
added a comment - 11/Nov/11 7:24 PM Sam - I am very impressed with your review. Thanks a lot for the valuable feedback. I just committed some trivial changes and fixes (going to push them later today to github) and here are some quick notes:
Re 1 Yes, I am aware that some respected core developers enforced such rule. I was just on that side of the barricade that did not agree with that. So most probably I forgot to update my local template for new Moodle files.
Re 2 I stronkly disagree with this rule. phpDocs inheritance works here as usually so the file's @copyright and @licence applies (see http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_elements.pkg.html#class.inheritance )
Re 7 I can see the point of renaming 7.1, 7.3 and 7.4 and will try to do it. With regards to 7.2 (raterid to userid), the field was actually called userid in early DB scheme designs. It was then renamed from userid to raterid as userid was confusing and used to be misinterpretted as the user being graded.
Re 8 Well spotted! I discussed with Petr again and it turns out my assumptions on the module vs course level caps were not correct. Going to change the context level to CONTEXT_COURSE and put extra caps setting into the Assignment module
Re 10 Right. I just added more documentation for that factory function. The grading manager was designed as a handy access point to grading controllers. It is basically an object that "watches" or "is focused at" a given area and is able to invoke its current active grading controller, get the list of available grading methods etc. So it has status (the identification of the currently watched area) that can be changed - eg to iterate over a set of area in the same context by simply setting the area name. It is a pattern that was successfully used in the moodle1 converters subsystem.
There are two basic ways how to call get_grading_manager(). Either by passing $areaid if it is known (as that is all the function needs to know to load the area record) or providing context, component and areaname. The current implementation is sort of an attempt to mimic function overloading in PHP ( http://en.wikipedia.org/wiki/Function_overloading ). The explicit way would be to have two factory functions: get_grading_manager_by_areaid($areaid) and get_grading_manager_by_location($context, $component, $areaname).
Re 11 Fixed as a part of 10
Re 12 I disagree - the manager needs to know the name of the component in the given context. For example in Workshop, the activity's context is not enough if the areas are actually provided by the subplugins. So we will need to pass the activity's context and the name of the component (which can be both the module or any of its submodules). That is whay we actually have the component field here. One context can be occupied by multiple components.
Re 19 I believe that whenever we can do a job in one query, we must do it in a one query. There's no other option. It does not matter whether a rubric is small or not. Every single query matters. Please ellaborate what's wrong with the readablity here - I can't see any coding style violation here.
Re 23 Fixed, thanks
Re 24.1 I just followed the convention of these files from other /pix directories - see /pix/t/TINY-ICON, /pix/s/SMILEYS etc
Re 24.2 Sure, the origin of the files is recorded in the commit message. They come from the Tango Desktop Project and are released to Public Domain. I am very sensitive wrt licenses so I was careful not to violate anything.
Re 28 Marina, let me suggest to use CSS
{cursor: text;}
Re 32 I definitely agree. For me, the most important unit test would be those that actually check the grade calculation works as expected. Really, grades are critical part of any LMS (it's like money in e-banking systems) and we must pay extra attention to them. Not to talk about the fact that TDD would
lead to better APIs.

Re 8 (revised) Actually, Sam's comment confused me as originally the scope of the capability was CONTEXT_MODULE and then it was changed to CONTEXT_COURSE. I re-checked with Petr and he confirmed the change from module scope to the course scope was correct. So the capability behaves similarly to accessallgroups. It's scope is set to CONTEXT_COURSE in lib/db/access.php and plugins (mods and blocks) that support it declare so via xxx_get_extra_capabilities().

David Mudrák
added a comment - 11/Nov/11 9:05 PM Re 8 (revised) Actually, Sam's comment confused me as originally the scope of the capability was CONTEXT_MODULE and then it was changed to CONTEXT_COURSE. I re-checked with Petr and he confirmed the change from module scope to the course scope was correct. So the capability behaves similarly to accessallgroups. It's scope is set to CONTEXT_COURSE in lib/db/access.php and plugins (mods and blocks) that support it declare so via xxx_get_extra_capabilities().

The capability to manage grading forms can be overridden at the Assignment module level (thanks skodak for making this clearer to me)

All associated grading areas are now deleted when the context is being deleted (thanks skodak for rising this)

DB fields formid renamed to definitionid (re 7.1 and 7.3)

DB field forminstanceid renamed to instanceid (re 7.4)

Please note the changes in the DB scheme were done without steps in upgrade.php. If you have a local installation based on the rubric branch, you need to reinstall it now (sorry for that but I did it for obvious reasons, there is no need to have a rubbish in upgrade.php before the release.

I tested installing a new site from master, switching to rubric, upgrade and basic QA workflow for rubrics (see testing instructions). It works for me. I will try to be online tonight to be able to assist Sam, should he decide to merge the branch today.

David Mudrák
added a comment - 13/Nov/11 9:37 PM The rubric branch updated to reflect some issues spotted during the integration
OK, so I was working on some issues spotted by Sam and some others during the weekend. Here is a quick summary of improvements together with the eventual reference to the original Sam's comment.
Removed global variables where they are not needed (re 17.1 and 17.3)
Fixed the typo in the lang pack (re 23)
Improved get_grading_method() documentation (re 10)
Fixed grading method selector display - hide it for non-privileged users (spotted by myself during a test)
The capability to manage grading forms can be overridden at the Assignment module level (thanks skodak for making this clearer to me)
All associated grading areas are now deleted when the context is being deleted (thanks skodak for rising this)
DB fields formid renamed to definitionid (re 7.1 and 7.3)
DB field forminstanceid renamed to instanceid (re 7.4)
Please note the changes in the DB scheme were done without steps in upgrade.php. If you have a local installation based on the rubric branch, you need to reinstall it now (sorry for that but I did it for obvious reasons, there is no need to have a rubbish in upgrade.php before the release.
I tested installing a new site from master, switching to rubric, upgrade and basic QA workflow for rubrics (see testing instructions). It works for me. I will try to be online tonight to be able to assist Sam, should he decide to merge the branch today.

Sam Hemelryk
added a comment - 14/Nov/11 5:08 PM Thank you for flying Air Moodle... you have now arrived at your destination and we hope you enjoyed your journey.
.... thats right folks its in!!!
Hoorah and great job David + Marina

Helen Foster
added a comment - 16/Nov/11 11:48 PM Removing docs_required label as David has written some great docs: http://docs.moodle.org/en/Advanced_grading_methods and http://docs.moodle.org/en/Rubrics - thanks David
Also removing qa_test_required label as we now have QA tests covering all the points mentioned in the testing instructions.
In case anyone wishes to add themselves as a watcher to the QA tests, here are links to the advanced grading methods tests in the 2.2 QA cycle:
MDLQA-1422
MDLQA-1423
MDLQA-1424
MDLQA-1425
Feedback on the QA tests is always welcome!