CTP-4639 grade decimal validation #62

watson8 · 2025-08-14T10:05:57Z

No description provided.

leonstr

It would be nice to have some automated test coverage for this issue. For example, if we added the following to grade_decimals.feature it fails without this fix, but passes with this fix (at the time of writing):

  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"

leonstr · 2025-08-15T10:30:32Z

classes/forms/assessor_feedback_mform.php

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


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

Thanks I addressed this in commit 084a58f

leonstr · 2025-08-15T10:37:12Z

classes/forms/assessor_feedback_mform.php

+        $errors = parent::validation($data, $files);
+        if (isset($data['stage_identifier']) && $data['stage_identifier'] == 'final_agreed_1') {
+            if (!$this->grade_in_range($data['grade'])) {
+                $errors['grade'] = get_string('err_numeric', 'form');


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.

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)

leonstr · 2025-08-15T10:39:04Z

classes/controllers/feedback_controller.php

+            $teacherfeedback->save();
+            $form->save_feedback_files($teacherfeedback);
+        } else {
+            \core\notification::error(get_string('pleasecorrecterrors', 'mod_coursework'));


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.

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

watson8 · 2025-08-15T12:57:44Z

It would be nice to have some automated test coverage for this issue. For example, if we added the following to grade_decimals.feature it fails without this fix, but passes with this fix (at the time of writing):

  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"

Thanks I added this in commit 17e7385

watson8 · 2025-08-15T13:30:12Z

Comments addressed above

leonstr · 2025-08-15T15:21:21Z

tests/behat/grade_decimals.feature

+    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"


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)

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)

leonstr · 2025-08-26T11:19:43Z

styles.css

+/* Hide activity header - date and description displayed differently. */
+.activity-header {
+    display: none !important;
+}


Rebasing this on CTP-4710---use-mustache-templates has a merge conflict with this file. As far as I can see the current styles.css in branch CTP-4639-grade-form-validation has been out-dented from the one these commits are based on. In 6d580cc (which CTP-4639-grade-form-validation is based on) this file starts:

5 .path-mod-coursework { 6 /* Hide activity header - date and description displayed differently. */ 7 .activity-header { 8 display: none !important; 9 }

The indenting here looks correct to me. Whereas in a636f06 we have:

5 .path-mod-coursework { 6 /* Hide activity header - date and description displayed differently. */ 7 .activity-header { 8 display: none !important; 9 }

which looks like the indenting has been inadvertently removed?

To fix this I think we just need to start from the indented styles.css from 6d580cc and add the #id_removefeedbackbutton {} block?

(In CTP-4710---use-mustache-templates I've added a .coursework-feedback {} block, and removed the hopefully-redundant -.path-mod-foo {} block.

leonstr · 2025-08-26T11:27:15Z

actions/feedbacks/update.php


-global $CFG, $USER;
+global $CFG, $USER, $PAGE;


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

CTP-4639 grade decimal validation

339485a

watson8 requested a review from leonstr August 14, 2025 10:05

leonstr requested changes Aug 15, 2025

View reviewed changes

CTP-4639 grade decimal behat test

17e7385

CTP-4639 limit grade validation

084a58f

leonstr requested changes Aug 15, 2025

View reviewed changes

watson8 added 3 commits August 19, 2025 12:58

CTP-4639 fix form & behat, remove ajax

a636f06

CTP-4639 simplify & improve renderer

04389b7

CTP-4639 remove redundant modal code

3b24730

leonstr reviewed Aug 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CTP-4639 grade decimal validation #62

CTP-4639 grade decimal validation #62

Uh oh!

watson8 commented Aug 14, 2025

Uh oh!

leonstr left a comment

Uh oh!

leonstr Aug 15, 2025

Uh oh!

watson8 Aug 15, 2025

Uh oh!

leonstr Aug 15, 2025

Uh oh!

watson8 Aug 15, 2025

Uh oh!

leonstr Aug 15, 2025

Uh oh!

watson8 Aug 15, 2025

Uh oh!

watson8 commented Aug 15, 2025

Uh oh!

watson8 commented Aug 15, 2025

Uh oh!

leonstr Aug 15, 2025

Uh oh!

watson8 Aug 19, 2025

Uh oh!

leonstr Aug 26, 2025

Uh oh!

leonstr Aug 26, 2025

Uh oh!

Uh oh!

CTP-4639 grade decimal validation #62

Are you sure you want to change the base?

CTP-4639 grade decimal validation #62

Uh oh!

Conversation

watson8 commented Aug 14, 2025

Uh oh!

leonstr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watson8 commented Aug 15, 2025

Uh oh!

watson8 commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!