diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index fa31ce00447..f5db77c8a3e 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -4916,6 +4916,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. @@ -5041,45 +5047,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, + ]; } /** @@ -5088,37 +5074,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; } /** @@ -5127,16 +5085,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; } /** @@ -5147,17 +5098,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 @@ -5201,28 +5154,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. } diff --git a/backup/util/dbops/restore_dbops.class.php b/backup/util/dbops/restore_dbops.class.php index 0633fd8567c..d587e7748ac 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; 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); + } + } +}