better handling of quotes and strings in student answers (#257)

Author: PhilippImhof

classes/external/answervalidation.php

@@ -100,7 +100,9 @@ public static function validate_student_answer(string $answer, int $answertype,
             $parser = new answer_parser($number);
             // As we are in a try-catch block anyway, we can just throw an Exception if the answer
             // is not acceptable.
-            if (!$parser->is_acceptable_for_answertype($answertype)) {
+            $isacceptable = $parser->is_acceptable_for_answertype($answertype);
+            $containsquote = preg_match('/[\'"]/', $number);
+            if (!$isacceptable || $containsquote) {
                 $answertypestrings = [
                     qtype_formulas::ANSWER_TYPE_NUMBER => 'number',
                     qtype_formulas::ANSWER_TYPE_NUMERIC => 'numeric',

question.php

@@ -1335,10 +1335,12 @@ public function get_evaluated_answers(): array {
      */
     private static function wrap_algebraic_formulas_in_quotes(array $formulas): array {
         foreach ($formulas as &$formula) {
-            // If the formula is aready wrapped in quotes (e. g. after an earlier call to this
-            // function), there is nothing to do.
+            // If the formula is aready wrapped in quotes, we throw an Exception, because that
+            // should not happen. It will happen, if the student puts quotes around their response, but
+            // we want that to be graded wrong. The exception will be caught and dealt with upstream,
+            // so we do not need to be more precise.
             if (preg_match('/^\"[^\"]*\"$/', $formula)) {
-                continue;
+                throw new Exception();
             }
 
             $formula = '"' . $formula . '"';
@@ -1495,7 +1497,12 @@ public function grade(array $response, bool $finalsubmit = false): array {
         // formulas, we must wrap the answers in quotes before we move on. Also, we reset the conversion
         // factor, because it is not needed for algebraic answers.
         if ($isalgebraic) {
-            $response = self::wrap_algebraic_formulas_in_quotes($response);
+            try {
+                $response = self::wrap_algebraic_formulas_in_quotes($response);
+            } catch (Throwable $t) {
+                // TODO: convert to non-capturing catch.
+                return ['answer' => 0, 'unit' => $unitcorrect];
+            }
             $conversionfactor = 1;
         }
 

tests/answer_parser_test.php

@@ -90,6 +90,7 @@ public static function provide_numbers(): array {
             [false, '#'],
             [false, ''],
             [false, '""'],
+            [false, '"foo"'],
         ];
     }
 
@@ -110,7 +111,6 @@ public static function provide_algebraic_formulas(): array {
             [qtype_formulas::ANSWER_TYPE_NUMERICAL_FORMULA, 'sin(3)-3+exp(4)'],
             [qtype_formulas::ANSWER_TYPE_NUMERICAL_FORMULA, '3+exp(4+5)^sin(6+7)'],
             [qtype_formulas::ANSWER_TYPE_NUMERIC, '3+4^-(9)'],
-            [qtype_formulas::ANSWER_TYPE_ALGEBRAIC, '"foo"'],
             [qtype_formulas::ANSWER_TYPE_ALGEBRAIC, 'sin(a)-a+exp(b)'],
             [qtype_formulas::ANSWER_TYPE_ALGEBRAIC, '3e 10'],
             [qtype_formulas::ANSWER_TYPE_NUMERIC, '3e8(4.e8+2)(.5e8/2)5'],
@@ -173,6 +173,7 @@ public static function provide_algebraic_formulas(): array {
             [false, '\sin(pi)'],
             [false, '1+sin'],
             [false, '""'],
+            [false, '"foo"'],
             [false, '5 "foo"'],
         ];
     }

tests/question_test.php

@@ -1445,11 +1445,11 @@ public static function provide_formulas_for_wrapping(): array {
         return [
             [['""'], ['']],
             [['"a"'], ['a']],
-            [['"a"'], ['"a"']],
+            ['!', ['"a"']],
             [['"1"', '"2"'], ['1', '2']],
             [['""', '""'], ['', '']],
-            [['""', '""'], ['""', '""']],
-            [['"1"', '"x"'], ['"1"', '"x"']],
+            ['!', ['""', '""']],
+            ['!', ['"1"', '"x"']],
         ];
     }
 
@@ -1465,7 +1465,17 @@ public function test_wrap_algebraic_formulas_in_quotes($expected, $input): void
         $func = new \ReflectionMethod(qtype_formulas_part::class, 'wrap_algebraic_formulas_in_quotes');
         $func->setAccessible(true);
 
-        $wrapped = $func->invoke(null, $input);
+        $error = false;
+        try {
+            $wrapped = $func->invoke(null, $input);
+        } catch (Exception $e) {
+            $error = true;
+        }
+
+        if ($expected === '!') {
+            self::assertTrue($error);
+            return;
+        }
 
         foreach ($expected as $i => $exp) {
             self::assertEquals($exp, $wrapped[$i]);

tests/renderer_test.php

@@ -213,6 +213,19 @@ public function test_render_question_with_algebraic_answer(): void {
         $this->check_output_contains_lang_string('correctansweris', 'qtype_formulas', '5 * x^2');
         $this->check_output_does_not_contain('a*x^2');
 
+        // Submit correct answer in quotes, which will be graded wrong.
+        $this->start_attempt_at_question($q, 'immediatefeedback', 1);
+        $this->process_submission(['0_0' => '"5x^2"', '-submit' => 1]);
+        $this->check_current_state(question_state::$gradedwrong);
+        $this->check_current_mark(0);
+        $this->check_current_output(
+                $this->get_contains_mark_summary(0),
+        );
+        $this->render();
+        $this->check_output_contains_text_input('0_0', '"5x^2"', false);
+        $this->check_output_contains_lang_string('correctansweris', 'qtype_formulas', '5 * x^2');
+        $this->check_output_does_not_contain('a*x^2');
+
         // Submit right answer.
         $this->start_attempt_at_question($q, 'immediatefeedback', 1);
         $this->process_submission(['0_0' => '5*x^2', '-submit' => 1]);