MDL-69246 questions: allow for float issues when validating manual marks

As well as fixing the bug, I also rewrote the test to use
data providers, which should lead to more useful failure messages.

And, I moved the magic number we used as the float tolerence to
be a named constant.

diff --git a/question/behaviour/behaviourbase.php b/question/behaviour/behaviourbase.php
index 1b3ac4b..090d114 100644 (file)
--- a/question/behaviour/behaviourbase.php
+++ b/question/behaviour/behaviourbase.php
@@ -784,7 +784,7 @@ abstract class question_cbm {
             // errors for people who have used TGM's patches.
             return 0;
         }
-        if ($fraction <= 0.00000005) {
+        if ($fraction <= question_utils::MARK_TOLERANCE) {
             return self::$wrongscore[$certainty];
         } else {
             return self::$rightscore[$certainty] * $fraction;

diff --git a/question/engine/lib.php b/question/engine/lib.php
index edd8c6d..cc8ebc2 100644 (file)
--- a/question/engine/lib.php
+++ b/question/engine/lib.php
@@ -790,6 +790,16 @@ class question_out_of_sequence_exception extends moodle_exception {
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 abstract class question_utils {
+    /**
+     * @var float tolerance to use when comparing question mark/fraction values.
+     *
+     * When comparing floating point numbers in a computer, the representation is not
+     * necessarily exact. Therefore, we need to allow a tolerance.
+     * Question marks are stored in the database as decimal numbers with 7 decimal places.
+     * Therefore, this is the appropriate tolerance to use.
+     */
+    const MARK_TOLERANCE = 0.00000005;
+
     /**
      * Tests to see whether two arrays have the same keys, with the same values
      * (as compared by ===) for each key. However, the order of the arrays does

diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php
index 0ab9995..ba41f67 100644 (file)
--- a/question/engine/questionattempt.php
+++ b/question/engine/questionattempt.php
@@ -695,7 +695,7 @@ class question_attempt {
     /** @return bool whether this question attempt has a non-zero maximum mark. */
     public function has_marks() {
         // Since grades are stored in the database as NUMBER(12,7).
-        return $this->maxmark >= 0.00000005;
+        return $this->maxmark >= question_utils::MARK_TOLERANCE;
     }
 
     /**
@@ -1158,7 +1158,8 @@ class question_attempt {
         }
 
         $maxmark = $this->get_max_mark();
-        if ($mark > $maxmark * $this->get_max_fraction() || $mark < $maxmark * $this->get_min_fraction()) {
+        if ($mark > $maxmark * $this->get_max_fraction() + question_utils::MARK_TOLERANCE ||
+                $mark < $maxmark * $this->get_min_fraction() - question_utils::MARK_TOLERANCE) {
             return get_string('manualgradeoutofrange', 'question');
         }
 

diff --git a/question/engine/tests/questionattempt_with_steps_test.php b/question/engine/tests/questionattempt_with_steps_test.php
index e8febb6..b5194c0 100644 (file)
--- a/question/engine/tests/questionattempt_with_steps_test.php
+++ b/question/engine/tests/questionattempt_with_steps_test.php
@@ -170,19 +170,45 @@ class question_attempt_with_steps_test extends advanced_testcase {
         $qa->get_max_fraction();
     }
 
-    public function test_validate_manual_mark() {
-        $this->qa->set_min_fraction(0);
-        $this->qa->set_max_fraction(1);
-        $this->assertSame('', $this->qa->validate_manual_mark(null));
-        $this->assertSame('', $this->qa->validate_manual_mark(''));
-        $this->assertSame('', $this->qa->validate_manual_mark('0'));
-        $this->assertSame('', $this->qa->validate_manual_mark('0.0'));
-        $this->assertSame('', $this->qa->validate_manual_mark('2,0'));
-        $this->assertSame(get_string('manualgradeinvalidformat', 'question'),
-                $this->qa->validate_manual_mark('frog'));
-        $this->assertSame(get_string('manualgradeoutofrange', 'question'),
-                $this->qa->validate_manual_mark('2.1'));
-        $this->assertSame(get_string('manualgradeoutofrange', 'question'),
-                $this->qa->validate_manual_mark('-0,01'));
+    /**
+     * Test cases for {@see test_validate_manual_mark()}.
+     *
+     * @return array test cases
+     */
+    public function validate_manual_mark_cases(): array {
+        // Recall, the DB schema stores question grade information to 7 decimal places.
+        return [
+            [0, 1, 2, null, ''],
+            [0, 1, 2, '', ''],
+            [0, 1, 2, '0', ''],
+            [0, 1, 2, '0.0', ''],
+            [0, 1, 2, '2,0', ''],
+            [0, 1, 2, 'frog', get_string('manualgradeinvalidformat', 'question')],
+            [0, 1, 2, '2.1', get_string('manualgradeoutofrange', 'question')],
+            [0, 1, 2, '-0,01', get_string('manualgradeoutofrange', 'question')],
+            [-0.3333333, 1, 0.75, '0.75', ''],
+            [-0.3333333, 1, 0.75, '0.7500001', get_string('manualgradeoutofrange', 'question')],
+            [-0.3333333, 1, 0.75, '-0.25', ''],
+            [-0.3333333, 1, 0.75, '-0.2500001', get_string('manualgradeoutofrange', 'question')],
+        ];
+    }
+
+    /**
+     * Test validate_manual_mark.
+     *
+     * @dataProvider validate_manual_mark_cases
+     *
+     * @param float $minfraction minimum fraction for the question being attempted.
+     * @param float $maxfraction maximum fraction for the question being attempted.
+     * @param float $maxmark marks for the question attempt.
+     * @param string|null $currentmark submitted mark.
+     * @param string $expectederror expected error, if any.
+     */
+    public function test_validate_manual_mark(float $minfraction, float $maxfraction,
+            float $maxmark, ?string $currentmark, string $expectederror) {
+        $this->qa->set_min_fraction($minfraction);
+        $this->qa->set_max_fraction($maxfraction);
+        $this->qa->set_max_mark($maxmark);
+        $this->assertSame($expectederror, $this->qa->validate_manual_mark($currentmark));
     }
 }