From a0dab33a2306cbfb7d6637cbe2866b1659d7ab19 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Tue, 3 Jun 2025 14:08:54 +0100 Subject: [PATCH 1/2] MDL-85210 question: Update move_question_set_references move_question_set_references expected the set reference to use the old filter format. This change converts the filter to the new format if required, before updating the category and context ids. --- lib/questionlib.php | 19 +++--- lib/tests/questionlib_test.php | 106 +++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 9 deletions(-) diff --git a/lib/questionlib.php b/lib/questionlib.php index f8abcf56fb7..bc042109744 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -667,16 +667,17 @@ function move_question_set_references(int $oldcategoryid, int $newcatgoryid, if ($delete || $oldcontextid !== $newcontextid) { $setreferences = $DB->get_recordset('question_set_references', ['questionscontextid' => $oldcontextid]); foreach ($setreferences as $setreference) { - $filter = json_decode($setreference->filtercondition); - if (isset($filter->questioncategoryid)) { - if ((int)$filter->questioncategoryid === $oldcategoryid) { - $setreference->questionscontextid = $newcontextid; - if ($oldcategoryid !== $newcatgoryid) { - $filter->questioncategoryid = $newcatgoryid; - $setreference->filtercondition = json_encode($filter); - } - $DB->update_record('question_set_references', $setreference); + $filter = json_decode($setreference->filtercondition, true); + if (isset($filter['questioncategoryid'])) { + $filter = question_reference_manager::convert_legacy_set_reference_filter_condition($filter); + } + if ((int)$filter['filter']['category']['values'][0] === $oldcategoryid) { + $setreference->questionscontextid = $newcontextid; + if ($oldcategoryid !== $newcatgoryid) { + $filter['filter']['category']['values'][0] = $newcatgoryid; + $setreference->filtercondition = json_encode($filter); } + $DB->update_record('question_set_references', $setreference); } } $setreferences->close(); diff --git a/lib/tests/questionlib_test.php b/lib/tests/questionlib_test.php index ead871f8f93..fdedc8ce57e 100644 --- a/lib/tests/questionlib_test.php +++ b/lib/tests/questionlib_test.php @@ -17,6 +17,7 @@ namespace core; use core_question\local\bank\question_bank_helper; +use mod_quiz\quiz_settings; use question_bank; defined('MOODLE_INTERNAL') || die(); @@ -1673,4 +1674,109 @@ final class questionlib_test extends \advanced_testcase { } + /** + * Update the context for a set reference, keeping the original category. + * + * @covers ::move_question_set_references() + */ + public function test_move_question_set_references_context(): void { + $this->setAdminUser(); + // Create a course with a quiz containing a random question from a qbank context. + $randomcourse = self::getDataGenerator()->create_course(['shortname' => 'Random']); + $qbank1 = self::getDataGenerator()->get_plugin_generator('mod_qbank')->create_instance(['course' => $randomcourse->id]); + $context1 = \context_module::instance($qbank1->cmid); + $qbank2 = self::getDataGenerator()->get_plugin_generator('mod_qbank')->create_instance(['course' => $randomcourse->id]); + $context2 = \context_module::instance($qbank2->cmid); + $topcategory = question_get_top_category($context1->id, true); + $randomcategory = self::getDataGenerator()->get_plugin_generator('core_question')->create_question_category( + ['parent' => $topcategory->id], + ); + $randomquiz = self::getDataGenerator()->get_plugin_generator('mod_quiz')->create_instance( + [ + 'course' => $randomcourse->id, + 'grade' => 100.0, + 'sumgrades' => 2, + 'layout' => '1,0', + ], + ); + + $randomquizsettings = quiz_settings::create($randomquiz->id); + $structure = $randomquizsettings->get_structure(); + + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'jointype' => \core_question\local\bank\condition::JOINTYPE_DEFAULT, + 'values' => [$randomcategory->id], + 'filteroptions' => ['includesubcategories' => true], + ], + ], + ]; + $structure->add_random_questions(1, 1, $filtercondition); + $structure = $randomquizsettings->get_structure(); + $randomquestion = $structure->get_question_in_slot(1); + + $this->assertEquals($randomquestion->contextid, $context1->id); + $this->assertEquals($randomquestion->filtercondition['filter']['category']['values'][0], $randomcategory->id); + + move_question_set_references($randomcategory->id, $randomcategory->id, $context1->id, $context2->id); + + $structure = $randomquizsettings->get_structure(); + $randomquestion = $structure->get_question_in_slot(1); + + $this->assertEquals($randomquestion->contextid, $context2->id); + $this->assertEquals($randomquestion->filtercondition['filter']['category']['values'][0], $randomcategory->id); + } + + /** + * Update the context and category for a set reference. + * + * @covers ::move_question_set_references() + */ + public function test_move_question_set_references_category(): void { + $this->setAdminUser(); + // Create a course with a quiz containing a random question from a qbank context. + $randomcourse = self::getDataGenerator()->create_course(['shortname' => 'Random']); + $qbank1 = self::getDataGenerator()->get_plugin_generator('mod_qbank')->create_instance(['course' => $randomcourse->id]); + $context1 = \context_module::instance($qbank1->cmid); + $qbank2 = self::getDataGenerator()->get_plugin_generator('mod_qbank')->create_instance(['course' => $randomcourse->id]); + $context2 = \context_module::instance($qbank2->cmid); + $topcategory1 = question_get_top_category($context1->id, true); + $topcategory2 = question_get_top_category($context2->id, true); + $randomquiz = self::getDataGenerator()->get_plugin_generator('mod_quiz')->create_instance( + [ + 'course' => $randomcourse->id, + 'grade' => 100.0, + 'sumgrades' => 2, + 'layout' => '1,0', + ], + ); + + $randomquizsettings = quiz_settings::create($randomquiz->id); + $structure = $randomquizsettings->get_structure(); + + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'jointype' => \core_question\local\bank\condition::JOINTYPE_DEFAULT, + 'values' => [$topcategory1->id], + 'filteroptions' => ['includesubcategories' => true], + ], + ], + ]; + $structure->add_random_questions(1, 1, $filtercondition); + $structure = $randomquizsettings->get_structure(); + $randomquestion = $structure->get_question_in_slot(1); + + $this->assertEquals($randomquestion->contextid, $context1->id); + $this->assertEquals($randomquestion->filtercondition['filter']['category']['values'][0], $topcategory1->id); + + move_question_set_references($topcategory1->id, $topcategory2->id, $context1->id, $context2->id); + + $structure = $randomquizsettings->get_structure(); + $randomquestion = $structure->get_question_in_slot(1); + + $this->assertEquals($randomquestion->contextid, $context2->id); + $this->assertEquals($randomquestion->filtercondition['filter']['category']['values'][0], $topcategory2->id); + } } From 89a613460a694e81c4a7f329313dd1a79f0c2ec1 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Tue, 3 Jun 2025 11:26:44 +0100 Subject: [PATCH 2/2] MDL-85210 mod_qbank: Update question set references during upgrade When moving question categories during the migration to mod_qbank, set references using questions in those categories were not updated with the new context. Furthermore, since top categories from system, category and course contexts are deleted and not moved, and set references that filtered based on a top category were left pointing to a non-existant category. This change updates set references using the top category to point to the new top category where its subcategories are moved, and updates set references for all subcategories to set the new context. --- lib/questionlib.php | 1 + .../task/transfer_question_categories.php | 2 + .../transfer_question_categories_test.php | 96 +++++++++++++++++++ 3 files changed, 99 insertions(+) diff --git a/lib/questionlib.php b/lib/questionlib.php index bc042109744..0cd76b49fa6 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -729,6 +729,7 @@ function question_move_category_to_context($categoryid, $oldcontextid, $newconte $subcatids = $DB->get_records_menu('question_categories', ['parent' => $categoryid], '', 'id,1'); foreach ($subcatids as $subcatid => $notused) { + move_question_set_references($subcatid, $subcatid, $oldcontextid, $newcontext->id); $DB->set_field('question_categories', 'contextid', $newcontextid, ['id' => $subcatid]); question_move_category_to_context($subcatid, $oldcontextid, $newcontextid); } diff --git a/mod/qbank/classes/task/transfer_question_categories.php b/mod/qbank/classes/task/transfer_question_categories.php index 2313bd5a68c..ec20fc5cad7 100644 --- a/mod/qbank/classes/task/transfer_question_categories.php +++ b/mod/qbank/classes/task/transfer_question_categories.php @@ -171,6 +171,8 @@ class transfer_question_categories extends adhoc_task { $newtopcategory = question_get_top_category($newcontext->id, true); + move_question_set_references($oldtopcategory->id, $newtopcategory->id, $oldtopcategory->contextid, $newcontext->id, true); + // This function moves subcategories, so we have to start at the top. question_move_category_to_context($oldtopcategory->id, $oldtopcategory->contextid, $newcontext->id); diff --git a/mod/qbank/tests/task/transfer_question_categories_test.php b/mod/qbank/tests/task/transfer_question_categories_test.php index a3af54d9871..9418282e91d 100644 --- a/mod/qbank/tests/task/transfer_question_categories_test.php +++ b/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_question\local\bank\random_question_loader; +use mod_quiz\quiz_settings; use stdClass; use core_question\local\bank\question_version_status; @@ -159,6 +161,30 @@ 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); + // 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( + [ + 'course' => $randomcourse->id, + 'grade' => 100.0, + 'sumgrades' => 2, + 'layout' => '1,0', + ], + ); + $randomquizsettings = quiz_settings::create($randomquiz->id); + $structure = $randomquizsettings->get_structure(); + $topcategory = $DB->get_record('question_categories', ['contextid' => $sitecontext->id, 'parent' => 0]); + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'jointype' => \core_question\local\bank\condition::JOINTYPE_DEFAULT, + 'values' => [$topcategory->id], + 'filteroptions' => ['includesubcategories' => true], + ], + ], + ]; + $structure->add_random_questions(1, 1, $filtercondition); + // Create a course category and then a question category attached to that context. $coursecategory = self::getDataGenerator()->create_category(); $this->coursecatcontext = context_coursecat::instance($coursecategory->id); @@ -275,6 +301,20 @@ final class transfer_question_categories_test extends \advanced_testcase { $question4 = $questiongenerator->create_question('shortanswer', null, ['category' => $unusedcategory->id]); $quiz = $quizgenerator->create_instance(['course' => $course->id, 'grade' => 100.0, 'sumgrades' => 2, 'layout' => '1,0']); quiz_add_quiz_question($question1->id, $quiz, 1); + + // The quiz also contains a random question from the used category. + $quizsettings = quiz_settings::create($quiz->id); + $structure = $quizsettings->get_structure(); + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'jointype' => \core_question\local\bank\condition::JOINTYPE_DEFAULT, + 'values' => [$usedcategory->id], + 'filteroptions' => ['includesubcategories' => false], + ], + ], + ]; + $structure->add_random_questions(1, 1, $filtercondition); } /** @@ -305,6 +345,24 @@ final class transfer_question_categories_test extends \advanced_testcase { $this->assertEquals($parentcat->id, $parentcatq->categoryid); $this->assertEquals($childcat->id, $childcatq->categoryid); + // Make sure the "Random" course has 1 quiz with 1 random question that returns the questions from the system top category. + $randomcourse = $DB->get_record('course', ['shortname' => 'Random']); + $coursemods = get_course_mods($randomcourse->id); + $randomquiz = reset($coursemods); + $randomquizsettings = quiz_settings::create($randomquiz->instance); + $structure = $randomquizsettings->get_structure(); + $randomquestionslot = $structure->get_question_in_slot(1); + $this->assertEquals($randomquestionslot->contextid, $sitecontext->id); + $loader = new random_question_loader(new \qubaid_list([])); + $randomquestions = $loader->get_filtered_questions($randomquestionslot->filtercondition['filter']); + $this->assertCount(2, $randomquestions); + $randomq1 = reset($randomquestions); + $randomq2 = end($randomquestions); + $this->assertEquals($parentcatq->id, $randomq1->id); + $this->assertEquals($parentcat->id, $randomq1->category); + $this->assertEquals($childcatq->id, $randomq2->id); + $this->assertEquals($childcat->id, $randomq2->category); + // Make sure that the course category has a question category below 'top'. $allcoursecatcats = $DB->get_records('question_categories', ['contextid' => $this->coursecatcontext->id], 'id ASC'); $this->assertCount(2, $allcoursecatcats); @@ -360,6 +418,15 @@ final class transfer_question_categories_test extends \advanced_testcase { $this->assertCount(2, $this->get_question_data([$unusedcat->id])); $emptycat = next($questioncats); $this->assertCount(0, $this->get_question_data([$emptycat->id])); + + // The question reference for the random question is using the "used" category, and the site context. + $coursemods = get_course_mods($this->usedunusedcontext->instanceid); + $quiz = reset($coursemods); + $quizsettings = quiz_settings::create($quiz->instance); + $structure = $quizsettings->get_structure(); + $randomquestionslot = $structure->get_question_in_slot(2); + $this->assertEquals($this->usedunusedcontext->id, $randomquestionslot->contextid); + $this->assertEquals($usedcat->id, $randomquestionslot->filtercondition['filter']['category']['values'][0]); } /** @@ -410,6 +477,22 @@ final class transfer_question_categories_test extends \advanced_testcase { $this->assertEquals($topcat->id, $parentcat->parent); $this->assertEquals($parentcat->id, $childcat->parent); + // The random question should now point to the questions in the site course question bank. + $randomcourse = $DB->get_record('course', ['shortname' => 'Random']); + $coursemods = get_course_mods($randomcourse->id); + $randomquiz = reset($coursemods); + $randomquizsettings = quiz_settings::create($randomquiz->instance); + $structure = $randomquizsettings->get_structure(); + $randomquestionslot = $structure->get_question_in_slot(1); + $this->assertEquals($randomquestionslot->contextid, $sitemodcontext->id); + $loader = new random_question_loader(new \qubaid_list([])); + $randomquestions = $loader->get_filtered_questions($randomquestionslot->filtercondition['filter']); + $this->assertCount(2, $randomquestions); + $randomq1 = reset($randomquestions); + $randomq2 = end($randomquestions); + $this->assertEquals($parentcat->id, $randomq1->category); + $this->assertEquals($childcat->id, $randomq2->category); + // Course category context checks. // Make sure that the course category has no question categories, not even 'top'. @@ -525,6 +608,19 @@ final class transfer_question_categories_test extends \advanced_testcase { $this->assertEmpty($this->get_question_data([$usedunusedcats['top']->id])); $this->assertCount(2, $this->get_question_data([$usedunusedcats['Used Question Cat']->id])); $this->assertCount(2, $this->get_question_data([$usedunusedcats['Unused Question Cat']->id])); + + // The question reference for the random question is using the same category, but the new context. + $modinfo = get_fast_modinfo($this->usedunusedcontext->instanceid); + $quizzes = $modinfo->get_instances_of('quiz'); + $quiz = reset($quizzes); + $quizsettings = quiz_settings::create($quiz->instance); + $structure = $quizsettings->get_structure(); + $randomquestionslot = $structure->get_question_in_slot(2); + $this->assertEquals($usedunusedqbank->context->id, $randomquestionslot->contextid); + $this->assertEquals( + $usedunusedcats['Used Question Cat']->id, + $randomquestionslot->filtercondition['filter']['category']['values'][0] + ); } public function test_fix_wrong_parents(): void {