Skip to content

CTP-4639 grade decimal validation #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: CTP-4710---use-mustache-templates
Choose a base branch
from
Open

CTP-4639 grade decimal validation #62

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
339485a
CTP-4639 grade decimal validation
watson8 Aug 14, 2025
17e7385
CTP-4639 grade decimal behat test
watson8 Aug 15, 2025
084a58f
CTP-4639 limit grade validation
watson8 Aug 15, 2025
a636f06
CTP-4639 fix form & behat, remove ajax
watson8 Aug 19, 2025
04389b7
CTP-4639 simplify & improve renderer
watson8 Aug 19, 2025
3b24730
CTP-4639 remove redundant modal code
watson8 Aug 19, 2025
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
6 changes: 5 additions & 1 deletion actions/feedbacks/update.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@
*/

require_once(dirname(__FILE__) . '/../../../../config.php');
require_login();
require_sesskey();

global $CFG, $USER;
global $CFG, $USER, $PAGE;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this declaration, $CFG, $USER and $PAGE are all in scope.


$feedbackid = required_param('feedbackid', PARAM_INT);
$finalised = (bool)optional_param('submitbutton', 0, PARAM_TEXT);
Expand All @@ -46,5 +48,7 @@
$params['cell_type'] = required_param('cell_type', PARAM_TEXT);
}

$PAGE->set_url(new moodle_url('/mod/coursework/actions/feedbacks/update.php', $params));

$controller = new mod_coursework\controllers\feedback_controller($params);
$controller->update_feedback();
31 changes: 21 additions & 10 deletions classes/controllers/feedback_controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ protected function create_feedback() {
'code' => 'guidenotcompleted',
]);
} else {
\core\notification::error(get_string('guidenotcompleted', 'gradingform_guide'));
$renderer = $this->get_page_renderer();
$renderer->new_feedback_page($teacherfeedback);
}
Expand All @@ -331,7 +332,7 @@ protected function create_feedback() {
}

/**
* Saves the new feedback form for the first time.
* Saves feedback form when updated from /actions/feedbacks/update.php.
*/
protected function update_feedback() {

Expand Down Expand Up @@ -372,9 +373,11 @@ protected function update_feedback() {

} else {
$teacherfeedback->destroy();
// Clear cache.
\mod_coursework\models\feedback::remove_cache($this->submission->courseworkid);
\mod_coursework\models\submission::remove_cache($this->submission->courseworkid);
if ($this->submission) {
// Clear cache.
\mod_coursework\models\feedback::remove_cache($this->submission->courseworkid);
\mod_coursework\models\submission::remove_cache($this->submission->courseworkid);
}

// Remove associated files
$fs = get_file_storage();
Expand Down Expand Up @@ -424,12 +427,20 @@ protected function update_feedback() {
if ($form->is_cancelled()) {
redirect($courseworkpageurl);
}

$teacherfeedback = $form->process_data($teacherfeedback);

$teacherfeedback->save();
$form->save_feedback_files($teacherfeedback);

$formdata = $form->get_data();
$hasadvancedgrading = !empty($formdata->advancedgrading);
if ($hasadvancedgrading || (isset($formdata->grade) && $form->grade_in_range($formdata->grade))) {
$teacherfeedback = $form->process_data($teacherfeedback);
$teacherfeedback->save();
$form->save_feedback_files($teacherfeedback);
} else {
\core\notification::error(get_string('pleasecorrecterrors', 'mod_coursework'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to show Please correct errors and re-submit but doesn't give any indication exactly what the errors are and where they are.

Maybe given mod_coursework's idiosyncratic form handling it's enough to stop a user saving an out-of-range number, but some indication of this (e.g. Grade must be between 0 and 100) would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed it would be better to give more information but I wanted to avoid introducing more complexity. As it's only a single field I thought we could get away with this

global $OUTPUT;
echo $OUTPUT->header();
$form->display();
echo $OUTPUT->footer();
die();
}
$gradeeditingtime = $teacherfeedback->get_coursework()->get_grade_editing_time();
if (empty($gradeeditingtime) || time() > $teacherfeedback->timecreated + $gradeeditingtime) {
$this->try_auto_feedback_creation($teacherfeedback->get_submission());
Expand Down
33 changes: 32 additions & 1 deletion classes/forms/assessor_feedback_mform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ public function definition() {
} else if ($feedback->stage_identifier == 'final_agreed_1') {
$mform->addElement('text', 'grade', get_string('grade', 'mod_coursework'));
$mform->setType('grade', PARAM_RAW);
$mform->addRule('grade', get_string('error'), 'numeric', null, 'client');
} else {
$mform->addElement('select',
'grade',
Expand Down Expand Up @@ -181,7 +182,6 @@ public function validate_grade($data) {
* @return feedback
*/
public function process_data(feedback $feedback) {

$formdata = $this->get_data();
$coursework = $feedback->get_coursework();

Expand Down Expand Up @@ -259,5 +259,36 @@ public function get_editor_options() {
$options = $editor->get_options();
return $options;
}

/**
* If there are errors return array of errors ("fieldname" => "error message").
*
* Server side rules do not work for uploaded files, implement serverside rules here if needed.
*
* @param array $data array of ("fieldname"=>value) of submitted data
* @param array $files array of uploaded files "element_name"=>tmp_file_path
* @return array of "element_name"=>"error_description" if there are errors,
* or an empty array if everything is OK (true allowed for backwards compatibility too).
*/
public function validation($data, $files) {
$errors = parent::validation($data, $files);
$hasadvancedgrading = !empty($data['advancedgrading']);
if (!$hasadvancedgrading && isset($data['stage_identifier']) && $data['stage_identifier'] == 'final_agreed_1') {
if (!$this->grade_in_range($data['grade'])) {
$errors['grade'] = get_string('err_numeric', 'form');
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see this message ever gets shown to the user? Also, this message, You must enter a number here., is returned (but not shown) if you enter 12345 (i.e. a number), so I'm not sure this is the best message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it isn't shown, as the rest of the code is not handling the form in the standard moodle way. However it does stop the form being submitted, which was my main reason for including it, and then the core\notification::error() call notifies the user (not ideal but saves lots of refactoring)

}
}
return $errors;
}

/**
* Agreed grade can be entered as text field (float or int) so need to validate it.
*/
public function grade_in_range(string $grade): bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the coursework instance has Grading method = "Rubric" then when the manager attempts to save the agreed grade they get:

Exception - mod_coursework\forms\assessor_feedback_mform::grade_in_range(): Argument #1 ($grade) must be of type string, null given, called in [dirroot]/mod/coursework/classes/forms/assessor_feedback_mform.php on line 276

[More information about this error](https://docs.moodle.org/405/en/error/moodle/generalexceptionmessage)

Debug info:
Error code: generalexceptionmessage×Dismiss this notification
Stack trace:
line 286 of /mod/coursework/classes/forms/assessor_feedback_mform.php: TypeError thrown
line 276 of /mod/coursework/classes/forms/assessor_feedback_mform.php: call to mod_coursework\forms\assessor_feedback_mform->grade_in_range()
line 674 of /lib/formslib.php: call to mod_coursework\forms\assessor_feedback_mform->validation()
line 610 of /lib/formslib.php: call to moodleform->validate_defined_fields()
line 720 of /lib/formslib.php: call to moodleform->is_validated()
line 224 of /mod/coursework/classes/controllers/feedback_controller.php: call to moodleform->get_data()
line ? of unknownfile: call to mod_coursework\controllers\feedback_controller->create_feedback()
line 175 of /mod/coursework/classes/controllers/controller_base.php: call to call_user_func()
line 52 of /mod/coursework/actions/feedbacks/create.php: call to mod_coursework\controllers\controller_base->__call()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I addressed this in commit 084a58f

$feedback = $this->_customdata['feedback'];
$coursework = $feedback->get_coursework();
$gradeoptions = array_keys(make_grades_menu($coursework->grade));
return is_numeric($grade) && $grade >= min($gradeoptions) && $grade <= max($gradeoptions);
}
}

1 change: 1 addition & 0 deletions lang/en/coursework.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@
$string['initialmarkingdeadline_help'] = 'Set the date that initial grading should be completed by';
$string['item'] = 'Item';
$string['itsyou'] = '(you)';
$string['pleasecorrecterrors'] = 'Please correct errors and re-submit';
$string['lastedited'] = 'Last edited by {$a->name} on {$a->time}';
$string['lasteditedby'] = 'Last edited by';
$string['lastmoderatedby'] = 'Last moderated by';
Expand Down
18 changes: 18 additions & 0 deletions tests/behat/grade_decimals.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,21 @@ Feature: For the final grade the mark should be to the decimal point
And I click the new multiple final feedback button for the student
And I grade the submission as 56.12 using the ajax form
Then I should see the final grade as 56.12 on the multiple marker page

Scenario: If I enter non-numeric characters for the grade I should see an error
Given there are feedbacks from both teachers
And I log in as a manager
And I visit the coursework page
And I click on "Add agreed feedback" "link"
And I set the field "Grade" to "abc"
And I wait "1" seconds
Then I should see "- Error"

Scenario: If I enter a grade which is out of range I should see an error
Given there are feedbacks from both teachers
And I log in as a manager
And I visit the coursework page
And I click on "Add agreed feedback" "link"
And I set the field "Grade" to "1234"
And I press "Save and finalise"
Then I should see "Please correct errors and re-submit"
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit 084a58f changed this error message so this test fails currently:

001 Scenario: If I enter a grade which is out of range I should see an error # /var/www/moodle/mod/coursework/tests/behat/grade_decimals.feature:57
      Then I should see "Please correct errors and re-submit"                # /var/www/moodle/mod/coursework/tests/behat/grade_decimals.feature:64
        "Please correct errors and re-submit" text was not found in the page (Behat\Mink\Exception\ExpectationException)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great I fixed this in commit a636f06. Also fixed some other issues in the test and added some more checks so it is pretty thorough now. Also figured out why the form was not displaying validation messages as it should and fixed that. Finally removed some of the now redundant "ajax" code. Note the change to styles.css was tiny though the diff here is showing up some formatting changes only (tabs)

Loading