From 4bad37f98facf9bc59767db27b59ba974ea209bb 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 | 27 +++++++++++ public/question/tests/backup_test.php | 2 + .../question/tests/category_manager_test.php | 45 +++++++++++++++++++ public/version.php | 2 +- 6 files changed, 91 insertions(+), 8 deletions(-) diff --git a/public/backup/moodle2/restore_stepslib.php b/public/backup/moodle2/restore_stepslib.php index dde023e0303..49bb8ef8c7f 100644 --- a/public/backup/moodle2/restore_stepslib.php +++ b/public/backup/moodle2/restore_stepslib.php @@ -5723,20 +5723,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 1982286b31a..7b30caadcc5 100644 --- a/public/lib/db/upgrade.php +++ b/public/lib/db/upgrade.php @@ -1740,5 +1740,14 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2026021000.01); } + if ($oldversion < 2026022700.01) { + $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, 2026022700.01); + } + return true; } diff --git a/public/question/classes/category_manager.php b/public/question/classes/category_manager.php index 3dbb4696085..dadbcd14169 100644 --- a/public/question/classes/category_manager.php +++ b/public/question/classes/category_manager.php @@ -409,4 +409,31 @@ 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. + * @todo Deprecate in 6.0 MDL-87844 for Removal in 7.0 MDL-87845. + */ + 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 3fb830c9ae2..a1a3e19f478 100644 --- a/public/question/tests/category_manager_test.php +++ b/public/question/tests/category_manager_test.php @@ -666,4 +666,49 @@ 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. + * + * @todo Deprecate in 6.0 MDL-87844 for Removal in 7.0 MDL-87845. + */ + 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 827e40e7dcb..fa9caadc83c 100644 --- a/public/version.php +++ b/public/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2026022700.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2026022700.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '5.2dev (Build: 20260227)'; // Human-friendly version name