Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions amd/build/rubric_ranges.min.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions amd/build/rubric_ranges.min.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 47 additions & 0 deletions amd/src/rubric_ranges.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

/**
* Modify ranged rubric grading form.
*
* @author Sumaiya Javed <sumaiya.javed@catalyst.net.nz
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing closing angle bracket in email address. The author tag should end with '>' after the email address.

Suggested change
* @author Sumaiya Javed <sumaiya.javed@catalyst.net.nz
* @author Sumaiya Javed <sumaiya.javed@catalyst.net.nz>

Copilot uses AI. Check for mistakes.
* @copyright Catalyst IT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing year, presumably this should be:

 * @copyright 2025 Catalyst IT

* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/

define([], function() {
return {
init: function(gdata) {
if (gdata) {
const rows = Object.values(gdata);
rows.forEach(function(row) {
const elementName = "advancedgrading-criteria-" + row.criterionid;
const elementLevel = document.getElementById(elementName + "-levels-" + row.avglevel);
if (elementLevel) {
elementLevel.classList.add('checked');
}
const elementDefinition = document.getElementById(elementName + "-levels-" + row.avglevel + "-definition");
if (elementDefinition) {
elementDefinition.checked = true;
}
const elementGrade = document.getElementById(elementName + "-grade");
if (elementGrade) {
elementGrade.value = row.avggrade;
}
});
}
}
};
});
9 changes: 9 additions & 0 deletions classes/controllers/feedback_controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,15 @@ protected function new_feedback() {
$urlparams['stageidentifier'] = $teacherfeedback->stageidentifier;
$PAGE->set_url('/mod/coursework/actions/feedbacks/new.php', $urlparams);

// Autopopulate average grade from initial assessors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ($coursework && $coursework->is_using_ranged_rubric() && $teacherfeedback->stageidentifier == 'final_agreed_1') {
if (str_contains($coursework->automaticagreementstrategy, 'none')) {
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
if (str_contains($coursework->automaticagreementstrategy, 'none')) {
if ($coursework->automaticagreementstrategy === 'none') {

Copilot uses AI. Check for mistakes.
Comment on lines +140 to +141
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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') {
        ) {

$gdata = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed. Why bother initialising $gdata when it's assigned on the next line?

$gdata = $coursework->get_advanced_grading_average_grade_range_rubric($teacherfeedback->get_submission()->id);
$PAGE->requires->js_call_amd('mod_coursework/rubric_ranges', 'init', [$gdata]);
}
}

// auto-populate Agreed Feedback with comments from initial marking
if ($coursework && $coursework->autopopulatefeedbackcomment_enabled() && $teacherfeedback->stageidentifier == 'final_agreed_1') {
// get all initial stages feedbacks for this submission
Expand Down
27 changes: 26 additions & 1 deletion classes/models/coursework.php
Original file line number Diff line number Diff line change
Expand Up @@ -2119,7 +2119,23 @@ public function get_advanced_grading_active_controller() {
}

/**
* @return grading_manager
* @return bool|gradingform_controller|null
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Suggested change
* @return bool|gradingform_controller|null
* @return array|false|null

Copilot uses AI. Check for mistakes.
*/
public function get_advanced_grading_average_grade_range_rubric($submissionid) {
global $DB;
if ($submissionid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$sql = "SELECT rr.criterionid, ROUND(AVG(rr.levelid)) as avglevel, ROUND(AVG(rr.grade)) as avggrade
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

GROUP BY rr.criterionid ORDER BY rr.criterionid ";
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double space in ORDER BY clause. Remove the extra space between 'ORDER' and 'BY' for consistency with SQL formatting standards.

Suggested change
GROUP BY rr.criterionid ORDER BY rr.criterionid ";
GROUP BY rr.criterionid ORDER BY rr.criterionid ";

Copilot uses AI. Check for mistakes.
return $DB->get_records_sql($sql, ['submissionid' => $submissionid]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If if ($submissionid) {} is false then nothing is returned, won't that break the caller? Should we have return false here?


/**
* @return \grading_manager
*/
protected function get_advanced_grading_manager() {
return get_grading_manager($this->get_context(), 'mod_coursework', 'submissions');
Expand All @@ -2143,6 +2159,15 @@ public function is_using_marking_guide(): bool {
return self::get_advanced_grading_method() === 'guide';
}

/**
* Check if ranged rubric is used in the current coursework.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*
* @return bool
*/
public function is_using_ranged_rubric(): bool {
return self::get_advanced_grading_method() === 'rubric_ranges';
}

/**
* Get advanced grading method used in the current coursework.
*
Expand Down