From 0a34c6aa10396137a5e80edf45545f96ec817bcb Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 23 Jan 2025 15:14:58 +0100 Subject: [PATCH] MDL-84305 qbank upgrade: fix category hierarchies before upgrade We know that, due to old bugs in places like backup and restore, you can get question categories with a different contextid from their parents. This was breaking the upgrade, so this is code to auto-fix them before the upgrade continues. Co-authored-by: Stephan Robotta --- .../task/transfer_question_categories.php | 118 ++++++++++++++++-- .../transfer_question_categories_test.php | 116 +++++++++++++++-- 2 files changed, 212 insertions(+), 22 deletions(-) rename mod/qbank/tests/{ => task}/transfer_question_categories_test.php (81%) diff --git a/mod/qbank/classes/task/transfer_question_categories.php b/mod/qbank/classes/task/transfer_question_categories.php index 958e0b0a3be..2313bd5a68c 100644 --- a/mod/qbank/classes/task/transfer_question_categories.php +++ b/mod/qbank/classes/task/transfer_question_categories.php @@ -19,13 +19,11 @@ namespace mod_qbank\task; use context_system; use core\context; use core\task\adhoc_task; -use core\task\manager; use core_course_category; use core_question\local\bank\question_bank_helper; use stdClass; /** - * /** * This script transfers question categories at CONTEXT_SITE, CONTEXT_COURSE, & CONTEXT_COURSECAT to a new qbank instance * context. * @@ -49,17 +47,22 @@ use stdClass; class transfer_question_categories extends adhoc_task { /** - * Run the install task. - * - * @return void + * @var array a cache [ context id => question category ] of the top category in each context. + * Used by get_top_category_id_for_context() to avoid repeated DB queries. + * 0 is cached if this context id has no corresponding top category. */ - public function execute() { + private array $topcategorycache = []; + + #[\Override] + public function execute(): void { global $DB, $CFG; require_once($CFG->dirroot . '/course/modlib.php'); require_once($CFG->libdir . '/questionlib.php'); + $this->fix_wrong_parents(); + $recordset = $DB->get_recordset('question_categories', ['parent' => 0]); foreach ($recordset as $oldtopcategory) { @@ -79,7 +82,7 @@ class transfer_question_categories extends adhoc_task { ); // This gives us categories in parent -> child order so array_reverse it, // because we should process stale categories from the bottom up. - $subcategories = array_reverse(\sort_categories_by_tree($subcategories, $oldtopcategory->id)); + $subcategories = array_reverse(sort_categories_by_tree($subcategories, $oldtopcategory->id)); foreach ($subcategories as $subcategory) { \qbank_managecategories\helper::question_remove_stale_questions_from_category($subcategory->id); if ($this->question_category_is_empty($subcategory->id)) { @@ -109,7 +112,7 @@ class transfer_question_categories extends adhoc_task { break; case CONTEXT_COURSECAT: $coursecategory = core_course_category::get($oldcontext->instanceid); - $courseshortname = "{$coursecategory->name}-{$coursecategory->id}"; + $courseshortname = "$coursecategory->name-$coursecategory->id"; $course = $this->create_course($coursecategory, $courseshortname); $bankname = question_bank_helper::get_bank_name_string('sharedbank', 'mod_qbank', $coursecategory->name); break; @@ -146,14 +149,14 @@ class transfer_question_categories extends adhoc_task { * @param string $shortname * @return stdClass */ - protected function create_course(\core_course_category $coursecategory, string $shortname): stdClass { + protected function create_course(core_course_category $coursecategory, string $shortname): stdClass { $data = (object) [ 'enablecompletion' => 0, 'fullname' => get_string('coursecategory', 'mod_qbank', $coursecategory->name), 'shortname' => $shortname, 'category' => $coursecategory->id, ]; - return \create_course($data); + return create_course($data); } /** @@ -175,12 +178,105 @@ class transfer_question_categories extends adhoc_task { $DB->set_field('question_categories', 'parent', $newtopcategory->id, ['parent' => $oldtopcategory->id]); } + /** + * Find the Top category for a context, if there is one. + * + * @param int $contextid the id of a context (which might not exist). + * @return int a Top category id, or 0 if none is found. + */ + protected function get_top_category_id_for_context(int $contextid): int { + global $DB; + + // Use the cache if we have already loaded this. + if (array_key_exists($contextid, $this->topcategorycache)) { + return $this->topcategorycache[$contextid]; + } + + $topcategoryid = (int) $DB->get_field('question_categories', 'id', + ['contextid' => $contextid, 'parent' => 0]); + + $this->topcategorycache[$contextid] = $topcategoryid; + return $topcategoryid; + } + + /** + * Fix the context of child categories whose contextid does not match that of their parents. + * + * Fix here means: + * + * - if the child category's context exists, and has a 'Top' category, we move the child + * category to be just under that Top category. That is where they would have appeared + * before, e.g. in the return from question_categorylist(). + * + * - if the child category points to a context that does not exist at all, then we + * instead change its context to be the same as it's parent's context. This may + * break things like images in the question text of questions there, but there is + * no real alternative. + * + * This is necessary because, due to old bugs, for example in backup and restoree code, + * we know there can be question categories in the databases of old Moodle sites with + * the wrong context id. + */ + public function fix_wrong_parents(): void { + global $DB; + + $categoriestofix = $this->get_categories_in_a_different_context_to_their_parent(); + foreach ($categoriestofix as $childcategoryid => $childcontextid) { + + $topcategoryid = $this->get_top_category_id_for_context($childcontextid); + if ($topcategoryid) { + // Suitable Top category in the child's current context, so move to be a parent of that. + $DB->set_field('question_categories', 'parent', $topcategoryid, ['id' => $childcategoryid]); + } else { + // Top not found. Change the child to have the same context as its parent. + // This is not efficient in DB queries, but we expect this to be a rare case, and this is simple and right. + $childcategory = $DB->get_record('question_categories', ['id' => $childcategoryid]); + $parentcontextid = $DB->get_field('question_categories', 'contextid', ['id' => $childcategory->parent]); + $this->move_category_and_its_children($childcategoryid, $parentcontextid); + } + } + } + + /** + * Get question categories that are in a different context to their parent. + * + * @return int[] child category id => context id of the child category. + */ + public function get_categories_in_a_different_context_to_their_parent(): array { + global $DB; + + return $DB->get_records_sql_menu(' + SELECT c.id, c.contextid + FROM {question_categories} c + JOIN {question_categories} p ON p.id = c.parent + WHERE p.contextid <> c.contextid + ORDER BY c.id + '); + } + + /** + * Set the contextid of category $categoryid and all its children to $newcontextid. + * + * @param int $categoryid a question_category id. + * @param int $newcontextid the place to move to. + */ + public function move_category_and_its_children(int $categoryid, int $newcontextid): void { + global $DB; + + $DB->set_field('question_categories', 'contextid', $newcontextid, ['id' => $categoryid]); + $children = $DB->get_records('question_categories', ['parent' => $categoryid], '', 'id, contextid'); + foreach ($children as $child) { + if ($child->contextid != $newcontextid) { + $this->move_category_and_its_children($child->id, $newcontextid); + } + } + } + /** * Recursively check if a question category or its children contain any questions. * * @param int $categoryid The parent category to check from. * @return bool True if neither the category nor its children contain any questions. - * @throws \dml_exception */ protected function question_category_is_empty(int $categoryid): bool { global $DB; diff --git a/mod/qbank/tests/transfer_question_categories_test.php b/mod/qbank/tests/task/transfer_question_categories_test.php similarity index 81% rename from mod/qbank/tests/transfer_question_categories_test.php rename to mod/qbank/tests/task/transfer_question_categories_test.php index 51852c83a0d..a3af54d9871 100644 --- a/mod/qbank/tests/transfer_question_categories_test.php +++ b/mod/qbank/tests/task/transfer_question_categories_test.php @@ -14,8 +14,9 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -namespace mod_qbank; +namespace mod_qbank\task; +use context; use context_course; use context_coursecat; use context_module; @@ -371,7 +372,7 @@ final class transfer_question_categories_test extends \advanced_testcase { $this->resetAfterTest(); $this->setup_pre_install_data(); - $task = new \mod_qbank\task\transfer_question_categories(); + $task = new transfer_question_categories(); $task->execute(); // Site context checks. @@ -421,10 +422,10 @@ final class transfer_question_categories_test extends \advanced_testcase { $coursecat = $DB->get_record('course_categories', ['id' => $newcourse->category]); // Make sure the new course shortname is a unique name based on the category name and id. - $this->assertEquals("{$coursecat->name}-{$coursecat->id}", $newcourse->shortname); + $this->assertEquals("$coursecat->name-$coursecat->id", $newcourse->shortname); // Make sure the new course fullname is based on the category name. - $this->assertEquals("Shared teaching resources for category: {$coursecat->name}", $newcourse->fullname); + $this->assertEquals("Shared teaching resources for category: $coursecat->name", $newcourse->fullname); $coursemodinfo = get_fast_modinfo($newcourse); $coursecatqbanks = $coursemodinfo->get_instances_of('qbank'); @@ -434,7 +435,7 @@ final class transfer_question_categories_test extends \advanced_testcase { $coursecatqbank = reset($coursecatqbanks); // Make sure the new module name is what we expect. - $this->assertEquals("{$coursecat->name} shared question bank", $coursecatqbank->name); + $this->assertEquals("$coursecat->name shared question bank", $coursecatqbank->name); $coursecatqcats = $DB->get_records('question_categories', ['contextid' => $coursecatqbank->context->id], 'parent ASC'); @@ -460,7 +461,7 @@ final class transfer_question_categories_test extends \advanced_testcase { // The module name should be what we expect. $courseqbank = reset($courseqbanks); - $this->assertEquals("{$course->shortname} shared question bank", $courseqbank->name); + $this->assertEquals("$course->shortname shared question bank", $courseqbank->name); // Make sure the question categories still exist and that we have a new top one at the new module context. $topcat = question_get_top_category($courseqbank->context->id); @@ -507,7 +508,7 @@ final class transfer_question_categories_test extends \advanced_testcase { $usedunusedcourse = $usedunusedmodinfo->get_course(); $usedunusedqbanks = $usedunusedmodinfo->get_instances_of('qbank'); $usedunusedqbank = reset($usedunusedqbanks); - $this->assertEquals("{$usedunusedcourse->shortname} shared question bank", $usedunusedqbank->name); + $this->assertEquals("$usedunusedcourse->shortname shared question bank", $usedunusedqbank->name); // We should now only have 3 categories. Top, used and unused. $usedunusedcats = $DB->get_records( @@ -516,13 +517,106 @@ final class transfer_question_categories_test extends \advanced_testcase { fields: 'name, id', ); $this->assertCount(3, $usedunusedcats); - $this->assertTrue(array_key_exists('top', $usedunusedcats)); - $this->assertTrue(array_key_exists('Used Question Cat', $usedunusedcats)); - $this->assertTrue(array_key_exists('Unused Question Cat', $usedunusedcats)); - $this->assertFalse(array_key_exists('Empty Question Cat', $usedunusedcats)); + $this->assertArrayHasKey('top', $usedunusedcats); + $this->assertArrayHasKey('Used Question Cat', $usedunusedcats); + $this->assertArrayHasKey('Unused Question Cat', $usedunusedcats); + $this->assertArrayNotHasKey('Empty Question Cat', $usedunusedcats); $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])); } + + public function test_fix_wrong_parents(): void { + $this->resetAfterTest(); + $this->setup_pre_install_data(); + + // Create a second course. + $course2 = self::getDataGenerator()->create_course(); + $course2context = context_course::instance($course2->id); + + // In course2 we build this category structure: + // - $course2parentcat -- context $course2context + // - - $wrongchild1 -- context $this->coursecontext (wrong) + // - - - $wronggrandchild1 -- context $this->coursecontext (same wrong) + // - - - $doublywronggrandchild1 -- context $course2context (back right, but not matching its parent) + // - - $wrongchild2 -- context non-existant A + // - - - $wronggrandchild2 -- context non-existent A + // - - - $doublywronggrandchild2 -- context non-existent B. + $course2parentcat = $this->create_question_category( + 'Course2 parent cat', $course2context->id); + + $wrongchild1 = $this->create_question_category( + 'Child cat with wrong context', $this->coursecontext->id, $course2parentcat->id); + $wronggrandchild1 = $this->create_question_category( + 'Grandchild of child1 in same wrong context', $this->coursecontext->id, $wrongchild1->id); + $doublywronggrandchild1 = $this->create_question_category( + 'Grandchild of child1 back in the right context', $course2context->id, $wrongchild1->id); + + $wrongchild2 = $this->create_question_category( + 'Child cat with non-existent context', $course2context->id + 1000, $course2parentcat->id); + $wronggrandchild2 = $this->create_question_category( + 'Grandchild of child2 with same non-existent context', $course2context->id + 1000, $wrongchild2->id); + $doublywronggrandchild2 = $this->create_question_category( + 'Grandchild of child2 with different non-existent context', $course2context->id + 2000, $wrongchild2->id); + + // Before we clean up, check that the expected categories are picked up. + // $wronggrandchild1 & $wronggrandchild2 are not seen, because their contexts match + // their parent's even though both are wrong. They should still get fixed. + $task = new transfer_question_categories(); + $this->assertEquals( + [ + $wrongchild1->id => $wrongchild1->contextid, + $doublywronggrandchild1->id => $course2context->id, + $wrongchild2->id => $wrongchild2->contextid, + $doublywronggrandchild2->id => $doublywronggrandchild2->contextid, + ], + $task->get_categories_in_a_different_context_to_their_parent(), + ); + + // Call the cleanup method. + $task->fix_wrong_parents(); + + // Now we expect no mismatches. + $this->assertEmpty($task->get_categories_in_a_different_context_to_their_parent()); + + // Assert that the child categories have been moved to the locations they should have been. + $this->assert_category_is_in_context_with_parent($this->coursecontext, null, $wrongchild1->id); + $this->assert_category_is_in_context_with_parent($this->coursecontext, $wrongchild1, $wronggrandchild1->id); + $this->assert_category_is_in_context_with_parent($course2context, null, $doublywronggrandchild1->id); + $this->assert_category_is_in_context_with_parent($course2context, $course2parentcat, $wrongchild2->id); + $this->assert_category_is_in_context_with_parent($course2context, $wrongchild2, $wronggrandchild2->id); + $this->assert_category_is_in_context_with_parent($course2context, $wrongchild2, $doublywronggrandchild2->id); + } + + /** + * Assert that the category with id $categoryid is in context $expectedcontext, with the given parent. + * + * @param context $expectedcontext the expected context for the category with id $categoryid. + * @param stdClass|null $expectedparent the expected parent category. + * null means the Top category in $expectedcontext. + * @param int $categoryid the id of the category to check. + */ + protected function assert_category_is_in_context_with_parent( + context $expectedcontext, + ?stdClass $expectedparent, + int $categoryid, + ): void { + global $DB; + + if ($expectedparent === null) { + $expectedparent = $DB->get_record( + 'question_categories', + ['contextid' => $expectedcontext->id, 'parent' => 0], + '*', + MUST_EXIST, + ); + } + + $actualcategory = $DB->get_record('question_categories', ['id' => $categoryid]); + $this->assertEquals($expectedparent->id, $actualcategory->parent, + "Checking parent of category $actualcategory->name."); + $this->assertEquals($expectedcontext->id, $actualcategory->contextid, + "Checking context of category $actualcategory->name."); + } }