From 329725c21eade34b20921cb0e45d950a3abe25bf Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Tue, 6 Jan 2026 16:08:07 +0000 Subject: [PATCH] MDL-86154 questions: Delete unused questions after restore After restoring an activity using shared questions without the question bank they belong to, we may have left over question categories in the target course context. These were being deleted, but any questions they contained weren't. This replaces the bulk delete of question category records with a call to `question_category_delete_safe()` for each category, which will also delete the questions they contain. --- public/backup/moodle2/restore_stepslib.php | 14 +++--- public/lib/db/upgrade.php | 9 ++++ public/question/classes/category_manager.php | 26 +++++++++++ public/question/tests/backup_test.php | 2 + .../question/tests/category_manager_test.php | 43 +++++++++++++++++++ public/version.php | 2 +- 6 files changed, 88 insertions(+), 8 deletions(-) diff --git a/public/backup/moodle2/restore_stepslib.php b/public/backup/moodle2/restore_stepslib.php index 86994a1576e..c05c8b0f87d 100644 --- a/public/backup/moodle2/restore_stepslib.php +++ b/public/backup/moodle2/restore_stepslib.php @@ -5715,20 +5715,20 @@ class restore_move_module_questions_categories extends restore_execution_step { ); } } - // Remove any remaining course-level question categories from the restored course. + // Remove any remaining course-level question categories and their questions from the restored course. $coursecatsql = " - SELECT qc.id AS categoryid + SELECT qc.id AS id, qc.contextid AS contextid FROM {question_categories} qc JOIN {context} c ON c.id = qc.contextid WHERE c.contextlevel = :courselevel AND c.instanceid = :courseid "; - $DB->delete_records_subquery( - 'question_categories', - 'id', - 'categoryid', + $categories = $DB->get_records_sql( $coursecatsql, - ['courselevel' => context_course::LEVEL, 'courseid' => $this->task->get_courseid()] + ['courselevel' => context_course::LEVEL, 'courseid' => $this->task->get_courseid()], ); + foreach ($categories as $category) { + question_category_delete_safe($category); + } } } diff --git a/public/lib/db/upgrade.php b/public/lib/db/upgrade.php index 854d960d8d9..de54a015290 100644 --- a/public/lib/db/upgrade.php +++ b/public/lib/db/upgrade.php @@ -2453,5 +2453,14 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2025100603.01); } + if ($oldversion < 2025100603.05) { + $orphanedquestions = core_question\category_manager::cleanup_questions_without_categories(); + if ($orphanedquestions > 0) { + upgrade_log(UPGRADE_LOG_NORMAL, null, "Cleaned up {$orphanedquestions} questions left over from restores."); + } + + upgrade_main_savepoint(true, 2025100603.05); + } + return true; } diff --git a/public/question/classes/category_manager.php b/public/question/classes/category_manager.php index 10fe2368c69..9fba85e21f7 100644 --- a/public/question/classes/category_manager.php +++ b/public/question/classes/category_manager.php @@ -403,4 +403,30 @@ class category_manager { $DB->update_record('question_categories', $categorytofix, true); } } + + /** + * Upgrade step to find questions with no category and delete them. + * + * Due to MDL-86154, there may be questions left in the database after a restore, whose category has been deleted. This will + * find any questions like that and delete them. These questions will always be unused. + * + * Now that we have prevented this occurring, this function is used by the upgrade process to clean up these questions. + * + * @return int A count of deleted questions. + */ + public static function cleanup_questions_without_categories(): int { + global $DB; + $questionids = $DB->get_fieldset_sql(" + SELECT q.id + FROM {question_bank_entries} qbe + JOIN {question_versions} qv ON qv.questionbankentryid = qbe.id + JOIN {question} q ON qv.questionid = q.id + LEFT JOIN {question_categories} qc ON qbe.questioncategoryid = qc.id + WHERE qc.id IS NULL + "); + foreach ($questionids as $questionid) { + question_delete_question($questionid); + } + return count($questionids); + } } diff --git a/public/question/tests/backup_test.php b/public/question/tests/backup_test.php index 6266385346e..add46dc0651 100644 --- a/public/question/tests/backup_test.php +++ b/public/question/tests/backup_test.php @@ -716,6 +716,8 @@ final class backup_test extends \advanced_testcase { // Check we have the expected restored categories. $this->assertEquals(2, $DB->count_records('question_categories', ['stamp' => $data->quizcategory->stamp])); $this->assertEquals(1, $DB->count_records('question_categories', ['stamp' => $data->qbankcategory->stamp])); + // Check there is no additional copy of the referenced question bank question. + $this->assertEquals(1, $DB->count_records('question', ['name' => $data->qbankquestion->name])); } /** diff --git a/public/question/tests/category_manager_test.php b/public/question/tests/category_manager_test.php index a8354ccdb82..18870f0cefc 100644 --- a/public/question/tests/category_manager_test.php +++ b/public/question/tests/category_manager_test.php @@ -664,4 +664,47 @@ final class category_manager_test extends \advanced_testcase { $this->assertEquals($quiz2top->id, $DB->get_field('question_categories', 'parent', ['id' => $quiz2nontop->id])); $this->assertEquals($qbank2top->id, $DB->get_field('question_categories', 'parent', ['id' => $qbank2nontop->id])); } + + /** + * A question with no category should be deleted, while other questions remain as-is. + */ + public function test_cleanup_questions_without_categories(): void { + global $DB; + $this->setAdminUser(); + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + $quiz = $this->getDataGenerator()->create_module('quiz', ['course' => $course->id]); + $context = \context_module::instance($quiz->cmid); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $topcategory = question_get_top_category($context->id, true); + $defaultcategory = question_get_default_category($context->id); + $deletedcategory = $questiongenerator->create_question_category( + ['contextid' => $context->id, 'parent' => $topcategory->id], + ); + // Create 2 questions. One in the default category, and in the category being deleted. + $question = $questiongenerator->create_question('truefalse', overrides: ['category' => $defaultcategory->id]); + $orphan = $questiongenerator->create_question('truefalse', overrides: ['category' => $deletedcategory->id]); + + $DB->delete_records('question_categories', ['id' => $deletedcategory->id]); + + $this->assertEquals(1, category_manager::cleanup_questions_without_categories()); + + // The default category question is unchanged. + $this->assertTrue( + $DB->record_exists_sql( + "SELECT * + FROM {question} q + JOIN {question_versions} qv on qv.questionid = q.id + JOIN {question_bank_entries} qbe ON qv.questionbankentryid = qbe.id + WHERE q.id = :questionid AND qbe.questioncategoryid = :categoryid", + [ + 'questionid' => $question->id, + 'categoryid' => $defaultcategory->id, + ], + ), + ); + // The orphaned question has been deleted. + $this->assertFalse($DB->record_exists('question', ['id' => $orphan->id])); + } } diff --git a/public/version.php b/public/version.php index ecab60599a7..db24808cf3b 100644 --- a/public/version.php +++ b/public/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2025100603.04; // 20251006 = branching date YYYYMMDD - do not modify! +$version = 2025100603.05; // 20251006 = branching date YYYYMMDD - do not modify! // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '5.1.3+ (Build: 20260227)'; // Human-friendly version name