From cfaccbf953305d60b78cccdb460be4bd4b007726 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Fri, 14 Nov 2025 11:08:17 +0000 Subject: [PATCH] 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 +++ 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 | 14 ++ public/lib/questionlib.php | 15 +- 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 +- 12 files changed, 385 insertions(+), 17 deletions(-) create mode 100644 .upgradenotes/MDL-86798-2025111411591681.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/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 d8d892490f9..645d206dd10 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 @@ -5355,6 +5356,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 cd8587f97d3..0a3aa1e894f 100644 --- a/public/lib/db/upgrade.php +++ b/public/lib/db/upgrade.php @@ -2369,5 +2369,19 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2025100601.02); } + if ($oldversion < 2025100601.04) { + // 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); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2025100601.04); + } + return true; } diff --git a/public/lib/questionlib.php b/public/lib/questionlib.php index dbbb23d31ea..a665c0dbd06 100644 --- a/public/lib/questionlib.php +++ b/public/lib/questionlib.php @@ -1936,20 +1936,7 @@ function get_question_version($questionid): array { * @throws dml_exception */ 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 144bb403662..120fe25acd5 100644 --- a/public/version.php +++ b/public/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2025100601.03; // 20251006 = branching date YYYYMMDD - do not modify! +$version = 2025100601.04; // 20251006 = branching date YYYYMMDD - do not modify! // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '5.1.1+ (Build: 20251219)'; // Human-friendly version name