L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-85721 qtype_multianswer: Clean up duplicate subquestions

Browse Source 
This adds an ad-hoc task to clean up duplicate subquestions created by
the bug.

When the upgrade is run, it will run a query to check if any
subquestions have been duplicated (if there are multiple questions with
the same parent, text and stamp), and queue the cleanup task if there
are.

For each stamp identified, the task will find instances of the question
that are not referred to in their parent's sequence field, confirm that
they have 0 usages, and delete them.
This commit is contained in:
Mark Johnson
2025-07-01 11:59:16 +01:00
parent 201f1ea665
commit e8002198f3
4 changed files with 476 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
question/type/multianswer/classes/task/cleanup_duplicate_subquestions.php
+156
View File
@@ -0,0 +1,156 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
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 <[email protected]>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class cleanup_duplicate_subquestions extends \core\task\adhoc_task {
use stored_progress_task_trait;
/**
* 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;
");
}
#[\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.");
$this->start_stored_progress();
$progress = $this->get_progress();
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:");
$progress->start_progress($subquestion->stamp, $duplicatecount);
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]
);
$progress->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();
$progress->increment_progress();
}
$progress->end_progress();
}
}
}
question/type/multianswer/db/upgrade.php
+9
View File
@@ -43,5 +43,14 @@ function xmldb_qtype_multianswer_upgrade($oldversion) {
// Automatically generated Moodle v5.0.0 release upgrade line.
// Put any upgrade step following this.
if ($oldversion < 2025061000) {
$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, 2025061000, 'qtype', 'multianswer');
}
return true;
}
question/type/multianswer/tests/task/cleanup_duplicate_subquestions_test.php
+310
View File
@@ -0,0 +1,310 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
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 <[email protected]>
* @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],
),
);
}
}
}
question/type/multianswer/version.php
+1 -1
View File
@@ -26,7 +26,7 @@
defined('MOODLE_INTERNAL') || die();
$plugin->component = 'qtype_multianswer';
$plugin->version = 2025041400;
$plugin->version = 2025061000;
$plugin->requires = 2025040800;
$plugin->dependencies = [