From 1f60ddcebcfa8dae6ec56a7bc0952e8178f7cb12 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 14 Jun 2019 18:30:52 +0100 Subject: [PATCH] MDL-57239 qbehaviour_interactive: fix Try again button when reviewing --- question/behaviour/interactive/behaviour.php | 33 +++++++----- question/behaviour/interactive/renderer.php | 12 +++-- .../interactive/tests/walkthrough_test.php | 51 ++++++++++++++++++- 3 files changed, 77 insertions(+), 19 deletions(-) diff --git a/question/behaviour/interactive/behaviour.php b/question/behaviour/interactive/behaviour.php index b58f0fce406..c9d8564c277 100644 --- a/question/behaviour/interactive/behaviour.php +++ b/question/behaviour/interactive/behaviour.php @@ -32,7 +32,7 @@ defined('MOODLE_INTERNAL') || die(); * Question behaviour for the interactive model. * * Each question has a submit button next to it which the student can use to - * submit it. Once the qustion is submitted, it is not possible for the + * submit it. Once the question is submitted, it is not possible for the * student to change their answer any more, but the student gets full feedback * straight away. * @@ -41,14 +41,17 @@ defined('MOODLE_INTERNAL') || die(); */ class qbehaviour_interactive extends question_behaviour_with_multiple_tries { /** - * Special value used for {@link question_display_options::$readonly when - * we are showing the try again button to the student during an attempt. - * The particular number was chosen randomly. PHP will treat it the same - * as true, but in the renderer we reconginse it display the try again - * button enabled even though the rest of the question is disabled. - * @var integer + * Constant used only in {@link adjust_display_options()} below and + * {@link (qbehaviour_interactive_renderer}. + * @var int */ - const READONLY_EXCEPT_TRY_AGAIN = 23485299; + const TRY_AGAIN_VISIBLE = 0x10; + /** + * Constant used only in {@link adjust_display_options()} below and + * {@link (qbehaviour_interactive_renderer}. + * @var int + */ + const TRY_AGAIN_VISIBLE_READONLY = 0x11; public function is_compatible_question(question_definition $question) { return $question instanceof question_automatically_gradable; @@ -82,6 +85,14 @@ class qbehaviour_interactive extends question_behaviour_with_multiple_tries { return; } + // The question in in a try-again state. We need the to let the renderer know this. + // The API for question-rendering is defined by the question engine, but we + // don't want to add logic in the renderer, so we are limited in how we can do this. + // However, when the question is in this state, all the question-type controls + // need to be rendered read-only. Therefore, we can conveniently pass this information + // by setting special true-like values in $options->readonly (but this is a bit of a hack). + $options->readonly = $options->readonly ? self::TRY_AGAIN_VISIBLE_READONLY : self::TRY_AGAIN_VISIBLE; + // Let the hint adjust the options. $hint = $this->get_applicable_hint(); if (!is_null($hint)) { @@ -93,12 +104,6 @@ class qbehaviour_interactive extends question_behaviour_with_multiple_tries { parent::adjust_display_options($options); $options->feedback = $save->feedback; $options->numpartscorrect = $save->numpartscorrect; - - // In a try-again state, everything except the try again button - // Should be read-only. This is a mild hack to achieve this. - if (!$options->readonly) { - $options->readonly = self::READONLY_EXCEPT_TRY_AGAIN; - } } public function get_applicable_hint() { diff --git a/question/behaviour/interactive/renderer.php b/question/behaviour/interactive/renderer.php index 89c5d9c8928..86681d5de92 100644 --- a/question/behaviour/interactive/renderer.php +++ b/question/behaviour/interactive/renderer.php @@ -36,14 +36,19 @@ defined('MOODLE_INTERNAL') || die(); */ class qbehaviour_interactive_renderer extends qbehaviour_renderer { public function controls(question_attempt $qa, question_display_options $options) { - if ($options->readonly === qbehaviour_interactive::READONLY_EXCEPT_TRY_AGAIN) { + if ($options->readonly === qbehaviour_interactive::TRY_AGAIN_VISIBLE || + $options->readonly === qbehaviour_interactive::TRY_AGAIN_VISIBLE_READONLY) { + // We are in the try again state, so no submit button. return ''; } return $this->submit_button($qa, $options); } public function feedback(question_attempt $qa, question_display_options $options) { - if (!$qa->get_state()->is_active() || !$options->readonly) { + // Show the Try again button if we are in try-again state. + if (!$qa->get_state()->is_active() || + ($options->readonly !== qbehaviour_interactive::TRY_AGAIN_VISIBLE && + $options->readonly !== qbehaviour_interactive::TRY_AGAIN_VISIBLE_READONLY)) { return ''; } @@ -54,7 +59,8 @@ class qbehaviour_interactive_renderer extends qbehaviour_renderer { 'value' => get_string('tryagain', 'qbehaviour_interactive'), 'class' => 'submit btn', ); - if ($options->readonly !== qbehaviour_interactive::READONLY_EXCEPT_TRY_AGAIN) { + if ($options->readonly === qbehaviour_interactive::TRY_AGAIN_VISIBLE_READONLY) { + // This means the question really was rendered with read-only option. $attributes['disabled'] = 'disabled'; } $output = html_writer::empty_tag('input', $attributes); diff --git a/question/behaviour/interactive/tests/walkthrough_test.php b/question/behaviour/interactive/tests/walkthrough_test.php index 86e78d28997..5f370f9f788 100644 --- a/question/behaviour/interactive/tests/walkthrough_test.php +++ b/question/behaviour/interactive/tests/walkthrough_test.php @@ -18,8 +18,7 @@ * This file contains tests that walks a question through the interactive * behaviour. * - * @package qbehaviour - * @subpackage interactive + * @package qbehaviour_interactive * @copyright 2009 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ @@ -483,4 +482,52 @@ class qbehaviour_interactive_walkthrough_test extends qbehaviour_walkthrough_tes // and I am currently interested in testing regrading here. $this->check_current_mark(1); } + + public function test_review_of_interactive_questions_before_finished() { + // Create a multichoice multiple question. + $q = test_question_maker::make_question('shortanswer'); + $q->hints = array( + new question_hint_with_parts(0, 'This is the first hint.', FORMAT_HTML, true, true), + new question_hint_with_parts(0, 'This is the second hint.', FORMAT_HTML, true, true), + ); + $this->start_attempt_at_question($q, 'interactive', 3); + + // Check the initial state. + $this->check_current_state(question_state::$todo); + $this->check_current_mark(null); + $this->check_current_output( + $this->get_contains_submit_button_expectation(true), + $this->get_does_not_contain_feedback_expectation(), + $this->get_tries_remaining_expectation(3), + $this->get_does_not_contain_try_again_button_expectation()); + + // Now check what the teacher sees when they review the question. + $this->displayoptions->readonly = true; + $this->check_current_output( + $this->get_contains_submit_button_expectation(false), + $this->get_does_not_contain_feedback_expectation(), + $this->get_tries_remaining_expectation(3), + $this->get_does_not_contain_try_again_button_expectation()); + $this->displayoptions->readonly = false; + + // Submit a wrong answer. + $this->process_submission(array('answer' => 'cat', '-submit' => 1)); + + // Check the Try again button now shows up correctly. + $this->check_current_state(question_state::$todo); + $this->check_current_mark(null); + $this->check_current_output( + $this->get_does_not_contain_submit_button_expectation(), + $this->get_contains_hint_expectation('This is the first hint.'), + $this->get_tries_remaining_expectation(2), + $this->get_contains_try_again_button_expectation(true)); + + // And check that a disabled Try again button shows up when the question is reviewed. + $this->displayoptions->readonly = true; + $this->check_current_output( + $this->get_does_not_contain_submit_button_expectation(), + $this->get_contains_hint_expectation('This is the first hint.'), + $this->get_tries_remaining_expectation(2), + $this->get_contains_try_again_button_expectation(false)); + } }