From b2a79cc1e860beb13560e8ed2ac326b580143f2c Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 13 Sep 2012 19:12:45 +0100 Subject: [PATCH] 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 | 30 ++++- question/type/multianswer/tests/helper.php | 46 ++++++- .../multianswer/tests/walkthrough_test.php | 114 +++++++++++++++++- question/type/numerical/question.php | 12 +- .../type/numerical/tests/question_test.php | 5 + question/type/shortanswer/question.php | 6 +- question/type/shortanswer/questiontype.php | 2 +- 7 files changed, 203 insertions(+), 12 deletions(-) diff --git a/question/type/multianswer/renderer.php b/question/type/multianswer/renderer.php index 547a370dc7631..682e905bc8e39 100644 --- a/question/type/multianswer/renderer.php +++ b/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('
', $feedback), array('class' => 'outcome')); + return $result; } diff --git a/question/type/multianswer/tests/helper.php b/question/type/multianswer/tests/helper.php index 75a3d7dcf47b5..3a77001a1e6e3 100644 --- a/question/type/multianswer/tests/helper.php +++ b/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; + } } diff --git a/question/type/multianswer/tests/walkthrough_test.php b/question/type/multianswer/tests/walkthrough_test.php index 19af3c7564b14..9c9d291bf54e1 100644 --- a/question/type/multianswer/tests/walkthrough_test.php +++ b/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), '~') . '
~'); + } + 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('~]* 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()); + } } diff --git a/question/type/numerical/question.php b/question/type/numerical/question.php index 110cfdc02a941..0e48155c6deec 100644 --- a/question/type/numerical/question.php +++ b/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); diff --git a/question/type/numerical/tests/question_test.php b/question/type/numerical/tests/question_test.php index d4ba6f88a4e66..84fc7d0d7c47e 100644 --- a/question/type/numerical/tests/question_test.php +++ b/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'))); diff --git a/question/type/shortanswer/question.php b/question/type/shortanswer/question.php index a875c8c1a0546..72f0f4a38a1ce 100644 --- a/question/type/shortanswer/question.php +++ b/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); diff --git a/question/type/shortanswer/questiontype.php b/question/type/shortanswer/questiontype.php index 31212ce6e7889..2cf77d7c06fda 100644 --- a/question/type/shortanswer/questiontype.php +++ b/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;