MDL-51476 questions: manual grading validation for ungraded Qs

The change in MDL-51090 broke manually commenting on questions for which
no mark is given (max mark == 0). This fixes it, with unit tests.

Thanks to Nick Phillips for the original suggestion of the fix.

question/behaviour/manualgraded/tests/walkthrough_test.php

@@ -204,6 +204,63 @@ class qbehaviour_manualgraded_walkthrough_testcase extends qbehaviour_walkthroug
             new question_pattern_expectation('/' . preg_quote('Not good enough!', '/') . '/'));
     }
 
+    public function test_manual_grade_ungraded_question() {
+        global $PAGE;
+
+        // The current text editor depends on the users profile setting - so it needs a valid user.
+        $this->setAdminUser();
+        // Required to init a text editor.
+        $PAGE->set_url('/');
+
+        // Create an essay question.
+        $essay = test_question_maker::make_an_essay_question();
+        $this->start_attempt_at_question($essay, 'deferredfeedback', 0);
+
+        // Check the right model is being used.
+        $this->assertEquals('manualgraded', $this->quba->get_question_attempt(
+                $this->slot)->get_behaviour_name());
+
+        // Check the initial state.
+        $this->check_current_state(question_state::$todo);
+        $this->check_current_mark(null);
+        $this->check_current_output($this->get_contains_question_text_expectation($essay),
+                $this->get_does_not_contain_feedback_expectation());
+
+        // Simulate some data submitted by the student.
+        $this->process_submission(array('answer' => 'This is my wonderful essay!', 'answerformat' => FORMAT_HTML));
+
+        // Verify.
+        $this->check_current_state(question_state::$complete);
+        $this->check_current_mark(null);
+        $this->check_current_output(
+                new question_contains_tag_with_attribute('textarea', 'name',
+                $this->quba->get_question_attempt($this->slot)->get_qt_field_name('answer')),
+                $this->get_does_not_contain_feedback_expectation());
+
+        // Finish the attempt.
+        $this->quba->finish_all_questions();
+
+        // Verify.
+        $this->check_current_state(question_state::$needsgrading);
+        $this->check_current_mark(null);
+        $this->assertEquals('This is my wonderful essay!',
+                $this->quba->get_response_summary($this->slot));
+
+        // Process a manual comment. Note: null mark is the whole point here.
+        $this->manual_grade('Not good enough!', null, FORMAT_HTML);
+
+        // Verify.
+        // I am pretty sure this next assertion is incorrect. We should change
+        // the question state to indicate that this quetion has now been commented
+        // on. However, that is tricky, because what if, after that, the mam mark
+        // for the qusetions is changed. So, for now, this assertion verifies
+        // the current behaviour.
+        $this->check_current_state(question_state::$needsgrading);
+        $this->check_current_mark(null);
+        $this->check_current_output(
+                new question_pattern_expectation('/' . preg_quote('Not good enough!', '/') . '/'));
+    }
+
     public function test_manual_graded_ignore_repeat_sumbission() {
         // Create an essay question.
         $essay = test_question_maker::make_an_essay_question();

question/engine/lib.php

@@ -144,7 +144,8 @@ abstract class question_engine {
         $minfraction = optional_param($prefix . ':minfraction', null, PARAM_FLOAT);
         $maxfraction = optional_param($prefix . ':maxfraction', null, PARAM_FLOAT);
         return $mark === '' ||
-                ($mark !== null && $mark >= $minfraction * $maxmark && $mark <= $maxfraction * $maxmark);
+                ($mark !== null && $mark >= $minfraction * $maxmark && $mark <= $maxfraction * $maxmark) ||
+                ($mark === null && $maxmark === null);
     }
 
     /**

question/engine/tests/questionengine_test.php

@@ -91,4 +91,38 @@ class question_engine_test extends advanced_testcase {
         // Ignore unknown input in the disabled argument.
         $this->assertSame($in, question_engine::sort_behaviours($in, '', 'unknown', ''));
     }
-}
\ No newline at end of file
+
+    public function test_is_manual_grade_in_range() {
+        $_POST[] = array('q1:2_-mark' => 0.5, 'q1:2_-maxmark' => 1.0,
+                'q1:2_:minfraction' => 0, 'q1:2_:maxfraction' => 1);
+        $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2));
+    }
+
+    public function test_is_manual_grade_in_range_bottom_end() {
+        $_POST[] = array('q1:2_-mark' => -1.0, 'q1:2_-maxmark' => 2.0,
+                'q1:2_:minfraction' => -0.5, 'q1:2_:maxfraction' => 1);
+        $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2));
+    }
+
+    public function test_is_manual_grade_in_range_too_low() {
+        $_POST[] = array('q1:2_-mark' => -1.1, 'q1:2_-maxmark' => 2.0,
+                'q1:2_:minfraction' => -0.5, 'q1:2_:maxfraction' => 1);
+        $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2));
+    }
+
+    public function test_is_manual_grade_in_range_top_end() {
+        $_POST[] = array('q1:2_-mark' => 3.0, 'q1:2_-maxmark' => 1.0,
+                'q1:2_:minfraction' => -6.0, 'q1:2_:maxfraction' => 3.0);
+        $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2));
+    }
+
+    public function test_is_manual_grade_in_range_too_high() {
+        $_POST[] = array('q1:2_-mark' => 3.1, 'q1:2_-maxmark' => 1.0,
+                'q1:2_:minfraction' => -6.0, 'q1:2_:maxfraction' => 3.0);
+        $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2));
+    }
+
+    public function test_is_manual_grade_in_range_ungraded() {
+        $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2));
+    }
+}