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;