From ab8a4dd8cb1689f2e20a67b8c6dc5675d04d1f9d Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Thu, 15 Jun 2023 11:42:32 +0100 Subject: [PATCH 1/2] MDL-77745 core_question: Show question version in info box --- lang/en/question.php | 4 + question/bank/previewquestion/preview.php | 9 +- .../templates/preview_question.mustache | 8 +- .../tests/behat/preview_question.feature | 6 +- .../tests/behat/question_usage_column.feature | 1 + .../classes/output/question_version_info.php | 115 ++++++++++++++++++ question/engine/bank.php | 5 +- question/engine/lib.php | 5 + question/engine/questionattempt.php | 4 + question/engine/renderer.php | 4 + .../templates/question_version_info.mustache | 26 ++++ question/tests/version_test.php | 50 ++++++++ question/type/questionbase.php | 31 +++++ 13 files changed, 257 insertions(+), 11 deletions(-) create mode 100644 question/classes/output/question_version_info.php create mode 100644 question/templates/question_version_info.mustache diff --git a/lang/en/question.php b/lang/en/question.php index 5a49ddac89c..33ec7f7153b 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -457,6 +457,8 @@ $string['rightanswer'] = 'Right answer'; $string['rightanswer_help'] = 'An automatically generated summary of the correct response. This can be limited, so you may wish to consider explaining the correct solution in the general feedback for the question, and turning this option off.'; $string['saved'] = 'Saved: {$a}'; $string['settingsformultipletries'] = 'Multiple tries'; +$string['shortversioninfo'] = 'v{$a->version} (of {$a->latestversion})'; +$string['shortversioninfolatest'] = 'v{$a->version} (latest)'; $string['showhidden'] = 'Also show old questions'; $string['showmarkandmax'] = 'Show mark and max'; $string['showmaxmarkonly'] = 'Show max mark only'; @@ -505,6 +507,8 @@ $string['qbanknotfound'] = 'The \'{$a}\' question bank plugin doesn\'t exist or $string['noquestionbanks'] = 'No question bank plugin found.'; $string['questionloaderror'] = 'Could not load the question options.'; $string['version_selection'] = 'Version {$a->version}'; +$string['versioninfo'] = 'Version {$a->version} (of {$a->latestversion})'; +$string['versioninfolatest'] = 'Version {$a->version} (latest)'; $string['question_version'] = 'Question version'; // Deprecated since Moodle 4.0. diff --git a/question/bank/previewquestion/preview.php b/question/bank/previewquestion/preview.php index 437ba77a688..2ee19639192 100644 --- a/question/bank/previewquestion/preview.php +++ b/question/bank/previewquestion/preview.php @@ -83,6 +83,7 @@ $maxvariant = min($question->get_num_variants(), QUESTION_PREVIEW_MAX_VARIANTS); $options = new question_preview_options($question); $options->load_user_defaults(); $options->set_from_request(); +$options->versioninfo = false; $PAGE->set_url(helper::question_preview_url($id, $options->behaviour, $options->maxmark, $options, $options->variant, $context, null, $restartversion)); @@ -264,11 +265,9 @@ $previewdata = []; $previewdata['questionicon'] = print_question_icon($question); $previewdata['questionidumber'] = $question->idnumber; $previewdata['questiontitle'] = $question->name; -$islatestversion = is_latest($question->version, $question->questionbankentryid); -if ($islatestversion) { - $previewdata['versiontitle'] = get_string('versiontitlelatest', 'qbank_previewquestion', $question->version); -} else { - $previewdata['versiontitle'] = get_string('versiontitle', 'qbank_previewquestion', $question->version); +$versioninfo = new \core_question\output\question_version_info($question); +$previewdata['versiontitle'] = $versioninfo->export_for_template($OUTPUT); +if ($versioninfo->version !== $versioninfo->latestversion) { if ($restartversion == question_preview_options::ALWAYS_LATEST) { $newerversionparams = (object) [ 'currentversion' => $question->version, diff --git a/question/bank/previewquestion/templates/preview_question.mustache b/question/bank/previewquestion/templates/preview_question.mustache index 68f0d919dde..3d750537c97 100644 --- a/question/bank/previewquestion/templates/preview_question.mustache +++ b/question/bank/previewquestion/templates/preview_question.mustache @@ -41,7 +41,9 @@ "question": "
question html
", "questionicon": "", "questiontitle": "Question title", - "versiontitle": "Version 3 (latest)", + "versiontitle": { + "versioninfo": "Version 3 (latest)" + }, "questionidumber": "qidnumber1", "restartdisabled": "disabled='disabled'", "finishdisabled": "disabled='disabled'", @@ -58,7 +60,9 @@

{{{questionicon}}}

{{questiontitle}}

- {{versiontitle}} + {{#versiontitle}} + {{>core_question/question_version_info}} + {{/versiontitle}}

{{#newerversion}} diff --git a/question/bank/previewquestion/tests/behat/preview_question.feature b/question/bank/previewquestion/tests/behat/preview_question.feature index 45f66a0bfea..70bc0634113 100644 --- a/question/bank/previewquestion/tests/behat/preview_question.feature +++ b/question/bank/previewquestion/tests/behat/preview_question.feature @@ -149,18 +149,18 @@ Feature: A teacher can preview questions in the question bank | Test questions | Test question to be previewed | Question version 2 | And I choose "History" action for "Test question to be previewed" in the question bank And I choose "Preview" action for "Test question to be previewed" in the question bank - And I should see "Version 1" + And I should see "Version 1 (of 2)" And I expand all fieldsets And the field "Question version" matches value "1" And I set the field "Answer:" to "3.14" And I press "Submit and finish" - And I should see "Version 1" + And I should see "Version 1 (of 2)" And I should not see "The latest version is 2." And the following "core_question > updated questions" exist: | questioncategory | question | questiontext | | Test questions | Test question to be previewed | Question version 3 | When I press "Start again" - Then I should see "Version 1" + Then I should see "Version 1 (of 3)" And I should not see "Version 3 (latest)" Scenario: Question preview can be closed diff --git a/question/bank/usage/tests/behat/question_usage_column.feature b/question/bank/usage/tests/behat/question_usage_column.feature index ce711d76c24..64138c923dc 100644 --- a/question/bank/usage/tests/behat/question_usage_column.feature +++ b/question/bank/usage/tests/behat/question_usage_column.feature @@ -36,5 +36,6 @@ Feature: Use the qbank plugin manager page for question usage And I should see "0" on the usage column When I click "0" on the usage column Then I should see "Version 1" + And I should see "v1 (latest)" in the "Question 1" "question" And I click on "Close" "button" in the ".modal-dialog" "css_element" And I should see "0" on the usage column diff --git a/question/classes/output/question_version_info.php b/question/classes/output/question_version_info.php new file mode 100644 index 00000000000..a907b0d3776 --- /dev/null +++ b/question/classes/output/question_version_info.php @@ -0,0 +1,115 @@ +. + +namespace core_question\output; + +use renderer_base; + +/** + * Track and display question version information. + * + * This class handles rendering the question version information (the current version of the question, the total number of versions, + * and if the current version is the latest). It also tracks loaded question definitions that don't yet have the latest version + * loaded, and handles loading the latest version of all pending questions. + * + * @package core_question + * @copyright 2023 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 question_version_info implements \renderable, \templatable { + + /** + * @var array List of definitions that don't know whether they are the latest version yet. + */ + public static array $pendingdefinitions = []; + + /** + * @var int $version The current version number. + */ + public int $version; + + /** + * @var ?int $latestversion The latest version number of this question. + */ + public ?int $latestversion; + + /** + * @var bool $shortversion Are we displaying an abbreviation for "version" rather than the full word? + */ + protected bool $shortversion; + + /** + * Store the current and latest versions of the question, and whether we want to abbreviate the output string. + * + * @param \question_definition $question + * @param bool $shortversion + */ + public function __construct(\question_definition $question, bool $shortversion = false) { + $this->version = $question->version; + $this->latestversion = $question->latestversion; + $this->shortversion = $shortversion; + } + + /** + * Find and set the latest version of all pending question_definition objects. + * + * This will update all pending objects in one go, saving us having to do a query for each question. + * + * @return void + */ + public static function populate_latest_versions(): void { + global $DB; + $pendingentryids = array_map(fn($definition) => $definition->questionbankentryid, self::$pendingdefinitions); + [$insql, $params] = $DB->get_in_or_equal($pendingentryids); + + $sql = "SELECT questionbankentryid, MAX(version) AS latestversion + FROM {question_versions} + WHERE questionbankentryid $insql + GROUP BY questionbankentryid"; + $latestversions = $DB->get_records_sql_menu($sql, $params); + array_walk(self::$pendingdefinitions, function($definition) use ($latestversions) { + if (!isset($latestversions[$definition->questionbankentryid])) { + return; + } + $definition->set_latest_version($latestversions[$definition->questionbankentryid]); + unset(self::$pendingdefinitions[$definition->id]); + }); + } + + /** + * Return the question version info as a string, including the version number and whether this is the latest version. + * + * @param renderer_base $output + * @return array + * @throws \coding_exception + */ + public function export_for_template(renderer_base $output): array { + if (is_null($this->latestversion)) { + return []; + } + $identifier = 'versioninfo'; + if ($this->version === $this->latestversion) { + $identifier .= 'latest'; + } + if ($this->shortversion) { + $identifier = 'short' . $identifier; + } + return [ + 'versioninfo' => get_string($identifier, 'question', $this) + ]; + } +} diff --git a/question/engine/bank.php b/question/engine/bank.php index 866d7d15280..5c0e1dc8191 100644 --- a/question/engine/bank.php +++ b/question/engine/bank.php @@ -27,6 +27,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core_question\output\question_version_info; defined('MOODLE_INTERNAL') || die(); @@ -285,7 +286,9 @@ abstract class question_bank { * @return question_definition loaded from the database. */ public static function make_question($questiondata) { - return self::get_qtype($questiondata->qtype, false)->make_question($questiondata, false); + $definition = self::get_qtype($questiondata->qtype, false)->make_question($questiondata, false); + question_version_info::$pendingdefinitions[$definition->id] = $definition; + return $definition; } /** diff --git a/question/engine/lib.php b/question/engine/lib.php index 3824dce2324..213838a878d 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -653,6 +653,11 @@ class question_display_options { */ public $questionidentifier = null; + /** + * @var ?bool $versioninfo Should we display the version in the question info? + */ + public ?bool $versioninfo = null; + /** * Set all the feedback-related fields {@link $feedback}, {@link generalfeedback}, * {@link rightanswer} and {@link manualcomment} to diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index d6539c944f6..34d09486050 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -23,6 +23,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core_question\local\bank\question_edit_contexts; defined('MOODLE_INTERNAL') || die(); @@ -903,6 +904,9 @@ class question_attempt { global $PAGE; $page = $PAGE; } + if (is_null($options->versioninfo)) { + $options->versioninfo = (new question_edit_contexts($page->context))->have_one_edit_tab_cap('questions'); + } $qoutput = $page->get_renderer('core', 'question'); $qtoutput = $this->question->get_renderer($page); return $this->behaviour->render($options, $number, $qoutput, $qtoutput); diff --git a/question/engine/renderer.php b/question/engine/renderer.php index fe78e0099ae..44fb155e670 100644 --- a/question/engine/renderer.php +++ b/question/engine/renderer.php @@ -23,6 +23,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core_question\output\question_version_info; defined('MOODLE_INTERNAL') || die(); @@ -145,6 +146,9 @@ class core_question_renderer extends plugin_renderer_base { $output .= $this->mark_summary($qa, $behaviouroutput, $options); $output .= $this->question_flag($qa, $options->flags); $output .= $this->edit_question_link($qa, $options); + if ($options->versioninfo) { + $output .= $this->render(new question_version_info($qa->get_question(), true)); + } return $output; } diff --git a/question/templates/question_version_info.mustache b/question/templates/question_version_info.mustache new file mode 100644 index 00000000000..530fd6d807d --- /dev/null +++ b/question/templates/question_version_info.mustache @@ -0,0 +1,26 @@ +{{! + This file is part of Moodle - http://moodle.org/ + Moodle is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + Moodle is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + You should have received a copy of the GNU General Public License + along with Moodle. If not, see . +}} +{{! + @template core_question/question_version_info + + Displays a badge containing the version information of a question + * versioninfo - The version information string, including the number and whether it is the latest verision. + + Example context (json): + { + "versioninfo": "Version 2 (latest)" + } +}} + +{{versioninfo}} diff --git a/question/tests/version_test.php b/question/tests/version_test.php index 2a431637a95..9b879e93b53 100644 --- a/question/tests/version_test.php +++ b/question/tests/version_test.php @@ -17,6 +17,7 @@ namespace core_question; use core_question\local\bank\question_version_status; +use core_question\output\question_version_info; use question_bank; /** @@ -65,6 +66,11 @@ class version_test extends \advanced_testcase { $this->context = \context_module::instance($this->quiz->cmid); } + protected function tearDown(): void { + question_version_info::$pendingdefinitions = []; + parent::tearDown(); + } + /** * Test if creating a question a new version and bank entry records are created. * @@ -288,4 +294,48 @@ class version_test extends \advanced_testcase { $this->assertEquals(array_slice($questiondefinition, 1, 1)[0]->questionid, $questionid2); $this->assertEquals(array_slice($questiondefinition, 2, 1)[0]->questionid, $questionid1); } + + /** + * Test population of latestversion field in question_definition objects + * + * When an instance of question_definition is created, it is added to an array of pending definitions which + * do not yet have the latestversion field populated. When one definition has its latestversion property accessed, + * all pending definitions have their latestversion field populated at once. + * + * @covers \core_question\output\question_version_info::populate_latest_versions() + * @return void + */ + public function test_populate_definition_latestversions() { + $qcategory = $this->qgenerator->create_question_category(['contextid' => $this->context->id]); + $question1 = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcategory->id]); + $question2 = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcategory->id]); + $question3 = $this->qgenerator->update_question($question2, null, ['idnumber' => 'id2']); + + $latestversioninspector = new \ReflectionProperty('question_definition', 'latestversion'); + $latestversioninspector->setAccessible(true); + $this->assertEmpty(question_version_info::$pendingdefinitions); + + $questiondef1 = question_bank::load_question($question1->id); + $questiondef2 = question_bank::load_question($question2->id); + $questiondef3 = question_bank::load_question($question3->id); + + $this->assertContains($questiondef1, question_version_info::$pendingdefinitions); + $this->assertContains($questiondef2, question_version_info::$pendingdefinitions); + $this->assertContains($questiondef3, question_version_info::$pendingdefinitions); + $this->assertNull($latestversioninspector->getValue($questiondef1)); + $this->assertNull($latestversioninspector->getValue($questiondef2)); + $this->assertNull($latestversioninspector->getValue($questiondef3)); + + // Read latestversion from one definition. This should populate the field in all pending definitions. + $latestversion1 = $questiondef1->latestversion; + + $this->assertEmpty(question_version_info::$pendingdefinitions); + $this->assertNotNull($latestversioninspector->getValue($questiondef1)); + $this->assertNotNull($latestversioninspector->getValue($questiondef2)); + $this->assertNotNull($latestversioninspector->getValue($questiondef3)); + $this->assertEquals($latestversion1, $latestversioninspector->getValue($questiondef1)); + $this->assertEquals($questiondef1->version, $questiondef1->latestversion); + $this->assertNotEquals($questiondef2->version, $questiondef2->latestversion); + $this->assertEquals($questiondef3->version, $questiondef3->latestversion); + } } diff --git a/question/type/questionbase.php b/question/type/questionbase.php index cbee8a562b7..672f90a4ad9 100644 --- a/question/type/questionbase.php +++ b/question/type/questionbase.php @@ -41,6 +41,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core_question\output\question_version_info; defined('MOODLE_INTERNAL') || die(); @@ -130,6 +131,9 @@ abstract class question_definition { /** @var int Bank entry id for the question */ public $questionbankentryid; + /** @var ?int The latest version of the question. null if we haven't checked yet. */ + protected $latestversion = null; + /** * @var array of array of \core_customfield\data_controller objects indexed by fieldid for the questions custom fields. */ @@ -143,6 +147,21 @@ abstract class question_definition { public function __construct() { } + /** + * When a pending definition tries to read its latest version, fill in the latest version for all pending definitions + * + * @param string $name + * @return mixed + */ + public function __get($name) { + if ($name === 'latestversion') { + if (isset(question_version_info::$pendingdefinitions[$this->id])) { + question_version_info::populate_latest_versions(); + } + return $this->latestversion; + } + } + /** * @return string the name of the question type (for example multichoice) that this * question is. @@ -512,6 +531,18 @@ abstract class question_definition { DEBUG_DEVELOPER); return null; } + + /** + * Set the latest version. + * + * Making $this->latestversion public would break the magic __get() behaviour above, so allow it to be set externally. + * + * @param int $latestversion + * @return void + */ + public function set_latest_version(int $latestversion): void { + $this->latestversion = $latestversion; + } } From cc1ae8d5b3adbad5cfc0e8507f3a26915f77e881 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Wed, 21 Jun 2023 10:45:40 +0100 Subject: [PATCH 2/2] MDL-77745 mod_quiz: Confirm display of question version in behat tests --- mod/quiz/tests/behat/attempt_begin.feature | 1 + mod/quiz/tests/behat/preview.feature | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/mod/quiz/tests/behat/attempt_begin.feature b/mod/quiz/tests/behat/attempt_begin.feature index 81bc94ee47e..1b7508ae1a1 100644 --- a/mod/quiz/tests/behat/attempt_begin.feature +++ b/mod/quiz/tests/behat/attempt_begin.feature @@ -32,6 +32,7 @@ Feature: The various checks that may happen when an attept is started When I am on the "Quiz 1" "mod_quiz > View" page logged in as "student" And I press "Attempt quiz" Then I should see "Text of the first question" + And I should not see "v1" in the "Question 1" "question" @javascript Scenario: Start a quiz with time limit and password diff --git a/mod/quiz/tests/behat/preview.feature b/mod/quiz/tests/behat/preview.feature index 8d681aba98a..a569ce362e6 100644 --- a/mod/quiz/tests/behat/preview.feature +++ b/mod/quiz/tests/behat/preview.feature @@ -12,8 +12,8 @@ Feature: Preview a quiz as a teacher | fullname | shortname | category | | Course 1 | C1 | 0 | And the following "course enrolments" exist: - | user | course | role | - | teacher | C1 | teacher | + | user | course | role | + | teacher | C1 | editingteacher | And the following "question categories" exist: | contextlevel | reference | name | | Course | C1 | Test questions | @@ -38,6 +38,7 @@ Feature: Preview a quiz as a teacher When I am on the "Quiz 1" "mod_quiz > View" page logged in as "teacher" And I follow "Review" Then I should see "25.00 out of 100.00" + And I should see "v1 (latest)" in the "Question 1" "question" And I follow "Finish review" And "Review" "link" in the "Preview" "table_row" should be visible @@ -58,6 +59,7 @@ Feature: Preview a quiz as a teacher Given I am on the "Quiz 1" "mod_quiz > View" page logged in as "teacher" When I press "Preview quiz" Then I should see "Question 1" + And I should see "v1 (latest)" in the "Question 1" "question" And "Start a new preview" "button" should exist Scenario: Teachers should see a notice if the quiz is not available to students