diff --git a/public/lib/questionlib.php b/public/lib/questionlib.php index f3c258d94af..6e183c72712 100644 --- a/public/lib/questionlib.php +++ b/public/lib/questionlib.php @@ -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(); } /** diff --git a/public/mod/qbank/tests/task/transfer_question_categories_test.php b/public/mod/qbank/tests/task/transfer_question_categories_test.php index 91f0d0e5892..fe6c87415e6 100644 --- a/public/mod/qbank/tests/task/transfer_question_categories_test.php +++ b/public/mod/qbank/tests/task/transfer_question_categories_test.php @@ -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(); diff --git a/public/tag/classes/tag.php b/public/tag/classes/tag.php index bda28e13767..ae2d92bf439 100644 --- a/public/tag/classes/tag.php +++ b/public/tag/classes/tag.php @@ -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 = ?