From 37e8faf0ed51926357b43f5c06889f059b0cbb41 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 23 Mar 2022 14:57:03 +0000 Subject: [PATCH 1/5] MDL-74255 quiz: Remove un-needed method question_array_sort --- mod/quiz/attemptlib.php | 4 +- .../classes/question/bank/qbank_helper.php | 41 ++++++------------- mod/quiz/report/reportlib.php | 4 +- mod/quiz/report/statistics/report.php | 3 +- mod/quiz/tests/qbank_helper_test.php | 2 - 5 files changed, 19 insertions(+), 35 deletions(-) diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index 6b2537411e6..febc31505b2 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -152,8 +152,8 @@ class quiz { if (!empty($questionids)) { $questiondata = \mod_quiz\question\bank\qbank_helper::get_question_structure_data($this->quiz->id, $questionids, true); } - $allquestiondata = \mod_quiz\question\bank\qbank_helper::question_array_sort( - \mod_quiz\question\bank\qbank_helper::question_load_random_questions($this->quiz->id, $questiondata), 'slot'); + $allquestiondata = \mod_quiz\question\bank\qbank_helper::question_load_random_questions( + $this->quiz->id, $questiondata); $this->questions = $allquestiondata; } diff --git a/mod/quiz/classes/question/bank/qbank_helper.php b/mod/quiz/classes/question/bank/qbank_helper.php index 72bff77c620..2b05627be48 100644 --- a/mod/quiz/classes/question/bank/qbank_helper.php +++ b/mod/quiz/classes/question/bank/qbank_helper.php @@ -68,23 +68,6 @@ class qbank_helper { return $DB->get_records_sql($sql, [$questionid]); } - /** - * Sort the elements of an array according to a key. - * - * @param array $arrays - * @param string $on - * @param int $order - * @return array - */ - public static function question_array_sort($arrays, $on, $order = SORT_ASC): array { - $element = []; - foreach ($arrays as $array) { - $element[$array->$on] = $array; - } - ksort($element, $order); - return $element; - } - /** * Get the question id from slot id. * @@ -204,10 +187,10 @@ class qbank_helper { /** * Get the question structure data for the given quiz or question ids. * - * @param null $quizid + * @param int $quizid ID of the quiz to load data for. * @param array $questionids - * @param bool $attempt - * @return array + * @param bool $attempt if false (default) array key is slot number, else array key is question.id. + * @return array the question data, ordered by slot number. */ public static function get_question_structure_data($quizid, $questionids = [], $attempt = false) { global $DB; @@ -245,15 +228,13 @@ class qbank_helper { LEFT JOIN {question_categories} qc ON qc.id = qbe.questioncategoryid LEFT JOIN {question} q ON q.id = qv.questionid WHERE slot.quizid = :quizid - $condition"; + $condition + ORDER BY slot.slot"; $questiondatas = $DB->get_records_sql($sql, $params); foreach ($questiondatas as $questiondata) { $questiondata->_partiallyloaded = true; } - if (!empty($questiondatas)) { - return $questiondatas; - } - return []; + return $questiondatas; } /** @@ -274,7 +255,9 @@ class qbank_helper { } } - return self::question_array_sort(array_merge($firstslotsets, $secondslotsets), 'slot'); + $data = array_merge($firstslotsets, $secondslotsets); + ksort($data); + return $data; } /** @@ -325,11 +308,11 @@ class qbank_helper { $randomquestiondata->randomincludingsubcategories = $filtercondition->includingsubcategories; $randomquestiondata->questionid = $randomloader->get_next_question_id($randomquestiondata->randomfromcategory, $randomquestiondata->randomincludingsubcategories, $tagids); - $randomquestions [] = $randomquestiondata; + $randomquestions[] = $randomquestiondata; } foreach ($randomquestions as $randomquestion) { - // Should not add if there is no question found from the ramdom question loader, maybe empty category. + // Should not add if there is no question found from the random question loader, maybe empty category. if ($randomquestion->questionid === null) { continue; } @@ -346,6 +329,8 @@ class qbank_helper { $questiondata[$question->id] = $question; } + uasort($questiondata, function (\stdClass $a, \stdClass $b): int { return ($a->slot <=> $b->slot); }); + return $questiondata; } diff --git a/mod/quiz/report/reportlib.php b/mod/quiz/report/reportlib.php index a0e04516ee2..c6f52511973 100644 --- a/mod/quiz/report/reportlib.php +++ b/mod/quiz/report/reportlib.php @@ -114,11 +114,11 @@ function quiz_report_get_significant_questions($quiz) { } $qsbyslot[$slotreport->slot] = $slotreport; } - $qsbyslot = \mod_quiz\question\bank\qbank_helper::question_array_sort($qsbyslot, 'slot'); + ksort($qsbyslot); $number = 1; foreach ($qsbyslot as $question) { $question->number = $number; - $number ++; + $number++; } return $qsbyslot; } diff --git a/mod/quiz/report/statistics/report.php b/mod/quiz/report/statistics/report.php index fb5bb8a9cc9..3492d0ae3b6 100644 --- a/mod/quiz/report/statistics/report.php +++ b/mod/quiz/report/statistics/report.php @@ -856,7 +856,8 @@ class quiz_statistics_report extends quiz_default_report { foreach ($randomquestions as $randomquestion) { $questiondata[$randomquestion->slot] = $randomquestion; } - return \mod_quiz\question\bank\qbank_helper::question_array_sort($questiondata, 'slot'); + ksort($questiondata); + return $questiondata; } /** diff --git a/mod/quiz/tests/qbank_helper_test.php b/mod/quiz/tests/qbank_helper_test.php index 4b06fc3815e..2cddfc8bb53 100644 --- a/mod/quiz/tests/qbank_helper_test.php +++ b/mod/quiz/tests/qbank_helper_test.php @@ -80,7 +80,6 @@ class qbank_helper_test extends \advanced_testcase { * @covers ::get_question_for_redo * @covers ::get_always_latest_version_question_ids * @covers ::question_load_random_questions - * @covers ::question_array_sort */ public function test_reference_records() { $this->resetAfterTest(); @@ -130,7 +129,6 @@ class qbank_helper_test extends \advanced_testcase { * * @covers ::get_question_structure * @covers ::get_question_structure_data - * @covers ::question_array_sort * @covers ::get_always_latest_version_question_ids * @covers ::question_load_random_questions */ From 5fff990e25e2150ca6a32a22f30aba29bb24fb67 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 5 Apr 2022 16:42:03 +0100 Subject: [PATCH 2/5] MDL-74255 quiz: clean up lots of stuff mainly in the tests This fixes lots of stuff like outdated or incomplete PHPdoc comments or test heler functions where the arguments don't have their types declared. A few more significant fixes, like places were a silly method was used to get a context which was readily available. --- lib/modinfolib.php | 2 +- lib/questionlib.php | 2 +- .../classes/local/structure/slot_random.php | 2 +- mod/quiz/classes/output/edit_renderer.php | 2 - mod/quiz/locallib.php | 3 +- .../responses_from_steps_walkthrough_test.php | 2 +- .../stats_from_steps_walkthrough_test.php | 2 +- .../attempt_walkthrough_from_csv_test.php | 28 ++-- .../behat/quiz_question_versions.feature | 10 +- mod/quiz/tests/external/external_test.php | 2 +- .../local_structure_slot_random_test.php | 7 +- mod/quiz/tests/qbank_helper_test.php | 14 +- .../tests/quiz_question_helper_test_trait.php | 19 ++- mod/quiz/tests/quiz_question_restore_test.php | 47 ++++--- mod/quiz/tests/structure_test.php | 133 +++++++++--------- mod/quiz/tests/tags_test.php | 2 + question/type/questiontypebase.php | 2 +- 17 files changed, 151 insertions(+), 128 deletions(-) diff --git a/lib/modinfolib.php b/lib/modinfolib.php index 1334d8e1433..f84c0fcbfac 100644 --- a/lib/modinfolib.php +++ b/lib/modinfolib.php @@ -271,7 +271,7 @@ class course_modinfo { /** * Obtains all instances of a particular module on this course. - * @param $modname Name of module (not full frankenstyle) e.g. 'label' + * @param string $modname Name of module (not full frankenstyle) e.g. 'label' * @return cm_info[] Array from instance id => cm_info for modules on this course; empty if none */ public function get_instances_of($modname) { diff --git a/lib/questionlib.php b/lib/questionlib.php index a986f7aec83..abfda93f9b7 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -2102,7 +2102,7 @@ function is_latest(string $version, string $questionbankentryid) : bool { * filter settings, theme, lang, etc.) Defaults to $PAGE->context. * @return moodle_url the URL. * @deprecated since Moodle 4.0 - * @see qbank_previewquestion\previewquestion_helper::question_preview_url() + * @see qbank_previewquestion\helper::question_preview_url() * @todo Final deprecation on Moodle 4.4 MDL-72438 */ function question_preview_url($questionid, $preferredbehaviour = null, diff --git a/mod/quiz/classes/local/structure/slot_random.php b/mod/quiz/classes/local/structure/slot_random.php index 504fda331ac..fe2abedc166 100644 --- a/mod/quiz/classes/local/structure/slot_random.php +++ b/mod/quiz/classes/local/structure/slot_random.php @@ -220,4 +220,4 @@ class slot_random { ]); $event->trigger(); } -} \ No newline at end of file +} diff --git a/mod/quiz/classes/output/edit_renderer.php b/mod/quiz/classes/output/edit_renderer.php index be22a1af3ef..eb6924bc705 100644 --- a/mod/quiz/classes/output/edit_renderer.php +++ b/mod/quiz/classes/output/edit_renderer.php @@ -28,7 +28,6 @@ defined('MOODLE_INTERNAL') || die(); use mod_quiz\question\bank\qbank_helper; use \mod_quiz\structure; use \html_writer; -use \qbank_previewquestion\helper; use renderable; /** @@ -733,7 +732,6 @@ class edit_renderer extends \plugin_renderer_base { * @return string HTML to output. */ public function question(structure $structure, int $slot, \moodle_url $pageurl) { - global $DB; // Get the data required by the question_slot template. $slotid = $structure->get_slot_id_for_slot($slot); diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index b27895799f9..12268bf5956 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -28,7 +28,6 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ - defined('MOODLE_INTERNAL') || die(); require_once($CFG->dirroot . '/mod/quiz/lib.php'); @@ -81,7 +80,7 @@ define('QUIZ_SHOWIMAGE_LARGE', 2); * * @param object $quizobj the quiz object to create an attempt for. * @param int $attemptnumber the sequence number for the attempt. - * @param object $lastattempt the previous attempt by this user, if any. Only needed + * @param stdClass|null $lastattempt the previous attempt by this user, if any. Only needed * if $attemptnumber > 1 and $quiz->attemptonlast is true. * @param int $timenow the time the attempt was started at. * @param bool $ispreview whether this new attempt is a preview. diff --git a/mod/quiz/report/responses/tests/responses_from_steps_walkthrough_test.php b/mod/quiz/report/responses/tests/responses_from_steps_walkthrough_test.php index f4b74f15e07..878bf1e076a 100644 --- a/mod/quiz/report/responses/tests/responses_from_steps_walkthrough_test.php +++ b/mod/quiz/report/responses/tests/responses_from_steps_walkthrough_test.php @@ -42,7 +42,7 @@ require_once($CFG->dirroot . '/mod/quiz/report/reportlib.php'); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class quiz_report_responses_from_steps_testcase extends mod_quiz_attempt_walkthrough_from_csv_testcase { - protected function get_full_path_of_csv_file($setname, $test) { + protected function get_full_path_of_csv_file(string $setname, string $test): string { // Overridden here so that __DIR__ points to the path of this file. return __DIR__."/fixtures/{$setname}{$test}.csv"; } diff --git a/mod/quiz/report/statistics/tests/stats_from_steps_walkthrough_test.php b/mod/quiz/report/statistics/tests/stats_from_steps_walkthrough_test.php index da2333ed157..77c987b1573 100644 --- a/mod/quiz/report/statistics/tests/stats_from_steps_walkthrough_test.php +++ b/mod/quiz/report/statistics/tests/stats_from_steps_walkthrough_test.php @@ -56,7 +56,7 @@ class quiz_report_statistics_from_steps_testcase extends mod_quiz_attempt_walkth */ protected $report; - protected function get_full_path_of_csv_file($setname, $test) { + protected function get_full_path_of_csv_file(string $setname, string $test): string { // Overridden here so that __DIR__ points to the path of this file. return __DIR__."/fixtures/{$setname}{$test}.csv"; } diff --git a/mod/quiz/tests/attempt_walkthrough_from_csv_test.php b/mod/quiz/tests/attempt_walkthrough_from_csv_test.php index 6c86ced48f8..d421f4508ad 100644 --- a/mod/quiz/tests/attempt_walkthrough_from_csv_test.php +++ b/mod/quiz/tests/attempt_walkthrough_from_csv_test.php @@ -72,6 +72,7 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { global $SITE, $DB; $this->setAdminUser(); + /** @var core_question_generator $questiongenerator */ $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); $slots = array(); $qidsbycat = array(); @@ -147,8 +148,8 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { * @param array $quizsettings Quiz overrides for this quiz. * @param array $csvdata Data loaded from csv files for this test. */ - protected function create_quiz_simulate_attempts_and_check_results($quizsettings, $csvdata) { - $this->resetAfterTest(true); + protected function create_quiz_simulate_attempts_and_check_results(array $quizsettings, array $csvdata) { + $this->resetAfterTest(); $this->create_quiz($quizsettings, $csvdata['questions']); @@ -166,7 +167,7 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { * @param string $test * @return string full path of file. */ - protected function get_full_path_of_csv_file($setname, $test) { + protected function get_full_path_of_csv_file(string $setname, string $test): string { return __DIR__."/fixtures/{$setname}{$test}.csv"; } @@ -177,7 +178,7 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { * @param string $test * @return array */ - protected function load_csv_data_file($setname, $test='') { + protected function load_csv_data_file(string $setname, string $test = ''): array { $files = array($setname => $this->get_full_path_of_csv_file($setname, $test)); return $this->dataset_from_files($files)->get_rows([$setname]); } @@ -188,7 +189,7 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { * @param array $row from csv file with field names with parts separate by '.'. * @return array the row with each part of the field name following a '.' being a separate sub array's index. */ - protected function explode_dot_separated_keys_to_make_subindexs(array $row) { + protected function explode_dot_separated_keys_to_make_subindexs(array $row): array { $parts = array(); foreach ($row as $columnkey => $value) { $newkeys = explode('.', trim($columnkey)); @@ -214,7 +215,7 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { * @return array One array element for each run of the test. Each element contains an array with the params for * test_walkthrough_from_csv. */ - public function get_data_for_walkthrough() { + public function get_data_for_walkthrough(): array { $quizzes = $this->load_csv_data_file('quizzes')['quizzes']; $datasets = array(); foreach ($quizzes as $quizsettings) { @@ -230,10 +231,10 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { } /** - * @param $steps array the step data from the csv file. + * @param array $steps the step data from the csv file. * @return array attempt no as in csv file => the id of the quiz_attempt as stored in the db. */ - protected function walkthrough_attempts($steps) { + protected function walkthrough_attempts(array $steps): array { global $DB; $attemptids = array(); foreach ($steps as $steprow) { @@ -256,7 +257,7 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { $prevattempts = quiz_get_user_attempts($this->quiz->id, $user->id, 'all', true); $attemptnumber = count($prevattempts) + 1; $timenow = time(); - $attempt = quiz_create_attempt($quizobj, $attemptnumber, false, $timenow, false, $user->id); + $attempt = quiz_create_attempt($quizobj, $attemptnumber, null, $timenow, false, $user->id); // Select variant and / or random sub question. if (!isset($step['variants'])) { $step['variants'] = array(); @@ -291,10 +292,10 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { } /** - * @param $results array the results data from the csv file. - * @param $attemptids array attempt no as in csv file => the id of the quiz_attempt as stored in the db. + * @param array $results the results data from the csv file. + * @param array $attemptids attempt no as in csv file => the id of the quiz_attempt as stored in the db. */ - protected function check_attempts_results($results, $attemptids) { + protected function check_attempts_results(array $results, array $attemptids) { foreach ($results as $resultrow) { $result = $this->explode_dot_separated_keys_to_make_subindexs($resultrow); // Re-load quiz attempt data. @@ -308,9 +309,8 @@ class mod_quiz_attempt_walkthrough_from_csv_testcase extends advanced_testcase { * * @param array $result row of data read from csv file. * @param quiz_attempt $attemptobj the attempt object loaded from db. - * @throws coding_exception */ - protected function check_attempt_results($result, $attemptobj) { + protected function check_attempt_results(array $result, quiz_attempt $attemptobj) { foreach ($result as $fieldname => $value) { if ($value === '!NULL!') { $value = null; diff --git a/mod/quiz/tests/behat/quiz_question_versions.feature b/mod/quiz/tests/behat/quiz_question_versions.feature index 061d38f6f71..fcc80577c63 100644 --- a/mod/quiz/tests/behat/quiz_question_versions.feature +++ b/mod/quiz/tests/behat/quiz_question_versions.feature @@ -7,13 +7,13 @@ Feature: Quiz question versioning Background: Given the following "courses" exist: | fullname | shortname | category | groupmode | - | Course 1 | C1 | 0 | 1 | + | Course 1 | C1 | 0 | 1 | And the following "users" exist: - | username | firstname | lastname | email | - | teacher1 | Teacher | 1 | teacher1@example.com | + | username | firstname | lastname | email | + | teacher | Teacher | 1 | teacher@example.com | And the following "course enrolments" exist: - | user | course | role | - | teacher1 | C1 | editingteacher | + | user | course | role | + | teacher | C1 | editingteacher | And the following "question categories" exist: | contextlevel | reference | name | | Course | C1 | Test questions | diff --git a/mod/quiz/tests/external/external_test.php b/mod/quiz/tests/external/external_test.php index 966e4e111fc..7df091cf3ec 100644 --- a/mod/quiz/tests/external/external_test.php +++ b/mod/quiz/tests/external/external_test.php @@ -319,7 +319,7 @@ class external_test extends externallib_advanced_testcase { // We only see a limited set of fields. $this->assertCount(4, $result['quizzes'][0]); $this->assertEquals($quiz2->id, $result['quizzes'][0]['id']); - $this->assertEquals($quiz2->coursemodule, $result['quizzes'][0]['coursemodule']); + $this->assertEquals($quiz2->cmid, $result['quizzes'][0]['coursemodule']); $this->assertEquals($quiz2->course, $result['quizzes'][0]['course']); $this->assertEquals($quiz2->name, $result['quizzes'][0]['name']); $this->assertEquals($quiz2->course, $result['quizzes'][0]['course']); diff --git a/mod/quiz/tests/local_structure_slot_random_test.php b/mod/quiz/tests/local_structure_slot_random_test.php index 576eba0a60f..b979885e937 100644 --- a/mod/quiz/tests/local_structure_slot_random_test.php +++ b/mod/quiz/tests/local_structure_slot_random_test.php @@ -25,6 +25,8 @@ defined('MOODLE_INTERNAL') || die(); +use mod_quiz\question\bank\qbank_helper; + /** * Class mod_quiz_local_structure_slot_random_test * Class for tests related to the {@link \mod_quiz\local\structure\slot_random} class. @@ -352,7 +354,7 @@ class mod_quiz_local_structure_slot_random_test extends advanced_testcase { } public function test_insert() { - global $SITE, $DB; + global $SITE; $this->resetAfterTest(); $this->setAdminUser(); @@ -360,6 +362,7 @@ class mod_quiz_local_structure_slot_random_test extends advanced_testcase { // Create a quiz. $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); $quiz = $quizgenerator->create_instance(array('course' => $SITE->id, 'questionsperpage' => 3, 'grade' => 100.0)); + $quizcontext = context_module::instance($quiz->cmid); // Create a question category in the system context. $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); @@ -400,7 +403,7 @@ class mod_quiz_local_structure_slot_random_test extends advanced_testcase { $randomslotdata = new stdClass(); $randomslotdata->quizid = $quiz->id; $randomslotdata->maxmark = 1; - $randomslotdata->usingcontextid = context_module::instance($quiz->cmid)->id; + $randomslotdata->usingcontextid = $quizcontext->id; $randomslotdata->questionscontextid = $category->contextid; // Insert the random question to the quiz. diff --git a/mod/quiz/tests/qbank_helper_test.php b/mod/quiz/tests/qbank_helper_test.php index 2cddfc8bb53..7491e907817 100644 --- a/mod/quiz/tests/qbank_helper_test.php +++ b/mod/quiz/tests/qbank_helper_test.php @@ -36,6 +36,11 @@ require_once(__DIR__ . '/quiz_question_helper_test_trait.php'); class qbank_helper_test extends \advanced_testcase { use \quiz_question_helper_test_trait; + /** + * @var \stdClass test student user. + */ + protected $student; + /** * Called before every test. */ @@ -79,29 +84,33 @@ class qbank_helper_test extends \advanced_testcase { * @covers ::get_version_options * @covers ::get_question_for_redo * @covers ::get_always_latest_version_question_ids - * @covers ::question_load_random_questions */ public function test_reference_records() { $this->resetAfterTest(); + $quiz = $this->create_test_quiz($this->course); // Test for questions from a different context. $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); + // Create a couple of questions. + /** @var \core_question_generator $questiongenerator */ $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $questiongenerator->create_question_category(['contextid' => $context->id]); $numq = $questiongenerator->create_question('essay', null, ['category' => $cat->id, 'name' => 'This is the first version']); + // Create two version. $questiongenerator->update_question($numq, null, ['name' => 'This is the second version']); $questiongenerator->update_question($numq, null, ['name' => 'This is the third version']); quiz_add_quiz_question($numq->id, $quiz); + // Create the quiz object. $quizobj = \quiz::create($quiz->id); $quizobj->preload_questions(); $quizobj->load_questions(); $questions = $quizobj->get_questions(); $question = reset($questions); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $slots = $structure->get_slots(); $slot = reset($slots); $this->assertEquals(3, count(qbank_helper::get_version_options($question->id))); @@ -119,6 +128,7 @@ class qbank_helper_test extends \advanced_testcase { $questions = $quizobj->get_questions(); $question = reset($questions); $this->assertEquals($question->id, qbank_helper::get_question_for_redo($slot->id)); + // Test always latest version question ids. $latestquestionids = qbank_helper::get_always_latest_version_question_ids($quiz->id); $this->assertEquals($question->id, reset($latestquestionids)); diff --git a/mod/quiz/tests/quiz_question_helper_test_trait.php b/mod/quiz/tests/quiz_question_helper_test_trait.php index 8237def5321..1d315d1d25c 100644 --- a/mod/quiz/tests/quiz_question_helper_test_trait.php +++ b/mod/quiz/tests/quiz_question_helper_test_trait.php @@ -45,16 +45,15 @@ trait quiz_question_helper_test_trait { */ protected function create_test_quiz(\stdClass $course): \stdClass { + /** @var mod_quiz_generator $quizgenerator */ $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); - $quiz = $quizgenerator->create_instance([ + return $quizgenerator->create_instance([ 'course' => $course->id, 'questionsperpage' => 0, 'grade' => 100.0, 'sumgrades' => 2, ]); - $quiz->coursemodule = $quiz->cmid; - return $quiz; } /** @@ -64,7 +63,7 @@ trait quiz_question_helper_test_trait { * @param \stdClass $quiz * @param array $override */ - protected function add_regular_questions($questiongenerator, \stdClass $quiz, $override = null): void { + protected function add_two_regular_questions($questiongenerator, \stdClass $quiz, $override = null): void { // Create a couple of questions. $cat = $questiongenerator->create_question_category($override); @@ -86,7 +85,7 @@ trait quiz_question_helper_test_trait { * @param \stdClass $quiz * @param array $override */ - protected function add_random_questions($questiongenerator, \stdClass $quiz, $override = []): void { + protected function add_one_random_question($questiongenerator, \stdClass $quiz, $override = []): void { // Create a random question. $cat = $questiongenerator->create_question_category($override); $questiongenerator->create_question('truefalse', null, array('category' => $cat->id)); @@ -106,18 +105,18 @@ trait quiz_question_helper_test_trait { $this->setUser($user); $starttime = time(); - $quizobj = \quiz::create($quiz->id, $user->id); + $quizobj = quiz::create($quiz->id, $user->id); - $quba = \question_engine::make_questions_usage_by_activity('mod_quiz', $quizobj->get_context()); + $quba = question_engine::make_questions_usage_by_activity('mod_quiz', $quizobj->get_context()); $quba->set_preferred_behaviour($quizobj->get_quiz()->preferredbehaviour); // Start the attempt. - $attempt = quiz_create_attempt($quizobj, $attemptnumber, false, $starttime, false, $user->id); + $attempt = quiz_create_attempt($quizobj, $attemptnumber, null, $starttime, false, $user->id); quiz_start_new_attempt($quizobj, $quba, $attempt, $attemptnumber, $starttime); quiz_attempt_save_started($quizobj, $quba, $attempt); // Finish the attempt. - $attemptobj = \quiz_attempt::create($attempt->id); + $attemptobj = quiz_attempt::create($attempt->id); $attemptobj->process_finish($starttime, false); $this->setUser(); @@ -140,7 +139,7 @@ trait quiz_question_helper_test_trait { $backupid = 'test-question-backup-restore'; - $bc = new backup_controller(backup::TYPE_1ACTIVITY, $quiz->coursemodule, backup::FORMAT_MOODLE, + $bc = new backup_controller(backup::TYPE_1ACTIVITY, $quiz->cmid, backup::FORMAT_MOODLE, backup::INTERACTIVE_NO, backup::MODE_GENERAL, $user->id); $bc->execute_plan(); diff --git a/mod/quiz/tests/quiz_question_restore_test.php b/mod/quiz/tests/quiz_question_restore_test.php index decf6867ad4..257c2cb627c 100644 --- a/mod/quiz/tests/quiz_question_restore_test.php +++ b/mod/quiz/tests/quiz_question_restore_test.php @@ -38,6 +38,11 @@ require_once($CFG->dirroot . '/mod/quiz/locallib.php'); class quiz_question_restore_test extends \advanced_testcase { use \quiz_question_helper_test_trait; + /** + * @var \stdClass test student user. + */ + protected $student; + /** * Called before every test. */ @@ -81,17 +86,22 @@ class quiz_question_restore_test extends \advanced_testcase { * @covers ::get_question_structure */ public function test_quiz_restore_in_a_different_course_using_quiz_question_bank() { - global $DB; $this->resetAfterTest(); + + // Create the test quiz. $quiz = $this->create_test_quiz($this->course); // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); + $quizcontext = \context_module::instance($quiz->cmid); $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $this->add_regular_questions($questiongenerator, $quiz, ['contextid' => $context->id]); - $this->add_random_questions($questiongenerator, $quiz, ['contextid' => $context->id]); + $this->add_two_regular_questions($questiongenerator, $quiz, ['contextid' => $quizcontext->id]); + $this->add_one_random_question($questiongenerator, $quiz, ['contextid' => $quizcontext->id]); + + // Make the backup. $backupid = $this->backup_quiz($quiz, $this->user); + // Delete the current course to make sure there is no data. delete_course($this->course, false); + // Check if the questions and associated datas are deleted properly. $this->assertEquals(0, count(\mod_quiz\question\bank\qbank_helper::get_question_structure($quiz->id))); $newcourse = $this->getDataGenerator()->create_course(); @@ -103,11 +113,11 @@ class quiz_question_restore_test extends \advanced_testcase { /** * Count the questions for the context. * - * @param int $context + * @param int $contextid * @param string $extracondition - * @return int + * @return int the number of questions. */ - protected function question_count($context, $extracondition = ''): int { + protected function question_count(int $contextid, string $extracondition = ''): int { global $DB; return $DB->count_records_sql( "SELECT COUNT(q.id) @@ -116,7 +126,7 @@ class quiz_question_restore_test extends \advanced_testcase { JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid JOIN {question_categories} qc on qc.id = qbe.questioncategoryid WHERE qc.contextid = ? - $extracondition", [$context]); + $extracondition", [$contextid]); } /** @@ -130,8 +140,8 @@ class quiz_question_restore_test extends \advanced_testcase { // Test for questions from a different context. $context = \context_course::instance($this->course->id); $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $this->add_regular_questions($questiongenerator, $quiz, ['contextid' => $context->id]); - $this->add_random_questions($questiongenerator, $quiz, ['contextid' => $context->id]); + $this->add_two_regular_questions($questiongenerator, $quiz, ['contextid' => $context->id]); + $this->add_one_random_question($questiongenerator, $quiz, ['contextid' => $context->id]); // Count the questions in course context. $this->assertEquals(7, $this->question_count($context->id)); $newquiz = $this->duplicate_quiz($this->course, $quiz); @@ -150,10 +160,10 @@ class quiz_question_restore_test extends \advanced_testcase { $this->resetAfterTest(); $quiz = $this->create_test_quiz($this->course); // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); + $context = \context_module::instance($quiz->cmid); $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $this->add_regular_questions($questiongenerator, $quiz, ['contextid' => $context->id]); - $this->add_random_questions($questiongenerator, $quiz, ['contextid' => $context->id]); + $this->add_two_regular_questions($questiongenerator, $quiz, ['contextid' => $context->id]); + $this->add_one_random_question($questiongenerator, $quiz, ['contextid' => $context->id]); // Count the questions in course context. $this->assertEquals(7, $this->question_count($context->id)); $newquiz = $this->duplicate_quiz($this->course, $quiz); @@ -220,7 +230,7 @@ class quiz_question_restore_test extends \advanced_testcase { $modinfo = get_fast_modinfo($newcourseid); $quiz = array_values($modinfo->get_instances_of('quiz'))[0]; $quizobj = \quiz::create($quiz->instance); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); // Are the correct slots returned? $slots = $structure->get_slots(); @@ -232,8 +242,7 @@ class quiz_question_restore_test extends \advanced_testcase { $this->assertCount(2, $questions); // Count the questions in quiz qbank. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quizobj->get_quizid(), $newcourseid)->id); - $this->assertEquals(2, $this->question_count($context->id)); + $this->assertEquals(2, $this->question_count($quizobj->get_context()->id)); } /** @@ -244,6 +253,7 @@ class quiz_question_restore_test extends \advanced_testcase { public function test_pre_4_quiz_restore_for_random_questions() { global $USER, $DB; $this->resetAfterTest(); + $backupid = 'abc'; $backuppath = make_backup_temp_directory($backupid); get_file_packer('application/vnd.moodle.backup')->extract_to_pathname( @@ -263,7 +273,7 @@ class quiz_question_restore_test extends \advanced_testcase { $modinfo = get_fast_modinfo($newcourseid); $quiz = array_values($modinfo->get_instances_of('quiz'))[0]; $quizobj = \quiz::create($quiz->instance); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); // Are the correct slots returned? $slots = $structure->get_slots(); @@ -280,8 +290,7 @@ class quiz_question_restore_test extends \advanced_testcase { "AND q.qtype <> 'random'")); // Count the questions in quiz qbank. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quizobj->get_quizid(), $newcourseid)->id); - $this->assertEquals(0, $this->question_count($context->id)); + $this->assertEquals(0, $this->question_count($quizobj->get_context()->id)); } /** diff --git a/mod/quiz/tests/structure_test.php b/mod/quiz/tests/structure_test.php index 57fd22d9a69..00767a55a7f 100644 --- a/mod/quiz/tests/structure_test.php +++ b/mod/quiz/tests/structure_test.php @@ -23,6 +23,9 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use mod_quiz\question\bank\qbank_helper; +use mod_quiz\structure; + defined('MOODLE_INTERNAL') || die(); global $CFG; @@ -108,7 +111,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { } $quizobj = new quiz($quiz, $cm, $course); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); if (isset($headings[1])) { list($heading, $shuffle) = $this->parse_section_name($headings[1]); $sections = $structure->get_sections(); @@ -130,9 +133,9 @@ class mod_quiz_structure_testcase extends advanced_testcase { /** * Verify that the given layout matches that expected. * @param array $expectedlayout as for $layout in {@link create_test_quiz()}. - * @param \mod_quiz\structure $structure the structure to test. + * @param structure $structure the structure to test. */ - protected function assert_quiz_layout($expectedlayout, \mod_quiz\structure $structure) { + protected function assert_quiz_layout($expectedlayout, structure $structure) { $sections = $structure->get_sections(); $slot = 1; @@ -193,7 +196,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF1', 1, 'truefalse'), array('TF2', 1, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); // Are the correct slots returned? $slots = $structure->get_slots(); @@ -204,7 +207,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { $quizobj = $this->create_test_quiz(array( array('TF1', 1, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $sections = $structure->get_sections(); $this->assertCount(1, $sections); @@ -222,7 +225,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 2*', array('TF2', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $sections = $structure->get_sections(); $this->assertCount(2, $sections); @@ -245,13 +248,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 2', array('TF2', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $sections = $structure->get_sections(); $section = end($sections); $structure->remove_section_heading($section->id); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -264,7 +267,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 1', array('TF1', 1, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $sections = $structure->get_sections(); $section = reset($sections); @@ -279,13 +282,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 1, 'truefalse'), array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, $idmoveafter, '1'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF2', 1, 'truefalse'), @@ -299,13 +302,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 1, 'truefalse'), array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(2)->slotid; $structure->move_slot($idtomove, $idmoveafter, '2'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF2', 2, 'truefalse'), @@ -318,13 +321,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF1', 1, 'truefalse'), array('TF2', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, $idmoveafter, '1'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF2', 1, 'truefalse'), @@ -338,13 +341,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading', array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(2)->slotid; $structure->move_slot($idtomove, $idmoveafter, '2'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), 'Heading', @@ -360,13 +363,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 2, 'truefalse'), array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, $idmoveafter, '1'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF2', 1, 'truefalse'), @@ -380,13 +383,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 1, 'truefalse'), array('TF3', 1, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(3)->slotid; $structure->move_slot($idtomove, $idmoveafter, '1'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF3', 1, 'truefalse'), @@ -400,13 +403,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 2, 'truefalse'), array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(3)->slotid; $idmoveafter = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, $idmoveafter, '1'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF3', 1, 'truefalse'), @@ -420,13 +423,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 2, 'truefalse'), array('TF3', 3, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(3)->slotid; $structure->move_slot($idtomove, $idmoveafter, '3'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF3', 2, 'truefalse'), @@ -440,7 +443,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 2, 'truefalse'), array('TF3', 3, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(3)->slotid; $idmoveafter = $structure->get_question_in_slot(2)->slotid; @@ -454,7 +457,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 2, 'truefalse'), array('TF3', 3, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(1)->slotid; $idmoveafter = $structure->get_question_in_slot(2)->slotid; @@ -470,13 +473,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 2', array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(1)->slotid; $idmoveafter = $structure->get_question_in_slot(2)->slotid; $structure->move_slot($idtomove, $idmoveafter, '1'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF2', 1, 'truefalse'), @@ -494,13 +497,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 2', array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(3)->slotid; $structure->move_slot($idtomove, $idmoveafter, '2'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -518,12 +521,12 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF2', 2, 'truefalse'), array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(3)->slotid; $structure->move_slot($idtomove, 0, '1'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF3', 1, 'truefalse'), @@ -541,13 +544,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 2', array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(2)->slotid; $idmoveafter = $structure->get_question_in_slot(2)->slotid; $structure->move_slot($idtomove, $idmoveafter, '2'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -563,12 +566,12 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF1', 1, 'truefalse'), array('TF2', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, 0, '2'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -582,12 +585,12 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF1', 1, 'truefalse'), array('TF2', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, 0, '1'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -601,12 +604,12 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF1', 1, 'truefalse'), array('TF2', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, 0, ''); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -624,13 +627,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF3', 3, 'truefalse'), array('TF4', 3, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(3)->slotid; $idmoveafter = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, $idmoveafter, '2'); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -652,13 +655,13 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF3', 3, 'truefalse'), array('TF4', 3, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $idtomove = $structure->get_question_in_slot(4)->slotid; $idmoveafter = $structure->get_question_in_slot(1)->slotid; $structure->move_slot($idtomove, $idmoveafter, 1); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -677,11 +680,11 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 2', array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $structure->remove_slot(2); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), 'Heading 2', @@ -702,7 +705,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $questiongenerator->create_question_category(); quiz_add_random_questions($quizobj->get_quiz(), 1, $cat->id, 1, false); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $sql = 'SELECT qsr.* FROM {question_set_references} qsr JOIN {quiz_slots} qs ON qs.id = qsr.itemid @@ -713,7 +716,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { $structure->remove_slot(2); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), ), $structure); @@ -731,7 +734,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 2', array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $structure->remove_slot(1); $this->expectException(coding_exception::class); @@ -745,7 +748,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 2', array('TF3', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->expectException(coding_exception::class); $structure->remove_slot(3); @@ -756,7 +759,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { 'Heading 1', array('TF1', 1, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $structure->remove_slot(1); @@ -766,7 +769,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('name' => 'TF2', 'category' => $cat->id)); quiz_add_quiz_question($q->id, $quizobj->get_quiz(), 0); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', @@ -788,7 +791,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { quiz_add_quiz_question($q->id, $quizobj->get_quiz(), 1); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF3', 1, 'truefalse'), @@ -814,7 +817,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { quiz_add_quiz_question($q->id, $quizobj->get_quiz(), 1); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( 'Heading 1', array('TF1', 1, 'truefalse'), @@ -840,7 +843,7 @@ class mod_quiz_structure_testcase extends advanced_testcase { quiz_add_quiz_question($q->id, $quizobj->get_quiz(), 0); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), 'Heading 2', @@ -854,12 +857,12 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF1', 1, 'truefalse'), array('TF2', 2, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $slotid = $structure->get_question_in_slot(2)->slotid; $slots = $structure->update_page_break($slotid, \mod_quiz\repaginate::LINK); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF2', 1, 'truefalse'), @@ -871,12 +874,12 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF1', 1, 'truefalse'), array('TF2', 1, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $slotid = $structure->get_question_in_slot(2)->slotid; $slots = $structure->update_page_break($slotid, \mod_quiz\repaginate::UNLINK); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assert_quiz_layout(array( array('TF1', 1, 'truefalse'), array('TF2', 2, 'truefalse'), @@ -888,21 +891,21 @@ class mod_quiz_structure_testcase extends advanced_testcase { array('TF1', 1, 'truefalse'), array('TF2', 1, 'truefalse'), )); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); // Test adding a dependency. $slotid = $structure->get_slot_id_for_slot(2); $structure->update_question_dependency($slotid, true); // Having called update page break, we need to reload $structure. - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assertEquals(1, $structure->is_question_dependent_on_previous_slot(2)); // Test removing a dependency. $structure->update_question_dependency($slotid, false); // Having called update page break, we need to reload $structure. - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $this->assertEquals(0, $structure->is_question_dependent_on_previous_slot(2)); } @@ -920,11 +923,11 @@ class mod_quiz_structure_testcase extends advanced_testcase { $noneditingteacher = $generator->create_and_enrol($course, 'teacher'); $this->setUser($teacher); - $structure = \mod_quiz\structure::create_for_quiz($quiz); + $structure = structure::create_for_quiz($quiz); $this->assertTrue($structure->can_add_random_questions()); $this->setUser($noneditingteacher); - $structure = \mod_quiz\structure::create_for_quiz($quiz); + $structure = structure::create_for_quiz($quiz); $this->assertFalse($structure->can_add_random_questions()); } } diff --git a/mod/quiz/tests/tags_test.php b/mod/quiz/tests/tags_test.php index 0209076b0a7..ba565cd047f 100644 --- a/mod/quiz/tests/tags_test.php +++ b/mod/quiz/tests/tags_test.php @@ -14,6 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . +use mod_quiz\question\bank\qbank_helper; + /** * Test the restore of random question tags. * diff --git a/question/type/questiontypebase.php b/question/type/questiontypebase.php index ce2d79e5d44..c6c6bff8e11 100644 --- a/question/type/questiontypebase.php +++ b/question/type/questiontypebase.php @@ -903,7 +903,7 @@ class question_type { * specific information (it is passed by reference). */ public function get_question_options($question) { - global $CFG, $DB, $OUTPUT; + global $DB, $OUTPUT; if (!isset($question->options)) { $question->options = new stdClass(); From 839cccead4127be770db3668a124b99e57d48395 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 5 Apr 2022 16:44:17 +0100 Subject: [PATCH 3/5] MDL-74255 quiz: handle draft question status correctly The main issue to fix is that questions vesions which should not have been used (that is, hidden or draft versions) were getting offered as an option and acutally being used. As part of this I was able to substantially un-tangle mod_quiz\question\bank\qbank_helper, which previously was a mass of functions calling other functions in a complicated way. Hopefully, it is now a bit easer to understand, and perhaps less buggy. --- .../tests/quiz_restore_decode_links_test.php | 7 +- mod/quiz/attemptlib.php | 56 ++- .../backup/moodle2/restore_quiz_stepslib.php | 2 +- mod/quiz/classes/output/edit_renderer.php | 38 +- .../classes/question/bank/qbank_helper.php | 384 ++++++------------ mod/quiz/classes/structure.php | 106 ++--- mod/quiz/locallib.php | 62 ++- .../behat/quiz_question_versions.feature | 99 +++-- .../local_structure_slot_random_test.php | 12 +- mod/quiz/tests/qbank_helper_test.php | 114 ++---- mod/quiz/tests/quiz_question_restore_test.php | 78 ++-- mod/quiz/tests/structure_test.php | 33 ++ mod/quiz/tests/tags_test.php | 6 +- mod/quiz/upgrade.txt | 2 +- 14 files changed, 441 insertions(+), 558 deletions(-) diff --git a/backup/tests/quiz_restore_decode_links_test.php b/backup/tests/quiz_restore_decode_links_test.php index 3aa9a3f8e82..85c82ffd44e 100644 --- a/backup/tests/quiz_restore_decode_links_test.php +++ b/backup/tests/quiz_restore_decode_links_test.php @@ -80,10 +80,13 @@ class restore_quiz_decode_testcase extends \core_privacy\tests\provider_testcase $newcm = duplicate_module($course, get_fast_modinfo($course)->get_cm($quiz->cmid)); - $quizquestions = \mod_quiz\question\bank\qbank_helper::get_question_structure_data($newcm->instance); + $quizquestions = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $newcm->instance, context_module::instance($newcm->id)); $questionids = []; foreach ($quizquestions as $quizquestion) { - $questionids[] = $quizquestion->id; + if ($quizquestion->questionid) { + $questionids[] = $quizquestion->questionid; + } } list($condition, $param) = $DB->get_in_or_equal($questionids, SQL_PARAMS_NAMED, 'questionid'); $condition = 'WHERE qa.question ' . $condition; diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index febc31505b2..22e03edccdb 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -25,9 +25,10 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ - defined('MOODLE_INTERNAL') || die(); +use mod_quiz\question\bank\qbank_helper; + /** * Class for quiz exceptions. Just saves a couple of arguments on the @@ -79,7 +80,11 @@ class quiz { /** @var context the quiz context. */ protected $context; - /** @var stdClass[] of questions augmented with slot information. */ + /** + * @var stdClass[] of questions augmented with slot information. For non-random + * questions, the array key is question id. For random quesions it is 's' . $slotid. + * probalby best to use ->questionid field of the object instead. + */ protected $questions = null; /** @var stdClass[] of quiz_section rows. */ protected $sections = null; @@ -145,35 +150,33 @@ class quiz { * Load just basic information about all the questions in this quiz. */ public function preload_questions() { - $specificquestionids = \mod_quiz\question\bank\qbank_helper::get_specific_version_question_ids($this->quiz->id); - $latestquestionids = \mod_quiz\question\bank\qbank_helper::get_always_latest_version_question_ids($this->quiz->id); - $questionids = array_merge($specificquestionids, $latestquestionids); - $questiondata = []; - if (!empty($questionids)) { - $questiondata = \mod_quiz\question\bank\qbank_helper::get_question_structure_data($this->quiz->id, $questionids, true); + $slots = qbank_helper::get_question_structure($this->quiz->id, $this->context); + $this->questions = []; + foreach ($slots as $slot) { + $this->questions[$slot->questionid] = $slot; } - $allquestiondata = \mod_quiz\question\bank\qbank_helper::question_load_random_questions( - $this->quiz->id, $questiondata); - $this->questions = $allquestiondata; } /** * Fully load some or all of the questions for this quiz. You must call * {@link preload_questions()} first. * - * @param array|null $questionids question ids of the questions to load. null for all. + * @param array|null $deprecated no longer supported (it was not used). */ - public function load_questions($questionids = null) { + public function load_questions($deprecated = null) { + if ($deprecated !== null) { + debugging('The argument to quiz::load_questions is no longer supported. ' . + 'All questions are always loaded.', DEBUG_DEVELOPER); + } if ($this->questions === null) { throw new coding_exception('You must call preload_questions before calling load_questions.'); } - if (is_null($questionids)) { - $questionids = array_keys($this->questions); - } - $questionstoprocess = array(); - foreach ($questionids as $id) { - if (array_key_exists($id, $this->questions)) { - $questionstoprocess[$id] = $this->questions[$id]; + + $questionstoprocess = []; + foreach ($this->questions as $question) { + if (is_number($question->questionid)) { + $question->id = $question->questionid; + $questionstoprocess[$question->questionid] = $question; } } get_question_options($questionstoprocess); @@ -540,8 +543,17 @@ class quiz { $qcategories = array(); foreach ($this->get_questions() as $questiondata) { - if (!in_array($questiondata->qtype, $questiontypes)) { - $questiontypes[] = $questiondata->qtype; + if ($questiondata->qtype == 'random' and $includepotential) { + if (!isset($qcategories[$questiondata->category])) { + $qcategories[$questiondata->category] = false; + } + if ($questiondata->includingsubcategories) { + $qcategories[$questiondata->category] = true; + } + } else { + if (!in_array($questiondata->qtype, $questiontypes)) { + $questiontypes[] = $questiondata->qtype; + } } } diff --git a/mod/quiz/backup/moodle2/restore_quiz_stepslib.php b/mod/quiz/backup/moodle2/restore_quiz_stepslib.php index 1f1b588766f..1638e97fab5 100644 --- a/mod/quiz/backup/moodle2/restore_quiz_stepslib.php +++ b/mod/quiz/backup/moodle2/restore_quiz_stepslib.php @@ -353,7 +353,7 @@ class restore_quiz_activity_structure_step extends restore_questions_activity_st $questionreference->questionarea = 'slot'; $questionreference->itemid = $data->id; $questionreference->questionbankentryid = $question->questionbankentryid; - $questionreference->version = $question->version; + $questionreference->version = null; // Default to Always latest. $DB->insert_record('question_references', $questionreference); } } diff --git a/mod/quiz/classes/output/edit_renderer.php b/mod/quiz/classes/output/edit_renderer.php index eb6924bc705..0eac3937dec 100644 --- a/mod/quiz/classes/output/edit_renderer.php +++ b/mod/quiz/classes/output/edit_renderer.php @@ -756,7 +756,7 @@ class edit_renderer extends \plugin_renderer_base { $data['versionoptions'] = []; if ($structure->get_slot_by_number($slot)->qtype !== 'random') { $data['versionselection'] = true; - $data['versionoption'] = qbank_helper::get_question_version_info($structure->get_question_in_slot($slot)->id, $slotid); + $data['versionoption'] = $structure->get_version_choices_for_slot($slot); $this->page->requires->js_call_amd('mod_quiz/question_slot', 'init', [$slotid]); } @@ -819,8 +819,9 @@ class edit_renderer extends \plugin_renderer_base { $qtype = $structure->get_question_type_for_slot($slot); $questionicons = ''; if ($qtype !== 'random') { - $questionicons .= $this->question_preview_icon($structure->get_quiz(), $structure->get_question_in_slot($slot), - null, null, $qtype); + $questionicons .= $this->question_preview_icon($structure->get_quiz(), + $structure->get_question_in_slot($slot), + null, null, $qtype); } if ($structure->can_be_edited()) { $questionicons .= $this->question_remove_icon($structure, $slot, $pageurl); @@ -860,13 +861,19 @@ class edit_renderer extends \plugin_renderer_base { * Render the preview icon. * * @param \stdClass $quiz the quiz settings from the database. - * @param \stdClass $question data from the question and quiz_slots tables. + * @param \stdClass $questiondata which question to preview. + * If ->questionid is set, that is used instead of ->id. * @param bool $label if true, show the preview question label after the icon * @param int $variant which question variant to preview (optional). - * @param string $qtype the type of question * @return string HTML to output. */ - public function question_preview_icon($quiz, $question, $label = null, $variant = null, $qtype = null) { + public function question_preview_icon($quiz, $questiondata, $label = null, $variant = null) { + $question = clone($questiondata); + if (isset($question->questionid)) { + + $question->id = $question->questionid; + } + $url = quiz_question_preview_url($quiz, $question, $variant); // Do we want a label? @@ -992,7 +999,7 @@ class edit_renderer extends \plugin_renderer_base { $question = $structure->get_question_in_slot($slot); $editurl = new \moodle_url('/question/bank/editquestion/question.php', array( 'returnurl' => $pageurl->out_as_local_url(), - 'cmid' => $structure->get_cmid(), 'id' => $question->id)); + 'cmid' => $structure->get_cmid(), 'id' => $question->questionid)); $instancename = quiz_question_tostring($question); @@ -1034,9 +1041,6 @@ class edit_renderer extends \plugin_renderer_base { $temp->questiontext = ''; $instancename = quiz_question_tostring($temp); - $setreference = qbank_helper::get_random_question_data_from_slot($slot->id); - $filtercondition = json_decode($setreference->filtercondition); - $configuretitle = get_string('configurerandomquestion', 'quiz'); $qtype = \question_bank::get_qtype($question->qtype, false); $namestr = $qtype->local_name(); @@ -1044,15 +1048,15 @@ class edit_renderer extends \plugin_renderer_base { 'class' => 'icon activityicon', 'alt' => ' ', 'role' => 'presentation')); $editicon = $this->pix_icon('t/edit', $configuretitle, 'moodle', array('title' => '')); - $qbankurlparams = array( - 'cmid' => $structure->get_cmid(), - 'cat' => $filtercondition->questioncategoryid . ',' . $setreference->questionscontextid, - 'recurse' => !empty($setreference->questionscontextid) - ); + $qbankurlparams = [ + 'cmid' => $structure->get_cmid(), + 'cat' => $slot->category . ',' . $slot->contextid, + 'recurse' => $slot->randomrecurse, + ]; $slottags = []; - if (isset($filtercondition->tags)) { - $slottags = $filtercondition->tags; + if (isset($slot->randomtags)) { + $slottags = $slot->randomtags; } foreach ($slottags as $index => $slottag) { $slottag = explode(',', $slottag); diff --git a/mod/quiz/classes/question/bank/qbank_helper.php b/mod/quiz/classes/question/bank/qbank_helper.php index 2b05627be48..8b0b355f039 100644 --- a/mod/quiz/classes/question/bank/qbank_helper.php +++ b/mod/quiz/classes/question/bank/qbank_helper.php @@ -16,6 +16,8 @@ namespace mod_quiz\question\bank; +use core_question\local\bank\question_version_status; + defined('MOODLE_INTERNAL') || die(); require_once($CFG->dirroot . '/mod/quiz/accessmanager.php'); @@ -38,7 +40,7 @@ class qbank_helper { * @param int $slotid * @return bool */ - public static function is_random($slotid): bool { + public static function is_random(int $slotid): bool { global $DB; $params = [ 'itemid' => $slotid, @@ -49,23 +51,31 @@ class qbank_helper { } /** - * Get the version options for the question. + * Get the available versions of a question where one of the version has the given question id. * - * @param int $questionid - * @return array + * @param int $questionid id of a question. + * @return \stdClass[] other versions of this question. Each object has fields versionid, + * version and questionid. Array is returned most recent version first. */ - public static function get_version_options($questionid): array { + public static function get_version_options(int $questionid): array { global $DB; - $sql = "SELECT qv.id AS versionid, qv.version, qv.questionid - FROM {question_versions} qv - WHERE qv.questionbankentryid = (SELECT DISTINCT qbe.id - FROM {question_bank_entries} qbe - JOIN {question_versions} qv ON qbe.id = qv.questionbankentryid - JOIN {question} q ON qv.questionid = q.id - WHERE q.id = ?) - ORDER BY qv.version DESC"; - return $DB->get_records_sql($sql, [$questionid]); + return $DB->get_records_sql(" + SELECT allversions.id AS versionid, + allversions.version, + allversions.questionid + + FROM {question_versions} allversions + + WHERE allversions.questionbankentryid = ( + SELECT givenversion.questionbankentryid + FROM {question_versions} givenversion + WHERE givenversion.questionid = ? + ) + AND allversions.status <> ? + + ORDER BY allversions.version DESC + ", [$questionid, question_version_status::QUESTION_STATUS_DRAFT]); } /** @@ -74,7 +84,7 @@ class qbank_helper { * @param int $slotid * @return mixed */ - public static function get_question_for_redo($slotid) { + public static function get_question_for_redo(int $slotid) { global $DB; $params = [ 'itemid' => $slotid, @@ -102,236 +112,120 @@ class qbank_helper { } /** - * Get random question object from the slot id. + * Get the information about which questions should be used to create a quiz attempt. * - * @param int $slotid - * @return false|mixed|\stdClass + * Each element in the returned array is indexed by slot.slot (slot number) an each object hass: + * - All the field of the slot table. + * - contextid for where the question(s) come from. + * - category id for where the questions come from. + * - For non-random questions, All the fields of the question table (but id is in questionid). + * Also question version and question bankentryid. + * - For random questions, filtercondition, which is also unpacked into category, randomrecurse, + * randomtags, and note that these also have a ->name set and ->qtype set to 'random'. + * + * @param int $quizid the id of the quiz to load the data for. + * @param \context $quizcontext + * @return array indexed by slot, with information about the content of each slot. */ - public static function get_random_question_data_from_slot($slotid) { + public static function get_question_structure(int $quizid, \context $quizcontext) { global $DB; - $params = [ - 'itemid' => $slotid, - 'component' => 'mod_quiz', - 'questionarea' => 'slot' - ]; - return $DB->get_record('question_set_references', $params); - } - /** - * Get the question ids for specific question version. - * - * @param int $quizid - * @return array - */ - public static function get_specific_version_question_ids($quizid) { - global $DB; - $questionids = []; - $sql = 'SELECT qv.questionid - FROM {quiz_slots} qs - JOIN {question_references} qr ON qr.itemid = qs.id - JOIN {question_versions} qv ON qv.questionbankentryid = qr.questionbankentryid - AND qv.version = qr.version - WHERE qr.version IS NOT NULL - AND qs.quizid = ? - AND qr.component = ? - AND qr.questionarea = ?'; - $questions = $DB->get_records_sql($sql, [$quizid, 'mod_quiz', 'slot']); - foreach ($questions as $question) { - $questionids [] = $question->questionid; - } - return $questionids; - } - - /** - * Get the question ids for always latest options. - * - * @param int $quizid - * @return array - */ - public static function get_always_latest_version_question_ids($quizid) { - global $DB; - $questionids = []; - $sql = 'SELECT qr.questionbankentryid as entry - FROM {quiz_slots} qs - JOIN {question_references} qr ON qr.itemid = qs.id - WHERE qr.version IS NULL - AND qs.quizid = ? - AND qr.component = ? - AND qr.questionarea = ?'; - $entryids = $DB->get_records_sql($sql, [$quizid, 'mod_quiz', 'slot']); - $questionentries = []; - foreach ($entryids as $entryid) { - $questionentries [] = $entryid->entry; - } - if (empty($questionentries)) { - return $questionids; - } - list($questionidcondition, $params) = $DB->get_in_or_equal($questionentries); - $extracondition = 'AND qv.questionbankentryid ' . $questionidcondition; - $questionsql = "SELECT q.id - FROM {question} q - JOIN {question_versions} qv ON qv.questionid = q.id - WHERE qv.version = (SELECT MAX(v.version) - FROM {question_versions} v - JOIN {question_bank_entries} be - ON be.id = v.questionbankentryid - WHERE be.id = qv.questionbankentryid) - $extracondition"; - $questions = $DB->get_records_sql($questionsql, $params); - foreach ($questions as $question) { - $questionids [] = $question->id; - } - return $questionids; - } - - /** - * Get the question structure data for the given quiz or question ids. - * - * @param int $quizid ID of the quiz to load data for. - * @param array $questionids - * @param bool $attempt if false (default) array key is slot number, else array key is question.id. - * @return array the question data, ordered by slot number. - */ - public static function get_question_structure_data($quizid, $questionids = [], $attempt = false) { - global $DB; - $params = ['quizid' => $quizid]; - $condition = ''; - $joinon = 'AND qr.version = qv.version'; - if (!empty($questionids)) { - list($condition, $param) = $DB->get_in_or_equal($questionids, SQL_PARAMS_NAMED, 'questionid'); - $condition = 'AND q.id ' . $condition; - $joinon = ''; - $params = array_merge($params, $param); - } - if ($attempt) { - $selectstart = 'q.*, slot.id AS slotid, slot.slot,'; - } else { - $selectstart = 'slot.slot, slot.id AS slotid, q.*,'; - } - $sql = "SELECT $selectstart - q.id AS questionid, - q.name, - q.qtype, - q.length, + // Load all the data about each slot. + $slotdata = $DB->get_records_sql(" + SELECT slot.slot, + slot.id AS slotid, slot.page, slot.maxmark, slot.requireprevious, - qc.id as category, - qc.contextid,qv.status, - qv.id as versionid, + qsr.filtercondition, + qv.status, + qv.id AS versionid, qv.version, - qv.questionbankentryid + qr.version AS requestedversion, + qv.questionbankentryid, + q.id AS questionid, + q.*, + qc.id AS category, + COALESCE(qc.contextid, qsr.questionscontextid) AS contextid + FROM {quiz_slots} slot - LEFT JOIN {question_references} qr ON qr.itemid = slot.id AND qr.component = 'mod_quiz' AND qr.questionarea = 'slot' + + -- case where a particular question has been added to the quiz. + LEFT JOIN {question_references} qr ON qr.usingcontextid = :quizcontextid AND qr.component = 'mod_quiz' + AND qr.questionarea = 'slot' AND qr.itemid = slot.id LEFT JOIN {question_bank_entries} qbe ON qbe.id = qr.questionbankentryid - LEFT JOIN {question_versions} qv ON qv.questionbankentryid = qbe.id $joinon + LEFT JOIN {question_versions} qv ON qv.questionbankentryid = qbe.id + -- Either specified version, or latest ready version. + AND qv.version = COALESCE(qr.version, ( + SELECT MAX(version) + FROM {question_versions} + WHERE questionbankentryid = qbe.id AND status <> :draft + )) LEFT JOIN {question_categories} qc ON qc.id = qbe.questioncategoryid LEFT JOIN {question} q ON q.id = qv.questionid + + -- Case where a random question has been added. + LEFT JOIN {question_set_references} qsr ON qsr.usingcontextid = :quizcontextid2 AND qsr.component = 'mod_quiz' + AND qsr.questionarea = 'slot' AND qsr.itemid = slot.id + WHERE slot.quizid = :quizid - $condition - ORDER BY slot.slot"; - $questiondatas = $DB->get_records_sql($sql, $params); - foreach ($questiondatas as $questiondata) { - $questiondata->_partiallyloaded = true; - } - return $questiondatas; - } + ORDER BY slot.slot + ", ['draft' => question_version_status::QUESTION_STATUS_DRAFT, + 'quizcontextid' => $quizcontext->id, 'quizcontextid2' => $quizcontext->id, + 'quizid' => $quizid]); - /** - * Get question structure. - * - * @param int $quizid - * @return array - */ - public static function get_question_structure($quizid) { - $firstslotsets = self::get_question_structure_data($quizid); - $latestquestionids = self::get_always_latest_version_question_ids($quizid); - $secondslotsets = self::get_question_structure_data($quizid, $latestquestionids); - foreach ($firstslotsets as $key => $firstslotset) { - foreach ($secondslotsets as $secondslotset) { - if ($firstslotset->slotid === $secondslotset->slotid) { - unset($firstslotsets[$key]); - } - } - } + // Uppack the random info from question_set_reference. + foreach ($slotdata as $slot) { + // Ensure the right id is the id. + $slot->id = $slot->slotid; - $data = array_merge($firstslotsets, $secondslotsets); - ksort($data); - return $data; - } - - /** - * Load random questions. - * - * @param int $quizid - * @param array $questiondata - * @return array - */ - public static function question_load_random_questions($quizid, $questiondata) { - global $DB, $USER; - $sql = 'SELECT slot.id AS slotid, - slot.maxmark, - slot.slot, - slot.page, - qsr.filtercondition - FROM {question_set_references} qsr - JOIN {quiz_slots} slot ON slot.id = qsr.itemid - WHERE slot.quizid = ? - AND qsr.component = ? - AND qsr.questionarea = ?'; - $randomquestiondatas = $DB->get_records_sql($sql, [$quizid, 'mod_quiz', 'slot']); - - $randomquestions = []; - // Questions already added. - $usedquestionids = []; - foreach ($questiondata as $question) { - if (isset($usedquestions[$question->id])) { - $usedquestionids[$question->id] += 1; + if ($slot->filtercondition) { + // Unpack the information about a random question. + $filtercondition = json_decode($slot->filtercondition); + $slot->questionid = 's' . $slot->id; // Sometimes this is used as an array key, so needs to be unique. + $slot->category = $filtercondition->questioncategoryid; + $slot->randomrecurse = $filtercondition->includingsubcategories; + $slot->randomtags = isset($filtercondition->tags) ? (array) $filtercondition->tags : []; + $slot->qtype = 'random'; + $slot->name = get_string('random', 'quiz'); + $slot->length = 1; + } else if ($slot->qtype === null) { + // This question must have gone missing. Put in a placeholder. + $slot->questionid = 's' . $slot->id; // Sometimes this is used as an array key, so needs to be unique. + $slot->category = 0; + $slot->qtype = 'missingtype'; + $slot->name = get_string('missingquestion', 'quiz'); + $slot->maxmark = 0; + $slot->questiontext = ' '; + $slot->questiontextformat = FORMAT_HTML; + $slot->length = 1; + } else if (!\question_bank::qtype_exists($slot->qtype)) { + // Question of unknown type found in the database. Set to placeholder question types instead. + $slot->qtype = 'missingtype'; } else { - $usedquestionids[$question->id] = 1; + $slot->_partiallyloaded = 1; } } - // Usages for this user's previous quiz attempts. - $qubaids = new \mod_quiz\question\qubaids_for_users_attempts($quizid, $USER->id); - $randomloader = new \core_question\local\bank\random_question_loader($qubaids, $usedquestionids); - foreach ($randomquestiondatas as $randomquestiondata) { - $filtercondition = json_decode($randomquestiondata->filtercondition); - $tagids = []; - if (isset($filtercondition->tags)) { - foreach ($filtercondition->tags as $tag) { - $tagstring = explode(',', $tag); - $tagids [] = $tagstring[0]; - } - } - $randomquestiondata->randomfromcategory = $filtercondition->questioncategoryid; - $randomquestiondata->randomincludingsubcategories = $filtercondition->includingsubcategories; - $randomquestiondata->questionid = $randomloader->get_next_question_id($randomquestiondata->randomfromcategory, - $randomquestiondata->randomincludingsubcategories, $tagids); - $randomquestions[] = $randomquestiondata; + return $slotdata; + } + + /** + * Get this list of random selection tag ids from one of the slots returned by get_question_structure. + * + * @param \stdClass $slotdata one of the array elements returend by get_question_structure. + * @return array list of tag ids. + */ + public static function get_tag_ids_for_slot(\stdClass $slotdata): array { + if (empty($slot->randomtags)) { + return []; } - foreach ($randomquestions as $randomquestion) { - // Should not add if there is no question found from the random question loader, maybe empty category. - if ($randomquestion->questionid === null) { - continue; - } - $question = new \stdClass(); - $question->slotid = $randomquestion->slotid; - $question->maxmark = $randomquestion->maxmark; - $question->slot = $randomquestion->slot; - $question->page = $randomquestion->page; - $qdatas = question_preload_questions($randomquestion->questionid); - $qdatas = reset($qdatas); - foreach ($qdatas as $key => $qdata) { - $question->$key = $qdata; - } - $questiondata[$question->id] = $question; + $tagids = []; + foreach ($slotdata->randomtags as $tag) { + $tagids[] = $tag->id; } - - uasort($questiondata, function (\stdClass $a, \stdClass $b): int { return ($a->slot <=> $b->slot); }); - - return $questiondata; + return $tagids; } /** @@ -366,48 +260,4 @@ class qbank_helper { } return $newqusetionid; } - - /** - * Get the version information for a question to show in the version selection dropdown. - * - * @param int $questionid - * @param int $slotid - * @return array - */ - public static function get_question_version_info($questionid, $slotid): array { - global $DB; - $versiondata = []; - $versionsoptions = self::get_version_options($questionid); - $latestversion = reset($versionsoptions); - // Object for using the latest version. - $alwaysuselatest = new \stdClass(); - $alwaysuselatest->versionid = 0; - $alwaysuselatest->version = 0; - $alwaysuselatest->versionvalue = get_string('alwayslatest', 'quiz'); - array_unshift($versionsoptions, $alwaysuselatest); - $referencedata = $DB->get_record('question_references', - ['itemid' => $slotid, 'component' => 'mod_quiz', 'questionarea' => 'slot']); - if (!isset($referencedata->version) || ($referencedata->version === null)) { - $currentversion = 0; - } else { - $currentversion = $referencedata->version; - } - - foreach ($versionsoptions as $versionsoption) { - $versionsoption->selected = false; - if ($versionsoption->version === $currentversion) { - $versionsoption->selected = true; - } - if (!isset($versionsoption->versionvalue)) { - if ($versionsoption->version === $latestversion->version) { - $versionsoption->versionvalue = get_string('questionversionlatest', 'quiz', $versionsoption->version); - } else { - $versionsoption->versionvalue = get_string('questionversion', 'quiz', $versionsoption->version); - } - } - - $versiondata[] = $versionsoption; - } - return $versiondata; - } } diff --git a/mod/quiz/classes/structure.php b/mod/quiz/classes/structure.php index 87a9a3a5918..a5f7cde24d4 100644 --- a/mod/quiz/classes/structure.php +++ b/mod/quiz/classes/structure.php @@ -88,7 +88,7 @@ class structure { public static function create_for_quiz($quizobj) { $structure = self::create(); $structure->quizobj = $quizobj; - $structure->populate_structure($quizobj->get_quiz()); + $structure->populate_structure(); return $structure; } @@ -612,83 +612,28 @@ class structure { /** * Set up this class with the structure for a given quiz. - * @param \stdClass $quiz the quiz settings. */ - public function populate_structure($quiz) { + protected function populate_structure() { global $DB; - $slots = qbank_helper::get_question_structure($quiz->id); - - $slots = $this->populate_missing_questions($slots); + $slots = qbank_helper::get_question_structure($this->quizobj->get_quizid(), $this->quizobj->get_context()); $this->questions = []; $this->slotsinorder = []; foreach ($slots as $slotdata) { $this->questions[$slotdata->questionid] = $slotdata; - $slot = new \stdClass(); - $slot->id = $slotdata->slotid; - $slot->name = $slotdata->name; - $slot->slot = $slotdata->slot; - $slot->quizid = $quiz->id; - $slot->page = $slotdata->page; - $slot->questionid = $slotdata->questionid; - $slot->maxmark = $slotdata->maxmark; - $slot->requireprevious = $slotdata->requireprevious; - $slot->qtype = $slotdata->qtype; - $slot->length = $slotdata->length; - $slot->category = $slotdata->category; - $slot->questionbankentryid = $slotdata->questionbankentryid ?? null; - $slot->version = $slotdata->version ?? null; - + $slot = clone($slotdata); + $slot->quizid = $this->quizobj->get_quizid(); $this->slotsinorder[$slot->slot] = $slot; } // Get quiz sections in ascending order of the firstslot. - $this->sections = $DB->get_records('quiz_sections', array('quizid' => $quiz->id), 'firstslot ASC'); + $this->sections = $DB->get_records('quiz_sections', ['quizid' => $this->quizobj->get_quizid()], 'firstslot'); $this->populate_slots_with_sections(); $this->populate_question_numbers(); } - /** - * Used by populate. Make up fake data for any missing questions. - * @param \stdClass[] $slots the data about the slots and questions in the quiz. - * @return \stdClass[] updated $slots array. - */ - protected function populate_missing_questions($slots) { - global $DB; - // Address missing/random question types. - foreach ($slots as $slot) { - if ($slot->qtype === null) { - // Check if the question is random. - if ($setreference = $DB->get_record('question_set_references', - ['itemid' => $slot->slotid, 'component' => 'mod_quiz', 'questionarea' => 'slot'])) { - $filtercondition = json_decode($setreference->filtercondition); - $slot->id = $slot->slotid; - $slot->category = $filtercondition->questioncategoryid; - $slot->qtype = 'random'; - $slot->name = get_string('random', 'quiz'); - $slot->length = 1; - } else { - // If the questiontype is missing change the question type. - $slot->id = $slot->questionid; - $slot->category = 0; - $slot->qtype = 'missingtype'; - $slot->name = get_string('missingquestion', 'quiz'); - $slot->maxmark = 0; - $slot->requireprevious = 0; - $slot->questiontext = ' '; - $slot->questiontextformat = FORMAT_HTML; - $slot->length = 1; - } - } else if (!\question_bank::qtype_exists($slot->qtype)) { - $slot->qtype = 'missingtype'; - } - } - - return $slots; - } - /** * Fill in the section ids for each slot. */ @@ -721,6 +666,45 @@ class structure { } } + /** + * Get the version options to show on the Questions page for a particular question. + * + * @param int $slotnumber which slot to get the choices for. + * @return \stdClass[] other versions of this question. Each object has fields versionid, + * version and selected. Array is returned most recent version first. + */ + public function get_version_choices_for_slot(int $slotnumber): array { + $slot = $this->get_slot_by_number($slotnumber); + + // Get all the versions which exist. + $versions = qbank_helper::get_version_options($slot->questionid); + $latestversion = reset($versions); + + // Format the choices for display. + $versionoptions = []; + foreach ($versions as $version) { + $version->selected = $version->version === $slot->requestedversion; + + if ($version->version === $latestversion->version) { + $version->versionvalue = get_string('questionversionlatest', 'quiz', $version->version); + } else { + $version->versionvalue = get_string('questionversion', 'quiz', $version->version); + } + + $versionoptions[] = $version; + } + + // Make a choice for 'Always latest'. + $alwaysuselatest = new \stdClass(); + $alwaysuselatest->versionid = 0; + $alwaysuselatest->version = 0; + $alwaysuselatest->versionvalue = get_string('alwayslatest', 'quiz'); + $alwaysuselatest->selected = $slot->requestedversion === null; + array_unshift($versionoptions, $alwaysuselatest); + + return $versionoptions; + } + /** * Move a slot from its current location to a new location. * diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index 12268bf5956..b3f6de1720a 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -39,6 +39,7 @@ require_once($CFG->libdir . '/completionlib.php'); require_once($CFG->libdir . '/filelib.php'); require_once($CFG->libdir . '/questionlib.php'); +use mod_quiz\question\bank\qbank_helper; /** * @var int We show the countdown timer if there is less than this amount of time left before the @@ -170,12 +171,10 @@ function quiz_start_new_attempt($quizobj, $quba, $attempt, $attemptnumber, $time // First load all the non-random questions. $randomfound = false; - $randomtestfound = false; $slot = 0; $questions = array(); $maxmark = array(); $page = array(); - $questiondatarandom = []; foreach ($quizobj->get_questions() as $questiondata) { $slot += 1; $maxmark[$slot] = $questiondata->maxmark; @@ -184,34 +183,55 @@ function quiz_start_new_attempt($quizobj, $quba, $attempt, $attemptnumber, $time $randomfound = true; continue; } - - // Intended for testing purposes only. - foreach ($questionids as $key => $questionid) { - if ($questionid !== (int)$questiondata->id && $slot === $key) { - $randomtestfound = true; - $questiondatarandom[$key] = $questiondata; - continue 2; - } - } - if (!$quizobj->get_quiz()->shuffleanswers) { $questiondata->options->shuffleanswers = false; } $questions[$slot] = question_bank::make_question($questiondata); } - // Then find a question throw an error as something horribly wrong might have happened. + // Then find a question to go in place of each random question. if ($randomfound) { - throw new coding_exception( - 'Using "random" questions directly in an attempt is deprecated. Please use question_set_references table instead.' - ); - } + $slot = 0; + $usedquestionids = array(); + foreach ($questions as $question) { + if ($question->id && isset($usedquestions[$question->id])) { + $usedquestionids[$question->id] += 1; + } else { + $usedquestionids[$question->id] = 1; + } + } + $randomloader = new \core_question\local\bank\random_question_loader($qubaids, $usedquestionids); + + foreach ($quizobj->get_questions() as $questiondata) { + $slot += 1; + if ($questiondata->qtype != 'random') { + continue; + } + + $tagids = qbank_helper::get_tag_ids_for_slot($questiondata); - // Then find a question to go in place of each random question. Intended for testing purposes only. - if ($randomtestfound) { - foreach ($questiondatarandom as $slot => $questiondata) { // Deal with fixed random choices for testing. - $questions[$slot] = question_bank::load_question($questionids[$slot], $quizobj->get_quiz()->shuffleanswers); + if (isset($questionids[$quba->next_slot_number()])) { + if ($randomloader->is_question_available($questiondata->category, + (bool) $questiondata->questiontext, $questionids[$quba->next_slot_number()], $tagids)) { + $questions[$slot] = question_bank::load_question( + $questionids[$quba->next_slot_number()], $quizobj->get_quiz()->shuffleanswers); + continue; + } else { + throw new coding_exception('Forced question id not available.'); + } + } + + // Normal case, pick one at random. + $questionid = $randomloader->get_next_question_id($questiondata->category, + $questiondata->randomrecurse, $tagids); + if ($questionid === null) { + throw new moodle_exception('notenoughrandomquestions', 'quiz', + $quizobj->view_url(), $questiondata); + } + + $questions[$slot] = question_bank::load_question($questionid, + $quizobj->get_quiz()->shuffleanswers); } } diff --git a/mod/quiz/tests/behat/quiz_question_versions.feature b/mod/quiz/tests/behat/quiz_question_versions.feature index fcc80577c63..189602e01fc 100644 --- a/mod/quiz/tests/behat/quiz_question_versions.feature +++ b/mod/quiz/tests/behat/quiz_question_versions.feature @@ -21,82 +21,93 @@ Feature: Quiz question versioning | activity | name | course | idnumber | | quiz | Quiz 1 | C1 | quiz1 | And the following "questions" exist: - | questioncategory | qtype | name | questiontext | answer 1 | - | Test questions | truefalse | First question | Answer the first question | True | + | questioncategory | qtype | name | questiontext | + | Test questions | truefalse | First question | Answer the first question | + | Test questions | truefalse | Other question | Answer the first question | And quiz "Quiz 1" contains the following questions: | question | page | | First question | 1 | - And I log in as "teacher1" - And I am on "Course 1" course homepage @javascript - Scenario: Approriate question version should be displayed when not edited - When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher1" - And I should see "First question" + Scenario: Appropriate question version should be displayed when not edited + When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher" + Then I should see "First question" And I should see "Answer the first question" - And I should see "v1 (latest)" + And the field "version" matches value "Always latest" + And "v1 (latest)" "option" should exist in the "version" "select" # We check that the corresponding version is the appropriate one in preview And I click on "Preview question" "link" And I switch to "questionpreview" window And I should see "Version 1 (latest)" And I should see "Answer the first question" - And I press "Display options" - And I set the following fields to these values: - | id_feedback | Not shown | - | id_generalfeedback | Not shown | - | id_rightanswer | Shown | - And I press "id_saveupdate" - And I click on "finish" "button" - And I should see "The correct answer is 'True'." + And I click on "Submit and finish" "button" + And I should see "You should have selected true." @javascript Scenario: Approriate question version should be displayed when edited - When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher1" + When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher" And I click on "Edit question First question" "link" # We edit the question with new informations to generate a second version And I set the following fields to these values: - | id_name | Second question | - | id_questiontext | This is the second question text | - | id_correctanswer | False | + | Question name | First question (v2) | + | Question text | Answer the new first question | + | Correct answer | False | And I press "id_submitbutton" - And I set the field "version" to "v2" - And I should see "Second question" - And I should see "This is the second question text" + And the field "version" matches value "Always latest" + And "v1" "option" should exist in the "version" "select" + And I set the field "version" to "v2 (latest)" + Then I should see "First question (v2)" + And I should see "Answer the new first question" And I click on "Preview question" "link" And I switch to "questionpreview" window # We check that the corresponding version is the appropriate one in preview - # We also check that the new informations are properly displayed + # We also check that the new information is properly displayed And I should see "Version 2 (latest)" - And I should see "This is the second question text" - And I press "Display options" + And I should see "Answer the new first question" + + @javascript + Scenario: Appropriate question version displayed when later draft version exists + # Edit the question in the question bank to add a new draft version. + Given I am on the "First question" "core_question > edit" page logged in as teacher And I set the following fields to these values: - | id_feedback | Not shown | - | id_generalfeedback | Not shown | - | id_rightanswer | Shown | - And I press "id_saveupdate" - And I click on "finish" "button" - Then I should see "The correct answer is 'False'." + | Question name | First question (v2) | + | Question text | Answer the new first question | + | Correct answer | False | + | Question status | Draft | + And I press "id_submitbutton" + When I am on the "Quiz 1" "mod_quiz > Edit" page + Then I should see "First question" + And I should see "Answer the first question" + And the field "version" matches value "Always latest" + And "v1 (latest)" "option" should exist in the "version" "select" + And "v2" "option" should not exist in the "version" "select" + And "v2 (latest)" "option" should not exist in the "version" "select" + And I am on the "Quiz 1" "mod_quiz > View" page + And I press "Preview quiz" + And I should see "Answer the first question" @javascript Scenario: Creating a new question should have always latest in the version selection - When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher1" - And I click on "Add" "link" + When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher" + # Change the version of the existing question, to ensure it does not match later. + And I set the field "version" to "v1 (latest)" + And I open the "Page 1" add to quiz menu And I follow "a new question" And I set the field "item_qtype_essay" to "1" And I press "submitbutton" - Then I should see "Adding an Essay question" - And I set the field "Question name" to "Essay 01 new" - And I set the field "Question text" to "Please write 200 words about Essay 01" + And I set the following fields to these values: + | Question name | New essay | + | Question text | Write 200 words about quizzes. | And I press "id_submitbutton" - And I should see "Essay 01 new" on quiz page "1" - And I should see "Always latest" on quiz page "1" + And I should see "New essay" on quiz page "1" + And the field "version" in the "New essay" "list_item" matches value "Always latest" @javascript Scenario: Adding a question from question bank should have always latest in the version selection - When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher1" - And I click on "Add" "link" + When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher" + And I open the "Page 1" add to quiz menu And I follow "from question bank" - And I set the field with xpath "//input[@type='checkbox' and @id='qbheadercheckbox']" to "1" + And I click on "Select" "checkbox" in the "Other question" "table_row" And I press "Add selected questions to the quiz" - And I should see "First question" on quiz page "1" - And I should see "Always latest" on quiz page "1" + Then I should see "Other question" on quiz page "1" + And the field "version" in the "Other question" "list_item" matches value "Always latest" diff --git a/mod/quiz/tests/local_structure_slot_random_test.php b/mod/quiz/tests/local_structure_slot_random_test.php index b979885e937..baddd24c616 100644 --- a/mod/quiz/tests/local_structure_slot_random_test.php +++ b/mod/quiz/tests/local_structure_slot_random_test.php @@ -412,15 +412,13 @@ class mod_quiz_local_structure_slot_random_test extends advanced_testcase { $randomslot->set_filter_condition($filtercondition); $randomslot->insert(1); // Put the question on the first page of the quiz. - // Get the random question's quiz_slot. It is at the first slot. - $quizslot = $DB->get_record('quiz_slots', array('quizid' => $quiz->id, 'slot' => 1)); - // Get the random question's tags from quiz_slot_tags. It is at the first slot. - $setreference = \mod_quiz\question\bank\qbank_helper::get_random_question_data_from_slot($quizslot->id); + $slots = qbank_helper::get_question_structure($quiz->id, $quizcontext); + $quizslot = reset($slots); - $this->assertEquals($category->id, json_decode($setreference->filtercondition)->questioncategoryid); - $this->assertEquals(1, json_decode($setreference->filtercondition)->includingsubcategories); + $this->assertEquals($category->id, $quizslot->category); + $this->assertEquals(1, $quizslot->randomrecurse); $this->assertEquals(1, $quizslot->maxmark); - $tagspropery = (array)json_decode($setreference->filtercondition)->tags; + $tagspropery = $quizslot->randomtags; $this->assertCount(2, $tagspropery); $this->assertEqualsCanonicalizing( diff --git a/mod/quiz/tests/qbank_helper_test.php b/mod/quiz/tests/qbank_helper_test.php index 7491e907817..9216057a8fb 100644 --- a/mod/quiz/tests/qbank_helper_test.php +++ b/mod/quiz/tests/qbank_helper_test.php @@ -65,17 +65,14 @@ class qbank_helper_test extends \advanced_testcase { // Test for questions from a different context. $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $this->add_random_questions($questiongenerator, $quiz, ['contextid' => $context->id]); + $this->add_one_random_question($questiongenerator, $quiz, ['contextid' => $context->id]); // Create the quiz object. $quizobj = \quiz::create($quiz->id); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $structure = structure::create_for_quiz($quizobj); $slots = $structure->get_slots(); foreach ($slots as $slot) { $this->assertEquals(true, qbank_helper::is_random($slot->id)); - // Test random data for slot. - $this->assertEquals($slot->id, qbank_helper::get_random_question_data_from_slot($slot->id)->itemid); } - } /** @@ -83,7 +80,6 @@ class qbank_helper_test extends \advanced_testcase { * * @covers ::get_version_options * @covers ::get_question_for_redo - * @covers ::get_always_latest_version_question_ids */ public function test_reference_records() { $this->resetAfterTest(); @@ -119,8 +115,10 @@ class qbank_helper_test extends \advanced_testcase { $questions = $quizobj->get_questions(); $question = reset($questions); $this->assertEquals($question->id, qbank_helper::get_question_for_redo($slot->id)); + // Create another version. $questiongenerator->update_question($numq, null, ['name' => 'This is the latest version']); + // Change to always latest. submit_question_version::execute($slot->id, 0); $quizobj->preload_questions(); @@ -128,108 +126,46 @@ class qbank_helper_test extends \advanced_testcase { $questions = $quizobj->get_questions(); $question = reset($questions); $this->assertEquals($question->id, qbank_helper::get_question_for_redo($slot->id)); - - // Test always latest version question ids. - $latestquestionids = qbank_helper::get_always_latest_version_question_ids($quiz->id); - $this->assertEquals($question->id, reset($latestquestionids)); } /** * Test question structure data. * * @covers ::get_question_structure - * @covers ::get_question_structure_data * @covers ::get_always_latest_version_question_ids - * @covers ::question_load_random_questions */ public function test_get_question_structure() { $this->resetAfterTest(); + + // Create a quiz. $quiz = $this->create_test_quiz($this->course); - // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); - // Create a couple of questions. + $quizcontext = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); + + // Create a question in the quiz question bank. + /** @var \core_question_generator $questiongenerator */ $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $cat = $questiongenerator->create_question_category(['contextid' => $context->id]); - $numq = $questiongenerator->create_question('essay', null, + $cat = $questiongenerator->create_question_category(['contextid' => $quizcontext->id]); + $q = $questiongenerator->create_question('essay', null, ['category' => $cat->id, 'name' => 'This is the first version']); - // Create two version. - $questiongenerator->update_question($numq, null, ['name' => 'This is the second version']); - $questiongenerator->update_question($numq, null, ['name' => 'This is the third version']); - quiz_add_quiz_question($numq->id, $quiz); - // Create the quiz object. + + // Edit it to create a second and third version. + $questiongenerator->update_question($q, null, ['name' => 'This is the second version']); + $finalq = $questiongenerator->update_question($q, null, ['name' => 'This is the third version']); + + // Add the question to the quiz. + quiz_add_quiz_question($q->id, $quiz); + + // Load the quiz object and check. $quizobj = \quiz::create($quiz->id); $quizobj->preload_questions(); $quizobj->load_questions(); $questions = $quizobj->get_questions(); $question = reset($questions); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); + $this->assertEquals($finalq->id, $question->id); + + $structure = structure::create_for_quiz($quizobj); $slots = $structure->get_slots(); $slot = reset($slots); - $structuredatas = qbank_helper::get_question_structure($quiz->id); - $structuredata = reset($structuredatas); - $this->assertEquals($structuredata->slotid, $slot->id); - $this->assertEquals($structuredata->id, $question->id); + $this->assertEquals($finalq->id, $slot->questionid); } - - /** - * Test to get the version information for a question to show in the version selection dropdown. - * - * @covers ::get_question_version_info - */ - public function test_get_question_version_info() { - $this->resetAfterTest(); - $quiz = $this->create_test_quiz($this->course); - // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); - // Create a couple of questions. - $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $cat = $questiongenerator->create_question_category(['contextid' => $context->id]); - $numq = $questiongenerator->create_question('essay', null, - ['category' => $cat->id, 'name' => 'This is the first version']); - // Create two version. - $questiongenerator->update_question($numq, null, ['name' => 'This is the second version']); - $questiongenerator->update_question($numq, null, ['name' => 'This is the third version']); - quiz_add_quiz_question($numq->id, $quiz); - // Create the quiz object. - $quizobj = \quiz::create($quiz->id); - $quizobj->preload_questions(); - $quizobj->load_questions(); - $questions = $quizobj->get_questions(); - $question = reset($questions); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); - $slots = $structure->get_slots(); - $slot = reset($slots); - $versiondata = qbank_helper::get_question_version_info($question->id, $slot->id); - $this->assertEquals(4, count($versiondata)); - $this->assertEquals('Always latest', $versiondata[0]->versionvalue); - $this->assertEquals('v3 (latest)', $versiondata[1]->versionvalue); - $this->assertEquals('v1', $versiondata[3]->versionvalue); - } - - /** - * Test get the question ids for specific question version. - * - * @covers ::get_specific_version_question_ids - */ - public function test_get_specific_version_question_ids() { - global $DB; - $this->resetAfterTest(); - $quiz = $this->create_test_quiz($this->course); - // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); - // Create a couple of questions. - $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $cat = $questiongenerator->create_question_category(['contextid' => $context->id]); - $numq = $questiongenerator->create_question('essay', null, - ['category' => $cat->id, 'name' => 'This is the first version']); - // Create two version. - $questiongenerator->update_question($numq, null, ['name' => 'This is the second version']); - $questiongenerator->update_question($numq, null, ['name' => 'This is the third version']); - quiz_add_quiz_question($numq->id, $quiz); - submit_question_version::execute($DB->get_field('quiz_slots', 'id', ['quizid' => $quiz->id, 'slot' => 1]), 3); - $specificversionquestionid = qbank_helper::get_specific_version_question_ids($quiz->id); - $specificversionquestionid = reset($specificversionquestionid); - $this->assertEquals($numq->id, $specificversionquestionid); - } - } diff --git a/mod/quiz/tests/quiz_question_restore_test.php b/mod/quiz/tests/quiz_question_restore_test.php index 257c2cb627c..1758749b8cd 100644 --- a/mod/quiz/tests/quiz_question_restore_test.php +++ b/mod/quiz/tests/quiz_question_restore_test.php @@ -61,23 +61,37 @@ class quiz_question_restore_test extends \advanced_testcase { * @covers ::get_question_structure */ public function test_quiz_restore_in_a_different_course_using_course_question_bank() { - global $DB; $this->resetAfterTest(); + + // Create the test quiz. $quiz = $this->create_test_quiz($this->course); + $oldquizcontext = \context_module::instance($quiz->cmid); // Test for questions from a different context. - $context = \context_course::instance($this->course->id); + $coursecontext = \context_course::instance($this->course->id); $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $this->add_regular_questions($questiongenerator, $quiz, ['contextid' => $context->id]); - $this->add_random_questions($questiongenerator, $quiz, ['contextid' => $context->id]); + $this->add_two_regular_questions($questiongenerator, $quiz, ['contextid' => $coursecontext->id]); + $this->add_one_random_question($questiongenerator, $quiz, ['contextid' => $coursecontext->id]); + + // Make the backup. $backupid = $this->backup_quiz($quiz, $this->user); + // Delete the current course to make sure there is no data. delete_course($this->course, false); - // Check if the questions and associated datas are deleted properly. - $this->assertEquals(0, count(\mod_quiz\question\bank\qbank_helper::get_question_structure($quiz->id))); + + // Check if the questions and associated data are deleted properly. + $this->assertEquals(0, count(\mod_quiz\question\bank\qbank_helper::get_question_structure( + $quiz->id, $oldquizcontext))); + + /// Restore the course. $newcourse = $this->getDataGenerator()->create_course(); $this->restore_quiz($backupid, $newcourse, $this->user); - $module = $DB->get_record('quiz', ['course' => $newcourse->id]); - $this->assertEquals(3, count(\mod_quiz\question\bank\qbank_helper::get_question_structure($module->id))); + + // Verify. + $modules = get_fast_modinfo($newcourse->id)->get_instances_of('quiz'); + $module = reset($modules); + $questions = \mod_quiz\question\bank\qbank_helper::get_question_structure( + $module->instance, $module->context); + $this->assertCount(3, $questions); } /** @@ -103,11 +117,18 @@ class quiz_question_restore_test extends \advanced_testcase { delete_course($this->course, false); // Check if the questions and associated datas are deleted properly. - $this->assertEquals(0, count(\mod_quiz\question\bank\qbank_helper::get_question_structure($quiz->id))); + $this->assertEquals(0, count(\mod_quiz\question\bank\qbank_helper::get_question_structure( + $quiz->id, $quizcontext))); + + /// Restore the course. $newcourse = $this->getDataGenerator()->create_course(); $this->restore_quiz($backupid, $newcourse, $this->user); - $module = $DB->get_record('quiz', ['course' => $newcourse->id]); - $this->assertEquals(3, count(\mod_quiz\question\bank\qbank_helper::get_question_structure($module->id))); + + // Verify. + $modules = get_fast_modinfo($newcourse->id)->get_instances_of('quiz'); + $module = reset($modules); + $this->assertEquals(3, count(\mod_quiz\question\bank\qbank_helper::get_question_structure( + $module->instance, $module->context))); } /** @@ -179,28 +200,37 @@ class quiz_question_restore_test extends \advanced_testcase { * @covers ::get_question_structure */ public function test_quiz_restore_with_attempts() { - global $DB; $this->resetAfterTest(); + + // Create a quiz. $quiz = $this->create_test_quiz($this->course); - // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); + $quizcontext = \context_module::instance($quiz->cmid); $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $this->add_regular_questions($questiongenerator, $quiz, ['contextid' => $context->id]); - $this->add_random_questions($questiongenerator, $quiz, ['contextid' => $context->id]); - list($quizobj, $quba, $attemptobj) = $this->attempt_quiz($quiz, $this->student); - $userattempts = quiz_get_user_attempts($quiz->id, $this->student->id); - // Count the attempts for this quiz. + $this->add_two_regular_questions($questiongenerator, $quiz, ['contextid' => $quizcontext->id]); + $this->add_one_random_question($questiongenerator, $quiz, ['contextid' => $quizcontext->id]); + + // Attempt it as a student, and check. + /** @var \question_usage_by_activity $quba */ + [, $quba] = $this->attempt_quiz($quiz, $this->student); $this->assertEquals(3, $quba->question_count()); - $this->assertEquals(1, count($userattempts)); + $this->assertCount(1, quiz_get_user_attempts($quiz->id, $this->student->id)); + + // Make the backup. $backupid = $this->backup_quiz($quiz, $this->user); + // Delete the current course to make sure there is no data. delete_course($this->course, false); + + // Restore the backup. $newcourse = $this->getDataGenerator()->create_course(); $this->restore_quiz($backupid, $newcourse, $this->user); - $module = $DB->get_record('quiz', ['course' => $newcourse->id]); - $userattempts = quiz_get_user_attempts($module->id, $this->student->id); - $this->assertEquals(1, count($userattempts)); - $this->assertEquals(3, count(\mod_quiz\question\bank\qbank_helper::get_question_structure($module->id))); + + // Verify. + $modules = get_fast_modinfo($newcourse->id)->get_instances_of('quiz'); + $module = reset($modules); + $this->assertCount(1, quiz_get_user_attempts($module->instance, $this->student->id)); + $this->assertCount(3, \mod_quiz\question\bank\qbank_helper::get_question_structure( + $module->instance, $module->context)); } /** diff --git a/mod/quiz/tests/structure_test.php b/mod/quiz/tests/structure_test.php index 00767a55a7f..8c1b180bb74 100644 --- a/mod/quiz/tests/structure_test.php +++ b/mod/quiz/tests/structure_test.php @@ -930,4 +930,37 @@ class mod_quiz_structure_testcase extends advanced_testcase { $structure = structure::create_for_quiz($quiz); $this->assertFalse($structure->can_add_random_questions()); } + + /** + * Test to get the version information for a question to show in the version selection dropdown. + * + * @covers ::get_question_version_info + */ + public function test_get_version_choices_for_slot() { + $this->resetAfterTest(); + + $quizobj = $this->create_test_quiz([]); + + // Create a question with two versions. + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $cat = $questiongenerator->create_question_category(['contextid' => $quizobj->get_context()->id]); + $q = $questiongenerator->create_question('essay', null, + ['category' => $cat->id, 'name' => 'This is the first version']); + $questiongenerator->update_question($q, null, ['name' => 'This is the second version']); + $questiongenerator->update_question($q, null, ['name' => 'This is the third version']); + quiz_add_quiz_question($q->id, $quizobj->get_quiz()); + + // Create the quiz object. + $structure = structure::create_for_quiz($quizobj); + $versiondata = $structure->get_version_choices_for_slot(1); + $this->assertEquals(4, count($versiondata)); + $this->assertEquals('Always latest', $versiondata[0]->versionvalue); + $this->assertEquals('v3 (latest)', $versiondata[1]->versionvalue); + $this->assertEquals('v2', $versiondata[2]->versionvalue); + $this->assertEquals('v1', $versiondata[3]->versionvalue); + $this->assertTrue($versiondata[0]->selected); + $this->assertFalse($versiondata[1]->selected); + $this->assertFalse($versiondata[2]->selected); + $this->assertFalse($versiondata[3]->selected); + } } diff --git a/mod/quiz/tests/tags_test.php b/mod/quiz/tests/tags_test.php index ba565cd047f..63f3fae11f9 100644 --- a/mod/quiz/tests/tags_test.php +++ b/mod/quiz/tests/tags_test.php @@ -81,7 +81,7 @@ class mod_quiz_tags_testcase extends advanced_testcase { $this->assertEquals("{$tag2->id},{$tag2->name}", "{$slottags[0]},{$slottags[1]}"); $defaultcategory = question_get_default_category(context_course::instance($newcourseid)->id); - $this->assertEquals($defaultcategory->id, $question->categoryobject->id); + $this->assertEquals($defaultcategory->id, $question->category); $randomincludingsubcategories = $DB->get_record('question_set_references', ['itemid' => reset($slots)->id, 'component' => 'mod_quiz', 'questionarea' => 'slot']); $filtercondition = json_decode($randomincludingsubcategories->filtercondition); @@ -95,7 +95,9 @@ class mod_quiz_tags_testcase extends advanced_testcase { * @return array the tags. */ protected function get_tags_for_slot(int $slotid): array { - $referencedata = \mod_quiz\question\bank\qbank_helper::get_random_question_data_from_slot($slotid); + global $DB; + $referencedata = $DB->get_record('question_set_references', + ['itemid' => $slotid, 'component' => 'mod_quiz', 'questionarea' => 'slot']); if (isset($referencedata->filtercondition)) { $filtercondition = json_decode($referencedata->filtercondition); if (isset($filtercondition->tags)) { diff --git a/mod/quiz/upgrade.txt b/mod/quiz/upgrade.txt index da7c4ebfeae..2419d36bf44 100644 --- a/mod/quiz/upgrade.txt +++ b/mod/quiz/upgrade.txt @@ -17,7 +17,7 @@ This files describes API changes in the quiz code. * The quiz_slots_tags database table has been removed entirely, as has the get_slot_tags_for_slot_id() method from mod/quiz/classes/structure.php and the the locallib.php functions quiz_retrieve_slot_tags and quiz_retrieve_slot_tag_ids. This information is now stored in question_set_references - and can be accessed using qbank_helper::get_random_question_data_from_slot. + and can be accessed in the results of qbank_helper::get_question_structure. === 3.11 === From a43f56836a737948efe5ee0dd688c501e635a976 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 6 Apr 2022 22:23:46 +0100 Subject: [PATCH 4/5] MDL-74255 quiz: fix redo and regrade question version handling --- mod/quiz/attemptlib.php | 5 +- .../external/submit_question_version.php | 2 +- .../classes/question/bank/qbank_helper.php | 126 ++++++----------- mod/quiz/report/attemptsreport.php | 2 +- mod/quiz/report/overview/report.php | 44 +++--- .../report/overview/tests/report_test.php | 129 +++++++++++------- mod/quiz/tests/attempt_walkthrough_test.php | 3 - .../behat/attempt_redo_questions.feature | 47 ++++++- mod/quiz/tests/qbank_helper_test.php | 35 +---- mod/quiz/tests/quiz_question_restore_test.php | 4 +- question/tests/generator/lib.php | 2 +- 11 files changed, 197 insertions(+), 202 deletions(-) diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index 22e03edccdb..2e0b791cf3c 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -2111,8 +2111,9 @@ class quiz_attempt { $transaction = $DB->start_delegated_transaction(); // Add the question to the usage. It is important we do this before we choose a variant. - $newquestion = question_bank::load_question( - \mod_quiz\question\bank\qbank_helper::choose_question_for_redo($this->slots[$slot]->id, $qubaids)); + $newquestionid = qbank_helper::choose_question_for_redo($this->get_quizid(), + $this->get_quizobj()->get_context(), $this->slots[$slot]->id, $qubaids); + $newquestion = question_bank::load_question($newquestionid, $this->get_quiz()->shuffleanswers); $newslot = $this->quba->add_question_in_place_of_other($slot, $newquestion); // Choose the variant. diff --git a/mod/quiz/classes/external/submit_question_version.php b/mod/quiz/classes/external/submit_question_version.php index 15c7a844b17..dea250868f8 100644 --- a/mod/quiz/classes/external/submit_question_version.php +++ b/mod/quiz/classes/external/submit_question_version.php @@ -58,7 +58,7 @@ class submit_question_version extends external_api { * Set the questions slot parameters to display the question template. * * @param int $slotid Slot id to display. - * @param int $newversion + * @param int $newversion the version to set. 0 means 'always latest'. * @return array */ public static function execute(int $slotid, int $newversion): array { diff --git a/mod/quiz/classes/question/bank/qbank_helper.php b/mod/quiz/classes/question/bank/qbank_helper.php index 8b0b355f039..e3901b86a18 100644 --- a/mod/quiz/classes/question/bank/qbank_helper.php +++ b/mod/quiz/classes/question/bank/qbank_helper.php @@ -17,6 +17,9 @@ namespace mod_quiz\question\bank; use core_question\local\bank\question_version_status; +use core_question\local\bank\random_question_loader; +use qubaid_condition; +use quiz_attempt; defined('MOODLE_INTERNAL') || die(); @@ -34,22 +37,6 @@ require_once($CFG->dirroot . '/mod/quiz/attemptlib.php'); */ class qbank_helper { - /** - * Check if the slot is a random question or not. - * - * @param int $slotid - * @return bool - */ - public static function is_random(int $slotid): bool { - global $DB; - $params = [ - 'itemid' => $slotid, - 'component' => 'mod_quiz', - 'questionarea' => 'slot' - ]; - return $DB->record_exists('question_set_references', $params); - } - /** * Get the available versions of a question where one of the version has the given question id. * @@ -78,39 +65,6 @@ class qbank_helper { ", [$questionid, question_version_status::QUESTION_STATUS_DRAFT]); } - /** - * Get the question id from slot id. - * - * @param int $slotid - * @return mixed - */ - public static function get_question_for_redo(int $slotid) { - global $DB; - $params = [ - 'itemid' => $slotid, - 'component' => 'mod_quiz', - 'questionarea' => 'slot' - ]; - $referencerecord = $DB->get_record('question_references', $params); - if ($referencerecord->version === null) { - $questionsql = 'SELECT q.id - FROM {question} q - JOIN {question_versions} qv ON qv.questionid = q.id - WHERE qv.version = (SELECT MAX(v.version) - FROM {question_versions} v - JOIN {question_bank_entries} be - ON be.id = v.questionbankentryid - WHERE be.id = qv.questionbankentryid) - AND qv.questionbankentryid = ?'; - $questionid = $DB->get_record_sql($questionsql, [$referencerecord->questionbankentryid])->id; - } else { - $questionid = $DB->get_field('question_versions', 'questionid', - ['questionbankentryid' => $referencerecord->questionbankentryid, - 'version' => $referencerecord->version]); - } - return $questionid; - } - /** * Get the information about which questions should be used to create a quiz attempt. * @@ -124,12 +78,25 @@ class qbank_helper { * randomtags, and note that these also have a ->name set and ->qtype set to 'random'. * * @param int $quizid the id of the quiz to load the data for. - * @param \context $quizcontext + * @param \context_module $quizcontext the context of this quiz. + * @param int|null $slotid optional, if passed only load the data for this one slot (if it is in this quiz). * @return array indexed by slot, with information about the content of each slot. */ - public static function get_question_structure(int $quizid, \context $quizcontext) { + public static function get_question_structure(int $quizid, \context_module $quizcontext, + int $slotid = null): array { global $DB; + $params = [ + 'draft' => question_version_status::QUESTION_STATUS_DRAFT, + 'quizcontextid' => $quizcontext->id, 'quizcontextid2' => $quizcontext->id, + 'quizid' => $quizid + ]; + $slotidtest = ''; + if ($slotid !== null) { + $params['slotid'] = $slotid; + $slotidtest = ' AND slot.id = :slotid'; + } + // Load all the data about each slot. $slotdata = $DB->get_records_sql(" SELECT slot.slot, @@ -169,12 +136,12 @@ class qbank_helper { AND qsr.questionarea = 'slot' AND qsr.itemid = slot.id WHERE slot.quizid = :quizid - ORDER BY slot.slot - ", ['draft' => question_version_status::QUESTION_STATUS_DRAFT, - 'quizcontextid' => $quizcontext->id, 'quizcontextid2' => $quizcontext->id, - 'quizid' => $quizid]); + $slotidtest - // Uppack the random info from question_set_reference. + ORDER BY slot.slot + ", $params); + + // Unpack the random info from question_set_reference. foreach ($slotdata as $slot) { // Ensure the right id is the id. $slot->id = $slot->slotid; @@ -184,7 +151,7 @@ class qbank_helper { $filtercondition = json_decode($slot->filtercondition); $slot->questionid = 's' . $slot->id; // Sometimes this is used as an array key, so needs to be unique. $slot->category = $filtercondition->questioncategoryid; - $slot->randomrecurse = $filtercondition->includingsubcategories; + $slot->randomrecurse = (bool) $filtercondition->includingsubcategories; $slot->randomtags = isset($filtercondition->tags) ? (array) $filtercondition->tags : []; $slot->qtype = 'random'; $slot->name = get_string('random', 'quiz'); @@ -229,34 +196,31 @@ class qbank_helper { } /** - * Choose question for redo. + * Choose question for redo in a particular slot. * - * @param int $slotid - * @param \qubaid_condition $qubaids - * @return int + * @param int $quizid the id of the quiz to load the data for. + * @param \context_module $quizcontext the context of this quiz. + * @param int $slotid optional, if passed only load the data for this one slot (if it is in this quiz). + * @param qubaid_condition $qubaids attempts to consider when avoiding picking repeats of random questions. + * @return int the id of the question to use. */ - public static function choose_question_for_redo($slotid, $qubaids): int { - // Choose the replacement question. - if (!self::is_random($slotid)) { - $newqusetionid = self::get_question_for_redo($slotid); - } else { - $tagids = []; - $randomquestiondata = self::get_random_question_data_from_slot($slotid); - $filtercondition = json_decode($randomquestiondata->filtercondition); - if (isset($filtercondition->tags)) { - foreach ($filtercondition->tags as $tag) { - $tagstring = explode(',', $tag); - $tagids [] = $tagstring[0]; - } - } + public static function choose_question_for_redo(int $quizid, \context_module $quizcontext, + int $slotid, qubaid_condition $qubaids): int { + $slotdata = self::get_question_structure($quizid, $quizcontext, $slotid); + $slotdata = reset($slotdata); - $randomloader = new \core_question\local\bank\random_question_loader($qubaids, []); - $newqusetionid = $randomloader->get_next_question_id($filtercondition->questioncategoryid, - (bool) $filtercondition->includingsubcategories, $tagids); - if ($newqusetionid === null) { - throw new \moodle_exception('notenoughrandomquestions', 'quiz'); - } + // Non-random question. + if ($slotdata->qtype != 'random') { + return $slotdata->questionid; + } + // Random question. + $randomloader = new random_question_loader($qubaids, []); + $newqusetionid = $randomloader->get_next_question_id($slotdata->category, + $slotdata->randomrecurse, self::get_tag_ids_for_slot($slotdata)); + + if ($newqusetionid === null) { + throw new \moodle_exception('notenoughrandomquestions', 'quiz'); } return $newqusetionid; } diff --git a/mod/quiz/report/attemptsreport.php b/mod/quiz/report/attemptsreport.php index 48a51ae5c89..3ee0fa3d0c3 100644 --- a/mod/quiz/report/attemptsreport.php +++ b/mod/quiz/report/attemptsreport.php @@ -79,7 +79,7 @@ abstract class quiz_attempts_report extends quiz_default_report { * 3 => \core\dml\sql_join Contains joins, wheres, params for all the students to show in the report. * Will be the same as either element 1 or 2. */ - protected function init($mode, $formclass, $quiz, $cm, $course) { + public function init($mode, $formclass, $quiz, $cm, $course) { $this->mode = $mode; $this->context = context_module::instance($cm->id); diff --git a/mod/quiz/report/overview/report.php b/mod/quiz/report/overview/report.php index 9b2103ee674..b0f66d27853 100644 --- a/mod/quiz/report/overview/report.php +++ b/mod/quiz/report/overview/report.php @@ -22,6 +22,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use mod_quiz\question\bank\qbank_helper; defined('MOODLE_INTERNAL') || die(); @@ -45,11 +46,9 @@ class quiz_overview_report extends quiz_attempts_report { protected $hasgroupstudents; /** - * @var array|null used during regrades, to cache which new questionid to use for each old on. - * for slots which are not random, stores slot => questionid for the question to use. - * See get_new_question_for_regrade. + * @var array|null cached copy of qbank_helper::get_question_structure for use during regrades. */ - protected $slotquestionids = null; + protected $structureforregrade = null; /** * @var array|null used during regrades, to cache which new questionid to use for each old on. @@ -339,7 +338,7 @@ class quiz_overview_report extends quiz_attempts_report { * @param array $slots if null, regrade all questions, otherwise, just regrade * the questions with those slots. */ - protected function regrade_attempt($attempt, $dryrun = false, $slots = null) { + public function regrade_attempt($attempt, $dryrun = false, $slots = null) { global $DB; // Need more time for a quiz with many questions. core_php_time_limit::raise(300); @@ -394,6 +393,14 @@ class quiz_overview_report extends quiz_attempts_report { gc_collect_cycles(); } + /** + * For use in tests only. Clear the cached regrade data. + */ + public function clear_regrade_date_cache(): void { + $this->structureforregrade = null; + $this->newquestionidsforold = null; + } + /** * Work out of we should be using a new question version for a particular slot in a regrade. * @@ -407,40 +414,27 @@ class quiz_overview_report extends quiz_attempts_report { global $DB; // If the cache is empty, get information about all the slots. - if ($this->slotquestionids === null) { + if ($this->structureforregrade === null) { $this->newquestionidsforold = []; // Load the data about all the non-random slots now. - $this->slotquestionids = $DB->get_records_sql_menu(" - SELECT slot.slot, qve.questionid - FROM {quiz_slots} slot - JOIN {question_references} qref ON qref.itemid = slot.id - AND qref.component = 'mod_quiz' AND qref.questionarea = 'slot' AND qref.usingcontextid = ? - JOIN {question_versions} qve ON qve.questionbankentryid = qref.questionbankentryid - WHERE slot.quizid = ? - AND (qve.version = qref.version OR - qref.version IS NULL AND qve.version = ( - SELECT MAX(version) - FROM {question_versions} iqve - WHERE iqve.questionbankentryid = qref.questionbankentryid - ) - ) - ", [$this->context->id, $attempt->quiz]); + $this->structureforregrade = qbank_helper::get_question_structure( + $attempt->quiz, $this->context); } // If this is a non-random slot, we will have the right info cached. - if (array_key_exists($slot, $this->slotquestionids)) { + if ($this->structureforregrade[$slot]->qtype != 'random') { // This is a non-random slot. - return question_bank::load_question($this->slotquestionids[$slot]); + return question_bank::load_question($this->structureforregrade[$slot]->questionid); } // We must be dealing with a random question. Check that cache. $currentquestion = $quba->get_question_attempt($slot)->get_question(false); - if (array_key_exists($currentquestion->id, $this->newquestionidsforold)) { + if (isset($this->newquestionidsforold[$currentquestion->id])) { return question_bank::load_question($this->newquestionidsforold[$currentquestion->id]); } // This is a random question we have not seen yet. Find the latest version. - $versionsoptions = \mod_quiz\question\bank\qbank_helper::get_version_options($currentquestion->id); + $versionsoptions = qbank_helper::get_version_options($currentquestion->id); $latestversion = reset($versionsoptions); $this->newquestionidsforold[$currentquestion->id] = $latestversion->questionid; return question_bank::load_question($latestversion->questionid); diff --git a/mod/quiz/report/overview/tests/report_test.php b/mod/quiz/report/overview/tests/report_test.php index 284061b5f17..2e6d5bf15d6 100644 --- a/mod/quiz/report/overview/tests/report_test.php +++ b/mod/quiz/report/overview/tests/report_test.php @@ -22,8 +22,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core_question\local\bank\question_version_status; use mod_quiz\external\submit_question_version; -use mod_quiz\question\bank\qbank_helper; defined('MOODLE_INTERNAL') || die(); @@ -32,6 +32,7 @@ require_once($CFG->dirroot . '/mod/quiz/locallib.php'); require_once($CFG->dirroot . '/mod/quiz/report/reportlib.php'); require_once($CFG->dirroot . '/mod/quiz/report/default.php'); require_once($CFG->dirroot . '/mod/quiz/report/overview/report.php'); +require_once($CFG->dirroot . '/mod/quiz/report/overview/overview_form.php'); require_once($CFG->dirroot . '/mod/quiz/report/overview/tests/helpers.php'); require_once($CFG->dirroot . '/mod/quiz/tests/quiz_question_helper_test_trait.php'); @@ -41,7 +42,6 @@ require_once($CFG->dirroot . '/mod/quiz/tests/quiz_question_helper_test_trait.ph * * @copyright 2014 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - * @coversDefaultClass \question_usage_by_activity */ class quiz_overview_report_testcase extends advanced_testcase { use \quiz_question_helper_test_trait; @@ -51,19 +51,19 @@ class quiz_overview_report_testcase extends advanced_testcase { * * @return array the data for the test sub-cases. */ - public function report_sql_cases() { + public function report_sql_cases(): array { return [[null], ['csv']]; // Only need to test on or off, not all download types. } /** * Test how the report queries the database. * - * @param bool $isdownloading a download type, or null. + * @param string|null $isdownloading a download type, or null. * @dataProvider report_sql_cases */ - public function test_report_sql($isdownloading) { + public function test_report_sql(?string $isdownloading): void { global $DB; - $this->resetAfterTest(true); + $this->resetAfterTest(); // Create a course and a quiz. $generator = $this->getDataGenerator(); @@ -74,6 +74,7 @@ class quiz_overview_report_testcase extends advanced_testcase { 'attempts' => 10)); // Add one question. + /** @var core_question_generator $questiongenerator */ $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $questiongenerator->create_question_category(); $q = $questiongenerator->create_question('essay', 'plain', ['category' => $cat->id]); @@ -244,7 +245,7 @@ class quiz_overview_report_testcase extends advanced_testcase { * Bands provider. * @return array */ - public function get_bands_count_and_width_provider() { + public function get_bands_count_and_width_provider(): array { return [ [10, [20, .5]], [20, [20, 1]], @@ -263,8 +264,8 @@ class quiz_overview_report_testcase extends advanced_testcase { * @param int $grade grade * @param array $expected */ - public function test_get_bands_count_and_width($grade, $expected) { - $this->resetAfterTest(true); + public function test_get_bands_count_and_width(int $grade, array $expected): void { + $this->resetAfterTest(); $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); $quiz = $quizgenerator->create_instance(['course' => SITEID, 'grade' => $grade]); $this->assertEquals($expected, quiz_overview_report::get_bands_count_and_width($quiz)); @@ -273,8 +274,8 @@ class quiz_overview_report_testcase extends advanced_testcase { /** * Test delete_selected_attempts function. */ - public function test_delete_selected_attempts() { - $this->resetAfterTest(true); + public function test_delete_selected_attempts(): void { + $this->resetAfterTest(); $timestamp = 1234567890; $timestart = $timestamp + 3600; @@ -292,6 +293,7 @@ class quiz_overview_report_testcase extends advanced_testcase { ]); // Add one question. + /** @var core_question_generator $questiongenerator */ $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $questiongenerator->create_question_category(); $q = $questiongenerator->create_question('essay', 'plain', ['category' => $cat->id]); @@ -326,53 +328,80 @@ class quiz_overview_report_testcase extends advanced_testcase { * @covers ::regrade_question */ public function test_regrade_question() { + global $DB; $this->resetAfterTest(); + $this->setAdminUser(); + $course = $this->getDataGenerator()->create_course(); $quiz = $this->create_test_quiz($course); - $student = $this->getDataGenerator()->create_user(); - // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $course->id)->id); + $cm = get_fast_modinfo($course->id)->get_cm($quiz->cmid); + $context = context_module::instance($quiz->cmid); + + /** @var core_question_generator $questiongenerator */ $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); // Create a couple of questions. $cat = $questiongenerator->create_question_category(['contextid' => $context->id]); - $numq = $questiongenerator->create_question('essay', null, - ['category' => $cat->id, 'name' => 'This is the first version', 'correctanswer' => false]); - // Create two version. - $numq2 = $questiongenerator->update_question($numq, null, - ['name' => 'This is the second version', 'correctanswer' => true]); - $numq3 = $questiongenerator->update_question($numq, null, - ['name' => 'This is the third version', 'correctanswer' => false]); - quiz_add_quiz_question($numq->id, $quiz); - // Create the quiz object. - $quizobj = \quiz::create($quiz->id); - $structure = \mod_quiz\structure::create_for_quiz($quizobj); - $slots = $structure->get_slots(); - $slot = reset($slots); - // Now change the version using the external service. - $versions = qbank_helper::get_version_options($slot->questionid); - // We dont want the current version. - $selectversions = []; - foreach ($versions as $version) { - if ($version->version === $slot->version) { - continue; - } - $selectversions[$version->version] = $version; - } - // Change to version 1, with correct response. - $this->expectException('moodle_exception'); - submit_question_version::execute($slot->id, (int)$selectversions[1]->version); - list($quizobj, $quba, $attemptobj) = $this->attempt_quiz($quiz, $student); + $q = $questiongenerator->create_question('shortanswer', null, + ['category' => $cat->id, 'name' => 'Toad scores 0.8']); + + // Create a version, the last one draft. + // Sadly, update_question is a bit dodgy, so it can't handle updating the answer score. + $q2 = $questiongenerator->update_question($q, null, + ['name' => 'Toad now scores 1.0']); + $toadanswer = $DB->get_record_select('question_answers', + 'question = ? AND ' . $DB->sql_compare_text('answer') . ' = ?', + [$q2->id, 'toad'], '*', MUST_EXIST); + $DB->set_field('question_answers', 'fraction', 1, ['id' => $toadanswer->id]); + + // Add the question to the quiz. + quiz_add_quiz_question($q2->id, $quiz, 0, 10); + + // Attempt the quiz, submitting response 'toad'. + $quizobj = quiz::create($quiz->id); + $attempt = quiz_prepare_and_start_new_attempt($quizobj, 1, null); + $attemptobj = quiz_attempt::create($attempt->id); + $attemptobj->process_submitted_actions(time(), false, [1 => ['answer' => 'toad']]); + $attemptobj->process_finish(time(), false); + + // We should be using 'always latest' version, which is currently v2, so should be right. $this->assertEquals(10, $attemptobj->get_question_usage()->get_total_mark()); - // Change to version 2, with wrong response. - submit_question_version::execute($slot->id, (int)$selectversions[2]->version); - $quba->regrade_question(1, quiz_attempt::FINISHED, null, $numq2->id); - $this->assertEquals(0, $attemptobj->get_question_usage()->get_total_mark()); + // Now change the quiz to use fixed version 1. + $slot = $quizobj->get_question($q2->id); + submit_question_version::execute($slot->slotid, 1); - // Change to version 3, with correct response. - submit_question_version::execute($slot->id, (int)$selectversions[3]->version); - $quba->regrade_question(1, quiz_attempt::FINISHED, null, $numq3->id); - $this->assertEquals(10, $attemptobj->get_question_usage()->get_total_mark()); + // Regrade. + $report = new quiz_overview_report(); + $report->init('overview', 'quiz_overview_settings_form', $quiz, $cm, $course); + $report->regrade_attempt($attempt); + + // The mark should now be 8. + $attemptobj = quiz_attempt::create($attempt->id); + $this->assertEquals(8, $attemptobj->get_question_usage()->get_total_mark()); + + // Now add two more versions, the second of which is draft. + $q3 = $questiongenerator->update_question($q, null, + ['name' => 'Toad now scores 0.5']); + $toadanswer = $DB->get_record_select('question_answers', + 'question = ? AND ' . $DB->sql_compare_text('answer') . ' = ?', + [$q3->id, 'toad'], '*', MUST_EXIST); + $DB->set_field('question_answers', 'fraction', 0.5, ['id' => $toadanswer->id]); + + $q4 = $questiongenerator->update_question($q, null, + ['name' => 'Toad now scores 0.3', + 'status' => question_version_status::QUESTION_STATUS_DRAFT]); + $toadanswer = $DB->get_record_select('question_answers', + 'question = ? AND ' . $DB->sql_compare_text('answer') . ' = ?', + [$q4->id, 'toad'], '*', MUST_EXIST); + $DB->set_field('question_answers', 'fraction', 0.3, ['id' => $toadanswer->id]); + + // Now change the quiz back to always latest and regrade again. + submit_question_version::execute($slot->slotid, 0); + $report->clear_regrade_date_cache(); + $report->regrade_attempt($attempt); + + // Score should now be 5, because v3 is the latest non-draft version. + $attemptobj = quiz_attempt::create($attempt->id); + $this->assertEquals(5, $attemptobj->get_question_usage()->get_total_mark()); } - } diff --git a/mod/quiz/tests/attempt_walkthrough_test.php b/mod/quiz/tests/attempt_walkthrough_test.php index f4c5268c333..f8d5ddcf50d 100644 --- a/mod/quiz/tests/attempt_walkthrough_test.php +++ b/mod/quiz/tests/attempt_walkthrough_test.php @@ -86,9 +86,6 @@ class mod_quiz_attempt_walkthrough_testcase extends advanced_testcase { $attemptobj = quiz_attempt::create($attempt->id); $this->assertFalse($attemptobj->has_response_to_at_least_one_graded_question()); - $prefix1 = $quba->get_field_prefix(1); - $prefix2 = $quba->get_field_prefix(2); - $tosubmit = array(1 => array('answer' => 'frog'), 2 => array('answer' => '3.14')); diff --git a/mod/quiz/tests/behat/attempt_redo_questions.feature b/mod/quiz/tests/behat/attempt_redo_questions.feature index 11c0b3ab06c..74b01c6c360 100644 --- a/mod/quiz/tests/behat/attempt_redo_questions.feature +++ b/mod/quiz/tests/behat/attempt_redo_questions.feature @@ -6,16 +6,18 @@ Feature: Allow students to redo questions in a practice quiz, without starting a Background: Given the following "users" exist: - | username | firstname | lastname | email | - | student | Student | One | student@example.com | - | teacher | Teacher | One | teacher@example.com | + | username | firstname | lastname | + | student | Student | One | + | teacher | Teacher | One | + | editor | Question | Editor | And the following "courses" exist: | fullname | shortname | category | | Course 1 | C1 | 0 | And the following "course enrolments" exist: - | user | course | role | - | student | C1 | student | - | teacher | C1 | teacher | + | user | course | role | + | student | C1 | student | + | teacher | C1 | teacher | + | editor | C1 | editingteacher | And the following "question categories" exist: | contextlevel | reference | name | | Course | C1 | Test questions | @@ -41,6 +43,39 @@ Feature: Allow students to redo questions in a practice quiz, without starting a Then the state of "First question" question is shown as "Not complete" And I should see "Marked out of 2.00" in the "First question" "question" + @javascript + Scenario: Start attempt, teacher edits question, redo picks up latest non-draft version + # Start attempt as student. + Given I am on the "Quiz 1" "mod_quiz > View" page logged in as "student" + And I press "Attempt quiz" + And I click on "False" "radio" in the "First question" "question" + And I click on "Check" "button" in the "First question" "question" + And I log out + + # Now edit the question as teacher to add a real version and a draft version. + # Would be nice to do this with a generator, but I don't have time right now. + And I am on the "TF1" "core_question > edit" page logged in as "editor" + And I set the following fields to these values: + | Question name | TF1-v2 | + | Question text | The new first question | + | Correct answer | False | + And I press "id_submitbutton" + And I am on the "TF1-v2" "core_question > edit" page + And I set the following fields to these values: + | Question name | TF1-v3 | + | Question text | This is only draft for now | + | Correct answer | True | + | Question status | Draft | + And I press "id_submitbutton" + And I log out + + When I am on the "Quiz 1" "mod_quiz > View" page logged in as "student" + And I press "Continue your attempt" + And I press "Try another question like this one" + Then the state of "The new first question" question is shown as "Not complete" + And I should see "Marked out of 2.00" in the "The new first question" "question" + And I should not see "This is only draft for now" + @javascript Scenario: The redo question button is visible but disabled for teachers Given I am on the "Quiz 1" "mod_quiz > View" page logged in as "student" diff --git a/mod/quiz/tests/qbank_helper_test.php b/mod/quiz/tests/qbank_helper_test.php index 9216057a8fb..71d3474dbbf 100644 --- a/mod/quiz/tests/qbank_helper_test.php +++ b/mod/quiz/tests/qbank_helper_test.php @@ -53,40 +53,17 @@ class qbank_helper_test extends \advanced_testcase { $this->user = $USER; } - /** - * Test is random. - * - * @covers ::is_random - * @covers ::get_random_question_data_from_slot - */ - public function test_is_random() { - $this->resetAfterTest(); - $quiz = $this->create_test_quiz($this->course); - // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); - $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); - $this->add_one_random_question($questiongenerator, $quiz, ['contextid' => $context->id]); - // Create the quiz object. - $quizobj = \quiz::create($quiz->id); - $structure = structure::create_for_quiz($quizobj); - $slots = $structure->get_slots(); - foreach ($slots as $slot) { - $this->assertEquals(true, qbank_helper::is_random($slot->id)); - } - } - /** * Test reference records. * * @covers ::get_version_options - * @covers ::get_question_for_redo */ public function test_reference_records() { $this->resetAfterTest(); $quiz = $this->create_test_quiz($this->course); // Test for questions from a different context. - $context = \context_module::instance(get_coursemodule_from_instance("quiz", $quiz->id, $this->course->id)->id); + $context = \context_module::instance($quiz->cmid); // Create a couple of questions. /** @var \core_question_generator $questiongenerator */ @@ -110,11 +87,8 @@ class qbank_helper_test extends \advanced_testcase { $slots = $structure->get_slots(); $slot = reset($slots); $this->assertEquals(3, count(qbank_helper::get_version_options($question->id))); - $quizobj->preload_questions(); - $quizobj->load_questions(); - $questions = $quizobj->get_questions(); - $question = reset($questions); - $this->assertEquals($question->id, qbank_helper::get_question_for_redo($slot->id)); + $this->assertEquals($question->id, qbank_helper::choose_question_for_redo( + $quiz->id, $context, $slot->id, new \qubaid_list([]))); // Create another version. $questiongenerator->update_question($numq, null, ['name' => 'This is the latest version']); @@ -125,7 +99,8 @@ class qbank_helper_test extends \advanced_testcase { $quizobj->load_questions(); $questions = $quizobj->get_questions(); $question = reset($questions); - $this->assertEquals($question->id, qbank_helper::get_question_for_redo($slot->id)); + $this->assertEquals($question->id, qbank_helper::choose_question_for_redo( + $quiz->id, $context, $slot->id, new \qubaid_list([]))); } /** diff --git a/mod/quiz/tests/quiz_question_restore_test.php b/mod/quiz/tests/quiz_question_restore_test.php index 1758749b8cd..0e4726d7482 100644 --- a/mod/quiz/tests/quiz_question_restore_test.php +++ b/mod/quiz/tests/quiz_question_restore_test.php @@ -82,7 +82,7 @@ class quiz_question_restore_test extends \advanced_testcase { $this->assertEquals(0, count(\mod_quiz\question\bank\qbank_helper::get_question_structure( $quiz->id, $oldquizcontext))); - /// Restore the course. + // Restore the course. $newcourse = $this->getDataGenerator()->create_course(); $this->restore_quiz($backupid, $newcourse, $this->user); @@ -120,7 +120,7 @@ class quiz_question_restore_test extends \advanced_testcase { $this->assertEquals(0, count(\mod_quiz\question\bank\qbank_helper::get_question_structure( $quiz->id, $quizcontext))); - /// Restore the course. + // Restore the course. $newcourse = $this->getDataGenerator()->create_course(); $this->restore_quiz($backupid, $newcourse, $this->user); diff --git a/question/tests/generator/lib.php b/question/tests/generator/lib.php index 6ad85a22708..05427126e45 100644 --- a/question/tests/generator/lib.php +++ b/question/tests/generator/lib.php @@ -129,7 +129,7 @@ class core_question_generator extends component_generator_base { $fromform = test_question_maker::get_question_form_data($qtype, $which); $fromform = (object) $this->datagenerator->combine_defaults_and_record((array) $question, $fromform); $fromform = (object) $this->datagenerator->combine_defaults_and_record((array) $fromform, $overrides); - $fromform->status = $question->status; + $fromform->status = $fromform->status ?? $question->status; $question = question_bank::get_qtype($qtype)->save_question($question, $fromform); From 5be2fc1ddcf3360b829c3510e62c1d515a4767a5 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 8 Apr 2022 11:41:24 +0100 Subject: [PATCH 5/5] MDL-74255 quiz/question versions: update SQL to work in Oracle --- .../classes/question/bank/qbank_helper.php | 40 ++++++++++++++----- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/mod/quiz/classes/question/bank/qbank_helper.php b/mod/quiz/classes/question/bank/qbank_helper.php index e3901b86a18..fccb4ec6d56 100644 --- a/mod/quiz/classes/question/bank/qbank_helper.php +++ b/mod/quiz/classes/question/bank/qbank_helper.php @@ -88,13 +88,19 @@ class qbank_helper { $params = [ 'draft' => question_version_status::QUESTION_STATUS_DRAFT, - 'quizcontextid' => $quizcontext->id, 'quizcontextid2' => $quizcontext->id, - 'quizid' => $quizid + 'quizcontextid' => $quizcontext->id, + 'quizcontextid2' => $quizcontext->id, + 'quizcontextid3' => $quizcontext->id, + 'quizid' => $quizid, + 'quizid2' => $quizid, ]; $slotidtest = ''; + $slotidtest2 = ''; if ($slotid !== null) { $params['slotid'] = $slotid; + $params['slotid2'] = $slotid; $slotidtest = ' AND slot.id = :slotid'; + $slotidtest2 = ' AND lslot.id = :slotid2'; } // Load all the data about each slot. @@ -121,18 +127,34 @@ class qbank_helper { LEFT JOIN {question_references} qr ON qr.usingcontextid = :quizcontextid AND qr.component = 'mod_quiz' AND qr.questionarea = 'slot' AND qr.itemid = slot.id LEFT JOIN {question_bank_entries} qbe ON qbe.id = qr.questionbankentryid + + -- This way of getting the latest version for each slot is a bit more complicated + -- than we would like, but the simpler SQL did not work in Oracle 11.2. + -- (It did work find in Oracle 19.x, so once we have updated our min supported + -- version we could consider digging the old code out of git history from + -- just before the commit that added this comment. + -- For relevant question_bank_entries, this gets the latest non-draft slot number. + LEFT JOIN ( + SELECT lv.questionbankentryid, MAX(lv.version) AS version + FROM {quiz_slots} lslot + JOIN {question_references} lqr ON lqr.usingcontextid = :quizcontextid2 AND lqr.component = 'mod_quiz' + AND lqr.questionarea = 'slot' AND lqr.itemid = lslot.id + JOIN {question_versions} lv ON lv.questionbankentryid = lqr.questionbankentryid + WHERE lslot.quizid = :quizid2 + $slotidtest2 + AND lqr.version IS NULL + AND lv.status <> :draft + GROUP BY lv.questionbankentryid + ) latestversions ON latestversions.questionbankentryid = qr.questionbankentryid + LEFT JOIN {question_versions} qv ON qv.questionbankentryid = qbe.id - -- Either specified version, or latest ready version. - AND qv.version = COALESCE(qr.version, ( - SELECT MAX(version) - FROM {question_versions} - WHERE questionbankentryid = qbe.id AND status <> :draft - )) + -- Either specified version, or latest ready version. + AND qv.version = COALESCE(qr.version, latestversions.version) LEFT JOIN {question_categories} qc ON qc.id = qbe.questioncategoryid LEFT JOIN {question} q ON q.id = qv.questionid -- Case where a random question has been added. - LEFT JOIN {question_set_references} qsr ON qsr.usingcontextid = :quizcontextid2 AND qsr.component = 'mod_quiz' + LEFT JOIN {question_set_references} qsr ON qsr.usingcontextid = :quizcontextid3 AND qsr.component = 'mod_quiz' AND qsr.questionarea = 'slot' AND qsr.itemid = slot.id WHERE slot.quizid = :quizid