From 049bff63bc119112efef286367c742126e29c957 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 11 Dec 2023 19:11:45 +0000 Subject: [PATCH] MDL-79527 questions: category tree loading handling bad data It should always be the case that a question category and its parent category belong to the same context. However, over the years, we have observed that this becomes violated by some data in some Moodle sites. (The suspicion is that this is a backup/restor bug of some sort which has never been tracked down.) Since this happens, we should make the code robust to it, which is what this change does. It ensure that when loading child categories, we only consider ones in the same context. --- lib/questionlib.php | 42 +++++++------ lib/tests/questionlib_test.php | 101 +++++++++++++++++++++++++++++-- question/tests/generator/lib.php | 6 +- 3 files changed, 123 insertions(+), 26 deletions(-) diff --git a/lib/questionlib.php b/lib/questionlib.php index c0771b4a5d3..b292ee06392 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -1306,10 +1306,11 @@ function question_categorylist($categoryid): array { global $DB; // Final list of category IDs. - $categorylist = array(); + $categorylist = []; // A list of category IDs to check for any sub-categories. - $subcategories = array($categoryid); + $subcategories = [$categoryid]; + $contextid = $DB->get_field('question_categories', 'contextid', ['id' => $categoryid]); while ($subcategories) { foreach ($subcategories as $subcategory) { @@ -1320,37 +1321,40 @@ function question_categorylist($categoryid): array { $categorylist[$subcategory] = $subcategory; } - list ($in, $params) = $DB->get_in_or_equal($subcategories); + [$in, $params] = $DB->get_in_or_equal($subcategories); + $params[] = $contextid; - $subcategories = $DB->get_records_select_menu('question_categories', "parent $in", $params, - null, 'id,id AS id2'); + // Order by id is not strictly needed, but it will be cheap, and makes the results deterministic. + $subcategories = $DB->get_records_select_menu('question_categories', + "parent $in AND contextid = ?", $params, 'id', 'id,id AS id2'); } return $categorylist; } /** - * Get all parent categories of a given question category in decending order. + * Get all parent categories of a given question category in descending order. + * * @param int $categoryid for which you want to find the parents. * @return array of question category ids of all parents categories. */ function question_categorylist_parents(int $categoryid): array { global $DB; - $parent = $DB->get_field('question_categories', 'parent', array('id' => $categoryid)); - if (!$parent) { - return []; - } - $categorylist = [$parent]; - $currentid = $parent; - while ($currentid) { - $currentid = $DB->get_field('question_categories', 'parent', array('id' => $currentid)); - if ($currentid) { - $categorylist[] = $currentid; + + $category = $DB->get_record('question_categories', ['id' => $categoryid]); + $contextid = $category->contextid; + + $categorylist = []; + while ($category->parent) { + $category = $DB->get_record('question_categories', ['id' => $category->parent]); + if (!$category || $category->contextid != $contextid) { + break; } + $categorylist[] = $category->id; } - // Present the list in decending order (the top category at the top). - $categorylist = array_reverse($categorylist); - return $categorylist; + + // Present the list in descending order (the top category at the top). + return array_reverse($categorylist); } // Import/Export Functions. diff --git a/lib/tests/questionlib_test.php b/lib/tests/questionlib_test.php index fdec7014422..7c6cc9c98fc 100644 --- a/lib/tests/questionlib_test.php +++ b/lib/tests/questionlib_test.php @@ -2009,9 +2009,73 @@ class questionlib_test extends \advanced_testcase { } /** - * Test of question_categorylist_parents function. + * Test of question_categorylist function. + * + * @covers ::question_categorylist() */ - public function test_question_categorylist_parents() { + public function test_question_categorylist(): void { + $this->resetAfterTest(); + + // Create a category tree. + /** @var \core_question_generator $questiongenerator */ + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $context = \context_system::instance(); + + $top = question_get_top_category($context->id, true); + $cat1 = $questiongenerator->create_question_category(['parent' => $top->id]); + $sub11 = $questiongenerator->create_question_category(['parent' => $cat1->id]); + $sub12 = $questiongenerator->create_question_category(['parent' => $cat1->id]); + $cat2 = $questiongenerator->create_question_category(['parent' => $top->id]); + $sub22 = $questiongenerator->create_question_category(['parent' => $cat2->id]); + + // Test - returned array has keys and values the same. + $this->assertEquals([$sub22->id], array_keys(question_categorylist($sub22->id))); + $this->assertEquals([$sub22->id], array_values(question_categorylist($sub22->id))); + $this->assertEquals([$cat1->id, $sub11->id, $sub12->id], array_keys(question_categorylist($cat1->id))); + $this->assertEquals([$cat1->id, $sub11->id, $sub12->id], array_values(question_categorylist($cat1->id))); + $this->assertEquals([$top->id, $cat1->id, $cat2->id, $sub11->id, $sub12->id, $sub22->id], + array_keys(question_categorylist($top->id))); + $this->assertEquals([$top->id, $cat1->id, $cat2->id, $sub11->id, $sub12->id, $sub22->id], + array_values(question_categorylist($top->id))); + } + + /** + * Test of question_categorylist function when there is bad data, with a category pointing to a parent in another context. + * + * This is a situation that should never arise (parents and their children should always belong to the same context) + * but it does, because bugs, so the code should be robust to it. + * + * @covers ::question_categorylist() + */ + public function test_question_categorylist_bad_data(): void { + $this->resetAfterTest(); + + // Create a category tree. + $course = $this->getDataGenerator()->create_course(); + $coursecontext = \context_course::instance($course->id); + /** @var \core_question_generator $questiongenerator */ + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $context = \context_system::instance(); + + $top = question_get_top_category($context->id, true); + $cat1 = $questiongenerator->create_question_category(['parent' => $top->id]); + $sub11 = $questiongenerator->create_question_category(['parent' => $cat1->id]); + $sub12 = $questiongenerator->create_question_category(['parent' => $cat1->id]); + $cat2 = $questiongenerator->create_question_category(['parent' => $top->id, 'contextid' => $coursecontext->id]); + $sub22 = $questiongenerator->create_question_category(['parent' => $cat2->id]); + + // Test - returned array has keys and values the same. + $this->assertEquals([$cat2->id, $sub22->id], array_keys(question_categorylist($cat2->id))); + $this->assertEquals([$top->id, $cat1->id, $sub11->id, $sub12->id], + array_keys(question_categorylist($top->id))); + } + + /** + * Test of question_categorylist_parents function. + * + * @covers ::question_categorylist_parents() + */ + public function test_question_categorylist_parents(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator(); /** @var \core_question_generator $questiongenerator */ @@ -2023,11 +2087,36 @@ class questionlib_test extends \advanced_testcase { // Add sub-categories. $cat1 = $questiongenerator->create_question_category(['parent' => $cat0->id]); $cat2 = $questiongenerator->create_question_category(['parent' => $cat1->id]); + // Test the 'get parents' function. - $parentcategories = question_categorylist_parents($cat2->id); - $this->assertEquals($cat0->id, $parentcategories[0]); - $this->assertEquals($cat1->id, $parentcategories[1]); - $this->assertCount(2, $parentcategories); + $this->assertEquals([$cat0->id, $cat1->id], question_categorylist_parents($cat2->id)); + } + + /** + * Test question_categorylist_parents when there is bad data, with a category pointing to a parent in another context. + * + * This is a situation that should never arise (parents and their children should always belong to the same context) + * but it does, because bugs, so the code should be robust to it. + * + * @covers ::question_categorylist_parents() + */ + public function test_question_categorylist_parents_bad_data(): void { + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + /** @var \core_question_generator $questiongenerator */ + $questiongenerator = $generator->get_plugin_generator('core_question'); + $category = $generator->create_category(); + $context = \context_coursecat::instance($category->id); + // Create a top category. + $cat0 = question_get_top_category($context->id, true); + // Add sub-categories - but in a different context. + $cat1 = $questiongenerator->create_question_category( + ['parent' => $cat0->id, 'contextid' => \context_system::instance()->id]); + $cat2 = $questiongenerator->create_question_category( + ['parent' => $cat1->id, 'contextid' => \context_system::instance()->id]); + + // Test the 'get parents' function only returns categories in the same context. + $this->assertEquals([$cat1->id], question_categorylist_parents($cat2->id)); } /** diff --git a/question/tests/generator/lib.php b/question/tests/generator/lib.php index a1d3deb6be8..3269428ef58 100644 --- a/question/tests/generator/lib.php +++ b/question/tests/generator/lib.php @@ -69,7 +69,11 @@ class core_question_generator extends component_generator_base { $record = $this->datagenerator->combine_defaults_and_record($defaults, $record); if (!isset($record['contextid'])) { - $record['contextid'] = context_system::instance()->id; + if (isset($record['parent'])) { + $record['contextid'] = $DB->get_field('question_categories', 'contextid', ['id' => $record['parent']]); + } else { + $record['contextid'] = context_system::instance()->id; + } } if (!isset($record['parent'])) { $record['parent'] = question_get_top_category($record['contextid'], true)->id;