From bab665dd4030b0e5aeb84aabfaf6906b223b0f27 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 3 Sep 2024 17:07:58 +0200 Subject: [PATCH 1/3] MDL-77625 question restore: fix pre-parsing of questions Restore of categories and questions happens in several phases. First, the file is scanned for which questions and categories it contains, to work out if these are new questions which need to be restored, or if they already exist in the database in a place that can be used. That code had not been updated to cope with the Moodle 4.0 versioning changes, so it is updated here (while still keeping the code to cope with the old backup format.) --- backup/util/dbops/restore_dbops.class.php | 7 ++--- ...store_questions_parser_processor.class.php | 28 ++++++++++++++----- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/backup/util/dbops/restore_dbops.class.php b/backup/util/dbops/restore_dbops.class.php index cb3ff984c03..42f3d18511a 100644 --- a/backup/util/dbops/restore_dbops.class.php +++ b/backup/util/dbops/restore_dbops.class.php @@ -671,8 +671,7 @@ abstract class restore_dbops { $questions = self::restore_get_questions($restoreid, $category->id); // Collect all the questions for this category into memory so we only talk to the DB once. - $questioncache = $DB->get_records_sql_menu('SELECT q.id, - q.stamp + $questioncache = $DB->get_records_sql_menu('SELECT q.stamp, q.id FROM {question} q JOIN {question_versions} qv ON qv.questionid = q.id @@ -683,8 +682,8 @@ abstract class restore_dbops { WHERE qc.id = ?', array($matchcat->id)); foreach ($questions as $question) { - if (isset($questioncache[$question->stamp." ".$question->version])) { - $matchqid = $questioncache[$question->stamp." ".$question->version]; + if (isset($questioncache[$question->stamp])) { + $matchqid = $questioncache[$question->stamp]; } else { $matchqid = false; } diff --git a/backup/util/helper/restore_questions_parser_processor.class.php b/backup/util/helper/restore_questions_parser_processor.class.php index 129b2f40ab6..be3d0c28108 100644 --- a/backup/util/helper/restore_questions_parser_processor.class.php +++ b/backup/util/helper/restore_questions_parser_processor.class.php @@ -35,22 +35,35 @@ require_once($CFG->dirroot.'/backup/util/xml/parser/processors/grouped_parser_pr * TODO: Complete phpdocs */ class restore_questions_parser_processor extends grouped_parser_processor { + /** @var string XML path in the questions.xml backup file to question categories. */ + protected const CATEGORY_PATH = '/question_categories/question_category'; - protected $restoreid; - protected $lastcatid; + /** @var string XML path in the questions.xml to question elements within question_category (Moodle 4.0+). */ + protected const QUESTION_SUBPATH = + '/question_bank_entries/question_bank_entry/question_version/question_versions/questions/question'; + + /** @var string XML path in the questions.xml to question elements within question_category (before Moodle 4.0). */ + protected const LEGACY_QUESTION_SUBPATH = '/questions/question'; + + /** @var string identifies the current restore. */ + protected string $restoreid; + + /** @var int during the restore, this tracks the last category we saw. Any questions we see will be in here. */ + protected int $lastcatid; public function __construct($restoreid) { $this->restoreid = $restoreid; $this->lastcatid = 0; - parent::__construct(array()); + parent::__construct(); // Set the paths we are interested on - $this->add_path('/question_categories/question_category'); - $this->add_path('/question_categories/question_category/questions/question'); + $this->add_path(self::CATEGORY_PATH); + $this->add_path(self::CATEGORY_PATH . self::QUESTION_SUBPATH); + $this->add_path(self::CATEGORY_PATH . self::LEGACY_QUESTION_SUBPATH); } protected function dispatch_chunk($data) { // Prepare question_category record - if ($data['path'] == '/question_categories/question_category') { + if ($data['path'] == self::CATEGORY_PATH) { $info = (object)$data['tags']; $itemname = 'question_category'; $itemid = $info->id; @@ -58,7 +71,8 @@ class restore_questions_parser_processor extends grouped_parser_processor { $this->lastcatid = $itemid; // Prepare question record - } else if ($data['path'] == '/question_categories/question_category/questions/question') { + } else if ($data['path'] == self::CATEGORY_PATH . self::QUESTION_SUBPATH || + $data['path'] == self::CATEGORY_PATH . self::LEGACY_QUESTION_SUBPATH) { $info = (object)$data['tags']; $itemname = 'question'; $itemid = $info->id; From c248dee33cf6f780ee8484f82f312a64a11bc352 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 4 Sep 2024 13:05:53 +0200 Subject: [PATCH 2/3] MDL-77625 question restore: fix restore of existing questions Continuing from the last commit, the code to restore question bank entries, versions and questions, was not taking note of whether the first stage had indentified questions that did not need to be restored. This is tricky, but the mapping can only be worked out for questions, but in the backup file, the questions are inside the question_bank_entries, and versions, so we encounter those first. Now, the code just saves the QBE and QV when it encounters them, and then does all the processing when it gets to the question, correctly taking note of whether each question should be restored or not. In cases where a particular question does not need to be restored, we still set up the corresponding mappings. --- backup/moodle2/restore_stepslib.php | 189 +++++++++++++--------------- 1 file changed, 90 insertions(+), 99 deletions(-) diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 1856dededda..04a264e2b92 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -5006,6 +5006,12 @@ class restore_create_categories_and_questions extends restore_structure_step { /** @var array $cachedcategory store a question category */ protected $cachedcategory = null; + /** @var stdClass the last question_bank_entry seen during the restore. Processed when we get to a question. */ + protected $latestqbe; + + /** @var stdClass the last question_version seen during the restore. Processed when we get to a question. */ + protected $latestversion; + protected function define_structure() { // Check if the backup is a pre 4.0 one. @@ -5131,45 +5137,25 @@ class restore_create_categories_and_questions extends restore_structure_step { } /** - * Process pre 4.0 question data where in creates the record for version and entry table. + * Set up date to allow restore of questions from pre-4.0 backups. * - * @param array $data the data from the XML file. + * @param stdClass $data the data from the XML file. */ protected function process_question_legacy_data($data) { - global $DB; + $this->latestqbe = (object) [ + 'id' => $data->id, + 'questioncategoryid' => $data->category, + 'ownerid' => $data->createdby, + 'idnumber' => $data->idnumber ?? null, + ]; - $oldid = $data->id; - // Process question bank entry. - $entrydata = new stdClass(); - $entrydata->questioncategoryid = $data->category; - $userid = $this->get_mappingid('user', $data->createdby); - if ($userid) { - $entrydata->ownerid = $userid; - } else { - if (!$this->task->is_samesite()) { - $entrydata->ownerid = $this->task->get_userid(); - } - } - // The idnumber if it exists also needs to be unique within a category or reset it to null. - if (isset($data->idnumber) && !$DB->record_exists('question_bank_entries', - ['idnumber' => $data->idnumber, 'questioncategoryid' => $data->category])) { - $entrydata->idnumber = $data->idnumber; - } - - $newentryid = $DB->insert_record('question_bank_entries', $entrydata); - // Process question versions. - $versiondata = new stdClass(); - $versiondata->questionbankentryid = $newentryid; - $versiondata->version = 1; - // Question id is updated after inserting the question. - $versiondata->questionid = 0; - $versionstatus = \core_question\local\bank\question_version_status::QUESTION_STATUS_READY; - if ((int)$data->hidden === 1) { - $versionstatus = \core_question\local\bank\question_version_status::QUESTION_STATUS_HIDDEN; - } - $versiondata->status = $versionstatus; - $newversionid = $DB->insert_record('question_versions', $versiondata); - $this->set_mapping('question_version_created', $oldid, $newversionid); + $this->latestversion = (object) [ + 'id' => $data->id, + 'version' => 1, + 'status' => $data->hidden ? + \core_question\local\bank\question_version_status::QUESTION_STATUS_HIDDEN : + \core_question\local\bank\question_version_status::QUESTION_STATUS_READY, + ]; } /** @@ -5178,37 +5164,9 @@ class restore_create_categories_and_questions extends restore_structure_step { * @param array $data the data from the XML file. */ protected function process_question_bank_entry($data) { - global $DB; - - $data = (object)$data; - $oldid = $data->id; - - $questioncreated = $this->get_mappingid('question_category_created', $data->questioncategoryid) ? true : false; - $recordexist = $DB->record_exists('question_bank_entries', ['id' => $data->id, - 'questioncategoryid' => $data->questioncategoryid]); - // Check we have category created. - if (!$questioncreated && $recordexist) { - return self::SKIP_ALL_CHILDREN; - } - - $data->questioncategoryid = $this->get_new_parentid('question_category'); - $userid = $this->get_mappingid('user', $data->ownerid); - if ($userid) { - $data->ownerid = $userid; - } else { - if (!$this->task->is_samesite()) { - $data->ownerid = $this->task->get_userid(); - } - } - - // The idnumber if it exists also needs to be unique within a category or reset it to null. - if (!empty($data->idnumber) && $DB->record_exists('question_bank_entries', - ['idnumber' => $data->idnumber, 'questioncategoryid' => $data->questioncategoryid])) { - unset($data->idnumber); - } - - $newitemid = $DB->insert_record('question_bank_entries', $data); - $this->set_mapping('question_bank_entry', $oldid, $newitemid); + // We can only determine the right way to process this once we get to + // process_question and have more information, so for now just store. + $this->latestqbe = (object) $data; } /** @@ -5217,16 +5175,9 @@ class restore_create_categories_and_questions extends restore_structure_step { * @param array $data the data from the XML file. */ protected function process_question_versions($data) { - global $DB; - - $data = (object)$data; - $oldid = $data->id; - - $data->questionbankentryid = $this->get_new_parentid('question_bank_entry'); - // Question id is updated after inserting the question. - $data->questionid = 0; - $newitemid = $DB->insert_record('question_versions', $data); - $this->set_mapping('question_versions', $oldid, $newitemid); + // We can only determine the right way to process this once we get to + // process_question and have more information, so for now just store. + $this->latestversion = (object) $data; } /** @@ -5237,17 +5188,19 @@ class restore_create_categories_and_questions extends restore_structure_step { protected function process_question($data) { global $DB; - $data = (object)$data; + $data = (object) $data; $oldid = $data->id; - // Check if the backup is a pre 4.0 one. + // Check we have one mapping for this question. + if (!$questionmapping = $this->get_mapping('question', $oldid)) { + // No mapping = this question doesn't need to be created/mapped. + return; + } + + // Check if this is a pre 4.0 backup, then there will not be a question bank entry + // or question version in the file. So, we need to set up that data ready to be used below. $restoretask = $this->get_task(); if ($restoretask->backup_release_compare('4.0', '<') || $restoretask->backup_version_compare(20220202, '<')) { - // Check we have one mapping for this question. - if (!$questionmapping = $this->get_mapping('question', $oldid)) { - return; // No mapping = this question doesn't need to be created/mapped. - } - // Get the mapped category (cannot use get_new_parentid() because not // all the categories have been created, so it is not always available // Instead we get the mapping for the question->parentitemid because @@ -5291,28 +5244,66 @@ class restore_create_categories_and_questions extends restore_structure_step { } } - $newitemid = $DB->insert_record('question', $data); - $this->set_mapping('question', $oldid, $newitemid); - // Also annotate them as question_created, we need - // that later when remapping parents (keeping the old categoryid as parentid). - $parentcatid = $this->get_old_parentid('question_category'); - $this->set_mapping('question_created', $oldid, $newitemid, false, null, $parentcatid); - // Now update the question_versions table with the new question id. we dont need to do that for random qtypes. - $legacyquestiondata = $this->get_mappingid('question_version_created', $oldid) ? true : false; - if ($legacyquestiondata) { - $parentitemid = $this->get_mappingid('question_version_created', $oldid); + // With newitemid = 0, let's create the question. + if (!$questionmapping->newitemid) { + // Now we know we are inserting a question, we may need to insert the questionbankentry. + if (empty($this->latestqbe->newid)) { + $this->latestqbe->oldid = $this->latestqbe->id; + + $this->latestqbe->questioncategoryid = $this->get_new_parentid('question_category'); + $userid = $this->get_mappingid('user', $this->latestqbe->ownerid); + if ($userid) { + $this->latestqbe->ownerid = $userid; + } else { + if (!$this->task->is_samesite()) { + $this->latestqbe->ownerid = $this->task->get_userid(); + } + } + + // The idnumber if it exists also needs to be unique within a category or reset it to null. + if (!empty($this->latestqbe->idnumber) && $DB->record_exists('question_bank_entries', + ['idnumber' => $this->latestqbe->idnumber, 'questioncategoryid' => $this->latestqbe->questioncategoryid])) { + unset($this->latestqbe->idnumber); + } + + $this->latestqbe->newid = $DB->insert_record('question_bank_entries', $this->latestqbe); + $this->set_mapping('question_bank_entry', $this->latestqbe->oldid, $this->latestqbe->newid); + } + + // Now store the question. + $newitemid = $DB->insert_record('question', $data); + $this->set_mapping('question', $oldid, $newitemid); + // Also annotate them as question_created, we need + // that later when remapping parents (keeping the old categoryid as parentid). + $parentcatid = $this->get_old_parentid('question_category'); + $this->set_mapping('question_created', $oldid, $newitemid, false, null, $parentcatid); + + // Also insert this question_version. + $oldqvid = $this->latestversion->id; + $this->latestversion->questionbankentryid = $this->latestqbe->newid; + $this->latestversion->questionid = $newitemid; + $newqvid = $DB->insert_record('question_versions', $this->latestversion); + $this->set_mapping('question_versions', $oldqvid, $newqvid); + } else { - $parentitemid = $this->get_new_parentid('question_versions'); + // By performing this set_mapping() we make get_old/new_parentid() to work for all the + // children elements of the 'question' one (so qtype plugins will know the question they belong to). + $this->set_mapping('question', $oldid, $questionmapping->newitemid); + + // Also create the question_bank_entry and version mappings, if required. + $newquestionversion = $DB->get_record('question_versions', ['questionid' => $questionmapping->newitemid]); + $this->set_mapping('question_versions', $this->latestversion->id, $newquestionversion->id); + if (empty($this->latestqbe->newid)) { + $this->latestqbe->oldid = $this->latestqbe->id; + $this->latestqbe->newid = $newquestionversion->questionbankentryid; + $this->set_mapping('question_bank_entry', $this->latestqbe->oldid, $this->latestqbe->newid); + } } - $version = new stdClass(); - $version->id = $parentitemid; - $version->questionid = $newitemid; - $DB->update_record('question_versions', $version); // Note, we don't restore any question files yet // as far as the CONTEXT_MODULE categories still // haven't their contexts to be restored to - // The {@link restore_create_question_files}, executed in the final step + // The {@see restore_create_question_files}, executed in the final // step will be in charge of restoring all the question files. } From 5ec6096eeac87d4ecc08e8288c5af56e9902d7d7 Mon Sep 17 00:00:00 2001 From: Julien <88390341+joinbean@users.noreply.github.com> Date: Tue, 3 Sep 2024 14:40:23 +0200 Subject: [PATCH 3/3] MDL-77625 question restore: test restoring a quiz into a course twice The verifies that the previous two commits are doing the right thing. --- .../tests/backup/repeated_restore_test.php | 155 ++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 mod/quiz/tests/backup/repeated_restore_test.php diff --git a/mod/quiz/tests/backup/repeated_restore_test.php b/mod/quiz/tests/backup/repeated_restore_test.php new file mode 100644 index 00000000000..204f3f1c958 --- /dev/null +++ b/mod/quiz/tests/backup/repeated_restore_test.php @@ -0,0 +1,155 @@ +. + +namespace mod_quiz\backup; + +use advanced_testcase; +use backup_controller; +use restore_controller; +use quiz_question_helper_test_trait; +use backup; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); +require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); +require_once($CFG->dirroot . '/question/engine/lib.php'); +require_once($CFG->dirroot . '/mod/quiz/locallib.php'); +require_once($CFG->dirroot . '/course/lib.php'); +require_once($CFG->dirroot . '/mod/quiz/tests/quiz_question_helper_test_trait.php'); + +/** + * Test repeatedly restoring a quiz into another course. + * + * @package mod_quiz + * @category test + * @copyright Julien Rädler + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \restore_questions_parser_processor + * @covers \restore_create_categories_and_questions + */ +final class repeated_restore_test extends advanced_testcase { + use quiz_question_helper_test_trait; + + /** + * Restore a quiz twice into the same target course, and verify the quiz uses the restored questions both times. + */ + public function test_restore_quiz_into_other_course_twice(): void { + global $USER; + $this->resetAfterTest(); + $this->setAdminUser(); + + // Step 1: Create two courses and a user with editing teacher capabilities. + $generator = $this->getDataGenerator(); + $course1 = $generator->create_course(); + $course2 = $generator->create_course(); + $teacher = $USER; + $generator->enrol_user($teacher->id, $course1->id, 'editingteacher'); + $generator->enrol_user($teacher->id, $course2->id, 'editingteacher'); + + // Create a quiz with questions in the first course. + $quiz = $this->create_test_quiz($course1); + $coursecontext = \context_course::instance($course1->id); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + + // Create a question category. + $cat = $questiongenerator->create_question_category(['contextid' => $coursecontext->id]); + + // Create a short answer question. + $saq = $questiongenerator->create_question('shortanswer', null, ['category' => $cat->id]); + // Update the question to simulate editing. + $questiongenerator->update_question($saq); + // Add question to quiz. + quiz_add_quiz_question($saq->id, $quiz); + + // Create a numerical question. + $numq = $questiongenerator->create_question('numerical', null, ['category' => $cat->id]); + // Update the question to simulate multiple versions. + $questiongenerator->update_question($numq); + $questiongenerator->update_question($numq); + // Add question to quiz. + quiz_add_quiz_question($numq->id, $quiz); + + // Create a true false question. + $tfq = $questiongenerator->create_question('truefalse', null, ['category' => $cat->id]); + // Update the question to simulate multiple versions. + $questiongenerator->update_question($tfq); + $questiongenerator->update_question($tfq); + // Add question to quiz. + quiz_add_quiz_question($tfq->id, $quiz); + + // Capture original question IDs for verification after import. + $modules1 = get_fast_modinfo($course1->id)->get_instances_of('quiz'); + $module1 = reset($modules1); + $questionscourse1 = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $module1->instance, $module1->context); + + $originalquestionids = []; + foreach ($questionscourse1 as $slot) { + array_push($originalquestionids, intval($slot->questionid)); + } + + // Step 2: Backup the first course. + $bc = new backup_controller(backup::TYPE_1COURSE, $course1->id, backup::FORMAT_MOODLE, + backup::INTERACTIVE_NO, backup::MODE_IMPORT, $teacher->id); + $backupid = $bc->get_backupid(); + $bc->execute_plan(); + $bc->destroy(); + + // Step 3: Import the backup into the second course. + $rc = new restore_controller($backupid, $course2->id, backup::INTERACTIVE_NO, backup::MODE_IMPORT, + $teacher->id, backup::TARGET_CURRENT_ADDING); + $rc->execute_precheck(); + $rc->execute_plan(); + $rc->destroy(); + + // Verify the question ids from the quiz in the original course are different + // from the question ids in the duplicated quiz in the second course. + $modules2 = get_fast_modinfo($course2->id)->get_instances_of('quiz'); + $module2 = reset($modules2); + $questionscourse2firstimport = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $module2->instance, $module2->context); + + foreach ($questionscourse2firstimport as $slot) { + $this->assertNotContains(intval($slot->questionid), $originalquestionids, + "Question ID $slot->questionid should not be in the original course's question IDs."); + } + + // Repeat the backup and import process to simulate a second import. + $bc = new backup_controller(backup::TYPE_1COURSE, $course1->id, backup::FORMAT_MOODLE, + backup::INTERACTIVE_NO, backup::MODE_IMPORT, $teacher->id); + $backupid = $bc->get_backupid(); + $bc->execute_plan(); + $bc->destroy(); + + $rc = new restore_controller($backupid, $course2->id, backup::INTERACTIVE_NO, backup::MODE_IMPORT, + $teacher->id, backup::TARGET_CURRENT_ADDING); + $rc->execute_precheck(); + $rc->execute_plan(); + $rc->destroy(); + + // Verify that the second restore has used the same new questions that were created by the first restore. + $modules3 = get_fast_modinfo($course2->id)->get_instances_of('quiz'); + $module3 = end($modules3); + $questionscourse2secondimport = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $module3->instance, $module3->context); + + foreach ($questionscourse2secondimport as $slot) { + $this->assertEquals($questionscourse2firstimport[$slot->slot]->questionid, $slot->questionid); + } + } +}