MDL-51090: mod_quiz grading validation of an essay question

An invalid format is casted to 0 (if a string) or to some truncated value in other cases (ex: 10..5).

lang/en/question.php

@@ -195,6 +195,7 @@
 $string['linkedfiledoesntexist'] = 'Linked file {$a} doesn\'t exist';
 $string['makechildof'] = 'Make child of \'{$a}\'';
 $string['maketoplevelitem'] = 'Move to top level';
+$string['manualgradeinvalidformat'] = 'That is not a valid number.';
 $string['matchgrades'] = 'Match grades';
 $string['matchgradeserror'] = 'Error if grade not listed';
 $string['matchgradesnearest'] = 'Nearest grade if not listed';

mod/quiz/tests/behat/validate_manual_mark_correct_format.feature

@@ -0,0 +1,59 @@
+@mod @mod_quiz
+Feature: In order to mannually mark a question I want
+  As a teacher
+  I must be able to set the marks I want on the Rapport question page.
+
+  Background:
+    Given the following "users" exist:
+      | username | firstname | lastname | email                |
+      | teacher1 | Teacher   | 1        | teacher1@example.com |
+      | student1 | Student   | 1        | student0@example.com |
+    And the following "courses" exist:
+      | fullname | shortname | category |
+      | Course 1 | C1        | 0        |
+    And the following "course enrolments" exist:
+      | user     | course | role           |
+      | teacher1 | C1     | editingteacher |
+      | student1 | C1     | student        |
+    And the following "question categories" exist:
+      | contextlevel | reference | name           |
+      | Course       | C1        | Test questions |
+    Given the following "questions" exist:
+      | questioncategory | qtype       | name  | questiontext    | defaultmark |
+      | Test questions   | essay       | TF1   | First question  | 20          |
+    And the following "activities" exist:
+      | activity   | name   | intro              | course | idnumber | grade |
+      | quiz       | Quiz 1 | Quiz 1 description | C1     | quiz1    | 20    |
+    And quiz "Quiz 1" contains the following questions:
+      | question | page | requireprevious |
+      | TF1      | 1    | 0               |
+    And I log in as "student1"
+    And I follow "Course 1"
+    And I follow "Quiz 1"
+    And I press "Attempt quiz now"
+    And I follow "Finish attempt ..."
+    And I press "Submit all and finish"
+    # Pop-up confirmation
+    And I click on "Submit all and finish" "button" in the "Confirmation" "dialogue"
+    And I log out
+    And I log in as "teacher1"
+    And I follow "Course 1"
+    And I follow "Quiz 1"
+    And I follow "Attempts: 1"
+    And I follow "Review attempt"
+
+  @javascript
+  Scenario: Validating the marking of an essay question attempt.
+    When I follow "Make comment or override mark"
+    And I switch to "commentquestion" window
+    And I set the field "Mark" to "25"
+    And I press "Save"
+    Then I should see "This grade is outside the valid range."
+    And I set the field "Mark" to "aa"
+    And I press "Save"
+    Then I should see "That is not a valid number."
+    And I set the field "Mark" to "10"
+    And I press "Save"
+    Then I should see "Changes saved"
+
+

question/behaviour/rendererbase.php

@@ -68,6 +68,8 @@ public function feedback(question_attempt $qa, question_display_options $options
         return '';
     }
 
+
+
     public function manual_comment_fields(question_attempt $qa, question_display_options $options) {
         $inputname = $qa->get_behaviour_field_name('comment');
         $id = $inputname . '_id';
@@ -124,10 +126,15 @@ public function manual_comment_fields(question_attempt $qa, question_display_opt
                 'name' => $markfield,
                 'id'=> $markfield
             );
-            if (!is_null($currentmark)) {
+            if (!is_null($currentmark) && $qa->manual_mark_format_is_ok() ) {
                 $attributes['value'] = $qa->format_fraction_as_mark(
                         $currentmark / $maxmark, $options->markdp);
             }
+            // We want that the wrong entry is shown.
+            else if (!is_null($currentmark)){
+                $attributes['value'] = $qa->get_submitted_var(
+                    $qa->get_behaviour_field_name('mark'), PARAM_RAW_TRIMMED);
+            }
             $a = new stdClass();
             $a->max = $qa->format_max_mark($options->markdp);
             $a->mark = html_writer::empty_tag('input', $attributes);
@@ -146,12 +153,10 @@ public function manual_comment_fields(question_attempt $qa, question_display_opt
                 'value' => $qa->get_max_fraction(),
             ));
 
+            $error = $qa->get_validation_message();
             $errorclass = '';
-            $error = '';
-            if ($currentmark > $maxmark * $qa->get_max_fraction() || $currentmark < $maxmark * $qa->get_min_fraction()) {
-                $errorclass = ' error';
-                $error = html_writer::tag('span', get_string('manualgradeoutofrange', 'question'),
-                        array('class' => 'error')) . html_writer::empty_tag('br');
+            if ($error !== ''){
+                $erroclass = 'error';
             }
 
             $mark = html_writer::tag('div', html_writer::tag('div',
@@ -167,6 +172,7 @@ public function manual_comment_fields(question_attempt $qa, question_display_opt
                 array('class' => 'fcontainer clearfix')), array('class' => 'hidden'));
     }
 
+
     public function manual_comment_view(question_attempt $qa, question_display_options $options) {
         $output = '';
         if ($qa->has_manual_comment()) {

question/engine/lib.php

@@ -131,6 +131,23 @@ public static function set_max_mark_in_attempts(qubaid_condition $qubaids,
         $dm->set_max_mark_in_attempts($qubaids, $slot, $newmaxmark);
     }
 
+    /**
+     * MDL-51090
+     * Validate that the manual grade submitted for a particular question is a
+     * float.
+     *
+     * @param string prefix as constructed in is_manual_grade_in_range.
+     * @return bool whether the submitted data is a float.
+     */
+    public static function is_manual_grade_float($prefix) {
+        $val = optional_param($prefix . '-mark', null, PARAM_RAW_TRIMMED);
+        $mark = unformat_float($val, true);
+        if (is_float($mark)) {
+            return true;
+        }
+        return false;
+    }
+
     /**
      * Validate that the manual grade submitted for a particular question is in range.
      * @param int $qubaid the question_usage id.
@@ -139,6 +156,9 @@ public static function set_max_mark_in_attempts(qubaid_condition $qubaids,
      */
     public static function is_manual_grade_in_range($qubaid, $slot) {
         $prefix = 'q' . $qubaid . ':' . $slot . '_';
+        if (!self::is_manual_grade_float($prefix)) {
+            return false;
+        }
         $mark = question_utils::optional_param_mark($prefix . '-mark');
         $maxmark = optional_param($prefix . '-maxmark', null, PARAM_FLOAT);
         $minfraction = optional_param($prefix . ':minfraction', null, PARAM_FLOAT);

question/engine/questionattempt.php

@@ -1054,6 +1054,42 @@ public function get_submitted_var($name, $type, $postdata = null) {
         }
     }
 
+
+    public function get_validation_message() {
+
+        if (!$this->manual_mark_format_is_ok()) {
+            $errorclass = ' error';
+            return html_writer::tag('span', get_string('manualgradeinvalidformat', 'question'),
+                array('class' => 'error')) . html_writer::empty_tag('br');
+        }
+        $currentmark = $this->get_current_manual_mark();
+        $maxmark = $this->get_max_mark();
+        if ($currentmark > $maxmark * $this->get_max_fraction() || $currentmark < $maxmark * $this->get_min_fraction()) {
+            $errorclass = ' error';
+            return html_writer::tag('span', get_string('manualgradeoutofrange', 'question'),
+                array('class' => 'error')) . html_writer::empty_tag('br');
+        }
+
+        return '';
+
+    }
+
+    /**
+     * Validates that the manual mark parameter is a float.
+     * @return bool.
+     */
+    public function manual_mark_format_is_ok() {
+        $val = $this->get_submitted_var($this->get_behaviour_field_name('mark'), PARAM_RAW_TRIMMED);
+
+        // If it is empy, we get null (it is always the case on start)
+        if ($val===null) {
+            return true;
+        }
+        $mark = unformat_float($val, true);
+
+        return is_float($mark);
+    }
+
     /**
      * Handle a submitted variable representing uploaded files.
      * @param string $name the field name.