MDL-35370 cloze qtype: distinguish wrong & unanswered subqs

This affects the subquestions that appear as an embedded text input box.

There are three cases:
1. Input for subq left blank
2. Input for subq was wrong, and matched by a * wildcard.
3. Input for subq was wrong, and did not match any answer.

2. and 3. should look identical, apart from any feedback in case 2.

1. is different. The state should be displayed as "Not answered" even
though the mark for this part is still shown as 0.

There are some new unit tests for these cases.

Also, we slighly improve handling of , for decimal point in multianswer,
although there are still issues.

While working on this, I made some minor clean-ups in shortanswer and
numerical qtypes.

question/type/multianswer/renderer.php

@@ -177,13 +177,18 @@ public function subquestion(question_attempt $qa, question_display_options $opti
         if ($subq->qtype->name() == 'shortanswer') {
             $matchinganswer = $subq->get_matching_answer(array('answer' => $response));
         } else if ($subq->qtype->name() == 'numerical') {
-            $matchinganswer = $subq->get_matching_answer($response, 1);
+            list($value, $unit, $multiplier) = $subq->ap->apply_units($response, '');
+            $matchinganswer = $subq->get_matching_answer($value, 1);
         } else {
             $matchinganswer = $subq->get_matching_answer($response);
         }
 
         if (!$matchinganswer) {
-            $matchinganswer = new question_answer(0, '', null, '', FORMAT_HTML);
+            if (is_null($response) || $response === '') {
+                $matchinganswer = new question_answer(0, '', null, '', FORMAT_HTML);
+            } else {
+                $matchinganswer = new question_answer(0, '', 0.0, '', FORMAT_HTML);
+            }
         }
 
         // Work out a good input field size.
@@ -281,6 +286,9 @@ public function subquestion(question_attempt $qa, question_display_options $opti
         $order = $subq->get_order($qa);
         $correctresponses = $subq->get_correct_response();
         $rightanswer = $subq->answers[$order[reset($correctresponses)]];
+        if (!$matchinganswer) {
+            $matchinganswer = new question_answer(0, '', null, '', FORMAT_HTML);
+        }
         $feedbackpopup = $this->feedback_popup($subq, $matchinganswer->fraction,
                 $subq->format_text($matchinganswer->feedback, $matchinganswer->feedbackformat,
                         $qa, 'question', 'answerfeedback', $matchinganswer->id),
@@ -366,16 +374,30 @@ public function subquestion(question_attempt $qa, question_display_options $opti
 
         $result .= $this->all_choices_wrapper_end();
 
+        $feedback = array();
         if ($options->feedback && $options->marks >= question_display_options::MARK_AND_MAX &&
                 $subq->maxmark > 0) {
             $a = new stdClass();
             $a->mark = format_float($fraction * $subq->maxmark, $options->markdp);
             $a->max =  format_float($subq->maxmark, $options->markdp);
 
-            $result .= html_writer::tag('div', get_string('markoutofmax', 'question', $a),
-                    array('class' => 'outcome'));
+            $feedback[] = html_writer::tag('div', get_string('markoutofmax', 'question', $a));
+        }
+
+        if ($options->rightanswer) {
+            foreach ($subq->answers as $ans) {
+                if (question_state::graded_state_for_fraction($ans->fraction) ==
+                        question_state::$gradedright) {
+                    $feedback[] = get_string('correctansweris', 'qtype_multichoice',
+                            $subq->format_text($ans->answer, $ans->answerformat,
+                                    $qa, 'question', 'answer', $ansid));
+                    break;
+                }
+            }
         }
 
+        $result .= html_writer::nonempty_tag('div', implode('<br />', $feedback), array('class' => 'outcome'));
+
         return $result;
     }
 

question/type/multianswer/tests/helper.php

@@ -38,7 +38,7 @@
  */
 class qtype_multianswer_test_helper extends question_test_helper {
     public function get_test_questions() {
-        return array('twosubq', 'fourmc');
+        return array('twosubq', 'fourmc', 'numericalzero');
     }
 
     /**
@@ -287,4 +287,48 @@ public function make_multianswer_question_fourmc() {
 
         return $q;
     }
+
+    /**
+     * Makes a multianswer question with one numerical subquestion, right answer 0.
+     * This is used for testing the MDL-35370 bug.
+     * @return qtype_multianswer_question
+     */
+    public function make_multianswer_question_numericalzero() {
+        question_bank::load_question_definition_classes('multianswer');
+        $q = new qtype_multianswer_question();
+        test_question_maker::initialise_a_question($q);
+        $q->name = 'Numerical zero';
+        $q->questiontext =
+                'Enter zero: {#1}.';
+        $q->generalfeedback = '';
+        $q->qtype = question_bank::get_qtype('multianswer');
+
+        $q->textfragments = array(
+            'Enter zero: ',
+            '.',
+        );
+        $q->places = array('1' => '1');
+
+        // Numerical subquestion.
+        question_bank::load_question_definition_classes('numerical');
+        $sub = new qtype_numerical_question();
+        test_question_maker::initialise_a_question($sub);
+        $sub->name = 'Numerical zero';
+        $sub->questiontext = '{1:NUMERICAL:=0:0}';
+        $sub->questiontextformat = FORMAT_HTML;
+        $sub->generalfeedback = '';
+        $sub->generalfeedbackformat = FORMAT_HTML;
+        $sub->answers = array(
+            13 => new qtype_numerical_answer(13, '0', 1.0, '', FORMAT_HTML, 0),
+        );
+        $sub->qtype = question_bank::get_qtype('numerical');
+        $sub->ap = new qtype_numerical_answer_processor(array());
+        $sub->maxmark = 1;
+
+        $q->subquestions = array(
+            1 => $sub,
+        );
+
+        return $q;
+    }
 }

question/type/multianswer/tests/walkthrough_test.php

@@ -37,9 +37,15 @@
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class qtype_multianswer_walkthrough_test extends qbehaviour_walkthrough_test_base {
+
+    protected function get_contains_subq_status(question_state $state) {
+        return new question_pattern_expectation('~' .
+                preg_quote($state->default_string(true), '~') . '<br />~');
+    }
+
     public function test_deferred_feedback() {
 
-        // Create a gapselect question.
+        // Create a multianswer question.
         $q = test_question_maker::make_question('multianswer', 'fourmc');
         $this->start_attempt_at_question($q, 'deferredfeedback', 4);
 
@@ -86,4 +92,110 @@ public function test_deferred_feedback() {
                 $this->get_contains_partcorrect_expectation(),
                 $this->get_does_not_contain_validation_error_expectation());
     }
+
+    public function test_deferred_feedback_numericalzero_not_answered() {
+        // Tests the situation found in MDL-35370.
+
+        // Create a multianswer question with one numerical subquestion, right answer zero.
+        $q = test_question_maker::make_question('multianswer', 'numericalzero');
+        $this->start_attempt_at_question($q, 'deferredfeedback', 1);
+
+        // Check the initial state.
+        $this->check_current_state(question_state::$todo);
+        $this->check_current_mark(null);
+        $this->check_current_output(
+                $this->get_contains_marked_out_of_summary(),
+                $this->get_does_not_contain_feedback_expectation(),
+                $this->get_does_not_contain_validation_error_expectation());
+
+        // Now submit all and finish.
+        $this->process_submission(array('-finish' => 1));
+
+        // Verify.
+        $this->check_current_state(question_state::$gaveup);
+        $this->check_current_mark(null);
+        $this->check_current_output(
+                $this->get_contains_marked_out_of_summary(),
+                new question_pattern_expectation('~<input[^>]* class="incorrect" [^>]*/>~'),
+                $this->get_contains_subq_status(question_state::$gaveup),
+                $this->get_does_not_contain_validation_error_expectation());
+    }
+
+    public function test_deferred_feedback_numericalzero_0_answer() {
+        // Tests the situation found in MDL-35370.
+
+        // Create a multianswer question with one numerical subquestion, right answer zero.
+        $q = test_question_maker::make_question('multianswer', 'numericalzero');
+        $this->start_attempt_at_question($q, 'deferredfeedback', 1);
+
+        // Check the initial state.
+        $this->check_current_state(question_state::$todo);
+        $this->check_current_mark(null);
+        $this->check_current_output(
+                $this->get_contains_marked_out_of_summary(),
+                $this->get_does_not_contain_feedback_expectation(),
+                $this->get_does_not_contain_validation_error_expectation());
+
+        // Save a the correct answer.
+        $this->process_submission(array('sub1_answer' => '0'));
+
+        // Verify.
+        $this->check_current_state(question_state::$complete);
+        $this->check_current_mark(null);
+        $this->check_current_output(
+                $this->get_contains_marked_out_of_summary(),
+                $this->get_does_not_contain_feedback_expectation(),
+                $this->get_does_not_contain_validation_error_expectation());
+
+        // Now submit all and finish.
+        $this->process_submission(array('-finish' => 1));
+
+        // Verify.
+        $this->check_current_state(question_state::$gradedright);
+        $this->check_current_mark(1);
+        $this->check_current_output(
+                $this->get_contains_mark_summary(1),
+                $this->get_contains_correct_expectation(),
+                $this->get_contains_subq_status(question_state::$gradedright),
+                $this->get_does_not_contain_validation_error_expectation());
+    }
+
+    public function test_deferred_feedback_numericalzero_0_wrong() {
+        // Tests the situation found in MDL-35370.
+
+        // Create a multianswer question with one numerical subquestion, right answer zero.
+        $q = test_question_maker::make_question('multianswer', 'numericalzero');
+        $this->start_attempt_at_question($q, 'deferredfeedback', 1);
+
+        // Check the initial state.
+        $this->check_current_state(question_state::$todo);
+        $this->check_current_mark(null);
+        $this->check_current_output(
+                $this->get_contains_marked_out_of_summary(),
+                $this->get_does_not_contain_feedback_expectation(),
+                $this->get_does_not_contain_validation_error_expectation());
+
+        // Save a the correct answer.
+        $this->process_submission(array('sub1_answer' => '42'));
+
+        // Verify.
+        $this->check_current_state(question_state::$complete);
+        $this->check_current_mark(null);
+        $this->check_current_output(
+                $this->get_contains_marked_out_of_summary(),
+                $this->get_does_not_contain_feedback_expectation(),
+                $this->get_does_not_contain_validation_error_expectation());
+
+        // Now submit all and finish.
+        $this->process_submission(array('-finish' => 1));
+
+        // Verify.
+        $this->check_current_state(question_state::$gradedwrong);
+        $this->check_current_mark(0);
+        $this->check_current_output(
+                $this->get_contains_mark_summary(0),
+                $this->get_contains_incorrect_expectation(),
+                $this->get_contains_subq_status(question_state::$gradedwrong),
+                $this->get_does_not_contain_validation_error_expectation());
+    }
 }

question/type/numerical/question.php

@@ -179,6 +179,10 @@ public function get_correct_response() {
      * @return question_answer the matching answer.
      */
     public function get_matching_answer($value, $multiplier) {
+        if (is_null($value) || $value === '') {
+            return null;
+        }
+
         if (!is_null($multiplier)) {
             $scaledvalue = $value * $multiplier;
         } else {
@@ -193,6 +197,7 @@ public function get_matching_answer($value, $multiplier) {
                 return $answer;
             }
         }
+
         return null;
     }
 
@@ -273,18 +278,17 @@ public function classify_response(array $response) {
     public function check_file_access($qa, $options, $component, $filearea, $args,
             $forcedownload) {
         if ($component == 'question' && $filearea == 'answerfeedback') {
-            $question = $qa->get_question();
             $currentanswer = $qa->get_last_qt_var('answer');
             if ($this->has_separate_unit_field()) {
                 $selectedunit = $qa->get_last_qt_var('unit');
             } else {
                 $selectedunit = null;
             }
-            list($value, $unit, $multiplier) = $question->ap->apply_units(
+            list($value, $unit, $multiplier) = $this->ap->apply_units(
                     $currentanswer, $selectedunit);
-            $answer = $question->get_matching_answer($value, $multiplier);
+            $answer = $this->get_matching_answer($value, $multiplier);
             $answerid = reset($args); // itemid is answer id.
-            return $options->feedback && $answerid == $answer->id;
+            return $options->feedback && $answer && $answerid == $answer->id;
 
         } else if ($component == 'question' && $filearea == 'hint') {
             return $this->check_hint_file_access($qa, $options, $args);

question/type/numerical/tests/question_test.php

@@ -181,6 +181,11 @@ public function test_summarise_response() {
         $this->assertEquals('3.1', $num->summarise_response(array('answer' => '3.1')));
     }
 
+    public function test_summarise_response_zero() {
+        $num = test_question_maker::make_question('numerical');
+        $this->assertEquals('0', $num->summarise_response(array('answer' => '0')));
+    }
+
     public function test_summarise_response_unit() {
         $num = test_question_maker::make_question('numerical', 'unit');
         $this->assertEquals('3.1', $num->summarise_response(array('answer' => '3.1')));

question/type/shortanswer/question.php

@@ -78,6 +78,10 @@ public function get_answers() {
     }
 
     public function compare_response_with_answer(array $response, question_answer $answer) {
+        if (!array_key_exists('answer', $response) || is_null($response['answer'])) {
+            return false;
+        }
+
         return self::compare_string_with_wildcard(
                 $response['answer'], $answer->answer, !$this->usecase);
     }
@@ -129,7 +133,7 @@ public function check_file_access($qa, $options, $component, $filearea,
             $currentanswer = $qa->get_last_qt_var('answer');
             $answer = $qa->get_question()->get_matching_answer(array('answer' => $currentanswer));
             $answerid = reset($args); // itemid is answer id.
-            return $options->feedback && $answerid == $answer->id;
+            return $options->feedback && $answer && $answerid == $answer->id;
 
         } else if ($component == 'question' && $filearea == 'hint') {
             return $this->check_hint_file_access($qa, $options, $args);

question/type/shortanswer/questiontype.php

@@ -117,7 +117,7 @@ public function save_question_options($question) {
 
         $this->save_hints($question);
 
-        // Perform sanity checks on fractional grades
+        // Perform sanity checks on fractional grades.
         if ($maxfraction != 1) {
             $result->noticeyesno = get_string('fractionsnomax', 'question', $maxfraction * 100);
             return $result;