From b82be2ab8c33b975935df0276f75e20a36731a0a Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Mon, 1 Dec 2025 13:13:23 +0000 Subject: [PATCH] MDL-86524 questions: Delegate filter condition restore to plugins The core backup code was taking sole responsibility for conversion and mapping of IDs in set reference filter conditions during restores. However, it was only actually processing category conditions, not tags or any of the more recently added conditions. This adds a new `restore_filtercondition` method to the `core_question\local\bank\condition` class which can be overridden by each condition to convert IDs and add mappings during the restore process. It also adds a new `maptags` option to `convert_legacy_set_reference_filter_condition`, so that if we are doing the conversion during a restore, we don't also create or map the tags as they are converted. We record ID mappings of the tags as the are restored, then these can be used by the `qbank_tagquestions` plugins later to map the converted condition. --- .upgradenotes/MDL-86524-2025120211115999.yml | 12 ++++ backup/moodle2/restore_stepslib.php | 63 +++++++++---------- .../classes/category_condition.php | 40 ++++++++++++ question/classes/local/bank/condition.php | 18 ++++++ .../classes/question_reference_manager.php | 16 +++-- 5 files changed, 111 insertions(+), 38 deletions(-) create mode 100644 .upgradenotes/MDL-86524-2025120211115999.yml diff --git a/.upgradenotes/MDL-86524-2025120211115999.yml b/.upgradenotes/MDL-86524-2025120211115999.yml new file mode 100644 index 00000000000..5a72101c37f --- /dev/null +++ b/.upgradenotes/MDL-86524-2025120211115999.yml @@ -0,0 +1,12 @@ +issueNumber: MDL-86524 +notes: + core_question: + - message: >- + During restore of a question_set_reference, mapping of IDs in the + filtercondition is now delegated to qbank plugins. + If your qbank plugin defines a filter condition that uses database + IDs, add an override of `restore_filtercondition()` to the `condition` + class, which checks the condition's data and replaces the IDs with + mapped values if required. See + `qbank_managecategories\category_condition` for an example. + type: improved diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index daa0cb6cad7..980c94bf005 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -5472,12 +5472,18 @@ class restore_create_categories_and_questions extends restore_structure_step { } $tagcontextid = $this->cachedcategory->contextid; // Add the tag to the question. - core_tag_tag::add_item_tag('core_question', + $taginstanceid = core_tag_tag::add_item_tag( + 'core_question', 'question', $newquestion, context::instance_by_id($tagcontextid), - $tagname + $tagname, ); + $tagid = $DB->get_field('tag_instance', 'tagid', ['id' => $taginstanceid]); + if ($tagid != $data->id) { + // The tag didn't exist already, map the new ID. + $this->set_mapping('tag', $data->id, $tagid); + } } } @@ -6495,50 +6501,41 @@ trait restore_question_set_reference_data_trait { public function process_question_set_reference($data) { global $DB; $data = (object) $data; - $owncontext = $data->usingcontextid == $data->questionscontextid; $data->usingcontextid = $this->get_mappingid('context', $data->usingcontextid); $data->itemid = $this->get_new_parentid('quiz_question_instance'); + + if ($context = $this->get_mappingid('context', $data->questionscontextid)) { + $data->questionscontextid = $context; + } else { + $this->log( + "question_set_reference with old id {$data->id} referenced question context " + . "{$data->questionscontextid} which was not included in the backup. Therefore, this has been " + . "restored with the old questionscontextid.", + backup::LOG_WARNING, + ); + } + $filtercondition = json_decode($data->filtercondition, true); if (!isset($filtercondition['filter'])) { // Pre-4.3, convert the old filtercondition format to the new format. + // Don't map tags to new IDs, the plugin will do that below. $filtercondition = \core_question\question_reference_manager::convert_legacy_set_reference_filter_condition( - $filtercondition); + $filtercondition, + false, + ); } - // Map category id used for category filter condition and corresponding context id. - $oldcategoryid = $filtercondition['filter']['category']['values'][0]; - // Decide if we're going to refer back to the original category, or to the new category. - // Are we restoring to a different site? - // Has the original context or category been deleted? - // Did the old category belong to the same context as the original set reference? - // Are we allowed to use its questions? - $questionscontext = context::instance_by_id($data->questionscontextid, IGNORE_MISSING); - if ( - !$this->get_task()->is_samesite() - || !$questionscontext - || !$DB->record_exists('question_categories', ['id' => $oldcategoryid]) - || $owncontext - || !has_capability('moodle/question:useall', $questionscontext) - ) { - $newcategoryid = $this->get_mappingid('question_category', $oldcategoryid); - $filtercondition['filter']['category']['values'][0] = $newcategoryid; + $qbankfeatureclasses = \core\component::get_plugin_list_with_class('qbank', 'plugin_feature'); - if ($context = $this->get_mappingid('context', $data->questionscontextid)) { - $data->questionscontextid = $context; - } else { - $this->log('question_set_reference with old id ' . $data->id . - ' referenced question context ' . $data->questionscontextid . - ' which was not included in the backup. Therefore, this has been ' . - ' restored with the old questionscontextid.', backup::LOG_WARNING); + foreach ($qbankfeatureclasses as $qbankfeatureclass) { + $qbankfeature = new $qbankfeatureclass(); + $filters = $qbankfeature->get_question_filters(); + foreach ($filters as $filter) { + $filtercondition = $filter->restore_filtercondition($filtercondition, $data, $this); } } - $filtercondition['cat'] = implode(',', [ - $filtercondition['filter']['category']['values'][0], - $data->questionscontextid, - ]); - $data->filtercondition = json_encode($filtercondition); $DB->insert_record('question_set_references', $data); diff --git a/question/bank/managecategories/classes/category_condition.php b/question/bank/managecategories/classes/category_condition.php index eafc71cba61..e7797537586 100644 --- a/question/bank/managecategories/classes/category_condition.php +++ b/question/bank/managecategories/classes/category_condition.php @@ -16,9 +16,12 @@ namespace qbank_managecategories; +use core\context; use core\output\datafilter; use core_question\local\bank\condition; use core_question\local\bank\view; +use restore_questions_activity_structure_step; +use stdClass; /** * This class controls from which category questions are listed. @@ -303,4 +306,41 @@ class category_condition extends condition { } + #[\Override] + public function restore_filtercondition( + array $filtercondition, + stdClass $setreference, + restore_questions_activity_structure_step $restorestep, + ): array { + global $DB; + // Map category id used for category filter condition and corresponding context id. + $oldcategoryid = $filtercondition['filter']['category']['values'][0]; + // Decide if we're going to refer back to the original category, or to the new category. + // Are we restoring to a different site? + // Has the original context or category been deleted? + // Did the old category belong to the same context as the original set reference? + // Are we allowed to use its questions? + $questionscontext = context::instance_by_id($setreference->questionscontextid, IGNORE_MISSING); + if ( + !$restorestep->get_task()->is_samesite() + || !$questionscontext + || !$DB->record_exists('question_categories', ['id' => $oldcategoryid]) + || $setreference->usingcontextid == $setreference->questionscontextid + || !has_capability('moodle/question:useall', $questionscontext) + ) { + $newcategoryid = $restorestep->get_mappingid('question_category', $oldcategoryid); + $filtercondition['filter']['category']['values'][0] = $newcategoryid; + } + + $filtercondition['cat'] = implode( + ',', + [ + $filtercondition['filter']['category']['values'][0], + $setreference->questionscontextid, + ], + ); + + return $filtercondition; + } + } diff --git a/question/classes/local/bank/condition.php b/question/classes/local/bank/condition.php index f5f1c02a508..8897881d7bc 100644 --- a/question/classes/local/bank/condition.php +++ b/question/classes/local/bank/condition.php @@ -17,6 +17,8 @@ namespace core_question\local\bank; use core\output\datafilter; +use restore_questions_activity_structure_step; +use stdClass; /** * An abstract class for filtering/searching questions. @@ -240,4 +242,20 @@ abstract class condition { * @return array ['SQL where condition', ['param1' => 'value1', 'param2' => 'value2', ...]] */ abstract public static function build_query_from_filter(array $filter): array; + + /** + * Convert and map values in the restored filtercondition to corresponding values on the current site/course. + * + * @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. + * @return array the modified $filtercondition + */ + public function restore_filtercondition( + array $filtercondition, + stdClass $setreference, + restore_questions_activity_structure_step $restorestep, + ): array { + return $filtercondition; + } } diff --git a/question/classes/question_reference_manager.php b/question/classes/question_reference_manager.php index 5c1a4e4974c..165c21cc895 100644 --- a/question/classes/question_reference_manager.php +++ b/question/classes/question_reference_manager.php @@ -83,9 +83,11 @@ class question_reference_manager { * pre-4.3 filter condition structure to the new one. * * @param array $filtercondition Pre-4.3 filter condition. + * @param bool $maptags Map tags to tags with the same name, creating them if necessary. If not, just convert existing IDs to + * the new structure. * @return array Post-4.3 filter condition. */ - public static function convert_legacy_set_reference_filter_condition(array $filtercondition): array { + public static function convert_legacy_set_reference_filter_condition(array $filtercondition, bool $maptags = true): array { global $DB; if (!isset($filtercondition['filter'])) { $filtercondition['filter'] = []; @@ -103,10 +105,14 @@ class question_reference_manager { if (isset($filtercondition['tags'])) { // Get the names of the tags in the condition. Find or create corresponding tags, // and set their ids in the new condition. - $oldtags = array_map(fn($oldtag) => explode(',', $oldtag)[1], $filtercondition['tags']); - $questiontagcollid = \core_tag_area::get_collection('core_question', 'question'); - $newtags = \core_tag_tag::create_if_missing($questiontagcollid, $oldtags); - $newtagids = array_map(fn($newtag) => $newtag->id, $newtags); + if ($maptags) { + $oldtags = array_map(fn($oldtag) => explode(',', $oldtag)[1], $filtercondition['tags']); + $questiontagcollid = \core_tag_area::get_collection('core_question', 'question'); + $newtags = \core_tag_tag::create_if_missing($questiontagcollid, $oldtags); + $newtagids = array_map(fn($newtag) => $newtag->id, $newtags); + } else { + $newtagids = array_map(fn($oldtag) => explode(',', $oldtag)[0], $filtercondition['tags']); + } $filtercondition['filter']['qtagids'] = [ 'jointype' => \qbank_tagquestion\tag_condition::JOINTYPE_DEFAULT,