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 547a370dc76..682e905bc8e 100644 --- a/question/type/multianswer/renderer.php +++ b/question/type/multianswer/renderer.php @@ -177,13 +177,18 @@ class qtype_multianswer_textfield_renderer extends qtype_multianswer_subq_render 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 @@ class qtype_multianswer_multichoice_inline_renderer $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 @@ class qtype_multianswer_multichoice_vertical_renderer extends qtype_multianswer_ $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 75a3d7dcf47..3a77001a1e6 100644 --- a/question/type/multianswer/tests/helper.php +++ b/question/type/multianswer/tests/helper.php @@ -38,7 +38,7 @@ require_once($CFG->dirroot . '/question/type/multianswer/question.php'); */ 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 @@ class qtype_multianswer_test_helper extends question_test_helper { 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 19af3c7564b..9c9d291bf54 100644 --- a/question/type/multianswer/tests/walkthrough_test.php +++ b/question/type/multianswer/tests/walkthrough_test.php @@ -37,9 +37,15 @@ require_once($CFG->dirroot . '/question/engine/tests/helpers.php'); * @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 @@ class qtype_multianswer_walkthrough_test extends qbehaviour_walkthrough_test_bas $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 110cfdc02a9..0e48155c6de 100644 --- a/question/type/numerical/question.php +++ b/question/type/numerical/question.php @@ -179,6 +179,10 @@ class qtype_numerical_question extends question_graded_automatically { * @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 @@ class qtype_numerical_question extends question_graded_automatically { return $answer; } } + return null; } @@ -273,18 +278,17 @@ class qtype_numerical_question extends question_graded_automatically { 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 d4ba6f88a4e..84fc7d0d7c4 100644 --- a/question/type/numerical/tests/question_test.php +++ b/question/type/numerical/tests/question_test.php @@ -181,6 +181,11 @@ class qtype_numerical_question_test extends advanced_testcase { $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 a875c8c1a05..72f0f4a38a1 100644 --- a/question/type/shortanswer/question.php +++ b/question/type/shortanswer/question.php @@ -78,6 +78,10 @@ class qtype_shortanswer_question extends question_graded_by_strategy } 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 @@ class qtype_shortanswer_question extends question_graded_by_strategy $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 31212ce6e78..2cf77d7c06f 100644 --- a/question/type/shortanswer/questiontype.php +++ b/question/type/shortanswer/questiontype.php @@ -117,7 +117,7 @@ class qtype_shortanswer extends question_type { $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;