From 9547082cdd44918c51d0d18b24f5c0ee412534e2 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Mon, 26 Jan 2026 15:57:27 +0000 Subject: [PATCH] MDL-87315 backup: Update set reference question category ID on restore. When a question bank is restored along with a quiz that uses a random question from that bank, the set reference for the random question should now point to the category in the restored bank, rather than original. This uses the question bank context IDs recorded using the method added in MDL-86136 to decide if the set reference's question context is being restored, and update it accordingly. This required adding a new restore step in the restore_root_task, since when a restore is performed asynchronously, the temp IDs table is deleted after the prechecks are performed when the task is queued. Adding this step allows the IDs to be recorded again when the restore runs. --- .../moodle2/restore_root_task.class.php | 3 + public/backup/moodle2/restore_stepslib.php | 8 +- .../backup/restore_load_questionbanks.php | 48 +++++++++ .../mod/qbank/tests/backup/restore_test.php | 98 +++++++++++++++++++ .../classes/category_condition.php | 4 +- .../tests/category_condition_test.php | 21 ++-- .../tagquestion/classes/tag_condition.php | 1 + .../tagquestion/tests/tag_condition_test.php | 33 +++++++ .../question/classes/local/bank/condition.php | 2 + 9 files changed, 201 insertions(+), 17 deletions(-) create mode 100644 public/lib/classes/backup/restore_load_questionbanks.php diff --git a/public/backup/moodle2/restore_root_task.class.php b/public/backup/moodle2/restore_root_task.class.php index b3c31cf246b..132a710ae86 100644 --- a/public/backup/moodle2/restore_root_task.class.php +++ b/public/backup/moodle2/restore_root_task.class.php @@ -78,6 +78,9 @@ class restore_root_task extends restore_task { // Unconditionally, load create all the needed outcomes $this->add_step(new restore_outcomes_structure_step('create_scales', 'outcomes.xml')); + // If we haven't preloaded information, load all the question banks to temp_ids_table. + $this->add_step(new \core\backup\restore_load_questionbanks('load_questionbanks')); + // If we haven't preloaded information, load all the needed categories and questions (reduced) to temp_ids_table $this->add_step(new restore_load_categories_and_questions('load_categories_and_questions')); diff --git a/public/backup/moodle2/restore_stepslib.php b/public/backup/moodle2/restore_stepslib.php index 86994a1576e..b9f7fe2b031 100644 --- a/public/backup/moodle2/restore_stepslib.php +++ b/public/backup/moodle2/restore_stepslib.php @@ -6510,6 +6510,12 @@ trait restore_question_set_reference_data_trait { $data->usingcontextid = $this->get_mappingid('context', $data->usingcontextid); $data->itemid = $this->get_new_parentid('quiz_question_instance'); + $originalbankinbackup = (bool) restore_dbops::get_backup_ids_record( + $this->get_restoreid(), + 'questionbank', + $data->questionscontextid, + ); + if ($context = $this->get_mappingid('context', $data->questionscontextid)) { $data->questionscontextid = $context; } else { @@ -6538,7 +6544,7 @@ trait restore_question_set_reference_data_trait { $qbankfeature = new $qbankfeatureclass(); $filters = $qbankfeature->get_question_filters(); foreach ($filters as $filter) { - $filtercondition = $filter->restore_filtercondition($filtercondition, $data, $this); + $filtercondition = $filter->restore_filtercondition($filtercondition, $data, $this, $originalbankinbackup); } } diff --git a/public/lib/classes/backup/restore_load_questionbanks.php b/public/lib/classes/backup/restore_load_questionbanks.php new file mode 100644 index 00000000000..fc069e6d96a --- /dev/null +++ b/public/lib/classes/backup/restore_load_questionbanks.php @@ -0,0 +1,48 @@ +. + +namespace core\backup; + +defined('MOODLE_INTERNAL') || die(); +require_once($CFG->dirroot . '/backup/util/plan/restore_execution_step.class.php'); + +/** + * Load question bank context IDs. + * + * Execution step that, *conditionally* (if there isn't preloaded information) + * will load the context IDs of activities in the backup containing questions + * to backup_temp_ids. They will be stored with "questionbank" itemname and their + * original context ID as itemid. + * + * @package core + * @copyright 2026 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 + */ +class restore_load_questionbanks extends \restore_execution_step { + /** + * If restore data has not already been loaded in this request, load the question bank context IDs. + */ + protected function define_execution(): void { + global $CFG; + require_once($CFG->dirroot . '/backup/util/dbops/restore_dbops.class.php'); + if ($this->task->get_preloaded_information()) { // If info is already preloaded, nothing to do. + return; + } + $path = $this->get_basepath() . '/activities'; + \restore_dbops::load_questionbanks_to_tempids($this->get_restoreid(), $path); + } +} diff --git a/public/mod/qbank/tests/backup/restore_test.php b/public/mod/qbank/tests/backup/restore_test.php index 74bf01213b7..7e75d736538 100644 --- a/public/mod/qbank/tests/backup/restore_test.php +++ b/public/mod/qbank/tests/backup/restore_test.php @@ -17,6 +17,11 @@ namespace mod_qbank\backup; use core\context\module; +use core_question\local\bank\condition; +use mod_quiz\external\add_random_questions; +use mod_quiz\question\bank\filter\custom_category_condition; +use mod_quiz\quiz_settings; +use mod_quiz\structure; /** * Tests to cover restoring or import a question bank and its questions multiple times to the same course. @@ -233,4 +238,97 @@ final class restore_test extends \advanced_testcase { $this->assertCount(2, array_filter($qbankquestions, fn($question) => $question->parent == $qbankmaq->id)); } } + + public function test_update_set_reference_on_restore(): void { + global $DB, $CFG, $USER; + require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); + require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); + + $this->resetAfterTest(); + $this->setAdminUser(); + + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + [, $course, $qcat, , $qbank] = $questiongenerator->setup_course_and_questions(); + $qbankcontext = module::instance($qbank->cmid); + $filtercondition = [ + 'cat' => "{$qcat->id},{$qbankcontext->id}", + 'sortdata' => [], + 'filter' => [ + 'category' => [ + 'jointype' => condition::JOINTYPE_DEFAULT, + 'values' => [$qcat->id], + 'filteroptions' => ['includesubcategories' => false], + ], + ], + ]; + $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); + $quiz = $quizgenerator->create_instance(['course' => $course->id]); + $settings = quiz_settings::create_for_cmid($quiz->cmid); + $structure = structure::create_for_quiz($settings); + $structure->add_random_questions(1, 1, $filtercondition); + + $setreference = $DB->get_record( + 'question_set_references', + [ + 'component' => 'mod_quiz', + 'questionarea' => 'slot', + 'usingcontextid' => $settings->get_context()->id, + 'questionscontextid' => $qbankcontext->id, + ], + ); + $this->assertNotFalse($setreference); + + // Backup qbank. + $bc = new \backup_controller( + \backup::TYPE_1COURSE, + $course->id, + \backup::FORMAT_MOODLE, + \backup::INTERACTIVE_NO, + \backup::MODE_IMPORT, + $USER->id, + ); + $backupid = $bc->get_backupid(); + $bc->execute_plan(); + $bc->destroy(); + + $course2 = $this->getDataGenerator()->create_course(); + // Restore the backup into another course twice. + $rc = new \restore_controller( + $backupid, + $course2->id, + \backup::INTERACTIVE_NO, + \backup::MODE_IMPORT, + $USER->id, + \backup::TARGET_CURRENT_ADDING, + ); + $rc->execute_precheck(droptemptablesafter: true); // Dropping temp tables simulates an asynchronous restore. + $rc->execute_plan(); + $rc->destroy(); + + $modinfo = get_fast_modinfo($course2->id); + $newquizzes = $modinfo->get_instances_of('quiz'); + $newquiz = reset($newquizzes); + $newqbanks = $modinfo->get_instances_of('qbank'); + $newqbank = reset($newqbanks); + $newqcat = $DB->get_record('question_categories', ['contextid' => $newqbank->context->id, 'stamp' => $qcat->stamp]); + + $newsetreference = $DB->get_record( + 'question_set_references', + [ + 'component' => 'mod_quiz', + 'questionarea' => 'slot', + 'usingcontextid' => $newquiz->context->id, + 'questionscontextid' => $newqbank->context->id, + ], + ); + $this->assertNotFalse($newsetreference); + $newfiltercondition = json_decode($newsetreference->filtercondition, true); + $this->assertEquals($newfiltercondition['cat'], "{$newqcat->id},{$newqbank->context->id}"); + $this->assertEquals($newfiltercondition['filter']['category']['values'][0], $newqcat->id); + + $newsettings = quiz_settings::create_for_cmid($newquiz->id); + $newstructure = structure::create_for_quiz($newsettings); + $randomslot = $newstructure->get_slot_by_number(1); + $this->assertEquals($randomslot->id, $newsetreference->itemid); + } } diff --git a/public/question/bank/managecategories/classes/category_condition.php b/public/question/bank/managecategories/classes/category_condition.php index aa28e7dc2a9..48ff11d3ffc 100644 --- a/public/question/bank/managecategories/classes/category_condition.php +++ b/public/question/bank/managecategories/classes/category_condition.php @@ -311,6 +311,7 @@ class category_condition extends condition { array $filtercondition, stdClass $setreference, restore_questions_activity_structure_step $restorestep, + bool $originalbankinbackup = false, ): array { global $DB; // Map category id used for category filter condition and corresponding context id. @@ -325,13 +326,12 @@ class category_condition extends condition { !$restorestep->get_task()->is_samesite() || !$questionscontext || !$DB->record_exists('question_categories', ['id' => $oldcategoryid]) + || $originalbankinbackup || $setreference->usingcontextid == $setreference->questionscontextid || !has_capability('moodle/question:useall', $questionscontext) ) { $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/public/question/bank/managecategories/tests/category_condition_test.php b/public/question/bank/managecategories/tests/category_condition_test.php index 7e13c0c9526..60a7b682b25 100644 --- a/public/question/bank/managecategories/tests/category_condition_test.php +++ b/public/question/bank/managecategories/tests/category_condition_test.php @@ -249,31 +249,26 @@ final class category_condition_test extends \advanced_testcase { } /** - * When the category is updated in the filter condition, the context ID is also updated. + * As {@see test_restore_filtercondition}, but the original question bank was present in the backup. */ - public function test_restore_filtercondition_update_context(): void { + public function test_restore_filtercondition_questionbankinbackup(): 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, + $category->id, ], ], ], - 'cat' => [ - "{$fakecategory},{$fakecontext}", - ], ]; $setreference = (object) [ - 'questionscontextid' => $fakecontext, - 'usingcontextid' => $category->contextid, + 'questionscontextid' => $category->contextid, + 'usingcontextid' => $category->contextid + 1, ]; - $mappedid = $category->id; + $mappedid = $category->id + 1; $mockstep = $this->get_mock_step($this->get_samesite_task()); $mockstep->method('get_mappingid')->willReturn($mappedid); @@ -281,10 +276,8 @@ final class category_condition_test extends \advanced_testcase { $condition = new category_condition(); - $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep); + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep, true); $this->assertEquals($mappedid, $filtercondition['filter']['category']['values'][0]); - $this->assertEquals($category->contextid, $setreference->questionscontextid); - $this->assertEquals($filtercondition['cat'], "{$mappedid},{$category->contextid}"); } } diff --git a/public/question/bank/tagquestion/classes/tag_condition.php b/public/question/bank/tagquestion/classes/tag_condition.php index 02c6a3f3c9c..b6360fce405 100644 --- a/public/question/bank/tagquestion/classes/tag_condition.php +++ b/public/question/bank/tagquestion/classes/tag_condition.php @@ -167,6 +167,7 @@ class tag_condition extends condition { array $filtercondition, stdClass $setreference, restore_questions_activity_structure_step $restorestep, + bool $originalbankinbackup = false, ): array { if (isset($filtercondition['filter']['qtagids'])) { $newtagids = []; diff --git a/public/question/bank/tagquestion/tests/tag_condition_test.php b/public/question/bank/tagquestion/tests/tag_condition_test.php index 41f49e59e1a..7ce8d7e8bb3 100644 --- a/public/question/bank/tagquestion/tests/tag_condition_test.php +++ b/public/question/bank/tagquestion/tests/tag_condition_test.php @@ -521,4 +521,37 @@ final class tag_condition_test extends \advanced_testcase { $this->assertArrayNotHasKey('qtagids', $filtercondition['filter']); } + + /** + * Restoring the filter when the original question bank was in the backup doesn't change the behaviour. + */ + public function test_restore_filtercondition_originalbankinbackup(): void { + $this->resetAfterTest(); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $questiongenerator->create_question_category(); + $tag1 = $this->getDataGenerator()->create_tag(); + $tag2 = $this->getDataGenerator()->create_tag(); + $filtercondition = [ + 'filter' => [ + 'qtagids' => [ + 'values' => [ + $tag1->id, + $tag2->id, + ], + ], + ], + ]; + $setreference = (object) [ + 'questionscontextid' => $category->contextid, + 'usingcontextid' => $category->contextid, + ]; + $mockstep = $this->get_mock_step($this->get_samesite_task()); + $mockstep->method('get_mappingid')->willReturn($tag2->id + 1); + + $condition = new tag_condition(); + + $filtercondition = $condition->restore_filtercondition($filtercondition, $setreference, $mockstep, true); + + $this->assertEquals([$tag1->id, $tag2->id], $filtercondition['filter']['qtagids']['values']); + } } diff --git a/public/question/classes/local/bank/condition.php b/public/question/classes/local/bank/condition.php index 8897881d7bc..92c6dcc2293 100644 --- a/public/question/classes/local/bank/condition.php +++ b/public/question/classes/local/bank/condition.php @@ -249,12 +249,14 @@ abstract class condition { * @param array $filtercondition The origin $filtercondition with preceding conversions applied. * @param stdClass $setreference The set reference record from the backup. * @param restore_questions_activity_structure_step $restorestep The restore step. + * @param bool $originalbankinbackup Was the original question bank these questions belonged to included in the backup? * @return array the modified $filtercondition */ public function restore_filtercondition( array $filtercondition, stdClass $setreference, restore_questions_activity_structure_step $restorestep, + bool $originalbankinbackup = false, ): array { return $filtercondition; }