diff --git a/question/type/multianswer/classes/task/cleanup_duplicate_subquestions.php b/question/type/multianswer/classes/task/cleanup_duplicate_subquestions.php new file mode 100644 index 00000000000..6c0a4f3cda3 --- /dev/null +++ b/question/type/multianswer/classes/task/cleanup_duplicate_subquestions.php @@ -0,0 +1,168 @@ +. + +namespace qtype_multianswer\task; + +use context_system; +use core\task\stored_progress_task_trait; +use core_question\local\bank\question_version_status; +use question_bank; +use question_engine_data_mapper; + +/** + * Cleanup duplicate subquestions + * + * Due to MDL-85721, there may be duplicated subquestions in the database. These have a question bank entry, question version, + * and question record with a parent, but they are not referred to in that parent's sequence. + * + * @package qtype_multianswer + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class cleanup_duplicate_subquestions extends \core\task\adhoc_task { + + /** + * @var int The current progress counter. + */ + protected int $counter; + + /** + * Find questions where there are other questions with identical text, stamp and multianswer parent + * + * We may have multiple subquestions with the same stamp but different text or parents due to historical bugs, + * so this includes the ID field from one of the duplicates to ensure we have a unique first field. + * + * @return array + */ + public function find_duplicated_subquestions(): array { + global $DB; + return $DB->get_records_sql(" + SELECT MIN(subq.id) AS firstid, subq.stamp, subq.questiontext, subq.parent, qm.sequence, COUNT(1) AS count + FROM {question} subq + JOIN {question} q ON q.id = subq.parent + JOIN {question_multianswer} qm ON q.id = qm.question + WHERE q.qtype = 'multianswer' + GROUP BY subq.stamp, subq.questiontext, subq.parent, qm.sequence + HAVING COUNT(1) > 1; + "); + } + + /** + * Output a progress dot, with a count of the total every 70 items. + */ + protected function increment_progress(): void { + mtrace('.', ''); + $this->counter++; + if ($this->counter % 70 === 0) { + mtrace(" {$this->counter}"); + } + } + + #[\Override] + public function execute() { + global $CFG, $DB; + require_once($CFG->libdir . '/questionlib.php'); + + $duplicatedsubquestions = $this->find_duplicated_subquestions(); + + $duplicatedcount = count($duplicatedsubquestions); + + if ($duplicatedcount === 0) { + mtrace("No duplicated questions found."); + return; + } + + mtrace("Found {$duplicatedcount} subquestions with duplicates."); + + foreach ($duplicatedsubquestions as $subquestion) { + // Find instances of the subquestion that do not appear in the sequence of the parent. + [$insql, $inparams] = $DB->get_in_or_equal(explode(',', $subquestion->sequence), equal: false); + $params = array_merge([$subquestion->parent, $subquestion->stamp], $inparams); + $duplicates = $DB->get_records_select('question', "parent = ? AND stamp = ? AND id {$insql}", $params); + $duplicatecount = count($duplicates); + // Delete each duplicate, with a progress bar. + mtrace(""); + mtrace("Deleting {$duplicatecount} duplicates of {$subquestion->stamp}:"); + $this->counter = 0; + foreach ($duplicates as $duplicate) { + // Based on question_delete_question(), without checking for parent usage or deleting children. + // If the question is being used, just mark it as hidden. Otherwise, delete the question, version and question bank + // entry. + $sql = "SELECT qv.id as versionid, + qv.version, + qbe.id as entryid, + qc.id as categoryid, + ctx.id as contextid + FROM {question} q + LEFT JOIN {question_versions} qv ON qv.questionid = q.id + LEFT JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid + LEFT JOIN {question_categories} qc ON qc.id = qbe.questioncategoryid + LEFT JOIN {context} ctx ON ctx.id = qc.contextid + WHERE q.id = ?"; + $questiondata = $DB->get_record_sql($sql, [$duplicate->id]); + + // Do not delete a question if it is used by an activity module. Just mark the version hidden. + if (questions_in_use([$duplicate->id])) { + $DB->set_field( + 'question_versions', + 'status', + question_version_status::QUESTION_STATUS_HIDDEN, + ['questionid' => $duplicate->id] + ); + $this->increment_progress(); + continue; + } + + // This sometimes happens in old sites with bad data. + if (!$questiondata->contextid) { + debugging('Deleting question ' . $duplicate->id . ' which is no longer linked to a context. ' . + 'Assuming system context to avoid errors, but this may mean that some data like files, ' . + 'tags, are not cleaned up.'); + $questiondata->contextid = context_system::instance()->id; + $questiondata->categoryid = 0; + } + + // Delete previews of the question. + $dm = new question_engine_data_mapper(); + $dm->delete_previews($duplicate->id); + + // Delete questiontype-specific data. + question_bank::get_qtype($duplicate->qtype, false)->delete_question($duplicate->id, $questiondata->contextid); + + // Finally delete the question record itself. + $DB->delete_records('question', ['id' => $duplicate->id]); + $DB->delete_records('question_versions', ['id' => $questiondata->versionid]); + $DB->delete_records('question_references', + [ + 'version' => $questiondata->version, + 'questionbankentryid' => $questiondata->entryid, + ]); + delete_question_bank_entry($questiondata->entryid); + question_bank::notify_question_edited($duplicate->id); + + // Log the deletion of this question. + $duplicate->category = $questiondata->categoryid; + $duplicate->contextid = $questiondata->contextid; + $event = \core\event\question_deleted::create_from_question_instance($duplicate); + $event->add_record_snapshot('question', $duplicate); + $event->trigger(); + + $this->increment_progress(); + } + } + } +} diff --git a/question/type/multianswer/db/upgrade.php b/question/type/multianswer/db/upgrade.php index ec472df467c..935a9ae41ac 100644 --- a/question/type/multianswer/db/upgrade.php +++ b/question/type/multianswer/db/upgrade.php @@ -43,5 +43,14 @@ function xmldb_qtype_multianswer_upgrade($oldversion) { // Automatically generated Moodle v4.5.0 release upgrade line. // Put any upgrade step following this. + if ($oldversion < 2024100701) { + $task = new \qtype_multianswer\task\cleanup_duplicate_subquestions(); + if (count($task->find_duplicated_subquestions()) > 0) { + mtrace('Duplicated subquestions found. Queueing cleanup task.'); + \core\task\manager::queue_adhoc_task($task); + } + upgrade_plugin_savepoint(true, 2024100701, 'qtype', 'multianswer'); + } + return true; } diff --git a/question/type/multianswer/questiontype.php b/question/type/multianswer/questiontype.php index b9b18b3abeb..0c44b18a56f 100644 --- a/question/type/multianswer/questiontype.php +++ b/question/type/multianswer/questiontype.php @@ -162,9 +162,9 @@ class qtype_multianswer extends question_type { // This function needs to be able to handle the case where the existing set of wrapped // questions does not match the new set of wrapped questions so that some need to be // created, some modified and some deleted. - // Unfortunately the code currently simply overwrites existing ones in sequence. This - // will make re-marking after a re-ordering of wrapped questions impossible and - // will also create difficulties if questiontype specific tables reference the id. + // Thanks to versioning, we no longer overwrite existing questions in the sequence + // by re-using IDs, but instead create a new version of each wrapped question for + // the new version of the parent. // First we get all the existing wrapped questions. $oldwrappedquestions = []; @@ -186,30 +186,12 @@ class qtype_multianswer extends question_type { $sequence = array(); foreach ($question->options->questions as $wrapped) { if (!empty($wrapped)) { - // If we still have some old wrapped question ids, reuse the next of them. + // If we still have some old wrapped question ids, reuse the next of them to save + // the new version against its question bank entry. $wrapped->id = 0; if (is_array($oldwrappedquestions) && $oldwrappedquestion = array_shift($oldwrappedquestions)) { $wrapped->oldid = $oldwrappedquestion->id; - if ($oldwrappedquestion->qtype != $wrapped->qtype) { - switch ($oldwrappedquestion->qtype) { - case 'multichoice': - $DB->delete_records('qtype_multichoice_options', - array('questionid' => $oldwrappedquestion->id)); - break; - case 'shortanswer': - $DB->delete_records('qtype_shortanswer_options', - array('questionid' => $oldwrappedquestion->id)); - break; - case 'numerical': - $DB->delete_records('question_numerical', - array('question' => $oldwrappedquestion->id)); - break; - default: - throw new moodle_exception('qtypenotrecognized', - 'qtype_multianswer', '', $oldwrappedquestion->qtype); - } - } } } $wrapped->name = $question->name; diff --git a/question/type/multianswer/tests/question_type_test.php b/question/type/multianswer/tests/question_type_test.php index 3b2b9e81da7..98c4ed963ba 100644 --- a/question/type/multianswer/tests/question_type_test.php +++ b/question/type/multianswer/tests/question_type_test.php @@ -20,7 +20,9 @@ use qtype_multianswer; use qtype_multianswer_edit_form; use qtype_multichoice_base; use question_bank; +use stdClass; use test_question_maker; +use core\context; defined('MOODLE_INTERNAL') || die(); @@ -79,11 +81,13 @@ final class question_type_test extends \advanced_testcase { $q->timemodified = time(); $q->createdby = $USER->id; $q->modifiedby = $USER->id; + $q->options = new stdClass(); $sadata = new \stdClass(); $sadata->id = 1; $sadata->qtype = 'shortanswer'; $sadata->defaultmark = 1; + $sadata->options = new stdClass(); $sadata->options->usecase = true; $sadata->options->answers[1] = (object) array('answer' => 'Bow-wow', 'fraction' => 0); $sadata->options->answers[2] = (object) array('answer' => 'Wiggly worm', 'fraction' => 0); @@ -93,6 +97,7 @@ final class question_type_test extends \advanced_testcase { $mcdata->id = 1; $mcdata->qtype = 'multichoice'; $mcdata->defaultmark = 1; + $mcdata->options = new stdClass(); $mcdata->options->single = true; $mcdata->options->answers[1] = (object) array('answer' => 'Dog', 'fraction' => 0); $mcdata->options->answers[2] = (object) array('answer' => 'Owl', 'fraction' => 1); @@ -434,4 +439,58 @@ final class question_type_test extends \advanced_testcase { $this->assertCount(2, $questiondata->options->questions); $this->assertEquals('subquestion_replacement', $questiondata->options->questions[$questiontodeletekey]->qtype); } + + /** + * Saving a new version of the question should retain the original subquestion versions, with their own qtype data. + */ + public function test_save_question_options(): void { + global $DB; + $this->resetAfterTest(true); + $this->setAdminUser(); + + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $cat = $generator->create_question_category([]); + $question = $generator->create_question('multianswer', 'twosubq', ['category' => $cat->id]); + + get_question_options($question); + $originalsubq1 = reset($question->options->questions); + $originalsubq2 = next($question->options->questions); + + // Assert that the original subquestions are the expected types, and they have options records. + $this->assertEquals('shortanswer', $originalsubq1->qtype); + $this->assertTrue($DB->record_exists('qtype_shortanswer_options', ['questionid' => $originalsubq1->id])); + $this->assertEquals('multichoice', $originalsubq2->qtype); + $this->assertTrue($DB->record_exists('qtype_multichoice_options', ['questionid' => $originalsubq2->id])); + + // Edit the question, replacing the subquestions with two new questions of different types. + $editedquestion = test_question_maker::get_question_data('multianswer', 'twosubq'); + $editedquestion->id = $question->id; + $editedquestion->category = $cat->id; + $editedquestion->context = context::instance_by_id($cat->contextid); + $editedsubq1 = test_question_maker::get_question_form_data('multichoice', 'one_of_four'); + $editedsubq1->id = $originalsubq1->id; + $editedsubq1->qtype = 'multichoice'; + $editedsubq2 = test_question_maker::get_question_form_data('shortanswer', 'frogtoad'); + $editedsubq2->id = $originalsubq2->id; + $editedsubq2->qtype = 'shortanswer'; + $editedquestion->options->questions = [$editedsubq1, $editedsubq2]; + $this->qtype->save_question_options($editedquestion); + + $newquestion = $DB->get_record('question', ['id' => $question->id]); + get_question_options($newquestion); + $newsubq1 = reset($newquestion->options->questions); + $newsubq2 = next($newquestion->options->questions); + + // The new subquestions are different types, and did not re-use IDs from the original subquestions. + $this->assertEquals('multichoice', $newsubq1->qtype); + $this->assertFalse(in_array($newsubq1->id, [$originalsubq1->id, $originalsubq2->id])); + $this->assertEquals('shortanswer', $newsubq2->qtype); + $this->assertFalse(in_array($newsubq2->id, [$originalsubq1->id, $originalsubq2->id])); + + // The original questions and option records still exist. + $this->assertTrue($DB->record_exists('question', ['id' => $originalsubq1->id])); + $this->assertTrue($DB->record_exists('qtype_shortanswer_options', ['questionid' => $originalsubq1->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $originalsubq2->id])); + $this->assertTrue($DB->record_exists('qtype_multichoice_options', ['questionid' => $originalsubq2->id])); + } } diff --git a/question/type/multianswer/tests/task/cleanup_duplicate_subquestions_test.php b/question/type/multianswer/tests/task/cleanup_duplicate_subquestions_test.php new file mode 100644 index 00000000000..f04df39f4d9 --- /dev/null +++ b/question/type/multianswer/tests/task/cleanup_duplicate_subquestions_test.php @@ -0,0 +1,310 @@ +. + +namespace qtype_multianswer\task; + +/** + * Unit tests for cleanup_duplicate_subquestions + * + * @package qtype_multianswer + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \qtype_multianswer\task\cleanup_duplicate_subquestions + */ +final class cleanup_duplicate_subquestions_test extends \advanced_testcase { + + /** + * Create a multianswer question and duplicate its subquestions. + * + * @return array + * @throws \dml_exception + */ + protected function generate_duplicated_subquestions(): array { + global $DB; + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $generator->create_question_category(); + $question = $generator->create_question('multianswer', 'twosubq', ['category' => $category->id]); + + $subquestions = $DB->get_records('question', ['parent' => $question->id]); + foreach ($subquestions as $subquestion) { + $version = $DB->get_record('question_versions', ['questionid' => $subquestion->id]); + $qbe = $DB->get_record('question_bank_entries', ['id' => $version->questionbankentryid]); + $duplicate = clone($subquestion); + unset($duplicate->id); + $duplicate->id = $DB->insert_record('question', $duplicate); + $duplicateqbe = clone($qbe); + unset($duplicateqbe->id); + $duplicateqbe->id = $DB->insert_record('question_bank_entries', $duplicateqbe); + $duplicateversion = clone($version); + unset($duplicateversion->id); + $duplicateversion->questionid = $duplicate->id; + $duplicateversion->questionbankentryid = $duplicateqbe->id; + $duplicateversion->id = $DB->insert_record('question_versions', $duplicateversion); + $subquestion->duplicate = (object) [ + 'question' => $duplicate, + 'version' => $duplicateversion, + 'questionbankentry' => $duplicateqbe, + ]; + } + return $subquestions; + } + + /** + * We should correctly find subquestions with duplicate records. + */ + public function test_find_duplicated_subquestions(): void { + $this->resetAfterTest(); + $task = new cleanup_duplicate_subquestions(); + + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $generator->create_question_category(); + $generator->create_question('multianswer', 'twosubq', ['category' => $category->id]); + + $this->assertEquals(0, count($task->find_duplicated_subquestions())); + + $this->generate_duplicated_subquestions(); + + $this->assertEquals(2, count($task->find_duplicated_subquestions())); + } + + /** + * Delete duplicate subquestions, but not the originals. + */ + public function test_execute(): void { + global $DB; + $this->resetAfterTest(); + $task = new cleanup_duplicate_subquestions(); + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $generator->create_question_category(); + $normalquestion = $generator->create_question('multianswer', 'twosubq', ['category' => $category->id]); + $normalsubquestions = $DB->get_records('question', ['parent' => $normalquestion->id]); + $duplicatedsubquestions = $this->generate_duplicated_subquestions(); + + foreach ($duplicatedsubquestions as $subquestion) { + $this->expectOutputRegex("~{$subquestion->stamp}~"); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->parent])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->duplicate->question->id])); + $this->assertTrue($DB->record_exists('question_versions', ['id' => $subquestion->duplicate->version->id])); + $this->assertTrue( + $DB->record_exists('question_bank_entries', ['id' => $subquestion->duplicate->questionbankentry->id]), + ); + } + + $this->expectOutputRegex('~Found 2 subquestions with duplicates~'); + $task->execute(); + + // The non-duplicated questions should not have been touched. + $this->assertTrue($DB->record_exists('question', ['id' => $normalquestion->id])); + foreach ($normalsubquestions as $subquestion) { + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + } + + // The duplicated questions should have the duplicates deleted, but the originals intact. + foreach ($duplicatedsubquestions as $subquestion) { + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->parent])); + $this->assertFalse($DB->record_exists('question', ['id' => $subquestion->duplicate->question->id])); + $this->assertFalse($DB->record_exists('question_versions', ['id' => $subquestion->duplicate->version->id])); + $this->assertFalse( + $DB->record_exists('question_bank_entries', ['id' => $subquestion->duplicate->questionbankentry->id]), + ); + } + } + + /** + * Don't delete a duplicate subquestion if its somehow being used somewhere. + * + * This should never really happen, but just to be on the safe side. + */ + public function test_execute_with_usage(): void { + global $DB; + $this->resetAfterTest(); + $task = new cleanup_duplicate_subquestions(); + $course = $this->getDataGenerator()->create_course(); + $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); + $quiz = $quizgenerator->create_instance(['course' => $course->id]); + + $duplicatedsubquestions = $this->generate_duplicated_subquestions(); + $firstsubquestion = reset($duplicatedsubquestions); + $secondsubquestion = next($duplicatedsubquestions); + + quiz_add_quiz_question($firstsubquestion->duplicate->question->id, $quiz); + + foreach ($duplicatedsubquestions as $subquestion) { + $this->expectOutputRegex("~{$subquestion->stamp}~"); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->parent])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->duplicate->question->id])); + $this->assertTrue($DB->record_exists('question_versions', ['id' => $subquestion->duplicate->version->id])); + $this->assertTrue( + $DB->record_exists('question_bank_entries', ['id' => $subquestion->duplicate->questionbankentry->id]), + ); + } + + $this->expectOutputRegex('~Found 2 subquestions with duplicates~'); + $task->execute(); + + // The subquestion duplicate which was added to the quiz has not been deleted. + $this->assertTrue($DB->record_exists('question', ['id' => $firstsubquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $firstsubquestion->parent])); + $this->assertTrue($DB->record_exists('question', ['id' => $firstsubquestion->duplicate->question->id])); + $this->assertTrue($DB->record_exists('question_versions', ['id' => $firstsubquestion->duplicate->version->id])); + $this->assertTrue( + $DB->record_exists('question_bank_entries', ['id' => $firstsubquestion->duplicate->questionbankentry->id]), + ); + + // The subquestion duplicate which was not added to the quiz, was deleted. + $this->assertTrue($DB->record_exists('question', ['id' => $secondsubquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $secondsubquestion->parent])); + $this->assertFalse($DB->record_exists('question', ['id' => $secondsubquestion->duplicate->question->id])); + $this->assertFalse($DB->record_exists('question_versions', ['id' => $secondsubquestion->duplicate->version->id])); + $this->assertFalse( + $DB->record_exists('question_bank_entries', ['id' => $secondsubquestion->duplicate->questionbankentry->id]), + ); + } + + /** + * Delete a duplicate subquestion even if its parent is being used. + */ + public function test_execute_with_parent_usage(): void { + global $DB; + $this->resetAfterTest(); + $task = new cleanup_duplicate_subquestions(); + $course = $this->getDataGenerator()->create_course(); + $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); + $quiz = $quizgenerator->create_instance(['course' => $course->id]); + + $duplicatedsubquestions = $this->generate_duplicated_subquestions(); + + $firstsubquestion = reset($duplicatedsubquestions); + quiz_add_quiz_question($firstsubquestion->parent, $quiz); + + foreach ($duplicatedsubquestions as $subquestion) { + $this->expectOutputRegex("~{$subquestion->stamp}~"); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->parent])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->duplicate->question->id])); + $this->assertTrue($DB->record_exists('question_versions', ['id' => $subquestion->duplicate->version->id])); + $this->assertTrue( + $DB->record_exists('question_bank_entries', ['id' => $subquestion->duplicate->questionbankentry->id]), + ); + } + + $this->expectOutputRegex('~Found 2 subquestions with duplicates~'); + $task->execute(); + + // The duplicated questions should have the duplicates deleted, but the originals intact. + foreach ($duplicatedsubquestions as $subquestion) { + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->parent])); + $this->assertFalse($DB->record_exists('question', ['id' => $subquestion->duplicate->question->id])); + $this->assertFalse($DB->record_exists('question_versions', ['id' => $subquestion->duplicate->version->id])); + $this->assertFalse( + $DB->record_exists('question_bank_entries', ['id' => $subquestion->duplicate->questionbankentry->id]), + ); + } + } + + /** + * For historical reasons, we might have multiple different questions with the same stamp. Ensure we can handle this. + */ + public function test_execute_duplicate_stamp(): void { + global $DB; + $this->resetAfterTest(); + $task = new cleanup_duplicate_subquestions(); + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $generator->create_question_category(); + $normalquestion = $generator->create_question('multianswer', 'twosubq', ['category' => $category->id]); + $normalsubquestions = $DB->get_records('question', ['parent' => $normalquestion->id]); + $duplicatedsubquestions1 = array_values($this->generate_duplicated_subquestions()); + $duplicatedsubquestions2 = array_values($this->generate_duplicated_subquestions()); + + $this->expectOutputRegex("~((?!Did you remember to make the first column something unique).)*$~"); + + foreach ($duplicatedsubquestions1 as $key => $subquestion) { + $this->expectOutputRegex("~{$subquestion->stamp}~"); + $this->expectOutputRegex("~((?!{$duplicatedsubquestions2[$key]->stamp}).)*$~"); + + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->parent])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->duplicate->question->id])); + $this->assertTrue($DB->record_exists('question_versions', ['id' => $subquestion->duplicate->version->id])); + $this->assertTrue( + $DB->record_exists('question_bank_entries', ['id' => $subquestion->duplicate->questionbankentry->id]), + ); + $this->assertTrue($DB->record_exists('question', ['id' => $duplicatedsubquestions2[$key]->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $duplicatedsubquestions2[$key]->parent])); + $this->assertTrue($DB->record_exists('question', ['id' => $duplicatedsubquestions2[$key]->duplicate->question->id])); + $this->assertTrue( + $DB->record_exists('question_versions', ['id' => $duplicatedsubquestions2[$key]->duplicate->version->id]) + ); + $this->assertTrue( + $DB->record_exists( + 'question_bank_entries', + ['id' => $duplicatedsubquestions2[$key]->duplicate->questionbankentry->id], + ), + ); + // Set the stamp of the second generated subquestion and its duplicate to match the first. + $DB->update_record( + 'question', + (object) [ + 'id' => $duplicatedsubquestions2[$key]->id, + 'stamp' => $subquestion->stamp, + ], + ); + $DB->update_record( + 'question', + (object) [ + 'id' => $duplicatedsubquestions2[$key]->duplicate->version->questionid, + 'stamp' => $subquestion->stamp, + ], + ); + } + + $this->expectOutputRegex('~Found 4 subquestions with duplicates~'); + $task->execute(); + + // The non-duplicated questions should not have been touched. + $this->assertTrue($DB->record_exists('question', ['id' => $normalquestion->id])); + foreach ($normalsubquestions as $subquestion) { + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + } + + // The duplicated questions should have the duplicates deleted, but the originals intact. + foreach ($duplicatedsubquestions1 as $key => $subquestion) { + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $subquestion->parent])); + $this->assertFalse($DB->record_exists('question', ['id' => $subquestion->duplicate->question->id])); + $this->assertFalse($DB->record_exists('question_versions', ['id' => $subquestion->duplicate->version->id])); + $this->assertFalse( + $DB->record_exists('question_bank_entries', ['id' => $subquestion->duplicate->questionbankentry->id]), + ); + $this->assertTrue($DB->record_exists('question', ['id' => $duplicatedsubquestions2[$key]->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $duplicatedsubquestions2[$key]->parent])); + $this->assertFalse($DB->record_exists('question', ['id' => $duplicatedsubquestions2[$key]->duplicate->question->id])); + $this->assertFalse( + $DB->record_exists('question_versions', ['id' => $duplicatedsubquestions2[$key]->duplicate->version->id])); + $this->assertFalse( + $DB->record_exists( + 'question_bank_entries', + ['id' => $duplicatedsubquestions2[$key]->duplicate->questionbankentry->id], + ), + ); + } + } +} diff --git a/question/type/multianswer/version.php b/question/type/multianswer/version.php index 1abfe71f3cd..02316fea40d 100644 --- a/question/type/multianswer/version.php +++ b/question/type/multianswer/version.php @@ -26,7 +26,7 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'qtype_multianswer'; -$plugin->version = 2024100700; +$plugin->version = 2024100701; $plugin->requires = 2024100100; $plugin->dependencies = [ diff --git a/question/type/multichoice/backup/moodle2/restore_qtype_multichoice_plugin.class.php b/question/type/multichoice/backup/moodle2/restore_qtype_multichoice_plugin.class.php index 7ef6ab68ec7..71120c4fec0 100644 --- a/question/type/multichoice/backup/moodle2/restore_qtype_multichoice_plugin.class.php +++ b/question/type/multichoice/backup/moodle2/restore_qtype_multichoice_plugin.class.php @@ -160,4 +160,19 @@ class restore_qtype_multichoice_plugin extends restore_qtype_plugin { return $contents; } + + #[\Override] + public static function convert_backup_to_questiondata(array $backupdata): \stdClass { + global $CFG; + require_once($CFG->dirroot . '/question/type/multichoice/questiontype.php'); + $questiondata = parent::convert_backup_to_questiondata($backupdata); + if (count(get_object_vars($questiondata->options)) <= 1) { + // Historically, old versions of multichoice subquestions had their options record deleted. + // As qtype_multichoice::get_question_options() sets default options in this case, we need + // to do the same here. See MDL-85721. + $defaultoptions = (new qtype_multichoice())->create_default_options($questiondata); + $questiondata->options = (object) array_merge((array) $questiondata->options, (array) $defaultoptions); + } + return $questiondata; + } } diff --git a/question/type/multichoice/questiontype.php b/question/type/multichoice/questiontype.php index b1d1b4ff0cf..59627b2d73a 100644 --- a/question/type/multichoice/questiontype.php +++ b/question/type/multichoice/questiontype.php @@ -88,7 +88,7 @@ class qtype_multichoice extends question_type { * @param object $question The queston we are working with. * @return object The options object. */ - protected function create_default_options($question) { + public function create_default_options($question) { // Create a default question options record. $options = new stdClass(); $options->questionid = $question->id; diff --git a/question/type/multichoice/tests/restore_test.php b/question/type/multichoice/tests/restore_test.php new file mode 100644 index 00000000000..7c981323a25 --- /dev/null +++ b/question/type/multichoice/tests/restore_test.php @@ -0,0 +1,86 @@ +. + +namespace qtype_multichoice; + +/** + * Unit tests for restore_qtype_multichoice_plugin + * + * @package qtype_multichoice + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \restore_qtype_multichoice_plugin + */ +final class restore_test extends \advanced_testcase { + /** + * Duplicate a quiz containing a multichoice question with no options record. + */ + public function test_restore_quiz_with_edited_questions(): void { + global $CFG, $DB, $USER; + require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); + require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); + $this->resetAfterTest(); + $this->setAdminUser(); + + // Create a course and a user with editing teacher capabilities. + $generator = $this->getDataGenerator(); + $course1 = $generator->create_course(); + $context = \context_course::instance($course1->id); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $initialcount = $DB->count_records('question'); + + // Create a question category. + $cat = $questiongenerator->create_question_category(['contextid' => $context->id]); + + // Create a quiz containing a multichoice question from the qbank. + $quiz = $this->getDataGenerator()->get_plugin_generator('mod_quiz')->create_instance(['course' => $course1->id]); + $question = $questiongenerator->create_question('multichoice', 'one_of_four', ['category' => $cat->id]); + quiz_add_quiz_question($question->id, $quiz); + + // Delete the multichoice_options record. + $DB->delete_records('qtype_multichoice_options', ['questionid' => $question->id]); + + // Confirm we have created 1 additional question. + $this->assertEquals($initialcount + 1, $DB->count_records('question')); + + // Backup quiz. + $bc = new \backup_controller(\backup::TYPE_1ACTIVITY, $quiz->cmid, \backup::FORMAT_MOODLE, + \backup::INTERACTIVE_NO, \backup::MODE_IMPORT, $USER->id); + $backupid = $bc->get_backupid(); + $bc->execute_plan(); + $bc->destroy(); + + // Restore the backup into the same course. + $rc = new \restore_controller($backupid, $course1->id, \backup::INTERACTIVE_NO, \backup::MODE_IMPORT, + $USER->id, \backup::TARGET_CURRENT_ADDING); + $rc->execute_precheck(); + $rc->execute_plan(); + $rc->destroy(); + $this->assertDebuggingCalled(); + + // Both quizzes should refer to the same original question. + $quizzes = get_fast_modinfo($course1->id)->get_instances_of('quiz'); + $this->assertCount(2, $quizzes); + foreach ($quizzes as $quiz) { + $structure = \mod_quiz\question\bank\qbank_helper::get_question_structure($quiz->instance, $quiz->context); + $this->assertEquals($structure[1]->questionid, $question->id); + } + + // There should be no additional questions created during the restore. + $this->assertEquals($initialcount + 1, $DB->count_records('question')); + } +} diff --git a/question/type/numerical/backup/moodle2/restore_qtype_numerical_plugin.class.php b/question/type/numerical/backup/moodle2/restore_qtype_numerical_plugin.class.php index 42cdcfeb4ca..990d5196da2 100644 --- a/question/type/numerical/backup/moodle2/restore_qtype_numerical_plugin.class.php +++ b/question/type/numerical/backup/moodle2/restore_qtype_numerical_plugin.class.php @@ -83,14 +83,35 @@ class restore_qtype_numerical_plugin extends restore_qtype_plugin { #[\Override] public static function convert_backup_to_questiondata(array $backupdata): \stdClass { + global $CFG; + require_once($CFG->dirroot . '/question/type/numerical/questiontype.php'); $questiondata = parent::convert_backup_to_questiondata($backupdata); - foreach ($backupdata['plugin_qtype_numerical_question']['numerical_records']['numerical_record'] as $record) { - foreach ($questiondata->options->answers as &$answer) { - if ($answer->id == $record['answer']) { - $answer->tolerance = $record['tolerance']; - continue 2; + if (count(get_object_vars($questiondata->options)) <= 2) { + // Old question, set defaults. + $qtype = new qtype_numerical(); + $questiondata->options->unitgradingtype = 0; + $questiondata->options->unitpenalty = 0.1; + if ($qtype->get_default_numerical_unit($questiondata)) { + $questiondata->options->showunits = $qtype::UNITINPUT; + } else { + $questiondata->options->showunits = $qtype::UNITNONE; + } + $questiondata->options->unitsleft = 0; + } + if (isset($backupdata['plugin_qtype_numerical_question']['numerical_records'])) { + foreach ($backupdata['plugin_qtype_numerical_question']['numerical_records']['numerical_record'] as $record) { + foreach ($questiondata->options->answers as &$answer) { + if ($answer->id == $record['answer']) { + $answer->tolerance = $record['tolerance']; + continue 2; + } } } + } else { + // If the numerical record is missing (e.g. MDL-85721), default tolerances to 0. + foreach ($questiondata->options->answers as &$answer) { + $answer->tolerance = 0; + } } return $questiondata; } diff --git a/question/type/numerical/questiontype.php b/question/type/numerical/questiontype.php index 1cbed5df3e7..387073d135b 100644 --- a/question/type/numerical/questiontype.php +++ b/question/type/numerical/questiontype.php @@ -64,22 +64,25 @@ class qtype_numerical extends question_type { } public function get_question_options($question) { - global $CFG, $DB, $OUTPUT; + global $DB; parent::get_question_options($question); // Get the question answers and their respective tolerances // Note: question_numerical is an extension of the answer table rather than // the question table as is usually the case for qtype // specific tables. + // If the numerical record is missing for some reason (e.g. MDL-85721), use a default tolerance. if (!$question->options->answers = $DB->get_records_sql( - "SELECT a.*, n.tolerance " . - "FROM {question_answers} a, " . - " {question_numerical} n " . - "WHERE a.question = ? " . - " AND a.id = n.answer " . - "ORDER BY a.id ASC", array($question->id))) { - echo $OUTPUT->notification('Error: Missing question answer for numerical question ' . + " + SELECT a.*, COALESCE(n.tolerance, '0') AS tolerance + FROM {question_answers} a + LEFT JOIN {question_numerical} n ON a.id = n.answer + WHERE a.question = ? + ORDER BY a.id ASC + ", + [$question->id], + )) { + debugging('Error: Missing question answer for numerical question ' . $question->id . '!'); - return false; } $question->hints = $DB->get_records('question_hints', diff --git a/question/type/numerical/tests/restore_test.php b/question/type/numerical/tests/restore_test.php new file mode 100644 index 00000000000..44b6930deef --- /dev/null +++ b/question/type/numerical/tests/restore_test.php @@ -0,0 +1,86 @@ +. + +namespace qtype_numerical; + +/** + * Unit tests for restore_qtype_numerical_plugin + * + * @package qtype_numerical + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \restore_qtype_numerical_plugin + */ +final class restore_test extends \advanced_testcase { + /** + * Duplicate a quiz containing a numerical question with no numerical record or numerical_options record. + */ + public function test_restore_quiz_with_edited_questions(): void { + global $CFG, $DB, $USER; + require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); + require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); + $this->resetAfterTest(); + $this->setAdminUser(); + + // Create a course and a user with editing teacher capabilities. + $generator = $this->getDataGenerator(); + $course1 = $generator->create_course(); + $context = \context_course::instance($course1->id); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $initialcount = $DB->count_records('question'); + + // Create a question category. + $cat = $questiongenerator->create_question_category(['contextid' => $context->id]); + + // Create a quiz containing a multichoice question from the qbank. + $quiz = $this->getDataGenerator()->get_plugin_generator('mod_quiz')->create_instance(['course' => $course1->id]); + $question = $questiongenerator->create_question('numerical', 'pi', ['category' => $cat->id]); + quiz_add_quiz_question($question->id, $quiz); + + // Delete the numerical and numerical_options record, so we rely on defaults. + $DB->delete_records('question_numerical', ['question' => $question->id]); + $DB->delete_records('question_numerical_options', ['question' => $question->id]); + + // Confirm we have created 1 additional question. + $this->assertEquals($initialcount + 1, $DB->count_records('question')); + + // Backup quiz. + $bc = new \backup_controller(\backup::TYPE_1ACTIVITY, $quiz->cmid, \backup::FORMAT_MOODLE, + \backup::INTERACTIVE_NO, \backup::MODE_IMPORT, $USER->id); + $backupid = $bc->get_backupid(); + $bc->execute_plan(); + $bc->destroy(); + + // Restore the backup into the same course. + $rc = new \restore_controller($backupid, $course1->id, \backup::INTERACTIVE_NO, \backup::MODE_IMPORT, + $USER->id, \backup::TARGET_CURRENT_ADDING); + $rc->execute_precheck(); + $rc->execute_plan(); + $rc->destroy(); + + // Both quizzes should refer to the same original question. + $quizzes = get_fast_modinfo($course1->id)->get_instances_of('quiz'); + $this->assertCount(2, $quizzes); + foreach ($quizzes as $quiz) { + $structure = \mod_quiz\question\bank\qbank_helper::get_question_structure($quiz->instance, $quiz->context); + $this->assertEquals($structure[1]->questionid, $question->id); + } + + // There should be no additional questions created during the restore. + $this->assertEquals($initialcount + 1, $DB->count_records('question')); + } +} diff --git a/question/type/questiontypebase.php b/question/type/questiontypebase.php index 599dd419cd4..3ab6bdfdfd4 100644 --- a/question/type/questiontypebase.php +++ b/question/type/questiontypebase.php @@ -904,7 +904,7 @@ class question_type { * specific information (it is passed by reference). */ public function get_question_options($question) { - global $DB, $OUTPUT; + global $DB; if (!isset($question->options)) { $question->options = new stdClass(); @@ -921,9 +921,8 @@ class question_type { $question->options->$field = $extra_data->$field; } } else { - echo $OUTPUT->notification('Failed to load question options from the table ' . + debugging('Failed to load question options from the table ' . $question_extension_table . ' for questionid ' . $question->id); - return false; } } @@ -938,9 +937,8 @@ class question_type { WHERE qa.question = ? ORDER BY qa.id", array($question->id)); if (!$answers) { - echo $OUTPUT->notification('Failed to load question answers from the table ' . - $answerextensiontable . 'for questionid ' . $question->id); - return false; + debugging('Failed to load question answers from the table ' . + $answerextensiontable . ' for questionid ' . $question->id); } } else { // Don't check for success or failure because some question types do diff --git a/question/type/shortanswer/tests/restore_test.php b/question/type/shortanswer/tests/restore_test.php new file mode 100644 index 00000000000..bffb3bd1f92 --- /dev/null +++ b/question/type/shortanswer/tests/restore_test.php @@ -0,0 +1,89 @@ +. + +namespace qtype_shortanswer; + +/** + * Unit tests for restore_qtype_shortanswer_plugin + * + * @package qtype_shortanswer + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \restore_qtype_shortanswer_plugin + */ +final class restore_test extends \advanced_testcase { + /** + * Duplicate a quiz containing a shortanswer question with no options record. + */ + public function test_restore_quiz_with_edited_questions(): void { + global $CFG, $DB, $USER; + require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); + require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); + $this->resetAfterTest(); + $this->setAdminUser(); + + // Create a course and a user with editing teacher capabilities. + $generator = $this->getDataGenerator(); + $course1 = $generator->create_course(); + $context = \context_course::instance($course1->id); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $initialcount = $DB->count_records('question'); + + // Create a question category. + $cat = $questiongenerator->create_question_category(['contextid' => $context->id]); + + // Create a quiz containing a multichoice question from the qbank. + $quiz = $this->getDataGenerator()->get_plugin_generator('mod_quiz')->create_instance(['course' => $course1->id]); + $question = $questiongenerator->create_question('shortanswer', 'frogtoad', ['category' => $cat->id]); + quiz_add_quiz_question($question->id, $quiz); + + // Delete the multichoice_options record. + $DB->delete_records('qtype_shortanswer_options', ['questionid' => $question->id]); + + // Confirm we have created 1 additional question. + $this->assertEquals($initialcount + 1, $DB->count_records('question')); + + // Backup quiz. + $bc = new \backup_controller(\backup::TYPE_1ACTIVITY, $quiz->cmid, \backup::FORMAT_MOODLE, + \backup::INTERACTIVE_NO, \backup::MODE_IMPORT, $USER->id); + $backupid = $bc->get_backupid(); + $bc->execute_plan(); + $bc->destroy(); + + // Restore the backup into the same course. + $rc = new \restore_controller($backupid, $course1->id, \backup::INTERACTIVE_NO, \backup::MODE_IMPORT, + $USER->id, \backup::TARGET_CURRENT_ADDING); + $rc->execute_precheck(); + $rc->execute_plan(); + $rc->destroy(); + + $this->assertDebuggingCalled( + "Failed to load question options from the table qtype_shortanswer_options for questionid {$question->id}", + ); + + // Both quizzes should refer to the same original question. + $quizzes = get_fast_modinfo($course1->id)->get_instances_of('quiz'); + $this->assertCount(2, $quizzes); + foreach ($quizzes as $quiz) { + $structure = \mod_quiz\question\bank\qbank_helper::get_question_structure($quiz->instance, $quiz->context); + $this->assertEquals($structure[1]->questionid, $question->id); + } + + // There should be no additional questions created during the restore. + $this->assertEquals($initialcount + 1, $DB->count_records('question')); + } +}