Commit 2bfb8d4a authored by Tim Hunt's avatar Tim Hunt
Browse files

MDL-63812 qtype_gapselect: fix questions with non-consecutive gap nos

parent fdbff6ce
...@@ -264,7 +264,6 @@ abstract class question_bank { ...@@ -264,7 +264,6 @@ abstract class question_bank {
* @return question_definition loaded from the database. * @return question_definition loaded from the database.
*/ */
public static function load_question($questionid, $allowshuffle = true) { public static function load_question($questionid, $allowshuffle = true) {
global $DB;
if (self::$testmode) { if (self::$testmode) {
// Evil, test code in production, but no way round it. // Evil, test code in production, but no way round it.
......
...@@ -34,7 +34,7 @@ defined('MOODLE_INTERNAL') || die(); ...@@ -34,7 +34,7 @@ defined('MOODLE_INTERNAL') || die();
*/ */
class qtype_ddwtos_test_helper extends question_test_helper { class qtype_ddwtos_test_helper extends question_test_helper {
public function get_test_questions() { 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 { ...@@ -128,6 +128,31 @@ class qtype_ddwtos_test_helper extends question_test_helper {
return $fromform; 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 * @return qtype_ddwtos_question
*/ */
......
...@@ -117,6 +117,31 @@ class qtype_ddwtos_test extends question_testcase { ...@@ -117,6 +117,31 @@ class qtype_ddwtos_test extends question_testcase {
$this->assertTrue($this->qtype->can_analyse_responses()); $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() { public function test_initialise_question_instance() {
$qdata = $this->get_test_question_data(); $qdata = $this->get_test_question_data();
......
...@@ -49,7 +49,35 @@ abstract class qtype_gapselect_base extends question_type { ...@@ -49,7 +49,35 @@ abstract class qtype_gapselect_base extends question_type {
public function save_question_options($question) { public function save_question_options($question) {
global $DB; global $DB;
$context = $question->context; $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', $oldanswers = $DB->get_records('question_answers',
array('question' => $question->id), 'id ASC'); array('question' => $question->id), 'id ASC');
...@@ -57,14 +85,12 @@ abstract class qtype_gapselect_base extends question_type { ...@@ -57,14 +85,12 @@ abstract class qtype_gapselect_base extends question_type {
// Insert all the new answers. // Insert all the new answers.
foreach ($question->choices as $key => $choice) { foreach ($question->choices as $key => $choice) {
if (trim($choice['answer']) == '') { // Answer guaranteed to be non-blank. See above.
continue;
}
$feedback = $this->choice_options_to_feedback($choice); $feedback = $this->choice_options_to_feedback($choice);
if ($answer = array_shift($oldanswers)) { if ($answer = array_shift($oldanswers)) {
$answer->answer = $choice['answer']; $answer->answer = trim($choice['answer']);
$answer->feedback = $feedback; $answer->feedback = $feedback;
$DB->update_record('question_answers', $answer); $DB->update_record('question_answers', $answer);
......
...@@ -33,10 +33,16 @@ defined('MOODLE_INTERNAL') || die(); ...@@ -33,10 +33,16 @@ defined('MOODLE_INTERNAL') || die();
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/ */
class qtype_gapselect_test_helper extends question_test_helper { class qtype_gapselect_test_helper extends question_test_helper {
public function get_test_questions() { 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() { public static function get_gapselect_question_data_fox() {
global $USER; global $USER;
...@@ -81,6 +87,31 @@ class qtype_gapselect_test_helper extends question_test_helper { ...@@ -81,6 +87,31 @@ class qtype_gapselect_test_helper extends question_test_helper {
return $gapselect; 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. * Get an example gapselect question to use for testing. This examples has one of each item.
* @return qtype_gapselect_question * @return qtype_gapselect_question
......
...@@ -58,6 +58,31 @@ class qtype_gapselect_test extends question_testcase { ...@@ -58,6 +58,31 @@ class qtype_gapselect_test extends question_testcase {
str_replace("\r\n", "\n", $xml)); 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. * Get some test question data.
* @return object the data to construct a question like * @return object the data to construct a question like
......
...@@ -323,9 +323,9 @@ class question_type { ...@@ -323,9 +323,9 @@ class question_type {
* is accurate any more.) * is accurate any more.)
*/ */
public function save_question($question, $form) { 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 (); $transaction = $DB->start_delegated_transaction ();
list($question->category) = explode(',', $form->category); list($question->category) = explode(',', $form->category);
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment