From 52db66247dcde013540fb3e4776f3af870c480f5 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Thu, 13 Nov 2025 09:12:49 +0000 Subject: [PATCH 1/3] MDL-86798 questions: Restore backups containing deleted versions If the latest version of a question is deleted, and then a new one created, the new one will currently re-use the version number of the deleted version. If the original version was backed up before it was deleted, then restored later, we end up with two questions in one question bank entry that have the same version. To resolve this, when restoring a question, we check if there is already a version with the same version number, then bump it and any higher versions up by 1. We also check for any references specifically using these versions and bump those up by 1. Any references using "always latest" will be fine, as the latest version will still be the latest. Additionally, if deleting a version made it hidden, and the backup contains the same version in "ready" state, restoring the backup will restore it to "ready" state. --- public/backup/moodle2/restore_stepslib.php | 60 ++++++++++- public/question/tests/backup_test.php | 113 +++++++++++++++++++++ 2 files changed, 172 insertions(+), 1 deletion(-) diff --git a/public/backup/moodle2/restore_stepslib.php b/public/backup/moodle2/restore_stepslib.php index e7270333b9c..aaf19c17e04 100644 --- a/public/backup/moodle2/restore_stepslib.php +++ b/public/backup/moodle2/restore_stepslib.php @@ -27,6 +27,8 @@ defined('MOODLE_INTERNAL') || die(); +use core_question\local\bank\question_version_status; + /** * delete old directories and conditionally create backup_temp_ids table */ @@ -5301,9 +5303,53 @@ class restore_create_categories_and_questions extends restore_structure_step { $oldqvid = $this->latestversion->id; $this->latestversion->questionbankentryid = $this->latestqbe->newid; $this->latestversion->questionid = $newitemid; + // In case the backed up version was deleted and a new one created in its place, increase the version numbers of + // conflicting versions to make room for this one. + $transaction = $DB->start_delegated_transaction(); + if ( + $DB->record_exists( + 'question_versions', + [ + 'questionbankentryid' => $this->latestversion->questionbankentryid, + 'version' => $this->latestversion->version, + ], + ) + ) { + // We'll update each higher version and any references one-at-a-time, starting with the highest, to avoid + // creating a duplicate questionbankentryid-version combination in question_versions. + $moveversions = $DB->get_records_select( + 'question_versions', + 'questionbankentryid = :questionbankentryid AND version >= :oldversion', + [ + 'questionbankentryid' => $this->latestversion->questionbankentryid, + 'oldversion' => $this->latestversion->version, + ], + 'version DESC', + ); + foreach ($moveversions as $moveversion) { + $DB->set_field( + 'question_versions', + 'version', + $moveversion->version + 1, + [ + 'questionbankentryid' => $moveversion->questionbankentryid, + 'version' => $moveversion->version, + ] + ); + $DB->set_field( + 'question_references', + 'version', + $moveversion->version + 1, + [ + 'questionbankentryid' => $moveversion->questionbankentryid, + 'version' => $moveversion->version, + ] + ); + } + } $newqvid = $DB->insert_record('question_versions', $this->latestversion); $this->set_mapping('question_versions', $oldqvid, $newqvid); - + $transaction->allow_commit(); } else { // By performing this set_mapping() we make get_old/new_parentid() to work for all the // children elements of the 'question' one (so qtype plugins will know the question they belong to). @@ -5311,6 +5357,18 @@ class restore_create_categories_and_questions extends restore_structure_step { // Also create the question_bank_entry and version mappings, if required. $newquestionversion = $DB->get_record('question_versions', ['questionid' => $questionmapping->newitemid]); + // Restore the version to ready state if it has been hidden. + if ( + $newquestionversion->status == question_version_status::QUESTION_STATUS_HIDDEN + && $this->latestversion->status == question_version_status::QUESTION_STATUS_READY + ) { + $DB->set_field( + 'question_versions', + 'status', + question_version_status::QUESTION_STATUS_READY, + ['questionid' => $questionmapping->newitemid], + ); + } $this->set_mapping('question_versions', $this->latestversion->id, $newquestionversion->id); if (empty($this->latestqbe->newid)) { $this->latestqbe->oldid = $this->latestqbe->id; diff --git a/public/question/tests/backup_test.php b/public/question/tests/backup_test.php index 782bcc448eb..a4110ab38d4 100644 --- a/public/question/tests/backup_test.php +++ b/public/question/tests/backup_test.php @@ -25,6 +25,7 @@ defined('MOODLE_INTERNAL') || die(); use backup; use core_question\local\bank\question_bank_helper; +use core_question\local\bank\question_version_status; use restore_controller; use restore_dbops; @@ -863,4 +864,116 @@ final class backup_test extends \advanced_testcase { $quizcatq = reset($quizcatqs); $this->assertEquals($expectedidentifiers[$i], $quizcatq->name); } + + /** + * Restore a backup containing question versions that were deleted, after new versions were created in their place. + * + * The new versions and any references to them should have their version numbers bumped up, and the original versions + * restored to their original numbers. + */ + public function test_restore_backup_containing_deleted_versions(): void { + global $DB; + self::setAdminUser(); + $this->resetAfterTest(); + $questiongenerator = self::getDataGenerator()->get_plugin_generator('core_question'); + $testdata = $this->add_course_quiz_and_qbank(); + $questionv1 = $testdata->qbankquestion; + $questionv2 = $questiongenerator->update_question($questionv1, null, ['name' => 'Version 2']); + $questionv3 = $questiongenerator->update_question($questionv2, null, ['name' => 'Version 3']); + $questionv4 = $questiongenerator->update_question($questionv3, null, ['name' => 'Version 4']); + + $quizsettings = quiz_settings::create($testdata->quiz->id); + $structure1 = $quizsettings->get_structure(); + + // Set the usage of the question to specifically use version 2. + quiz_add_quiz_question($questionv2->id, $testdata->quiz); + $structure1->update_slot_version($structure1->get_slot_id_for_slot(1), 2); + + $backupid = $this->backup_course($testdata->course); + + question_delete_question($questionv4->id); // Actually deleted. + question_delete_question($questionv3->id); // Actually deleted. + question_delete_question($questionv2->id); // Hidden, it's being used explicitly. + + $questionv5 = $questiongenerator->update_question($questionv1, null, ['name' => 'Version 5']); + $DB->set_field('question_versions', 'version', 3, ['questionid' => $questionv5->id]); + $questionv6 = $questiongenerator->update_question($questionv5, null, ['name' => 'Version 6']); + $DB->set_field('question_versions', 'version', 4, ['questionid' => $questionv6->id]); + + // Add a quiz specifically using "version 5" (with version number 3). + $quiz2 = self::getDataGenerator()->create_module('quiz', ['course' => $testdata->course->id]); + $quiz2settings = quiz_settings::create($quiz2->id); + quiz_add_quiz_question($questionv5->id, $quiz2); + $structure2 = $quiz2settings->get_structure(); + $structure2->update_slot_version($structure2->get_last_slot()->id, 3); + + // Add another quiz using the "always latest" version of the question. + $quiz3 = self::getDataGenerator()->create_module('quiz', ['course' => $testdata->course->id]); + $quiz3settings = quiz_settings::create($quiz3->id); + quiz_add_quiz_question($questionv6->id, $quiz3); + + $qbe = get_question_bank_entry($questionv1->id); + $versions = $DB->get_records( + 'question_versions', + ['questionbankentryid' => $qbe->id], + fields: 'version, questionid, status', + ); + + $this->assertCount(4, $versions); + + $this->assertEquals($questionv1->id, $versions[1]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[1]->status); + $this->assertEquals($questionv2->id, $versions[2]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_HIDDEN, $versions[2]->status); + $this->assertEquals($questionv5->id, $versions[3]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[3]->status); + $this->assertEquals($questionv6->id, $versions[4]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[4]->status); + + $structure1 = $quizsettings->get_structure(); + $this->assertEquals($questionv2->id, $structure1->get_question_in_slot(1)->questionid); + + $structure2 = $quiz2settings->get_structure(); + $this->assertEquals($questionv5->id, $structure2->get_question_in_slot(1)->questionid); + + $structure3 = $quiz3settings->get_structure(); + $this->assertEquals($questionv6->id, $structure3->get_question_in_slot(1)->questionid); + + $this->restore_to_course($backupid, $testdata->course->id); + + $versions = $DB->get_records( + 'question_versions', + ['questionbankentryid' => $qbe->id], + fields: 'version, questionid, status', + ); + + $this->assertCount(6, $versions); + + $this->assertEquals($questionv1->id, $versions[1]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[1]->status); + // Version 2 is restored to "ready" status. + $this->assertEquals($questionv2->id, $versions[2]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[2]->status); + // Version 3 is a new copy of $questionv3 with a different ID but the same content. + $this->assertEquals($questionv3->name, $DB->get_field('question', 'name', ['id' => $versions[3]->questionid])); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[3]->status); + // Version 4 is a new copy of $questionv4 with a different ID but the same content. + $this->assertEquals($questionv4->name, $DB->get_field('question', 'name', ['id' => $versions[4]->questionid])); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[4]->status); + // Versions 5 and 6 have had their version numbers bumped up by 1. + $this->assertEquals($questionv5->id, $versions[5]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[5]->status); + $this->assertEquals($questionv6->id, $versions[6]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[6]->status); + + // Questions referencing specific versions still point to the same question with its new version number. + $structure1 = $quizsettings->get_structure(); + $this->assertEquals($questionv2->id, $structure1->get_question_in_slot(1)->questionid); + + $structure2 = $quiz2settings->get_structure(); + $this->assertEquals($questionv5->id, $structure2->get_question_in_slot(1)->questionid); + + $structure3 = $quiz3settings->get_structure(); + $this->assertEquals($questionv6->id, $structure3->get_question_in_slot(1)->questionid); + } } From c25d5fd0638c0b1a3f8af171450a2cfad08d279f Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Fri, 14 Nov 2025 11:08:17 +0000 Subject: [PATCH 2/3] MDL-86798 questions: Track next version for question bank entries This adds a new "nextversion" field to `question_bank_entries` to track which version number the next version of the question should have. This ensures that version numbers are not re-used if the latest version is deleted. To use this field, you must call `\core_question\versions::get_next_version()`. This will initialise the field to the correct value if it is currently null. If you create a new version using this value, you must then call `\core_question\versions::increment_next_version()` to increment the counter. --- .upgradenotes/MDL-86798-2025111411591681.yml | 17 +++ .upgradenotes/MDL-86798-2025111412005118.yml | 7 + public/backup/moodle2/backup_stepslib.php | 1 + public/backup/moodle2/restore_stepslib.php | 3 + public/lib/db/install.xml | 3 +- public/lib/db/upgrade.php | 12 ++ public/lib/questionlib.php | 21 +-- public/question/classes/versions.php | 76 ++++++++++ public/question/format.php | 1 + public/question/tests/backup_test.php | 149 +++++++++++++++++++ public/question/tests/versions_test.php | 118 +++++++++++++++ public/question/type/questiontypebase.php | 3 +- public/version.php | 2 +- 13 files changed, 396 insertions(+), 17 deletions(-) create mode 100644 .upgradenotes/MDL-86798-2025111411591681.yml create mode 100644 .upgradenotes/MDL-86798-2025111412005118.yml create mode 100644 public/question/classes/versions.php create mode 100644 public/question/tests/versions_test.php diff --git a/.upgradenotes/MDL-86798-2025111411591681.yml b/.upgradenotes/MDL-86798-2025111411591681.yml new file mode 100644 index 00000000000..2ceacced1d4 --- /dev/null +++ b/.upgradenotes/MDL-86798-2025111411591681.yml @@ -0,0 +1,17 @@ +issueNumber: MDL-86798 +notes: + core_question: + - message: > + In order to prevent re-use of question version numbers after a version + is deleted, the `nextversion` column was added to + `question_bank_entries`. This serves as a counter incremented each time + a version is created. + + Do not query this field directly. Instead use + `core_question\versions::get_next_version()` to read the value, which + will initialise it based on the existing versions if it is not set yet. + By default, it will increment the version number automatically, unless + you pass `increment: false`. Because of this, it is advisable to call + it inside a transaction, that is only committed after the version number + is used in a `question_versions` record. + type: fixed diff --git a/.upgradenotes/MDL-86798-2025111412005118.yml b/.upgradenotes/MDL-86798-2025111412005118.yml new file mode 100644 index 00000000000..c8be3242c3f --- /dev/null +++ b/.upgradenotes/MDL-86798-2025111412005118.yml @@ -0,0 +1,7 @@ +issueNumber: MDL-86798 +notes: + core_question: + - message: >- + `get_next_version()` from questionlib.php is now deprecated. Use + `\core_question\versions::get_next_version()` instead. + type: deprecated diff --git a/public/backup/moodle2/backup_stepslib.php b/public/backup/moodle2/backup_stepslib.php index ad9bd73f230..127be8dc555 100644 --- a/public/backup/moodle2/backup_stepslib.php +++ b/public/backup/moodle2/backup_stepslib.php @@ -2649,6 +2649,7 @@ class backup_questions_structure_step extends backup_structure_step { 'questioncategoryid', 'idnumber', 'ownerid', + 'nextversion', ]); $questionversions = new backup_nested_element('question_version'); diff --git a/public/backup/moodle2/restore_stepslib.php b/public/backup/moodle2/restore_stepslib.php index aaf19c17e04..e2e71493653 100644 --- a/public/backup/moodle2/restore_stepslib.php +++ b/public/backup/moodle2/restore_stepslib.php @@ -28,6 +28,7 @@ defined('MOODLE_INTERNAL') || die(); use core_question\local\bank\question_version_status; +use core_question\versions; /** * delete old directories and conditionally create backup_temp_ids table @@ -5346,6 +5347,8 @@ class restore_create_categories_and_questions extends restore_structure_step { ] ); } + // Ensure the nextversion value has been initialised, and increment it to account for the additional version. + versions::get_next_version($this->latestversion->questionbankentryid); } $newqvid = $DB->insert_record('question_versions', $this->latestversion); $this->set_mapping('question_versions', $oldqvid, $newqvid); diff --git a/public/lib/db/install.xml b/public/lib/db/install.xml index a972b8e3a06..f0690b8ef05 100644 --- a/public/lib/db/install.xml +++ b/public/lib/db/install.xml @@ -1,5 +1,5 @@ - @@ -1484,6 +1484,7 @@ + diff --git a/public/lib/db/upgrade.php b/public/lib/db/upgrade.php index 8562517c66c..f3a538f28a2 100644 --- a/public/lib/db/upgrade.php +++ b/public/lib/db/upgrade.php @@ -1618,5 +1618,17 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2025121200.01); } + if ($oldversion < 2025121900.01) { + // Define field nextversion to be added to question_bank_entries. + $table = new xmldb_table('question_bank_entries'); + $field = new xmldb_field('nextversion', XMLDB_TYPE_INTEGER, '10', null, null, null, null, 'ownerid'); + + // Conditionally launch add field nextversion. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + upgrade_main_savepoint(true, 2025121900.01); + } + return true; } diff --git a/public/lib/questionlib.php b/public/lib/questionlib.php index dbbb23d31ea..f3c258d94af 100644 --- a/public/lib/questionlib.php +++ b/public/lib/questionlib.php @@ -1935,21 +1935,14 @@ function get_question_version($questionid): array { * @return int next version number. * @throws dml_exception */ +#[\core\attribute\deprecated( + '\core_question\versions::get_next_version()', + '5.2', + 'The next version is now an incrementing number stored in the database, to prevent duplicate version numbers', + 'MDL-86798', +)] function get_next_version(int $questionbankentryid): int { - global $DB; - - $sql = "SELECT MAX(qv.version) - FROM {question_versions} qv - JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid - WHERE qbe.id = :id"; - - $nextversion = $DB->get_field_sql($sql, ['id' => $questionbankentryid]); - - if ($nextversion) { - return (int)$nextversion + 1; - } - - return 1; + return \core_question\versions::get_next_version($questionbankentryid); } /** diff --git a/public/question/classes/versions.php b/public/question/classes/versions.php new file mode 100644 index 00000000000..a1daf6dbfdf --- /dev/null +++ b/public/question/classes/versions.php @@ -0,0 +1,76 @@ +. + +namespace core_question; + +/** + * Methods for finding and manipulating question versions + * + * @package core_question + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class versions { + /** + * Get the next version number for a question bank entry. + * + * This uses the value in the question bank entry record, but if it's not set, it will calculate it based on the + * current highest version in question_versions. + * + * If calling this with $increment = true, it is a good idea to do this inside a transaction which only commits after the + * new version number has been used to save a new version of the question. This avoids wasting version numbers if an + * error happens. + * + * @param int $questionbankentryid + * @param bool $increment If true, increment the version number by 1 after it is read. + * @return int The number of the next version. + */ + public static function get_next_version(int $questionbankentryid, bool $increment = true): int { + global $DB; + $transaction = $DB->start_delegated_transaction(); + $nextversion = $DB->get_field('question_bank_entries', 'nextversion', ['id' => $questionbankentryid]); + if (is_null($nextversion)) { + $nextversion = $DB->get_field_sql( + "SELECT COALESCE(MAX(qv.version), 0) + 1 + FROM {question_versions} qv + WHERE qv.questionbankentryid = :qbeid", + ['qbeid' => $questionbankentryid], + ); + $DB->set_field('question_bank_entries', 'nextversion', $nextversion, ['id' => $questionbankentryid]); + } + if ($increment) { + self::increment_next_version($questionbankentryid); + } + $transaction->allow_commit(); + return $nextversion; + } + + /** + * Increment the next version by 1 for the question bank entry + * + * @param int $questionbankentryid + */ + protected static function increment_next_version(int $questionbankentryid): bool { + global $DB; + return $DB->execute( + "UPDATE {question_bank_entries} + SET nextversion = nextversion + 1 + WHERE id = :id", + ['id' => $questionbankentryid], + ); + } +} diff --git a/public/question/format.php b/public/question/format.php index c362427876f..8298c1b0c82 100644 --- a/public/question/format.php +++ b/public/question/format.php @@ -472,6 +472,7 @@ class qformat_default { $questionbankentry->questioncategoryid = $question->category; $questionbankentry->idnumber = $question->idnumber ?? null; $questionbankentry->ownerid = $question->createdby; + $questionbankentry->nextversion = 2; $questionbankentry->id = $DB->insert_record('question_bank_entries', $questionbankentry); // Create a version for each question imported. $questionversion = new \stdClass(); diff --git a/public/question/tests/backup_test.php b/public/question/tests/backup_test.php index a4110ab38d4..14b5e017b47 100644 --- a/public/question/tests/backup_test.php +++ b/public/question/tests/backup_test.php @@ -24,6 +24,7 @@ use question_bank; defined('MOODLE_INTERNAL') || die(); use backup; +use core\context\module; use core_question\local\bank\question_bank_helper; use core_question\local\bank\question_version_status; use restore_controller; @@ -899,6 +900,7 @@ final class backup_test extends \advanced_testcase { $DB->set_field('question_versions', 'version', 3, ['questionid' => $questionv5->id]); $questionv6 = $questiongenerator->update_question($questionv5, null, ['name' => 'Version 6']); $DB->set_field('question_versions', 'version', 4, ['questionid' => $questionv6->id]); + $DB->set_field('question_bank_entries', 'nextversion', 5, ['id' => $questionv1->questionbankentryid]); // Add a quiz specifically using "version 5" (with version number 3). $quiz2 = self::getDataGenerator()->create_module('quiz', ['course' => $testdata->course->id]); @@ -975,5 +977,152 @@ final class backup_test extends \advanced_testcase { $structure3 = $quiz3settings->get_structure(); $this->assertEquals($questionv6->id, $structure3->get_question_in_slot(1)->questionid); + + // The nextversion was incremented when the old versions were inserted. + $this->assertEquals(7, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $questionv1->questionbankentryid])); + } + + /** + * Restore a backup containing question versions that were deleted, after new versions were created. + * + * Unlike {@see test_restore_backup_containing_deleted_versions}, we do not mess with the version numbers or nextversion + * counter, meaning a gap should be left for the deleted question, and it should be restore in its original place. + */ + public function test_restore_backup_containing_deleted_versions_using_nextversion(): void { + global $DB; + self::setAdminUser(); + $this->resetAfterTest(); + $questiongenerator = self::getDataGenerator()->get_plugin_generator('core_question'); + $testdata = $this->add_course_quiz_and_qbank(); + $questionv1 = $testdata->qbankquestion; + $questionv2 = $questiongenerator->update_question($questionv1, null, ['name' => 'Version 2']); + $questionv3 = $questiongenerator->update_question($questionv2, null, ['name' => 'Version 3']); + $questionv4 = $questiongenerator->update_question($questionv3, null, ['name' => 'Version 4']); + + $quizsettings = quiz_settings::create($testdata->quiz->id); + $structure1 = $quizsettings->get_structure(); + + // Set the usage of the question to specifically use version 2. + quiz_add_quiz_question($questionv2->id, $testdata->quiz); + $structure1->update_slot_version($structure1->get_slot_id_for_slot(1), 2); + + $backupid = $this->backup_course($testdata->course); + + question_delete_question($questionv4->id); // Actually deleted. + question_delete_question($questionv3->id); // Actually deleted. + question_delete_question($questionv2->id); // Hidden, it's being used explicitly. + + $questionv5 = $questiongenerator->update_question($questionv1, null, ['name' => 'Version 5']); + $questionv6 = $questiongenerator->update_question($questionv5, null, ['name' => 'Version 6']); + $this->assertEquals(7, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $questionv1->questionbankentryid])); + + // Add a quiz specifically using "version 5" . + $quiz2 = self::getDataGenerator()->create_module('quiz', ['course' => $testdata->course->id]); + $quiz2settings = quiz_settings::create($quiz2->id); + quiz_add_quiz_question($questionv5->id, $quiz2); + $structure2 = $quiz2settings->get_structure(); + $structure2->update_slot_version($structure2->get_last_slot()->id, 5); + + // Add another quiz using the "always latest" version of the question. + $quiz3 = self::getDataGenerator()->create_module('quiz', ['course' => $testdata->course->id]); + $quiz3settings = quiz_settings::create($quiz3->id); + quiz_add_quiz_question($questionv6->id, $quiz3); + + $qbe = get_question_bank_entry($questionv1->id); + $versions = $DB->get_records( + 'question_versions', + ['questionbankentryid' => $qbe->id], + fields: 'version, questionid, status', + ); + + $this->assertCount(4, $versions); + + $this->assertEquals($questionv1->id, $versions[1]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[1]->status); + $this->assertEquals($questionv2->id, $versions[2]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_HIDDEN, $versions[2]->status); + $this->assertArrayNotHasKey(3, $versions); + $this->assertArrayNotHasKey(4, $versions); + $this->assertEquals($questionv5->id, $versions[5]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[5]->status); + $this->assertEquals($questionv6->id, $versions[6]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[6]->status); + + $structure1 = $quizsettings->get_structure(); + $this->assertEquals($questionv2->id, $structure1->get_question_in_slot(1)->questionid); + + $structure2 = $quiz2settings->get_structure(); + $this->assertEquals($questionv5->id, $structure2->get_question_in_slot(1)->questionid); + + $structure3 = $quiz3settings->get_structure(); + $this->assertEquals($questionv6->id, $structure3->get_question_in_slot(1)->questionid); + + $this->restore_to_course($backupid, $testdata->course->id); + + $versions = $DB->get_records( + 'question_versions', + ['questionbankentryid' => $qbe->id], + fields: 'version, questionid, status', + ); + + $this->assertCount(6, $versions); + + $this->assertEquals($questionv1->id, $versions[1]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[1]->status); + // Version 2 is restored to "ready" status. + $this->assertEquals($questionv2->id, $versions[2]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[2]->status); + // Version 3 is a new copy of $questionv3 with a different ID but the same content. + $this->assertEquals($questionv3->name, $DB->get_field('question', 'name', ['id' => $versions[3]->questionid])); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[3]->status); + // Version 4 is a new copy of $questionv4 with a different ID but the same content. + $this->assertEquals($questionv4->name, $DB->get_field('question', 'name', ['id' => $versions[4]->questionid])); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[4]->status); + // Versions 5 and 6 have had their version numbers bumped up by 1. + $this->assertEquals($questionv5->id, $versions[5]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[5]->status); + $this->assertEquals($questionv6->id, $versions[6]->questionid); + $this->assertEquals(question_version_status::QUESTION_STATUS_READY, $versions[6]->status); + + // Questions referencing specific versions still point to the same question. + $structure1 = $quizsettings->get_structure(); + $this->assertEquals($questionv2->id, $structure1->get_question_in_slot(1)->questionid); + + $structure2 = $quiz2settings->get_structure(); + $this->assertEquals($questionv5->id, $structure2->get_question_in_slot(1)->questionid); + + $structure3 = $quiz3settings->get_structure(); + $this->assertEquals($questionv6->id, $structure3->get_question_in_slot(1)->questionid); + + // The nextversion has not changed during the restore. + $this->assertEquals(7, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $questionv1->questionbankentryid])); + } + + /** + * Restoring a question bank entry retains the original nextquestion value. + * + * @return void + * @throws \dml_exception + */ + public function test_backup_question_nextquestion_retained(): void { + global $DB; + self::setAdminUser(); + $this->resetAfterTest(); + $questiongenerator = self::getDataGenerator()->get_plugin_generator('core_question'); + $testdata = $this->add_course_quiz_and_qbank(); + $questionv1 = $testdata->qbankquestion; + $questiongenerator->update_question($questionv1, null, ['name' => 'Version 2']); + $this->assertEquals(3, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $questionv1->questionbankentryid])); + + $backupid = $this->backup_course($testdata->course); + + $newcourse = $this->getDataGenerator()->create_course(); + $this->restore_to_course($backupid, $newcourse->id); + $qbanks = get_fast_modinfo($newcourse)->get_instances_of('qbank'); + $qbank = reset($qbanks); + $qbankcontext = module::instance($qbank->id); + $category = question_get_default_category($qbankcontext->id); + // The question bank entry for the restored question has the same nextqueston value as the original. + $this->assertEquals(3, $DB->get_field('question_bank_entries', 'nextversion', ['questioncategoryid' => $category->id])); } } diff --git a/public/question/tests/versions_test.php b/public/question/tests/versions_test.php new file mode 100644 index 00000000000..f30a21f65f4 --- /dev/null +++ b/public/question/tests/versions_test.php @@ -0,0 +1,118 @@ +. + +namespace core_question; + +use core\context\module; + +/** + * Unit tests for versions + * + * @package core_question + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core_question\versions + */ +final class versions_test extends \advanced_testcase { + /** + * Generate 3 questions - one with 3 versions, one with 2, and one with 1. + * + * @return array + */ + protected function create_question_versions(): array { + $qbank = $this->getDataGenerator()->create_module('qbank', ['course' => SITEID]); + $qbankcontext = module::instance($qbank->cmid); + $category = question_get_default_category($qbankcontext->id); + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $q1v1 = $questiongenerator->create_question('shortanswer', null, ['category' => $category->id]); + $q1v2 = $questiongenerator->update_question($q1v1); + $questiongenerator->update_question($q1v2); + $q2v1 = $questiongenerator->create_question('shortanswer', null, ['category' => $category->id]); + $questiongenerator->update_question($q2v1); + $q3v1 = $questiongenerator->create_question('shortanswer', null, ['category' => $category->id]); + + return [ + $q1v1->questionbankentryid, + $q2v1->questionbankentryid, + $q3v1->questionbankentryid, + ]; + } + + /** + * We should get the correct next version for each question bank entry. + */ + public function test_get_next_version(): void { + $this->resetAfterTest(); + [$qbe1, $qbe2, $qbe3] = $this->create_question_versions(); + + $this->assertEquals(4, versions::get_next_version($qbe1)); + $this->assertEquals(3, versions::get_next_version($qbe2)); + $this->assertEquals(2, versions::get_next_version($qbe3)); + } + + /** + * Set the correct next version numbers for existing question bank entries. + */ + public function test_get_next_version_null(): void { + global $DB; + $this->resetAfterTest(); + [$qbe1, $qbe2, $qbe3] = $this->create_question_versions(); + + // Null the nextversion values so they have to be recalculated. + $DB->set_field('question_bank_entries', 'nextversion', null); + + $this->assertEquals(4, versions::get_next_version($qbe1)); + $this->assertEquals(3, versions::get_next_version($qbe2)); + $this->assertEquals(2, versions::get_next_version($qbe3)); + } + + /** + * The next version should be correctly incremented. + */ + public function test_increment_next_version(): void { + $this->resetAfterTest(); + global $DB; + [$qbe1, $qbe2, $qbe3] = $this->create_question_versions(); + + $this->assertEquals(4, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe1])); + $this->assertEquals(3, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe2])); + $this->assertEquals(2, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe3])); + + $this->assertEquals(3, versions::get_next_version($qbe2)); + + // The specified question bank entry has had its nextversion incremented, the others are the same. + $this->assertEquals(4, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe1])); + $this->assertEquals(4, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe2])); + $this->assertEquals(2, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe3])); + } + + /** + * We can get the next version without incrementing it. + */ + public function test_get_without_increment(): void { + $this->resetAfterTest(); + global $DB; + [, $qbe2] = $this->create_question_versions(); + + $this->assertEquals(3, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe2])); + + $this->assertEquals(3, versions::get_next_version($qbe2, increment: false)); + + // The nextversion value has not changed. + $this->assertEquals(3, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe2])); + } +} diff --git a/public/question/type/questiontypebase.php b/public/question/type/questiontypebase.php index 7800d16f0c5..63dbb69e86a 100644 --- a/public/question/type/questiontypebase.php +++ b/public/question/type/questiontypebase.php @@ -29,6 +29,7 @@ defined('MOODLE_INTERNAL') || die(); require_once($CFG->dirroot . '/question/engine/lib.php'); require_once($CFG->libdir . '/questionlib.php'); +use core_question\versions; /** * This is the base class for Moodle question types. @@ -494,7 +495,7 @@ class question_type { // Get the status field. It comes from the form, but for testing we can. $status = $form->status ?? $question->status ?? \core_question\local\bank\question_version_status::QUESTION_STATUS_READY; - $questionversion->version = get_next_version($questionbankentry->id); + $questionversion->version = versions::get_next_version($questionbankentry->id); $questionversion->status = $status; } else { $parentversion = get_question_version($form->parent); diff --git a/public/version.php b/public/version.php index ca1c4179be2..ffc7584d780 100644 --- a/public/version.php +++ b/public/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2025121900.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2025121900.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '5.2dev (Build: 20251219)'; // Human-friendly version name From 50375fed8df0d96168b3f9d653fae2ccaf4bc7e3 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Thu, 11 Dec 2025 11:19:09 +0000 Subject: [PATCH 3/3] MDL-86798 questions: Check original context is on the same site When checking if the original context exists during a restore, we need to check that we're looking at the context on the same site, not one on a different site that happens to have the same ID. --- public/backup/moodle2/restore_stepslib.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/public/backup/moodle2/restore_stepslib.php b/public/backup/moodle2/restore_stepslib.php index e2e71493653..1204e0251a9 100644 --- a/public/backup/moodle2/restore_stepslib.php +++ b/public/backup/moodle2/restore_stepslib.php @@ -5546,7 +5546,11 @@ class restore_move_module_questions_categories extends restore_execution_step { // but if that context still exists on the site and the user has access then point question references // to the originals. $originalcontext = context::instance_by_id($contextid, IGNORE_MISSING); - if ($originalcontext && has_capability('mod/qbank:view', $originalcontext)) { + if ( + $this->task->is_samesite() + && $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) {