From da518313d64efe5d3885d6fda1fa78d08de6220d Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Fri, 24 Oct 2025 09:15:53 +0100 Subject: [PATCH] MDL-87004 backup: Ensure backups include all random questions Backing up quizzes containing random questions from multiple different categories was only including one of the categories in the backup. This was due to doing $array + $array instead of array_merge(), meaning items in the second array with keys that were already present in the first did not get added to the result. --- backup/moodle2/backup_stepslib.php | 5 +- .../backup/backup_question_selection_test.php | 70 +++++++++++++------ 2 files changed, 53 insertions(+), 22 deletions(-) diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php index ebce6719a52..000ef84af8b 100644 --- a/backup/moodle2/backup_stepslib.php +++ b/backup/moodle2/backup_stepslib.php @@ -310,7 +310,10 @@ trait backup_question_set_reference_trait { foreach ($setreferenceconditions as $setreferencecondition) { $conditions = json_decode($setreferencecondition, true); $conditions = question_reference_manager::convert_legacy_set_reference_filter_condition($conditions); - $setreferencequestionids += array_keys($randomloader->get_filtered_questions($conditions['filter'], 0)); + $setreferencequestionids = array_merge( + $setreferencequestionids, + array_keys($randomloader->get_filtered_questions($conditions['filter'], 0)), + ); } if (empty($setreferencequestionids)) { return; diff --git a/mod/quiz/tests/backup/backup_question_selection_test.php b/mod/quiz/tests/backup/backup_question_selection_test.php index 3de162fa529..a333bd703e0 100644 --- a/mod/quiz/tests/backup/backup_question_selection_test.php +++ b/mod/quiz/tests/backup/backup_question_selection_test.php @@ -90,11 +90,16 @@ final class backup_question_selection_test extends \advanced_testcase { 'sharedq4' => 'shortanswer', ], ], - 'tagcat' => [ + 'tagcat1' => [ 'tagq1' => 'shortanswer', 'tagq2' => 'shortanswer', 'tagq3' => 'shortanswer', ], + 'tagcat2' => [ + 'tagq4' => 'shortanswer', + 'tagq5' => 'shortanswer', + 'tagq6' => 'shortanswer', + ], ] ); $quiz = $this->create_test_quiz($course); @@ -113,10 +118,11 @@ final class backup_question_selection_test extends \advanced_testcase { ] ); - $questiongenerator->create_question_tag(['questionid' => $sharedquestions['tagcat']['tagq1']->id, 'tag' => 'mytag']); - $questiongenerator->create_question_tag(['questionid' => $sharedquestions['tagcat']['tagq2']->id, 'tag' => 'mytag']); + $questiongenerator->create_question_tag(['questionid' => $sharedquestions['tagcat1']['tagq1']->id, 'tag' => 'mytag']); + $questiongenerator->create_question_tag(['questionid' => $sharedquestions['tagcat1']['tagq2']->id, 'tag' => 'mytag']); + $questiongenerator->create_question_tag(['questionid' => $sharedquestions['tagcat2']['tagq5']->id, 'tag' => 'mytag']); - $tags = \core_tag_tag::get_item_tags('core_question', 'question', $sharedquestions['tagcat']['tagq1']->id); + $tags = \core_tag_tag::get_item_tags('core_question', 'question', $sharedquestions['tagcat1']['tagq1']->id); $mytag = reset($tags); // Add a question from the shared bank child category. @@ -125,14 +131,27 @@ final class backup_question_selection_test extends \advanced_testcase { quiz_add_quiz_question($coursequestions['courseparentcat']['courseq2']->id, $quiz); // Add a question from the quiz bank categories. quiz_add_quiz_question($quizquestions['quizparentcat']['quizq1']->id, $quiz); - // Add a random question to select tagged questions. + // Add random question to select tagged questions from 2 different categories. $settings = quiz_settings::create($quiz->id); $structure = structure::create_for_quiz($settings); $structure->add_random_questions(1, 1, [ 'filter' => [ 'category' => [ 'jointype' => \core\output\datafilter::JOINTYPE_ANY, - 'values' => [$sharedquestions['tagcat']['tagq1']->category], + 'values' => [$sharedquestions['tagcat1']['tagq1']->category], + 'filteroptions' => ['includesubcategories' => false], + ], + 'qtagids' => [ + 'jointype' => \core\output\datafilter::JOINTYPE_ANY, + 'values' => [$mytag->id], + ], + ], + ]); + $structure->add_random_questions(1, 1, [ + 'filter' => [ + 'category' => [ + 'jointype' => \core\output\datafilter::JOINTYPE_ANY, + 'values' => [$sharedquestions['tagcat2']['tagq4']->category], 'filteroptions' => ['includesubcategories' => false], ], 'qtagids' => [ @@ -194,9 +213,10 @@ final class backup_question_selection_test extends \advanced_testcase { $this->assertContains((string) $quizquestions['quizparentcat']['quizq2']->id, $backupquestions); $this->assertContains((string) $quizquestions['quizparentcat']['quizchildcat']['quizq3']->id, $backupquestions); $this->assertContains((string) $quizquestions['quizparentcat']['quizchildcat']['quizq4']->id, $backupquestions); - // Backup should contain questions matched by random question filter. - $this->assertContains((string) $sharedquestions['tagcat']['tagq1']->id, $backupquestions); - $this->assertContains((string) $sharedquestions['tagcat']['tagq2']->id, $backupquestions); + // Backup should contain questions matched by random question filters. + $this->assertContains((string) $sharedquestions['tagcat1']['tagq1']->id, $backupquestions); + $this->assertContains((string) $sharedquestions['tagcat1']['tagq2']->id, $backupquestions); + $this->assertContains((string) $sharedquestions['tagcat2']['tagq5']->id, $backupquestions); // All other questions should be excluded. $this->assertNotContains((string) $sharedquestions['sharedparentcat']['sharedq1']->id, $backupquestions); $this->assertNotContains((string) $sharedquestions['sharedparentcat']['sharedq2']->id, $backupquestions); @@ -204,8 +224,10 @@ final class backup_question_selection_test extends \advanced_testcase { $this->assertNotContains((string) $coursequestions['courseparentcat']['courseq1']->id, $backupquestions); $this->assertNotContains((string) $coursequestions['courseparentcat']['coursechildcat']['courseq3']->id, $backupquestions); $this->assertNotContains((string) $coursequestions['courseparentcat']['coursechildcat']['courseq4']->id, $backupquestions); - $this->assertNotContains((string) $sharedquestions['tagcat']['tagq3']->id, $backupquestions); - $this->assertCount(8, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat1']['tagq3']->id, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat2']['tagq4']->id, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat2']['tagq6']->id, $backupquestions); + $this->assertCount(9, $backupquestions); // Clean up. $rc->execute_plan(); $rc->destroy(); @@ -271,9 +293,10 @@ final class backup_question_selection_test extends \advanced_testcase { $this->assertContains((string) $quizquestions['quizparentcat']['quizq2']->id, $backupquestions); $this->assertContains((string) $quizquestions['quizparentcat']['quizchildcat']['quizq3']->id, $backupquestions); $this->assertContains((string) $quizquestions['quizparentcat']['quizchildcat']['quizq4']->id, $backupquestions); - // Backup should contain questions matched by random question filter. - $this->assertContains((string) $sharedquestions['tagcat']['tagq1']->id, $backupquestions); - $this->assertContains((string) $sharedquestions['tagcat']['tagq2']->id, $backupquestions); + // Backup should contain questions matched by random question filters. + $this->assertContains((string) $sharedquestions['tagcat1']['tagq1']->id, $backupquestions); + $this->assertContains((string) $sharedquestions['tagcat1']['tagq2']->id, $backupquestions); + $this->assertContains((string) $sharedquestions['tagcat2']['tagq5']->id, $backupquestions); // All other questions should be excluded. $this->assertNotContains((string) $sharedquestions['sharedparentcat']['sharedq1']->id, $backupquestions); $this->assertNotContains((string) $sharedquestions['sharedparentcat']['sharedq2']->id, $backupquestions); @@ -281,8 +304,10 @@ final class backup_question_selection_test extends \advanced_testcase { $this->assertNotContains((string) $coursequestions['courseparentcat']['courseq1']->id, $backupquestions); $this->assertNotContains((string) $coursequestions['courseparentcat']['coursechildcat']['courseq3']->id, $backupquestions); $this->assertNotContains((string) $coursequestions['courseparentcat']['coursechildcat']['courseq4']->id, $backupquestions); - $this->assertNotContains((string) $sharedquestions['tagcat']['tagq3']->id, $backupquestions); - $this->assertCount(8, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat1']['tagq3']->id, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat2']['tagq4']->id, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat2']['tagq6']->id, $backupquestions); + $this->assertCount(9, $backupquestions); // Clean up. $rc->execute_plan(); $rc->destroy(); @@ -338,15 +363,18 @@ final class backup_question_selection_test extends \advanced_testcase { $this->assertContains((string) $quizquestions['quizparentcat']['quizq2']->id, $backupquestions); $this->assertContains((string) $quizquestions['quizparentcat']['quizchildcat']['quizq3']->id, $backupquestions); $this->assertContains((string) $quizquestions['quizparentcat']['quizchildcat']['quizq4']->id, $backupquestions); - // Backup should contain questions matched by random question filter. - $this->assertContains((string) $sharedquestions['tagcat']['tagq1']->id, $backupquestions); - $this->assertContains((string) $sharedquestions['tagcat']['tagq2']->id, $backupquestions); + // Backup should contain questions matched by random question filters. + $this->assertContains((string) $sharedquestions['tagcat1']['tagq1']->id, $backupquestions); + $this->assertContains((string) $sharedquestions['tagcat1']['tagq2']->id, $backupquestions); + $this->assertContains((string) $sharedquestions['tagcat2']['tagq5']->id, $backupquestions); // All other questions should be excluded. $this->assertNotContains((string) $sharedquestions['sharedparentcat']['sharedq1']->id, $backupquestions); $this->assertNotContains((string) $sharedquestions['sharedparentcat']['sharedq2']->id, $backupquestions); $this->assertNotContains((string) $sharedquestions['sharedparentcat']['sharedchildcat']['sharedq4']->id, $backupquestions); - $this->assertNotContains((string) $sharedquestions['tagcat']['tagq3']->id, $backupquestions); - $this->assertCount(11, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat1']['tagq3']->id, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat2']['tagq4']->id, $backupquestions); + $this->assertNotContains((string) $sharedquestions['tagcat2']['tagq6']->id, $backupquestions); + $this->assertCount(12, $backupquestions); // Clean up. $rc->execute_plan(); $rc->destroy();