diff --git a/question/engine/bank.php b/question/engine/bank.php index c239388d591..5f01a2ece02 100644 --- a/question/engine/bank.php +++ b/question/engine/bank.php @@ -264,7 +264,6 @@ abstract class question_bank { * @return question_definition loaded from the database. */ public static function load_question($questionid, $allowshuffle = true) { - global $DB; if (self::$testmode) { // Evil, test code in production, but no way round it. diff --git a/question/type/ddwtos/tests/helper.php b/question/type/ddwtos/tests/helper.php index ade05700c5f..3ccd35bfd50 100644 --- a/question/type/ddwtos/tests/helper.php +++ b/question/type/ddwtos/tests/helper.php @@ -34,7 +34,7 @@ defined('MOODLE_INTERNAL') || die(); */ class qtype_ddwtos_test_helper extends question_test_helper { public function get_test_questions() { - return array('fox', 'maths', 'oddgroups'); + return array('fox', 'maths', 'oddgroups', 'missingchoiceno'); } /** @@ -128,6 +128,31 @@ class qtype_ddwtos_test_helper extends question_test_helper { return $fromform; } + /** + * Get data required to save a drag-drop into text question where the author + * missed out one of the group numbers. + * + * @return stdClass data to create a ddwtos question. + */ + public function get_ddwtos_question_form_data_missingchoiceno() { + $fromform = new stdClass(); + + $fromform->name = 'Drag-drop into text question with one index missing'; + $fromform->questiontext = ['text' => 'The [[1]] sat on the [[3]].', 'format' => FORMAT_HTML]; + $fromform->defaultmark = 1.0; + $fromform->generalfeedback = array('text' => 'The right answer is: "The cat sat on the mat."', 'format' => FORMAT_HTML); + $fromform->choices = array( + array('answer' => 'cat', 'choicegroup' => '1'), + array('answer' => '', 'choicegroup' => '1'), + array('answer' => 'mat', 'choicegroup' => '1'), + ); + test_question_maker::set_standard_combined_feedback_form_data($fromform); + $fromform->shownumcorrect = 0; + $fromform->penalty = 0.3333333; + + return $fromform; + } + /** * @return qtype_ddwtos_question */ diff --git a/question/type/ddwtos/tests/questiontype_test.php b/question/type/ddwtos/tests/questiontype_test.php index ded8ebef094..ca7f562f0bd 100644 --- a/question/type/ddwtos/tests/questiontype_test.php +++ b/question/type/ddwtos/tests/questiontype_test.php @@ -117,6 +117,31 @@ class qtype_ddwtos_test extends question_testcase { $this->assertTrue($this->qtype->can_analyse_responses()); } + public function test_save_question() { + $this->resetAfterTest(); + + $syscontext = context_system::instance(); + /** @var core_question_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $generator->create_question_category(['contextid' => $syscontext->id]); + + $fromform = test_question_maker::get_question_form_data('ddwtos', 'missingchoiceno'); + $fromform->category = $category->id . ',' . $syscontext->id; + + $question = new stdClass(); + $question->category = $category->id; + $question->qtype = 'ddwtos'; + $question->createdby = 0; + + $this->qtype->save_question($question, $fromform); + $q = question_bank::load_question($question->id); + // We just want to verify that this does not cause errors, + // but also verify some of the outcome. + $this->assertEquals('The [[1]] sat on the [[2]].', $q->questiontext); + $this->assertEquals([1 => 1, 2 => 1], $q->places); + $this->assertEquals([1 => 1, 2 => 2], $q->rightchoices); + } + public function test_initialise_question_instance() { $qdata = $this->get_test_question_data(); diff --git a/question/type/gapselect/questiontypebase.php b/question/type/gapselect/questiontypebase.php index de9f1493e74..86f4a3d5c14 100644 --- a/question/type/gapselect/questiontypebase.php +++ b/question/type/gapselect/questiontypebase.php @@ -49,7 +49,35 @@ abstract class qtype_gapselect_base extends question_type { public function save_question_options($question) { global $DB; $context = $question->context; - $result = new stdClass(); + + // This question type needs the choices to be consecutively numbered, but + // there is no reason why the question author should have done that, + // so renumber if necessary. + // Insert all the new answers. + $nonblankchoices = []; + $questiontext = $question->questiontext; + $newkey = 0; + foreach ($question->choices as $key => $choice) { + if (trim($choice['answer']) == '') { + continue; + } + + $nonblankchoices[] = $choice; + if ($newkey != $key) { + // Safe to do this in this order, because we will always be replacing + // a bigger number with a smaller number that is not present. + // Numbers in the question text always one bigger than the array index. + $questiontext = str_replace('[[' . ($key + 1) . ']]', '[[' . ($newkey + 1) . ']]', + $questiontext); + } + $newkey += 1; + } + $question->choices = $nonblankchoices; + if ($questiontext !== $question->questiontext) { + $DB->set_field('question', 'questiontext', $questiontext, + ['id' => $question->id]); + $question->questiontext = $questiontext; + } $oldanswers = $DB->get_records('question_answers', array('question' => $question->id), 'id ASC'); @@ -57,14 +85,12 @@ abstract class qtype_gapselect_base extends question_type { // Insert all the new answers. foreach ($question->choices as $key => $choice) { - if (trim($choice['answer']) == '') { - continue; - } + // Answer guaranteed to be non-blank. See above. $feedback = $this->choice_options_to_feedback($choice); if ($answer = array_shift($oldanswers)) { - $answer->answer = $choice['answer']; + $answer->answer = trim($choice['answer']); $answer->feedback = $feedback; $DB->update_record('question_answers', $answer); diff --git a/question/type/gapselect/tests/helper.php b/question/type/gapselect/tests/helper.php index ae809adc13f..5907cc122c8 100644 --- a/question/type/gapselect/tests/helper.php +++ b/question/type/gapselect/tests/helper.php @@ -33,10 +33,16 @@ defined('MOODLE_INTERNAL') || die(); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class qtype_gapselect_test_helper extends question_test_helper { + public function get_test_questions() { - return array('fox', 'maths', 'currency', 'multilang'); + return array('fox', 'maths', 'currency', 'multilang', 'missingchoiceno'); } + /** + * Get data you would get by loading a typical select missing words question. + * + * @return stdClass as returned by question_bank::load_question_data for this qtype. + */ public static function get_gapselect_question_data_fox() { global $USER; @@ -81,6 +87,31 @@ class qtype_gapselect_test_helper extends question_test_helper { return $gapselect; } + /** + * Get data required to save a select missing words question where + * the author missed out one of the group numbers. + * + * @return stdClass data to create a gapselect question. + */ + public function get_gapselect_question_form_data_missingchoiceno() { + $fromform = new stdClass(); + + $fromform->name = 'Select missing words question'; + $fromform->questiontext = ['text' => 'The [[1]] sat on the [[3]].', 'format' => FORMAT_HTML]; + $fromform->defaultmark = 1.0; + $fromform->generalfeedback = ['text' => 'The right answer is: "The cat sat on the mat."', 'format' => FORMAT_HTML]; + $fromform->choices = [ + ['answer' => 'cat', 'choicegroup' => '1'], + ['answer' => '', 'choicegroup' => '1'], + ['answer' => 'mat', 'choicegroup' => '1'], + ]; + test_question_maker::set_standard_combined_feedback_form_data($fromform); + $fromform->shownumcorrect = 0; + $fromform->penalty = 0.3333333; + + return $fromform; + } + /** * Get an example gapselect question to use for testing. This examples has one of each item. * @return qtype_gapselect_question diff --git a/question/type/gapselect/tests/questiontype_test.php b/question/type/gapselect/tests/questiontype_test.php index dc076b07499..88ccadf28af 100644 --- a/question/type/gapselect/tests/questiontype_test.php +++ b/question/type/gapselect/tests/questiontype_test.php @@ -58,6 +58,31 @@ class qtype_gapselect_test extends question_testcase { str_replace("\r\n", "\n", $xml)); } + public function test_save_question() { + $this->resetAfterTest(); + + $syscontext = context_system::instance(); + /** @var core_question_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $generator->create_question_category(['contextid' => $syscontext->id]); + + $fromform = test_question_maker::get_question_form_data('gapselect', 'missingchoiceno'); + $fromform->category = $category->id . ',' . $syscontext->id; + + $question = new stdClass(); + $question->category = $category->id; + $question->qtype = 'gapselect'; + $question->createdby = 0; + + $this->qtype->save_question($question, $fromform); + $q = question_bank::load_question($question->id); + // We just want to verify that this does not cause errors, + // but also verify some of the outcome. + $this->assertEquals('The [[1]] sat on the [[2]].', $q->questiontext); + $this->assertEquals([1 => 1, 2 => 1], $q->places); + $this->assertEquals([1 => 1, 2 => 2], $q->rightchoices); + } + /** * Get some test question data. * @return object the data to construct a question like diff --git a/question/type/questiontypebase.php b/question/type/questiontypebase.php index d8f80435c78..2dccfcaff7b 100644 --- a/question/type/questiontypebase.php +++ b/question/type/questiontypebase.php @@ -323,9 +323,9 @@ class question_type { * is accurate any more.) */ public function save_question($question, $form) { - global $USER, $DB, $OUTPUT; + global $USER, $DB; - // The actuall update/insert done with multiple DB access, so we do it in a transaction. + // The actual update/insert done with multiple DB access, so we do it in a transaction. $transaction = $DB->start_delegated_transaction (); list($question->category) = explode(',', $form->category);