Conversation
- Final assessor sees initial assessors selected in the criterion and grade - Supports range rubric grading method.
c7a5378 to
25e142b
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces automatic population of ranged rubric grades during the final marking stage by averaging grades from initial assessors, allowing the final assessor to review and adjust criteria before submission.
- Adds database query to calculate average grades and levels from initial assessors' rubric selections
- Implements JavaScript module to pre-populate the final stage grading form with averaged values
- Conditionally applies auto-population only when using ranged rubrics with manual agreement strategy
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| classes/models/coursework.php | Adds method to retrieve averaged rubric grades and helper to detect ranged rubric usage |
| classes/controllers/feedback_controller.php | Integrates auto-population logic into final stage feedback workflow |
| amd/src/rubric_ranges.js | Implements client-side form population using averaged grade data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LEFT JOIN {grading_instances} gi ON cf.id = gi.itemid | ||
| LEFT JOIN {gradingform_rubric_ranges_f} rr ON gi.id = rr.instanceid | ||
| WHERE cf.submissionid = :submissionid AND cf.stageidentifier NOT LIKE 'final_agreed_1' AND cf.finalised = 1 AND gi.status = 1 | ||
| GROUP BY rr.criterionid ORDER BY rr.criterionid "; |
There was a problem hiding this comment.
Double space in ORDER BY clause. Remove the extra space between 'ORDER' and 'BY' for consistency with SQL formatting standards.
| GROUP BY rr.criterionid ORDER BY rr.criterionid "; | |
| GROUP BY rr.criterionid ORDER BY rr.criterionid "; |
| /** | ||
| * Modify ranged rubric grading form. | ||
| * | ||
| * @author Sumaiya Javed <sumaiya.javed@catalyst.net.nz |
There was a problem hiding this comment.
Missing closing angle bracket in email address. The author tag should end with '>' after the email address.
| * @author Sumaiya Javed <sumaiya.javed@catalyst.net.nz | |
| * @author Sumaiya Javed <sumaiya.javed@catalyst.net.nz> |
|
|
||
| /** | ||
| * @return grading_manager | ||
| * @return bool|gradingform_controller|null |
There was a problem hiding this comment.
The @return annotation specifies 'bool|gradingform_controller|null' but the function actually returns either an array of database records or null. Update the annotation to '@return array|false' to match the actual return type from get_records_sql().
| * @return bool|gradingform_controller|null | |
| * @return array|false|null |
|
|
||
| // Autopopulate average grade from initial assessors. | ||
| if ($coursework && $coursework->is_using_ranged_rubric() && $teacherfeedback->stageidentifier == 'final_agreed_1') { | ||
| if (str_contains($coursework->automaticagreementstrategy, 'none')) { |
There was a problem hiding this comment.
Using str_contains() with 'none' as a substring match is fragile and could match unintended values like 'nonesuch'. Consider using strict equality comparison (=== 'none') or checking against a defined constant if the strategy field contains exactly 'none'.
| if (str_contains($coursework->automaticagreementstrategy, 'none')) { | |
| if ($coursework->automaticagreementstrategy === 'none') { |
leonstr
left a comment
There was a problem hiding this comment.
I think this looks broadly OK, the only serious issue is whether we need the if in get_advanced_grading_average_grade_range_rubric() and if we do surely some default return value is needed (false or null?).
We probably also need to consider if this new behaviour should be enabled by default. Hopefully my colleagues will be able to give some input into this decision.
| FROM {coursework_feedbacks} cf | ||
| LEFT JOIN {grading_instances} gi ON cf.id = gi.itemid | ||
| LEFT JOIN {gradingform_rubric_ranges_f} rr ON gi.id = rr.instanceid | ||
| WHERE cf.submissionid = :submissionid AND cf.stageidentifier NOT LIKE 'final_agreed_1' AND cf.finalised = 1 AND gi.status = 1 |
There was a problem hiding this comment.
This uses NOT LIKE but the compared value doesn't contain a wildcard. Presumably this should be:
WHERE cf.submissionid = :submissionid AND cf.stageidentifier <> 'final_agreed_1' AND cf.finalised = 1 AND gi.status = 1
| // Autopopulate average grade from initial assessors. | ||
| if ($coursework && $coursework->is_using_ranged_rubric() && $teacherfeedback->stageidentifier == 'final_agreed_1') { | ||
| if (str_contains($coursework->automaticagreementstrategy, 'none')) { | ||
| $gdata = []; |
There was a problem hiding this comment.
I don't think this is needed. Why bother initialising $gdata when it's assigned on the next line?
| if ($coursework && $coursework->is_using_ranged_rubric() && $teacherfeedback->stageidentifier == 'final_agreed_1') { | ||
| if (str_contains($coursework->automaticagreementstrategy, 'none')) { |
There was a problem hiding this comment.
Rather than:
if (...) {
if (...) {
⋮
}
}
why not have a single if condition:
if ($coursework && $coursework->is_using_ranged_rubric()
&& $teacherfeedback->stageidentifier == 'final_agreed_1'
&& $coursework->automaticagreementstrategy === 'none') {
) {
| */ | ||
| public function get_advanced_grading_average_grade_range_rubric($submissionid) { | ||
| global $DB; | ||
| if ($submissionid) { |
There was a problem hiding this comment.
Do we need to check $submissionid here ? Could this actually be a falsy value? (Maybe it could but from a quick skim of the code I couldn't see how).
| GROUP BY rr.criterionid ORDER BY rr.criterionid "; | ||
| return $DB->get_records_sql($sql, ['submissionid' => $submissionid]); | ||
| } | ||
| } |
There was a problem hiding this comment.
If if ($submissionid) {} is false then nothing is returned, won't that break the caller? Should we have return false here?
| $urlparams['stageidentifier'] = $teacherfeedback->stageidentifier; | ||
| $PAGE->set_url('/mod/coursework/actions/feedbacks/new.php', $urlparams); | ||
|
|
||
| // Autopopulate average grade from initial assessors. |
There was a problem hiding this comment.
Given this relates to non-core functionality I think this comment should explicitly mention the plugin in this comment, for example:
// If using ranged rubric (gradingform_rubric_ranges) auto-populate average grade from initial assessors.
| } | ||
|
|
||
| /** | ||
| * Check if ranged rubric is used in the current coursework. |
There was a problem hiding this comment.
Again, since this relates to non-core functionality I think this comment should explicitly mention the plugin. For example:
* Check if ranged rubric (gradingform_rubric_ranges) is used in the
* current coursework.
| * Modify ranged rubric grading form. | ||
| * | ||
| * @author Sumaiya Javed <sumaiya.javed@catalyst.net.nz | ||
| * @copyright Catalyst IT |
There was a problem hiding this comment.
Missing year, presumably this should be:
* @copyright 2025 Catalyst IT
Summary
Introduce automatic averaging of ranged rubric grades at the final marking stage, with the ability for the final assessor to review and adjust individual rubric criteria before submission.
Use Case
When multiple assessors independently mark a submission using a Ranged Rubric, the final assessor should not need to re-enter grades from scratch. Instead, the system should pre-populate the final stage with the averaged results from earlier markers.
Configuration Requirements
mod_coursework activity settings
Marking workflow
Grade
Workflow
Expected Behaviour
Login as the final assessor and see that the average criteria and grade from the previous two selections has been selected for the final assessor to change and submit
Each rubric criterion in the final stage is pre-selected using the average value of the prior assessors' selections.
The final assessor retains full control to override any averaged value.
No automatic agreement is enforced; the averaged values act as a starting point only.

Benefits