From be0160ae0303879a358e071b370c85e28b0aebc2 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Mon, 1 Dec 2025 14:15:54 +0000 Subject: [PATCH] MDL-86524 qbank_managecateories: Fix questionscontextid on restore If a backup contains a set reference where the questionscontextid does not match the context of the question category (as the category was moved, but the reference was not updated correctly), then we might end up restoring a quiz with a broken question. This change ensures that we update the context to match the question category during the restore. --- .../classes/category_condition.php | 2 + .../tests/category_condition_test.php | 290 ++++++++++++++++++ .../classes/test/mock_restore_test_trait.php | 71 +++++ 3 files changed, 363 insertions(+) create mode 100644 question/bank/managecategories/tests/category_condition_test.php create mode 100644 question/classes/test/mock_restore_test_trait.php diff --git a/question/bank/managecategories/classes/category_condition.php b/question/bank/managecategories/classes/category_condition.php index e7797537586..aa28e7dc2a9 100644 --- a/question/bank/managecategories/classes/category_condition.php +++ b/question/bank/managecategories/classes/category_condition.php @@ -330,6 +330,8 @@ class category_condition extends condition { ) { $newcategoryid = $restorestep->get_mappingid('question_category', $oldcategoryid); $filtercondition['filter']['category']['values'][0] = $newcategoryid; + // Make sure the questions context matches the new category. + $setreference->questionscontextid = $DB->get_field('question_categories', 'contextid', ['id' => $newcategoryid]); } $filtercondition['cat'] = implode( diff --git a/question/bank/managecategories/tests/category_condition_test.php b/question/bank/managecategories/tests/category_condition_test.php new file mode 100644 index 00000000000..7e13c0c9526 --- /dev/null +++ b/question/bank/managecategories/tests/category_condition_test.php @@ -0,0 +1,290 @@ +. + +namespace qbank_managecategories; + +use core\context\module; +use core_question\category_manager; +use core_question\test\mock_restore_test_trait; + +/** + * Unit tests for category_condition + * + * @package qbank_managecategories + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \qbank_managecategories\category_condition + */ +final class category_condition_test extends \advanced_testcase { + use mock_restore_test_trait; + + /** + * Restore a filter where we should keep the reference to the original context. + * + * - We are restoring to the same site. + * - The question context exists. + * - The question category exists. + * - The using context and questions context are different (e.g. the quiz is referencing a category in a qbank). + * - The current user is allowed to use the questions. + */ + public function test_restore_filtercondition(): void { + $this->resetAfterTest(); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $questiongenerator->create_question_category(); + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'values' => [ + $category->id, + ], + ], + ], + ]; + $setreference = (object) [ + 'questionscontextid' => $category->contextid, + 'usingcontextid' => $category->contextid + 1, + ]; + $mappedid = $category->id + 1; + $mockstep = $this->get_mock_step($this->get_samesite_task()); + $mockstep->method('get_mappingid')->willReturn($mappedid); + + $this->setAdminUser(); + + $condition = new category_condition(); + + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep); + + $this->assertEquals($category->id, $filtercondition['filter']['category']['values'][0]); + } + + /** + * As {@see test_restore_filtercondition}, but the user does not have permission to use questions in the original category. + */ + public function test_restore_filtercondition_no_permissions(): void { + $this->resetAfterTest(); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $questiongenerator->create_question_category(); + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'values' => [ + $category->id, + ], + ], + ], + ]; + $setreference = (object) [ + 'questionscontextid' => $category->contextid, + 'usingcontextid' => $category->contextid + 1, + ]; + $mappedid = $category->id + 1; + $mockstep = $this->get_mock_step($this->get_samesite_task()); + $mockstep->method('get_mappingid')->willReturn($mappedid); + + $condition = new category_condition(); + + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep); + + $this->assertEquals($mappedid, $filtercondition['filter']['category']['values'][0]); + } + + /** + * As {@see test_restore_filtercondition}, but the questions context and using context are the same. + */ + public function test_restore_filtercondition_same_context(): void { + $this->resetAfterTest(); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $questiongenerator->create_question_category(); + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'values' => [ + $category->id, + ], + ], + ], + ]; + $setreference = (object) [ + 'questionscontextid' => $category->contextid, + 'usingcontextid' => $category->contextid, + ]; + $mappedid = $category->id + 1; + $mockstep = $this->get_mock_step($this->get_samesite_task()); + $mockstep->method('get_mappingid')->willReturn($mappedid); + + $this->setAdminUser(); + + $condition = new category_condition(); + + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep); + + $this->assertEquals($mappedid, $filtercondition['filter']['category']['values'][0]); + } + + /** + * As {@see test_restore_filtercondition}, but the original category has been deleted. + */ + public function test_restore_filtercondition_no_category(): void { + $this->resetAfterTest(); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $questiongenerator->create_question_category(); + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'values' => [ + $category->id, + ], + ], + ], + ]; + $setreference = (object) [ + 'questionscontextid' => $category->contextid, + 'usingcontextid' => $category->contextid, + ]; + $mappedid = $category->id + 1; + $mockstep = $this->get_mock_step($this->get_samesite_task()); + $mockstep->method('get_mappingid')->willReturn($mappedid); + + $this->setAdminUser(); + + $manager = new category_manager(); + $manager->delete_category($category->id); + + $condition = new category_condition(); + + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep); + + $this->assertEquals($mappedid, $filtercondition['filter']['category']['values'][0]); + } + + /** + * As {@see test_restore_filtercondition}, but the original context has been deleted. + */ + public function test_restore_filtercondition_no_context(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + $course = $this->getDataGenerator()->create_course(); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $qbank1 = $this->getDataGenerator()->create_module('qbank', ['course' => $course->id]); + $qbank1context = module::instance($qbank1->cmid); + $oldparent = question_get_default_category($qbank1context->id); + $category = $questiongenerator->create_question_category(['parent' => $oldparent->id]); + // Move the category to a different context. + $qbank2 = $this->getDataGenerator()->create_module('qbank', ['course' => $course->id]); + $qbank2context = module::instance($qbank2->cmid); + $newparent = question_get_default_category($qbank2context->id); + $manager = new category_manager(); + $manager->update_category($category->id, "{$newparent->id},{$qbank2context->id}", $category->name, $category->info); + // Delete the original context. + course_delete_module($qbank1->cmid); + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'values' => [ + $category->id, + ], + ], + ], + ]; + $setreference = (object) [ + 'questionscontextid' => $category->contextid, + 'usingcontextid' => $category->contextid + 1, + ]; + $mappedid = $category->id + 1; + $mockstep = $this->get_mock_step($this->get_samesite_task()); + $mockstep->method('get_mappingid')->willReturn($mappedid); + + $condition = new category_condition(); + + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep); + + $this->assertEquals($mappedid, $filtercondition['filter']['category']['values'][0]); + } + + /** + * As {@see test_restore_filtercondition}, but restoring to a different site. + */ + public function test_restore_filtercondition_different_site(): void { + $this->resetAfterTest(); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $questiongenerator->create_question_category(); + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'values' => [ + $category->id, + ], + ], + ], + ]; + $setreference = (object) [ + 'questionscontextid' => $category->contextid, + 'usingcontextid' => $category->contextid + 1, + ]; + $mappedid = $category->id + 1; + $mockstep = $this->get_mock_step($this->get_not_samesite_task()); + $mockstep->method('get_mappingid')->willReturn($mappedid); + + $this->setAdminUser(); + + $condition = new category_condition(); + + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep); + + $this->assertEquals($mappedid, $filtercondition['filter']['category']['values'][0]); + } + + /** + * When the category is updated in the filter condition, the context ID is also updated. + */ + public function test_restore_filtercondition_update_context(): void { + $this->resetAfterTest(); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $questiongenerator->create_question_category(); + $fakecategory = $category->id + 1; + $fakecontext = $category->contextid + 1; + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'values' => [ + $fakecategory, + ], + ], + ], + 'cat' => [ + "{$fakecategory},{$fakecontext}", + ], + ]; + $setreference = (object) [ + 'questionscontextid' => $fakecontext, + 'usingcontextid' => $category->contextid, + ]; + $mappedid = $category->id; + $mockstep = $this->get_mock_step($this->get_samesite_task()); + $mockstep->method('get_mappingid')->willReturn($mappedid); + + $this->setAdminUser(); + + $condition = new category_condition(); + + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep); + + $this->assertEquals($mappedid, $filtercondition['filter']['category']['values'][0]); + $this->assertEquals($category->contextid, $setreference->questionscontextid); + $this->assertEquals($filtercondition['cat'], "{$mappedid},{$category->contextid}"); + } +} diff --git a/question/classes/test/mock_restore_test_trait.php b/question/classes/test/mock_restore_test_trait.php new file mode 100644 index 00000000000..7639eec5c75 --- /dev/null +++ b/question/classes/test/mock_restore_test_trait.php @@ -0,0 +1,71 @@ +. + +namespace core_question\test; + +use PHPUnit\Framework\MockObject\MockObject; + +/** + * Trait for providing mocked question restore objects + * + * @package core_question + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +trait mock_restore_test_trait { + /** + * Include restore class dependencies. + */ + public static function setUpBeforeClass(): void { + global $CFG; + require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); + } + + /** + * Return a mocked restore_task that will say we are restoring to the same site. + * + * @return MockObject The mocked restore_task. + */ + protected function get_samesite_task(): MockObject { + $mock = $this->createMock(\restore_task::class); + $mock->method('is_samesite')->willReturn(true); + return $mock; + } + + /** + * Return a mocked restore_task that will say we are not restoring to the same site. + * + * @return MockObject The mocked restore_task. + */ + protected function get_not_samesite_task(): MockObject { + $mock = $this->createMock(\restore_task::class); + $mock->method('is_samesite')->willReturn(false); + return $mock; + } + + /** + * Restore a mocked restore step that will use the provided task. + * + * @param MockObject $task The mocked restore_task. + * @return MockObject The mocked restore_questions_activity_structure_step. + */ + protected function get_mock_step(MockObject $task): MockObject { + $mock = $this->createMock(\restore_questions_activity_structure_step::class); + $mock->method('get_task')->willReturn($task); + return $mock; + } +}