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

MDL-87287 mod_qbank: De-duplicate tags when migrating questions

Browse Source 
In the migration from old-style question banks to mod_qbank, if a
question had the same tag from multiple contexts, when it was moved to
its new question bank context, the migration tried to set multiple tag
instances for the same tag to the same context, which caused a unique
key violation.

This add some checking to the
`question_move_question_tags_to_new_context` function to identify any
tag instances to be moved that will trigger this key violation, and
delte any duplicates before the move so we end up with a single instance
of the tag in the new context. This is done in a transaction that will
only be committed once the remaining tags have been moved.

This required adding the `tiuserid` field to the data returned by
`core_tag_tag::get_items_tag` so we can assemble the unique key without
having to do additional queries.

This also extends the test coverage for the `transfer_questions` task
to ensure tags are moved to the new contexts, and tags on stale questions
are deleted.
This commit is contained in:
Mark Johnson
2026-01-09 10:40:00 +00:00
parent 52b4103388
commit 2fd133a905
3 changed files with 115 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
public/lib/questionlib.php
+13 -3
View File
@@ -501,7 +501,7 @@ function question_delete_activity($cm, $notused = false, bool $coursedeletion =
* @param context $newcontext The Moodle context the questions are being moved to, must be module context.
*/
function question_move_question_tags_to_new_context(array $questions, context $newcontext): void {
global $DB;
if ($newcontext->contextlevel !== CONTEXT_MODULE) {
debugging("Invalid contextlevel: {$newcontext->contextlevel}", DEBUG_DEVELOPER);
}
@@ -512,11 +512,12 @@ function question_move_question_tags_to_new_context(array $questions, context $n
}, $questions);
$questionstagobjects = core_tag_tag::get_items_tags('core_question', 'question', $questionids);
$transaction = $DB->start_delegated_transaction();
foreach ($questions as $question) {
$tagobjects = $questionstagobjects[$question->id] ?? [];
foreach ($tagobjects as $tagobject) {
$tagid = $tagobject->taginstanceid;
$taginstanceid = $tagobject->taginstanceid;
$tagcontextid = $tagobject->taginstancecontextid;
$istaginnewcontext = $tagcontextid == $newcontext->id;
@@ -526,7 +527,15 @@ function question_move_question_tags_to_new_context(array $questions, context $n
continue;
}
$instancesfornewcontext[] = $tagid;
$instancekey = implode('-', ['core_question', 'question', $question->id, $tagobject->tiuserid, $tagobject->id]);
if (array_key_exists($instancekey, $instancesfornewcontext)) {
// We have an identical instance that we are already moving to the new context. This instance will be a duplicate,
// so delete it.
$DB->delete_records('tag_instance', ['id' => $taginstanceid]);
continue;
}
$instancesfornewcontext[$instancekey] = $taginstanceid;
}
}
@@ -534,6 +543,7 @@ function question_move_question_tags_to_new_context(array $questions, context $n
// Update the tag instances to the new context id.
core_tag_tag::change_instances_context($instancesfornewcontext, $newcontext);
}
$transaction->allow_commit();
}
/**
public/mod/qbank/tests/task/transfer_question_categories_test.php
+100
View File
@@ -21,6 +21,8 @@ use context_course;
use context_coursecat;
use context_module;
use context_system;
use core\context\module;
use core\context\system;
use core\exception\moodle_exception;
use core\task\manager;
use core_question\local\bank\random_question_loader;
@@ -164,6 +166,9 @@ final class transfer_question_categories_test extends \advanced_testcase {
quiz_add_quiz_question($question1->id, $quiz, 1);
quiz_add_quiz_question($question2->id, $quiz, 1);
\core_tag_tag::add_item_tag('core_question', 'question', $question1->id, $sitecontext, 'tag1');
\core_tag_tag::add_item_tag('core_question', 'question', $question1->id, $sitecontext, 'tag2');
// Create a course with a quiz containing a random question from the system context.
$randomcourse = self::getDataGenerator()->create_course(['shortname' => 'Random']);
$randomquiz = $quizgenerator->create_instance(
@@ -228,6 +233,9 @@ final class transfer_question_categories_test extends \advanced_testcase {
quiz_add_quiz_question($question4->id, $quiz, 1);
quiz_add_quiz_question($question5->id, $quiz, 1);
\core_tag_tag::add_item_tag('core_question', 'question', $question4->id, $this->coursecontext, 'tag1');
\core_tag_tag::add_item_tag('core_question', 'question', $question4->id, $this->coursecontext, 'tag3');
// Include a stale question, which should not be migrated with the others.
$question6 = $questiongenerator->create_question('shortanswer', null, ['category' => $coursechildcat1->id]);
$DB->set_field(
@@ -237,6 +245,8 @@ final class transfer_question_categories_test extends \advanced_testcase {
['questionid' => $question6->id],
);
\core_tag_tag::add_item_tag('core_question', 'question', $question6->id, $this->coursecontext, 'staletag');
// Create some nested categories with no questions in use.
$course = self::getDataGenerator()->create_course();
$context = context_course::instance($course->id);
@@ -890,6 +900,10 @@ final class transfer_question_categories_test extends \advanced_testcase {
$this->resetAfterTest();
$this->setup_pre_install_data();
// We should still ahve all the tags in the course context.
$coursetags = \core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [$this->coursecontext]);
$this->assertCount(3, $coursetags);
$task = new \mod_qbank\task\transfer_question_categories();
$task->execute();
@@ -953,6 +967,24 @@ final class transfer_question_categories_test extends \advanced_testcase {
'3.png'
));
// Assert tags are still in their original contexts.
$sitecourse = get_course(SITEID);
$sitemodinfo = get_fast_modinfo($sitecourse);
$siteqbanks = $sitemodinfo->get_instances_of('qbank');
$siteqbank = reset($siteqbanks);
$siteqbankcontext = module::instance($siteqbank->id);
$testcourse = get_course($this->coursecontext->instanceid);
$testmodinfo = get_fast_modinfo($testcourse);
$testqbanks = $testmodinfo->get_instances_of('qbank');
$testqbank = reset($testqbanks);
$testqbankcontext = module::instance($testqbank->id);
$sitetags = \core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [context_system::instance()]);
$this->assertCount(2, $sitetags);
$coursetags = \core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [$this->coursecontext]);
$this->assertCount(2, $coursetags); // The stale tag was removed with the stale question when the categories were moved.
$this->assertEmpty(\core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [$siteqbankcontext]));
$this->assertEmpty(\core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [$testqbankcontext]));
$this->assertFalse(question_bank_helper::has_bank_migration_task_completed_successfully());
$questiontasks = manager::get_adhoc_tasks(transfer_questions::class);
@@ -1022,9 +1054,77 @@ final class transfer_question_categories_test extends \advanced_testcase {
'3.png'
));
// Assert tags have been moved still to their new contexts.
$sitetags = \core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [$siteqbankcontext]);
$this->assertCount(2, $sitetags);
$coursetags = \core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [$testqbankcontext]);
$this->assertCount(2, $coursetags);
$this->assertEmpty(\core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [system::instance()]));
$this->assertEmpty(\core_tag_tag::get_tags_by_area_in_contexts('core_question', 'question', [$this->coursecontext]));
$this->assertTrue(question_bank_helper::has_bank_migration_task_completed_successfully());
}
public function test_transfer_questions_with_duplicate_tags(): void {
global $DB;
$this->resetAfterTest();
// Create a second course.
$course = self::getDataGenerator()->create_course();
$course2context = context_course::instance($course->id);
// Create a parent category, and 2 child categories in a non-existant context.
$coursecat = $this->create_question_category('Course2 parent cat', $course2context->id);
$questiongenerator = self::getDataGenerator()->get_plugin_generator('core_question');
$question1 = $questiongenerator->create_question(
'shortanswer',
null,
['category' => $coursecat->id, 'status' => question_version_status::QUESTION_STATUS_READY]
);
$tag = self::getDataGenerator()->create_tag(['name' => 'testtag']);
// Insert two instances of the same tag on the same question, with different contexts.
$taginstance1 = (object) [
'tagid' => $tag->id,
'component' => 'core_question',
'itemid' => $question1->id,
'itemtype' => 'question',
'contextid' => $course2context->id,
'ordering' => 1,
'timecreated' => time(),
'timemodified' => time(),
'tiuserid' => 2,
];
$taginstance1->id = $DB->insert_record('tag_instance', $taginstance1);
$taginstance2 = (object) [
'tagid' => $tag->id,
'component' => 'core_question',
'itemid' => $question1->id,
'itemtype' => 'question',
'contextid' => system::instance()->id,
'ordering' => 2,
'timecreated' => time(),
'timemodified' => time(),
'tiuserid' => 2,
];
$taginstance2->id = $DB->insert_record('tag_instance', $taginstance2);
$task = new \mod_qbank\task\transfer_question_categories();
$task->execute();
// Run transfer_questions task.
$this->expectOutputRegex('~Moving files and tags for 1 questions~');
$this->run_all_adhoc_tasks();
$newcontextid = $DB->get_field('question_categories', 'contextid', ['id' => $coursecat->id]);
$this->assertTrue($DB->record_exists('tag_instance', ['id' => $taginstance1->id, 'contextid' => $newcontextid]));
$this->assertFalse($DB->record_exists('tag_instance', ['id' => $taginstance2->id]));
}
public function test_qbank_install_resilience(): void {
global $DB;
$this->resetAfterTest();
public/tag/classes/tag.php
+2 -2
View File
@@ -681,7 +681,7 @@ class core_tag_tag {
list($idsql, $params) = $DB->get_in_or_equal($itemids, SQL_PARAMS_NAMED);
// Note: if the fields in this query are changed, you need to do the same changes in core_tag_tag::get_correlated_tags().
$sql = "SELECT ti.id AS taginstanceid, tg.id, tg.isstandard, tg.name, tg.rawname, tg.flag,
tg.tagcollid, ti.ordering, ti.contextid AS taginstancecontextid, ti.itemid
tg.tagcollid, ti.ordering, ti.contextid AS taginstancecontextid, ti.itemid, ti.tiuserid
FROM {tag_instance} ti
JOIN {tag} tg ON tg.id = ti.tagid
WHERE ti.itemtype = :itemtype AND ti.itemid $idsql ".
@@ -1252,7 +1252,7 @@ class core_tag_tag {
// This is (and has to) return the same fields as the query in core_tag_tag::get_item_tags().
$sql = "SELECT ti.id AS taginstanceid, tg.id, tg.isstandard, tg.name, tg.rawname, tg.flag,
tg.tagcollid, ti.ordering, ti.contextid AS taginstancecontextid, ti.itemid
tg.tagcollid, ti.ordering, ti.contextid AS taginstancecontextid, ti.itemid, ti.tiuserid
FROM {tag} tg
INNER JOIN {tag_instance} ti ON tg.id = ti.tagid
WHERE tg.id $query AND tg.id <> ? AND tg.tagcollid = ?