From 541790a5d2e20363465e159b662f2401f946d515 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. --- backup/moodle2/restore_stepslib.php | 14 ++++---- lib/db/upgrade.php | 9 +++++ question/classes/category_manager.php | 27 +++++++++++++++ question/tests/backup_test.php | 2 ++ question/tests/category_manager_test.php | 43 ++++++++++++++++++++++++ version.php | 2 +- 6 files changed, 89 insertions(+), 8 deletions(-) diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 86994a1576e..c05c8b0f87d 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/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/lib/db/upgrade.php b/lib/db/upgrade.php index afcb215654e..224aa2becac 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2008,5 +2008,14 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2025041406.01); } + if ($oldversion < 2025041406.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, 2025041406.05); + } + return true; } diff --git a/question/classes/category_manager.php b/question/classes/category_manager.php index 10fe2368c69..ba2c118e458 100644 --- a/question/classes/category_manager.php +++ b/question/classes/category_manager.php @@ -16,6 +16,7 @@ namespace core_question; +use core\attribute\deprecated; use stdClass; use core\exception\moodle_exception; use core\context; @@ -403,4 +404,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/question/tests/backup_test.php b/question/tests/backup_test.php index 6266385346e..add46dc0651 100644 --- a/question/tests/backup_test.php +++ b/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/question/tests/category_manager_test.php b/question/tests/category_manager_test.php index a8354ccdb82..18870f0cefc 100644 --- a/question/tests/category_manager_test.php +++ b/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/version.php b/version.php index 730aa387c59..3b23f7a55a3 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2025041406.04; // 20250414 = branching date YYYYMMDD - do not modify! +$version = 2025041406.05; // 20250414 = branching date YYYYMMDD - do not modify! // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '5.0.6+ (Build: 20260227)'; // Human-friendly version name