From 0d89eada8bfd8d4b3bc26afa88a2e4bf968626b2 Mon Sep 17 00:00:00 2001 From: Daniel Ziegenberg Date: Thu, 14 Aug 2025 07:46:26 +0200 Subject: [PATCH] MDL-85869 qbank: Fix duplicate category stamps and idnumbers Migrating from Moodle 4.5 to Moodle 5.0 the adhoc task \mod_qbank\task\transfer_question_categories should transfer question categories into new question bank instances. The code for rescuing 'lost' categories where the linked contextid no longer exists was failing. It moved all these categories to the system context, which could lead to unique key violations. To fix this, we check for duplicates, and if neccessary change the stamp, or remove the idnumber. Since these questions were formerly 'lost', this is OK. Signed-off-by: Daniel Ziegenberg Co-authored-by: Tim Hunt --- .../task/transfer_question_categories.php | 28 +++++- .../transfer_question_categories_test.php | 92 +++++++++++++++++++ 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/mod/qbank/classes/task/transfer_question_categories.php b/mod/qbank/classes/task/transfer_question_categories.php index e7253764d87..25e9bd69c81 100644 --- a/mod/qbank/classes/task/transfer_question_categories.php +++ b/mod/qbank/classes/task/transfer_question_categories.php @@ -208,9 +208,31 @@ class transfer_question_categories extends adhoc_task { global $DB; $movedcategories = []; - $subcatids = $DB->get_fieldset('question_categories', 'id', ['parent' => $categoryid]); - foreach ($subcatids as $subcatid) { - $DB->set_field('question_categories', 'contextid', $newcontext->id, ['id' => $subcatid]); + $subcatids = $DB->get_records('question_categories', ['parent' => $categoryid]); + foreach ($subcatids as $subcatid => $data) { + // Because of the fallback above, where categories pointing to a + // missing contextid are all moved to the new shared system-level + // question bank, some categories are moved from previously + // separate contextids to the same context. This can violate + // unique indexes, so we fix this by ensuring uniqueness. + + // For the stamp, we just generate a new stamp if required. + if ($DB->record_exists('question_categories', ['stamp' => $data->stamp, 'contextid' => $newcontext->id])) { + $data->stamp = make_unique_id_code(); + } + + // The idnumber we just reset duplicates to null, as is done in other places. + if ( + $data->idnumber !== null && + $DB->record_exists('question_categories', ['idnumber' => $data->idnumber, 'contextid' => $newcontext->id]) + ) { + $data->idnumber = null; + } + + // Update the contextid and save the category. + $data->contextid = $newcontext->id; + $DB->update_record('question_categories', $data); + $movedcategories[] = $subcatid; $movedcategories = array_merge( $this->move_subcategories_to_context($subcatid, $newcontext), diff --git a/mod/qbank/tests/task/transfer_question_categories_test.php b/mod/qbank/tests/task/transfer_question_categories_test.php index b6cc82f274f..ba30745ade9 100644 --- a/mod/qbank/tests/task/transfer_question_categories_test.php +++ b/mod/qbank/tests/task/transfer_question_categories_test.php @@ -637,6 +637,98 @@ final class transfer_question_categories_test extends \advanced_testcase { ); } + /** + * Assert the installation task handles the missing contexts correctly. + * + * @return void + */ + public function test_qbank_install_with_missing_context(): void { + global $DB; + $this->resetAfterTest(); + self::setAdminUser(); + + $questiongenerator = self::getDataGenerator()->get_plugin_generator('core_question'); + + // The problem is that question categories that used to related to contextids + // which no longer exist are now all moved to the new system-level shared + // question bank. This moving categories together can cause unique key violations. + + // Create 2 orphaned categories where the contextid no longer exists, with the same stamp and idnumber. + // We need to do this by creating in a real context, then deleting the context, + // because create category logs, which needs a valid context id. + $tamperedstamp = make_unique_id_code(); + $context1 = context_course::instance(self::getDataGenerator()->create_course()->id); + $oldcat1 = $this->create_question_category('Lost category 1', $context1->id); + $oldcat1->stamp = $tamperedstamp; + $oldcat1->idnumber = 'tamperedidnumber'; + $DB->update_record('question_categories', $oldcat1); + $DB->delete_records('context', ['id' => $context1->id]); + + $context2 = context_course::instance(self::getDataGenerator()->create_course()->id); + $oldcat2 = $this->create_question_category('Lost category 2', $context2->id); + $oldcat2->stamp = $tamperedstamp; + $oldcat2->idnumber = 'tamperedidnumber'; + $DB->update_record('question_categories', $oldcat2); + $DB->delete_records('context', ['id' => $context2->id]); + + // Add a question to each category. + $question1 = $questiongenerator->create_question('shortanswer', null, ['category' => $oldcat1->id]); + $question2 = $questiongenerator->create_question('shortanswer', null, ['category' => $oldcat2->id]); + + // Make the questions 'in use'. + $quizcourse = self::getDataGenerator()->create_course(); + $quiz = self::getDataGenerator()->get_plugin_generator('mod_quiz')->create_instance( + ['course' => $quizcourse->id, 'grade' => 100.0, 'sumgrades' => 2, 'layout' => '1,0'] + ); + quiz_add_quiz_question($question1->id, $quiz); + quiz_add_quiz_question($question2->id, $quiz); + + // Make sure the caches are reset so that the contexts are not cached. + \core\context_helper::reset_caches(); + + // Run the task. + $task = new transfer_question_categories(); + $task->execute(); + // An important thing to verify is that the task completes without errors, + // for example unique key violations. + + // Verify - there should be a single question bank in the site course with the expected name. + $sitemodinfo = get_fast_modinfo(get_site()); + $siteqbanks = $sitemodinfo->get_instances_of('qbank'); + $this->assertCount(1, $siteqbanks); + $siteqbank = reset($siteqbanks); + $this->assertEquals('System shared question bank', $siteqbank->name); + + // The two previously orphaned categories should now be in this site questions bank, with a top category. + $sitemodcontext = context_module::instance($siteqbank->get_course_module_record()->id); + $sitemodcats = $DB->get_records_select( + 'question_categories', + 'parent <> 0 AND contextid = :contextid', + ['contextid' => $sitemodcontext->id], + 'id ASC', + ); + + // Work out which category is which. + $movedcat1 = null; + $movedcat2 = null; + foreach ($sitemodcats as $movedcat) { + if ($movedcat->name === $oldcat1->name) { + $movedcat1 = $movedcat; + } + if ($movedcat->name === $oldcat2->name) { + $movedcat2 = $movedcat; + } + } + $this->assertNotNull($movedcat1); + $this->assertNotNull($movedcat2); + + // Verify the properties of the moved categories. + $this->assertNotEquals($movedcat1->stamp, $movedcat2->stamp); + $this->assertNotEquals($movedcat1->idnumber, $movedcat2->idnumber); + $this->assertEquals(question_get_top_category($sitemodcontext->id)->id, $movedcat1->parent); + $this->assertEquals(question_get_top_category($sitemodcontext->id)->id, $movedcat2->parent); + } + public function test_fix_wrong_parents(): void { $this->resetAfterTest(); $this->setup_pre_install_data();