From a9fa52295a3ba5eb6df7d6ed319a1eb773788fb2 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 26 Apr 2023 10:42:11 +0100 Subject: [PATCH 1/6] MDL-78025 question: move hiding logic into question_delete_question This logic belongs in the API, so it is applied consistently. Also this avoids calling the expensive function questions_in_use twice per question. --- lib/questionlib.php | 5 ++++- question/bank/deletequestion/delete.php | 7 +------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/questionlib.php b/lib/questionlib.php index f7a2eff980b..82175a1b9d6 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -28,6 +28,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core_question\local\bank\question_version_status; defined('MOODLE_INTERNAL') || die(); @@ -360,8 +361,10 @@ function question_delete_question($questionid): void { $questionstocheck[] = $question->parent; } - // Do not delete a question if it is used by an activity module + // Do not delete a question if it is used by an activity module. Just mark the version hidden. if (questions_in_use($questionstocheck)) { + $DB->set_field('question_versions', 'status', + question_version_status::QUESTION_STATUS_HIDDEN, ['questionid' => $questionid]); return; } diff --git a/question/bank/deletequestion/delete.php b/question/bank/deletequestion/delete.php index b41ba32bdae..dbb697aafec 100644 --- a/question/bank/deletequestion/delete.php +++ b/question/bank/deletequestion/delete.php @@ -83,12 +83,7 @@ if ($deleteselected && ($confirm = optional_param('confirm', '', PARAM_ALPHANUM) foreach ($questionlist as $questionid) { $questionid = (int)$questionid; question_require_capability_on($questionid, 'edit'); - if (questions_in_use(array($questionid))) { - $DB->set_field('question_versions', 'status', - \core_question\local\bank\question_version_status::QUESTION_STATUS_HIDDEN, ['questionid' => $questionid]); - } else { - question_delete_question($questionid); - } + question_delete_question($questionid); } } redirect($returnurl); From d2d9d762ad9a220d4a1e35091e040c1ecf2a7654 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 25 Apr 2023 17:58:58 +0100 Subject: [PATCH 2/6] MDL-78025 question generator: make the behaviour less surprising * The object returned by update_question is alwasy a new clone and the $question passed in will not be modified. * The returned object has the fields like questionbankentryid and the ones related to versionning, so it is more like the data returned by question_bank::load_question_data. --- question/tests/generator/lib.php | 8 +++++++- question/tests/version_test.php | 29 +++++++++++++++++------------ question/upgrade.txt | 11 +++++++++++ 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/question/tests/generator/lib.php b/question/tests/generator/lib.php index 05427126e45..a1d3deb6be8 100644 --- a/question/tests/generator/lib.php +++ b/question/tests/generator/lib.php @@ -118,11 +118,12 @@ class core_question_generator extends component_generator_base { * @param string $which as for the corresponding argument of * {@link question_test_helper::get_question_form_data}. null for the default one. * @param array|stdClass $overrides any fields that should be different from the base example. - * @return stdClass the question data. + * @return stdClass the question data, including version info and questionbankentryid */ public function update_question($question, $which = null, $overrides = null) { global $CFG, $DB; require_once($CFG->dirroot . '/question/engine/tests/helpers.php'); + $question = clone($question); $qtype = $question->qtype; @@ -144,6 +145,11 @@ class core_question_generator extends component_generator_base { } $DB->update_record('question', $question); } + $questionversion = $DB->get_record('question_versions', ['questionid' => $question->id], '*', MUST_EXIST); + $question->versionid = $questionversion->id; + $question->questionbankentryid = $questionversion->questionbankentryid; + $question->version = $questionversion->version; + $question->status = $questionversion->status; return $question; } diff --git a/question/tests/version_test.php b/question/tests/version_test.php index 43c6c2e2ed8..2a431637a95 100644 --- a/question/tests/version_test.php +++ b/question/tests/version_test.php @@ -16,6 +16,7 @@ namespace core_question; +use core_question\local\bank\question_version_status; use question_bank; /** @@ -89,7 +90,7 @@ class version_test extends \advanced_testcase { $this->assertEquals($questionversion->questionbankentryid, $questiondefinition->questionbankentryid); // If a question is updated, a new version should be created. - $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']); + $question = $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']); $newquestiondefinition = question_bank::load_question($question->id); // The version should be 2. $this->assertEquals('2', $newquestiondefinition->version); @@ -112,7 +113,7 @@ class version_test extends \advanced_testcase { $questionfirstversionid = $question->id; // Create a new version and try to remove it. - $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']); + $question = $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']); // The new version and bank entry record should exist. $sql = "SELECT q.id, qv.id AS versionid, qv.questionbankentryid @@ -158,12 +159,14 @@ class version_test extends \advanced_testcase { * @covers ::question_delete_question */ public function test_delete_question_in_use() { + global $DB; + $qcategory = $this->qgenerator->create_question_category(['contextid' => $this->context->id]); $question = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcategory->id]); $questionfirstversionid = $question->id; // Create a new version and try to remove it after adding it to a quiz. - $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']); + $question = $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']); // Add it to the quiz. quiz_add_quiz_question($question->id, $this->quiz); @@ -173,11 +176,13 @@ class version_test extends \advanced_testcase { // Try to delete old version. question_delete_question($questionfirstversionid); - // The questions should exist even after trying to remove it. - $questionversion1 = question_bank::load_question($question->id); - $questionversion2 = question_bank::load_question($questionfirstversionid); - $this->assertEquals($questionversion1->id, $question->id); - $this->assertEquals($questionversion2->id, $questionfirstversionid); + // The used question version should exist even after trying to remove it, but now hidden. + $questionversion2 = question_bank::load_question($question->id); + $this->assertEquals($question->id, $questionversion2->id); + $this->assertEquals(question_version_status::QUESTION_STATUS_HIDDEN, $questionversion2->status); + + // The unused version should be completely gone. + $this->assertFalse($DB->record_exists('question', ['id' => $questionfirstversionid])); } /** @@ -233,10 +238,10 @@ class version_test extends \advanced_testcase { $questionid1 = $question->id; // Create a new version and try to remove it after adding it to a quiz. - $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']); + $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']); $questionid2 = $question->id; // Change the id number and get the question object. - $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']); + $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']); $questionid3 = $question->id; // The new version and bank entry record should exist. @@ -270,10 +275,10 @@ class version_test extends \advanced_testcase { $questionid1 = $question->id; // Create a new version. - $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']); + $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']); $questionid2 = $question->id; // Change the id number and get the question object. - $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']); + $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']); $questionid3 = $question->id; $questiondefinition = question_bank::get_all_versions_of_question($question->id); diff --git a/question/upgrade.txt b/question/upgrade.txt index d477f1a797c..ab40d694f66 100644 --- a/question/upgrade.txt +++ b/question/upgrade.txt @@ -1,5 +1,16 @@ This files describes API changes for code that uses the question API. +=== 4.1.4 === + +1) The core_question_generator::update_question has been changed so that it no longer modifies the $question + object that was passed in. Instead, the update question is returned (which was already the case). + If you were relying on the old behavioru in your tests, you will need a change like + $questiongenerator->update_question($question, ...); + to + $question = $questiongenerator->update_question($question, ...); + Also, the $question object returned now has fields questionbankentryid, versionid, version and status. + + === 4.1 === 1) get_bulk_action_key() in core_question\local\bank\bulk_action_base class is deprecated and renamed to get_key(). From 4a9c0b1410b8e6520aae8b1f8aad4eb7393ab6fe Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 25 Apr 2023 18:04:11 +0100 Subject: [PATCH 3/6] MDL-78025 questions: questions_in_use should check question_references This avoids the needs for plugins to do separate queries, which is easier for them, and better performing. --- lib/questionlib.php | 5 + .../classes/question_reference_manager.php | 77 +++++++++++ .../tests/question_reference_manager_test.php | 123 ++++++++++++++++++ 3 files changed, 205 insertions(+) create mode 100644 question/classes/question_reference_manager.php create mode 100644 question/engine/tests/question_reference_manager_test.php diff --git a/lib/questionlib.php b/lib/questionlib.php index 82175a1b9d6..51d086f7fad 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -29,6 +29,7 @@ */ use core_question\local\bank\question_version_status; +use core_question\question_reference_manager; defined('MOODLE_INTERNAL') || die(); @@ -123,6 +124,10 @@ function questions_in_use($questionids): bool { return true; } + if (question_reference_manager::questions_with_references($questionids)) { + return true; + } + // Check if any plugins are using these questions. $callbacksbytype = get_plugins_with_function('questions_in_use'); foreach ($callbacksbytype as $callbacks) { diff --git a/question/classes/question_reference_manager.php b/question/classes/question_reference_manager.php new file mode 100644 index 00000000000..ef1d3c69896 --- /dev/null +++ b/question/classes/question_reference_manager.php @@ -0,0 +1,77 @@ +. + +namespace core_question; + +use core_question\local\bank\question_version_status; + +/** + * This class should provide an API for managing question_references. + * + * Unfortunately, question_references were introduced in the DB structure + * without an nice API. This class is being added later, and is currently + * terribly incomplete, but hopefully it can be improved in time. + * + * @package core_question + * @copyright 2023 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class question_reference_manager { + /** + * Return a list of those questions from the list passed in, which are referenced. + * + * A question is referenced if either: + * - There is a question_reference pointing at exactly that version of that question; or + * - There is an 'always latest' reference, and the question id is the latest non-draft version + * of that question_bank_entry. + * + * @param array $questionids a list of question ids to check. + * @return array a list of the question ids from the input array which are referenced. + */ + public static function questions_with_references(array $questionids): array { + global $DB; + + if (empty($questionids)) { + return []; + } + + [$qidtest, $params] = $DB->get_in_or_equal($questionids, SQL_PARAMS_NAMED, 'outerqid'); + [$lqidtest, $lparams] = $DB->get_in_or_equal($questionids, SQL_PARAMS_NAMED, 'innerqid'); + + return $DB->get_fieldset_sql(" + SELECT qv.questionid + + FROM {question_versions} qv + + -- This is a performant to get the latest non-draft version for each + -- question_bank_entry that relates to one of our questionids. + LEFT JOIN ( + SELECT lqv.questionbankentryid, + MAX(lv.version) AS latestusableversion + FROM {question_versions} lqv + JOIN {question_versions} lv ON lv.questionbankentryid = lqv.questionbankentryid + WHERE lqv.questionid $lqidtest + AND lv.status <> :draft + GROUP BY lqv.questionbankentryid + ) latestversions ON latestversions.questionbankentryid = qv.questionbankentryid + + JOIN {question_references} qr ON qr.questionbankentryid = qv.questionbankentryid + AND (qr.version = qv.version OR qr.version IS NULL AND qv.version = latestversions.latestusableversion) + + WHERE qv.questionid $qidtest + ", array_merge($params, $lparams, ['draft' => question_version_status::QUESTION_STATUS_DRAFT])); + } +} diff --git a/question/engine/tests/question_reference_manager_test.php b/question/engine/tests/question_reference_manager_test.php new file mode 100644 index 00000000000..69d104aa157 --- /dev/null +++ b/question/engine/tests/question_reference_manager_test.php @@ -0,0 +1,123 @@ +. + +namespace core_question; + +use advanced_testcase; +use context_system; +use core_question\local\bank\question_version_status; +use core_question_generator; + +/** + * Unit tests for the {@see question_reference_manager} class. + * + * @package core_question + * @category test + * @copyright 2011 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core_question\question_reference_manager + */ +class question_reference_manager_test extends advanced_testcase { + + public function test_questions_with_references() { + global $DB; + $this->resetAfterTest(); + + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $systemcontext = context_system::instance(); + + // Create three questions, each with three versions. + // In each case, the third version is draft. + $cat = $questiongenerator->create_question_category(); + $q1v1 = $questiongenerator->create_question('truefalse', null, ['name' => 'Q1V1', 'category' => $cat->id]); + $q1v2 = $questiongenerator->update_question($q1v1, null, ['name' => 'Q1V2']); + $q1v3 = $questiongenerator->update_question($q1v2, null, + ['name' => 'Q1V3', 'status' => question_version_status::QUESTION_STATUS_DRAFT]); + $q2v1 = $questiongenerator->create_question('truefalse', null, ['name' => 'Q2V1', 'category' => $cat->id]); + $q2v2 = $questiongenerator->update_question($q2v1, null, ['name' => 'Q2V2']); + $q2v3 = $questiongenerator->update_question($q2v2, null, + ['name' => 'Q2V3', 'status' => question_version_status::QUESTION_STATUS_DRAFT]); + $q3v1 = $questiongenerator->create_question('truefalse', null, ['name' => 'Q3V1', 'category' => $cat->id]); + $q3v2 = $questiongenerator->update_question($q3v1, null, ['name' => 'Q3V2']); + $q3v3 = $questiongenerator->update_question($q3v2, null, + ['name' => 'Q3V3', 'status' => question_version_status::QUESTION_STATUS_DRAFT]); + + // Create specific references to Q2V1 and Q2V3. + $DB->insert_record('question_references', ['usingcontextid' => $systemcontext->id, + 'component' => 'core_question', 'questionarea' => 'test', 'itemid' => 0, + 'questionbankentryid' => $q2v1->questionbankentryid, 'version' => 1]); + $DB->insert_record('question_references', ['usingcontextid' => $systemcontext->id, + 'component' => 'core_question', 'questionarea' => 'test', 'itemid' => 1, + 'questionbankentryid' => $q2v1->questionbankentryid, 'version' => 3]); + + // Create an always-latest reference to Q3. + $DB->insert_record('question_references', ['usingcontextid' => $systemcontext->id, + 'component' => 'core_question', 'questionarea' => 'test', 'itemid' => 2, + 'questionbankentryid' => $q3v1->questionbankentryid, 'version' => null]); + + // Verify which versions of Q1 are used. + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([$q1v1->id])); + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([$q1v2->id])); + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([$q1v3->id])); + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([$q1v1->id, $q1v2->id, $q1v3->id])); + + // Verify which versions of Q2 are used. + $this->assertEqualsCanonicalizing([$q2v1->id], + question_reference_manager::questions_with_references([$q2v1->id])); + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([$q2v2->id])); + $this->assertEqualsCanonicalizing([$q2v3->id], + question_reference_manager::questions_with_references([$q2v3->id])); + $this->assertEqualsCanonicalizing([$q2v1->id, $q2v3->id], + question_reference_manager::questions_with_references([$q2v1->id, $q2v2->id, $q2v3->id])); + + // Verify which versions of Q1 are used. + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([$q3v1->id])); + $this->assertEqualsCanonicalizing([$q3v2->id], + question_reference_manager::questions_with_references([$q3v2->id])); + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([$q3v3->id])); + $this->assertEqualsCanonicalizing([$q3v2->id], + question_reference_manager::questions_with_references([$q3v1->id, $q3v2->id, $q3v3->id])); + + // Do some combined queries. + $this->assertEqualsCanonicalizing([$q2v1->id, $q2v3->id, $q3v2->id], + question_reference_manager::questions_with_references([ + $q1v1->id, $q1v2->id, $q1v3->id, + $q2v1->id, $q2v2->id, $q2v3->id, + $q3v1->id, $q3v2->id, $q3v3->id])); + $this->assertEqualsCanonicalizing([$q2v1->id, $q2v3->id, $q3v2->id], + question_reference_manager::questions_with_references([$q2v1->id, $q2v3->id, $q3v2->id])); + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([ + $q1v1->id, $q1v2->id, $q1v3->id, + $q2v2->id, + $q3v1->id, $q3v3->id])); + + // Test some edge cases. + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([])); + $this->assertEqualsCanonicalizing([], + question_reference_manager::questions_with_references([-1])); + + } +} From 384ab12c554b6a5e8bac1f0b43d175e9aa63dc55 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 25 Apr 2023 18:11:57 +0100 Subject: [PATCH 4/6] MDL-78025 quiz: fix the quiz_questions_in_use logic --- mod/quiz/lib.php | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/mod/quiz/lib.php b/mod/quiz/lib.php index b8fc686a9fc..bdfc85229cf 100644 --- a/mod/quiz/lib.php +++ b/mod/quiz/lib.php @@ -1432,25 +1432,15 @@ function quiz_get_post_actions() { } /** + * Standard callback used by questions_in_use. + * * @param array $questionids of question ids. * @return bool whether any of these questions are used by any instance of this module. */ function quiz_questions_in_use($questionids) { - global $DB; - list($test, $params) = $DB->get_in_or_equal($questionids); - $params['component'] = 'mod_quiz'; - $params['questionarea'] = 'slot'; - $sql = "SELECT qs.id - FROM {quiz_slots} qs - JOIN {question_references} qr ON qr.itemid = qs.id - JOIN {question_bank_entries} qbe ON qbe.id = qr.questionbankentryid - JOIN {question_versions} qv ON qv.questionbankentryid = qbe.id - WHERE qv.questionid $test - AND qr.component = ? - AND qr.questionarea = ?"; - return $DB->record_exists_sql($sql, $params) || question_engine::questions_in_use( - $questionids, new qubaid_join('{quiz_attempts} quiza', - 'quiza.uniqueid', 'quiza.preview = 0')); + return question_engine::questions_in_use($questionids, + new qubaid_join('{quiz_attempts} quiza', 'quiza.uniqueid', + 'quiza.preview = 0')); } /** From 15ff1352c2ae67805f0a2f03ec5fe7650008da08 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 26 Apr 2023 10:46:44 +0100 Subject: [PATCH 5/6] MDL-78025 questions: fix PHPdoc on question_require_capability_on --- lib/questionlib.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/questionlib.php b/lib/questionlib.php index 51d086f7fad..762b7d30081 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -1494,9 +1494,10 @@ function question_has_capability_on($questionorid, $cap, $notused = -1): bool { /** * Require capability on question. * - * @param object $question - * @param string $cap - * @return bool + * @param int|stdClass|question_definition $question object or id. + * If an object is passed, it should include ->contextid and ->createdby. + * @param string $cap 'add', 'edit', 'view', 'use', 'move' or 'tag'. + * @return bool true if the user has the capability. Throws exception if not. */ function question_require_capability_on($question, $cap): bool { if (!question_has_capability_on($question, $cap)) { From de8607067f93e0d4eb2a452b0237cd780fd819ce Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 26 Apr 2023 15:26:08 +0100 Subject: [PATCH 6/6] MDL-78025 questions: improve the comment on question_references.version --- lib/db/install.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/db/install.xml b/lib/db/install.xml index 518e510b245..1e719c7a844 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -1475,7 +1475,7 @@ - +