From 643eaa3418bf6f0dcf2e78b5829806cd1e517a39 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Wed, 21 May 2025 16:29:50 +0100 Subject: [PATCH 1/2] MDL-85556 questions: Fix repeated_restore unit test This unit test is meant to test restoring duplicated questions that exist in a shared question bank. However, due to improvements in the restore process from the introduction of mod_qbank, having the questions in a bank that is not in the restored course means the questions don't get restored at all, and we can't test that the process is working. This changes the test to use a bank in the target course instead. --- .upgradenotes/MDL-85556-2025052709221269.yml | 8 ++++++ .../tests/backup/repeated_restore_test.php | 27 ++++++++++++------- 2 files changed, 25 insertions(+), 10 deletions(-) create mode 100644 .upgradenotes/MDL-85556-2025052709221269.yml diff --git a/.upgradenotes/MDL-85556-2025052709221269.yml b/.upgradenotes/MDL-85556-2025052709221269.yml new file mode 100644 index 00000000000..c2a0a321a41 --- /dev/null +++ b/.upgradenotes/MDL-85556-2025052709221269.yml @@ -0,0 +1,8 @@ +issueNumber: MDL-85556 +notes: + core_question: + - message: > + The unit test repeated\_restore\_test::test\_restore\_course\_with\_same\_stamp\_questions was passing incorrectly on 5.x for question types that use answers. + + Maintainers of third-party question types may want to re-run the test with the fix in place, or if they have copied parts of this test as the basis of a test in their own plugin, review the changes and see if they should be reflected in their own test. + type: fixed diff --git a/mod/quiz/tests/backup/repeated_restore_test.php b/mod/quiz/tests/backup/repeated_restore_test.php index 1ad84dd0507..8a65551d4ae 100644 --- a/mod/quiz/tests/backup/repeated_restore_test.php +++ b/mod/quiz/tests/backup/repeated_restore_test.php @@ -516,16 +516,14 @@ final class repeated_restore_test extends advanced_testcase { $this->resetAfterTest(); $this->setAdminUser(); - // Create three courses and a user with editing teacher capabilities. + // Create two courses and a user with editing teacher capabilities. $generator = $this->getDataGenerator(); $course1 = $generator->create_course(); $course2 = $generator->create_course(); - $course3 = $generator->create_course(); - $qbank = $generator->get_plugin_generator('mod_qbank')->create_instance(['course' => $course3->id]); + $qbank = $generator->get_plugin_generator('mod_qbank')->create_instance(['course' => $course2->id]); $teacher = $USER; $generator->enrol_user($teacher->id, $course1->id, 'editingteacher'); $generator->enrol_user($teacher->id, $course2->id, 'editingteacher'); - $generator->enrol_user($teacher->id, $course3->id, 'editingteacher'); $context = \context_module::instance($qbank->cmid); $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); @@ -557,6 +555,13 @@ final class repeated_restore_test extends advanced_testcase { $DB->update_record('question_answers', $answer); } + $course1q1structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $quiz1->id, \context_module::instance($quiz1->cmid)); + $this->assertEquals($question1->id, $course1q1structure[1]->questionid); + $course1q2structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $quiz2->id, \context_module::instance($quiz2->cmid)); + $this->assertEquals($question2->id, $course1q2structure[1]->questionid); + // Backup course1. $bc = new backup_controller(backup::TYPE_1COURSE, $course1->id, backup::FORMAT_MOODLE, backup::INTERACTIVE_NO, backup::MODE_IMPORT, $teacher->id); @@ -573,19 +578,21 @@ final class repeated_restore_test extends advanced_testcase { // Verify that the newly-restored course's quizzes use the same questions as their counterparts of course1. $modules = get_fast_modinfo($course2->id)->get_instances_of('quiz'); - $course1structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $course1q1structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( $quiz1->id, \context_module::instance($quiz1->cmid)); $course2quiz1 = array_shift($modules); - $course2structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $course2q1structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( $course2quiz1->instance, $course2quiz1->context); - $this->assertEquals($course1structure[1]->questionid, $course2structure[1]->questionid); + $this->assertEquals($question1->id, $course1q1structure[1]->questionid); + $this->assertEquals($question1->id, $course2q1structure[1]->questionid); - $course1structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $course1q2structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( $quiz2->id, \context_module::instance($quiz2->cmid)); $course2quiz2 = array_shift($modules); - $course2structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $course2q2structure = \mod_quiz\question\bank\qbank_helper::get_question_structure( $course2quiz2->instance, $course2quiz2->context); - $this->assertEquals($course1structure[1]->questionid, $course2structure[1]->questionid); + $this->assertEquals($question2->id, $course1q2structure[1]->questionid); + $this->assertEquals($question2->id, $course2q2structure[1]->questionid); } /** From dfd0dc28e8c7c37b5ad5ddb2ea4ef56aa296bf61 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Wed, 21 May 2025 16:31:48 +0100 Subject: [PATCH 2/2] MDL-85556 backup: Only update question refs in restored course The previous query updated all question references matching the original question bank entry, regardless of whether they were part of the restore or not. When we are dealing with duplicated questions, this means we could accidentally change question references in the backed-up course as well as the restored one. This change filters the references to ensure they exist in a context within the restored course, before updating them. --- backup/moodle2/restore_stepslib.php | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 83e7ea8fcd1..3a65aaf2724 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -5487,18 +5487,39 @@ class restore_move_module_questions_categories extends restore_execution_step { $originalcontext = context::instance_by_id($contextid, IGNORE_MISSING); if ($originalcontext && has_capability('mod/qbank:view', $originalcontext)) { $originalquestions = get_questions_category(question_get_top_category($contextid), false); + $targetcoursecontext = context_course::instance($this->get_courseid()); foreach ($originalquestions as $originalquestion) { $backupids = restore_dbops::get_backup_ids_record( $this->get_restoreid(), 'question', $originalquestion->id, ); + // Restored question references will point to the restored copy of the question. Select question references + // that point to that restored copy, only if they are within the target course's context, so we can update + // them to point to the original question. + $conpathlike = $DB->sql_like('con.path', '?'); + $references = $DB->get_records_sql( + "SELECT qr.id, qr.questionbankentryid + FROM {question_references} qr + JOIN {context} con ON qr.usingcontextid = con.id + JOIN {question_versions} qv ON qv.questionbankentryid = qr.questionbankentryid + WHERE qv.questionid = ? + AND {$conpathlike}", + [ + $backupids->newitemid, + $targetcoursecontext->path . '/%', + ], + ); + if (empty($references)) { + continue; + } + [$refin, $refparams] = $DB->get_in_or_equal(array_keys($references)); $DB->set_field_select( 'question_references', 'questionbankentryid', $DB->get_field('question_versions', 'questionbankentryid', ['questionid' => $backupids->itemid]), - 'questionbankentryid = (SELECT questionbankentryid FROM {question_versions} WHERE questionid = ?)', - [$backupids->newitemid], + 'id ' . $refin, + $refparams, ); } continue;