From 14ce932d9a195929bd54cc3ca10ea27e652560f5 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 8 Mar 2024 17:11:59 +0000 Subject: [PATCH 1/3] MDL-81114 question: fix coding style in random_question_loader_test --- .../tests/random_question_loader_test.php | 122 ++++++++++-------- 1 file changed, 66 insertions(+), 56 deletions(-) diff --git a/question/tests/random_question_loader_test.php b/question/tests/random_question_loader_test.php index 13cc9dec3aa..1931b5a3892 100644 --- a/question/tests/random_question_loader_test.php +++ b/question/tests/random_question_loader_test.php @@ -16,6 +16,11 @@ namespace core_question; +use core_question\local\bank\condition; +use core_question\local\bank\question_version_status; +use core_question\local\bank\random_question_loader; +use core_question_generator; +use mod_quiz\quiz_settings; use qubaid_list; use question_bank; use question_engine; @@ -31,19 +36,20 @@ require_once($CFG->dirroot . '/mod/quiz/tests/quiz_question_helper_test_trait.ph * Tests for the {@see \core_question\local\bank\random_question_loader} class. * * @package core_question - * @copyright 2015 The Open University - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @copyright 2015 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \random_question_loader */ -class random_question_loader_test extends \advanced_testcase { +final class random_question_loader_test extends \advanced_testcase { use \quiz_question_helper_test_trait; - public function test_empty_category_gives_null() { + public function test_empty_category_gives_null(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertNull($loader->get_next_filtered_question_id($filters)); @@ -52,26 +58,26 @@ class random_question_loader_test extends \advanced_testcase { $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_unknown_category_behaves_like_empty() { + public function test_unknown_category_behaves_like_empty(): void { // It is up the caller to make sure the category id is valid. - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([-1], 1); $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_descriptions_not_returned() { + public function test_descriptions_not_returned(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); $info = $generator->create_question('description', null, ['category' => $cat->id]); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_hidden_questions_not_returned() { + public function test_hidden_questions_not_returned(): void { global $DB; $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); @@ -79,27 +85,27 @@ class random_question_loader_test extends \advanced_testcase { $cat = $generator->create_question_category(); $question1 = $generator->create_question('shortanswer', null, ['category' => $cat->id]); $DB->set_field('question_versions', 'status', - \core_question\local\bank\question_version_status::QUESTION_STATUS_HIDDEN, ['questionid' => $question1->id]); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + \core_question\local\bank\question_version_status::QUESTION_STATUS_HIDDEN, ['questionid' => $question1->id]); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_cloze_subquestions_not_returned() { + public function test_cloze_subquestions_not_returned(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); $question1 = $generator->create_question('multianswer', null, ['category' => $cat->id]); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertEquals($question1->id, $loader->get_next_filtered_question_id($filters)); $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_random_questions_not_returned() { + public function test_random_questions_not_returned(): void { $this->resetAfterTest(); $this->setAdminUser(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); @@ -108,19 +114,19 @@ class random_question_loader_test extends \advanced_testcase { $course = $this->getDataGenerator()->create_course(); $quiz = $this->getDataGenerator()->create_module('quiz', ['course' => $course]); $this->add_random_questions($quiz->id, 1, $cat->id, 1); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_one_question_category_returns_that_q_then_null() { + public function test_one_question_category_returns_that_q_then_null(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); $question1 = $generator->create_question('shortanswer', null, ['category' => $cat->id]); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id], 1); $this->assertEquals($question1->id, $loader->get_next_filtered_question_id($filters)); @@ -129,14 +135,14 @@ class random_question_loader_test extends \advanced_testcase { $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_two_question_category_returns_both_then_null() { + public function test_two_question_category_returns_both_then_null(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); $question1 = $generator->create_question('shortanswer', null, ['category' => $cat->id]); $question2 = $generator->create_question('shortanswer', null, ['category' => $cat->id]); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $questionids = []; $filters = question_filter_test_helper::create_filters([$cat->id]); @@ -149,7 +155,7 @@ class random_question_loader_test extends \advanced_testcase { $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_nested_categories() { + public function test_nested_categories(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); @@ -157,7 +163,7 @@ class random_question_loader_test extends \advanced_testcase { $cat2 = $generator->create_question_category(['parent' => $cat1->id]); $question1 = $generator->create_question('shortanswer', null, ['category' => $cat1->id]); $question2 = $generator->create_question('shortanswer', null, ['category' => $cat2->id]); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat2->id], 1); $this->assertEquals($question2->id, $loader->get_next_filtered_question_id($filters)); @@ -168,22 +174,22 @@ class random_question_loader_test extends \advanced_testcase { $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_used_question_not_returned_until_later() { + public function test_used_question_not_returned_until_later(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); $question1 = $generator->create_question('shortanswer', null, ['category' => $cat->id]); $question2 = $generator->create_question('shortanswer', null, ['category' => $cat->id]); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([]), - array($question2->id => 2)); + $loader = new random_question_loader(new qubaid_list([]), + [$question2->id => 2]); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertEquals($question1->id, $loader->get_next_filtered_question_id($filters)); $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_previously_used_question_not_returned_until_later() { + public function test_previously_used_question_not_returned_until_later(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); @@ -198,7 +204,7 @@ class random_question_loader_test extends \advanced_testcase { $quba->start_all_questions(); question_engine::save_questions_usage_by_activity($quba); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list(array($quba->get_id()))); + $loader = new random_question_loader(new qubaid_list([$quba->get_id()])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertEquals($question1->id, $loader->get_next_filtered_question_id($filters)); @@ -206,12 +212,12 @@ class random_question_loader_test extends \advanced_testcase { $this->assertNull($loader->get_next_filtered_question_id($filters)); } - public function test_empty_category_does_not_have_question_available() { + public function test_empty_category_does_not_have_question_available(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list(array())); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertFalse($loader->is_filtered_question_available($filters, 1)); @@ -219,13 +225,13 @@ class random_question_loader_test extends \advanced_testcase { $this->assertFalse($loader->is_filtered_question_available($filters, 1)); } - public function test_descriptions_not_available() { + public function test_descriptions_not_available(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); - $info = $generator->create_question('description', null, array('category' => $cat->id)); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list(array())); + $info = $generator->create_question('description', null, ['category' => $cat->id]); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertFalse($loader->is_filtered_question_available($filters, $info->id)); @@ -233,13 +239,13 @@ class random_question_loader_test extends \advanced_testcase { $this->assertFalse($loader->is_filtered_question_available($filters, $info->id)); } - public function test_existing_question_is_available_but_then_marked_used() { + public function test_existing_question_is_available_but_then_marked_used(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $generator->create_question_category(); - $question1 = $generator->create_question('shortanswer', null, array('category' => $cat->id)); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list(array())); + $question1 = $generator->create_question('shortanswer', null, ['category' => $cat->id]); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$cat->id]); $this->assertTrue($loader->is_filtered_question_available($filters, $question1->id)); @@ -250,8 +256,10 @@ class random_question_loader_test extends \advanced_testcase { /** * Data provider for the get_questions test. + * + * @return array testcases. */ - public function get_questions_test_cases() { + public static function get_questions_test_cases(): array { return [ 'empty category' => [ 'categoryindex' => 'emptycat', @@ -334,7 +342,7 @@ class random_question_loader_test extends \advanced_testcase { $includesubcategories, $usetagnames, $expectedquestionindexes - ) { + ): void { $this->resetAfterTest(); $categories = []; @@ -350,22 +358,22 @@ class random_question_loader_test extends \advanced_testcase { $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); // First category and questions. - list($category, $categoryquestions) = $this->create_category_and_questions(2, ['cat1', 'foo']); + [$category, $categoryquestions] = $this->create_category_and_questions(2, ['cat1', 'foo']); $categories['cat1'] = $category; $questions['cat1q1'] = $categoryquestions[0]; $questions['cat1q2'] = $categoryquestions[1]; // Second category and questions. - list($category, $categoryquestions) = $this->create_category_and_questions(2, ['cat2', 'foo']); + [$category, $categoryquestions] = $this->create_category_and_questions(2, ['cat2', 'foo']); $categories['cat2'] = $category; $questions['cat2q1'] = $categoryquestions[0]; $questions['cat2q2'] = $categoryquestions[1]; // Sub category and questions. - list($category, $categoryquestions) = $this->create_category_and_questions(2, ['subcat', 'foo'], $categories['cat1']); + [$category, $categoryquestions] = $this->create_category_and_questions(2, ['subcat', 'foo'], $categories['cat1']); $categories['subcat'] = $category; $questions['subcatq1'] = $categoryquestions[0]; $questions['subcatq2'] = $categoryquestions[1]; // Empty category. - list($category, $categoryquestions) = $this->create_category_and_questions(0); + [$category, $categoryquestions] = $this->create_category_and_questions(0); $categories['emptycat'] = $category; // Generate the arguments for the get_questions function. @@ -374,7 +382,7 @@ class random_question_loader_test extends \advanced_testcase { return $tags[$tagname]->id; }, $usetagnames); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $filters = question_filter_test_helper::create_filters([$category->id], $includesubcategories, $tagids); $result = $loader->get_filtered_questions($filters); // Generate the expected question set. @@ -393,15 +401,15 @@ class random_question_loader_test extends \advanced_testcase { /** * get_questions should allow limiting and offsetting of the result set. */ - public function test_get_questions_with_limit_and_offset() { + public function test_get_questions_with_limit_and_offset(): void { $this->resetAfterTest(); $numberofquestions = 5; $includesubcategories = false; $tagids = []; $limit = 1; $offset = 0; - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); - list($category, $questions) = $this->create_category_and_questions($numberofquestions); + $loader = new random_question_loader(new qubaid_list([])); + [$category, $questions] = $this->create_category_and_questions($numberofquestions); // Add questionid as key to find them easily later. $questionsbyid = []; @@ -428,15 +436,15 @@ class random_question_loader_test extends \advanced_testcase { * get_questions should allow retrieving questions with only a subset of * fields populated. */ - public function test_get_questions_with_restricted_fields() { + public function test_get_questions_with_restricted_fields(): void { $this->resetAfterTest(); $includesubcategories = false; $tagids = []; $limit = 10; $offset = 0; $fields = ['id', 'name']; - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); - list($category, $questions) = $this->create_category_and_questions(1); + $loader = new random_question_loader(new qubaid_list([])); + [$category, $questions] = $this->create_category_and_questions(1); $filters = question_filter_test_helper::create_filters([$category->id], $includesubcategories, $tagids); $result = $loader->get_filtered_questions( @@ -458,8 +466,10 @@ class random_question_loader_test extends \advanced_testcase { /** * Data provider for the count_questions test. + * + * @return array testcases. */ - public function count_questions_test_cases() { + public static function count_questions_test_cases(): array { return [ 'empty category' => [ 'categoryindex' => 'emptycat', @@ -542,7 +552,7 @@ class random_question_loader_test extends \advanced_testcase { $includesubcategories, $usetagnames, $expectedcount - ) { + ): void { $this->resetAfterTest(); $categories = []; @@ -558,22 +568,22 @@ class random_question_loader_test extends \advanced_testcase { $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); // First category and questions. - list($category, $categoryquestions) = $this->create_category_and_questions(2, ['cat1', 'foo']); + [$category, $categoryquestions] = $this->create_category_and_questions(2, ['cat1', 'foo']); $categories['cat1'] = $category; $questions['cat1q1'] = $categoryquestions[0]; $questions['cat1q2'] = $categoryquestions[1]; // Second category and questions. - list($category, $categoryquestions) = $this->create_category_and_questions(2, ['cat2', 'foo']); + [$category, $categoryquestions] = $this->create_category_and_questions(2, ['cat2', 'foo']); $categories['cat2'] = $category; $questions['cat2q1'] = $categoryquestions[0]; $questions['cat2q2'] = $categoryquestions[1]; // Sub category and questions. - list($category, $categoryquestions) = $this->create_category_and_questions(2, ['subcat', 'foo'], $categories['cat1']); + [$category, $categoryquestions] = $this->create_category_and_questions(2, ['subcat', 'foo'], $categories['cat1']); $categories['subcat'] = $category; $questions['subcatq1'] = $categoryquestions[0]; $questions['subcatq2'] = $categoryquestions[1]; // Empty category. - list($category, $categoryquestions) = $this->create_category_and_questions(0); + [$category, $categoryquestions] = $this->create_category_and_questions(0); $categories['emptycat'] = $category; // Generate the arguments for the get_questions function. @@ -583,7 +593,7 @@ class random_question_loader_test extends \advanced_testcase { }, $usetagnames); $filters = question_filter_test_helper::create_filters([$category->id], $includesubcategories, $tagids); - $loader = new \core_question\local\bank\random_question_loader(new qubaid_list([])); + $loader = new random_question_loader(new qubaid_list([])); $result = $loader->count_filtered_questions($filters); // Ensure the result matches what was expected. @@ -599,7 +609,7 @@ class random_question_loader_test extends \advanced_testcase { * @param stdClass|null $parentcategory The category to set as the parent of the created category. * @return array The category and questions. */ - protected function create_category_and_questions($questioncount, $tagnames = [], $parentcategory = null) { + protected function create_category_and_questions($questioncount, $tagnames = [], $parentcategory = null): array { $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); if ($parentcategory) { From db8d3fef9cf2513af4875cf0f3623f2299425230 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 8 Mar 2024 17:57:30 +0000 Subject: [PATCH 2/3] MDL-81114 question: clarify the SQL in ensure_filtered_questions_loaded This should not change the (currently buggy) behaviour at all, but my brain needs this to understand how to fix the bug. --- .../local/bank/random_question_loader.php | 82 +++++++++---------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/question/classes/local/bank/random_question_loader.php b/question/classes/local/bank/random_question_loader.php index 7daa6993d40..3e75c5b0deb 100644 --- a/question/classes/local/bank/random_question_loader.php +++ b/question/classes/local/bank/random_question_loader.php @@ -186,69 +186,69 @@ class random_question_loader { protected function ensure_filtered_questions_loaded(array $filters) { global $DB; + // Check if this is already done. $key = $this->get_filtered_questions_key($filters); if (isset($this->availablequestionscache[$key])) { // Data is already in the cache, nothing to do. return; } - [$extraconditions, $extraparams] = $DB->get_in_or_equal($this->excludedqtypes, - SQL_PARAMS_NAMED, 'excludedqtype', false); - - $previoussql = "SELECT COUNT(1) - FROM " . $this->qubaids->from_question_attempts('qa') . " - WHERE qa.questionid = q.id AND " . $this->qubaids->where(); - $previousparams = $this->qubaids->from_where_params(); - - // Latest version. - $latestversionsql = "SELECT MAX(v.version) - FROM {question_versions} v - JOIN {question_bank_entries} be ON be.id = v.questionbankentryid - WHERE be.id = qbe.id"; - - $sql = "SELECT q.id, ($previoussql) AS previous_attempts - FROM {question} q - JOIN {question_versions} qv ON qv.questionid = q.id - JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid - WHERE "; - - $where = [ - 'q.parent = :noparent', - 'qv.status = :ready', - "qv.version = ($latestversionsql)", - ]; - $params = array_merge( - $previousparams, - ['noparent' => 0, 'ready' => question_version_status::QUESTION_STATUS_READY]); - - // Get current enabled condition classes. - $conditionclasses = \core_question\local\bank\filter_condition_manager::get_condition_classes(); // Build filter conditions. - foreach ($conditionclasses as $conditionclass) { + $params = []; + $filterconditions = []; + foreach (filter_condition_manager::get_condition_classes() as $conditionclass) { $filter = $conditionclass::get_filter_from_list($filters); if (is_null($filter)) { continue; } + [$filterwhere, $filterparams] = $conditionclass::build_query_from_filter($filter); if (!empty($filterwhere)) { - $where[] = '(' . $filterwhere . ')'; + $filterconditions[] = '(' . $filterwhere . ')'; } if (!empty($filterparams)) { $params = array_merge($params, $filterparams); } } + $filtercondition = $filterconditions ? 'AND ' . implode(' AND ', $filterconditions) : ''; - // Extra conditions. - if ($extraconditions) { - $where[] = 'q.qtype ' . $extraconditions; - $params = array_merge($params, $extraparams); + // Prepare qtype check. + [$qtypecondition, $qtypeparams] = $DB->get_in_or_equal($this->excludedqtypes, + SQL_PARAMS_NAMED, 'excludedqtype', false); + if ($qtypecondition) { + $qtypecondition = 'AND q.qtype ' . $qtypecondition; } - // Build query. - $sql .= implode(' AND ', $where); - $sql .= "ORDER BY previous_attempts"; + $questionidsandcounts = $DB->get_records_sql_menu(" + SELECT q.id, + ( + SELECT COUNT(1) + FROM {$this->qubaids->from_question_attempts('qa')} + WHERE qa.questionid = q.id AND {$this->qubaids->where()} + ) AS previous_attempts - $questionidsandcounts = $DB->get_records_sql_menu($sql, $params); + FROM {question} q + JOIN {question_versions} qv ON qv.questionid = q.id + JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid + + WHERE q.parent = :noparent + AND qv.status = :ready + AND qv.version = ( + SELECT MAX(v.version) + FROM {question_versions} v + JOIN {question_bank_entries} be ON be.id = v.questionbankentryid + WHERE be.id = qbe.id + ) + $qtypecondition + $filtercondition + + ORDER BY previous_attempts + ", array_merge( + $params, + $this->qubaids->from_where_params(), + ['noparent' => 0, 'ready' => question_version_status::QUESTION_STATUS_READY], + $qtypeparams, + )); if (!$questionidsandcounts) { // No questions in this category. From 36e03243dae444864b7820fa11db1d2bbf917ad0 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 8 Mar 2024 17:20:26 +0000 Subject: [PATCH 3/3] MDL-81114 question: fix selection of random questions with draft status We need to pick the latest 'ready' version of each question (not questions where the latest version is ready). There are test of the behaviour of random_question_loader, and then a test in the quiz to prove it works in use. --- mod/quiz/tests/attempt_test.php | 48 +++++++++++++++++ .../tests/quiz_question_helper_test_trait.php | 4 +- .../local/bank/random_question_loader.php | 13 +++-- .../tests/random_question_loader_test.php | 52 +++++++++++++++++++ 4 files changed, 108 insertions(+), 9 deletions(-) diff --git a/mod/quiz/tests/attempt_test.php b/mod/quiz/tests/attempt_test.php index 73733506608..8d55acfb1cc 100644 --- a/mod/quiz/tests/attempt_test.php +++ b/mod/quiz/tests/attempt_test.php @@ -16,8 +16,11 @@ namespace mod_quiz; +use core_question\local\bank\condition; use core_question\local\bank\question_version_status; +use core_question_generator; use mod_quiz\output\view_page; +use mod_quiz_generator; use question_engine; defined('MOODLE_INTERNAL') || die(); @@ -25,6 +28,7 @@ defined('MOODLE_INTERNAL') || die(); global $CFG; require_once($CFG->dirroot . '/mod/quiz/locallib.php'); + /** * Tests for the quiz_attempt class. * @@ -32,6 +36,7 @@ require_once($CFG->dirroot . '/mod/quiz/locallib.php'); * @category test * @copyright 2014 Tim Hunt * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \mod_quiz\quiz_attempt */ class attempt_test extends \advanced_testcase { @@ -378,6 +383,49 @@ class attempt_test extends \advanced_testcase { $this->assertEquals($student1->id, $step->get_user_id()); } + /** + * Test quiz_prepare_and_start_new_attempt function + */ + public function test_quiz_prepare_and_start_new_attempt_random_draft() { + $this->resetAfterTest(); + $this->setAdminUser(); + + // Create course. + $course = $this->getDataGenerator()->create_course(); + // Create quiz. + /** @var mod_quiz_generator $quizgenerator */ + $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); + $quiz = $quizgenerator->create_instance(['course' => $course->id]); + + // Create question with 2 versions. V1 ready. V2 draft. + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $questiongenerator->create_question_category(); + $question = $questiongenerator->create_question('shortanswer', null, + ['questiontext' => 'V1', 'category' => $category->id]); + $questiongenerator->update_question($question, null, + ['questiontext' => 'V2', 'status' => question_version_status::QUESTION_STATUS_DRAFT]); + + // Add a random question form that category. + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'jointype' => condition::JOINTYPE_DEFAULT, + 'values' => [$category->id], + 'filteroptions' => ['includesubcategories' => false], + ], + ], + ]; + $quizobj = quiz_settings::create($quiz->id); + $quizobj->get_structure()->add_random_questions(1, 1, $filtercondition); + $quizobj->get_grade_calculator()->recompute_quiz_sumgrades(); + + // Create an attempt. + $quizobj = quiz_settings::create($quiz->id); + $attempt = quiz_prepare_and_start_new_attempt($quizobj, 1, null); + $this->assertEquals(1, $attempt->preview); + } + /** * Test check_page_access function * @covers \quiz_attempt::check_page_access diff --git a/mod/quiz/tests/quiz_question_helper_test_trait.php b/mod/quiz/tests/quiz_question_helper_test_trait.php index 198c41e9968..c5b2220c489 100644 --- a/mod/quiz/tests/quiz_question_helper_test_trait.php +++ b/mod/quiz/tests/quiz_question_helper_test_trait.php @@ -191,8 +191,8 @@ trait quiz_question_helper_test_trait { * @return void */ protected function add_random_questions(int $quizid, int $page, int $categoryid, int $number): void { - $settings = quiz_settings::create($quizid); - $structure = \mod_quiz\structure::create_for_quiz($settings); + $quizobj = quiz_settings::create($quizid); + $structure = $quizobj->get_structure(); $filtercondition = [ 'filter' => [ 'category' => [ diff --git a/question/classes/local/bank/random_question_loader.php b/question/classes/local/bank/random_question_loader.php index 3e75c5b0deb..8d66d2adca6 100644 --- a/question/classes/local/bank/random_question_loader.php +++ b/question/classes/local/bank/random_question_loader.php @@ -232,15 +232,14 @@ class random_question_loader { JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid WHERE q.parent = :noparent - AND qv.status = :ready - AND qv.version = ( - SELECT MAX(v.version) - FROM {question_versions} v - JOIN {question_bank_entries} be ON be.id = v.questionbankentryid - WHERE be.id = qbe.id - ) $qtypecondition $filtercondition + AND qv.version = ( + SELECT MAX(version) + FROM {question_versions} + WHERE questionbankentryid = qbe.id + AND status = :ready + ) ORDER BY previous_attempts ", array_merge( diff --git a/question/tests/random_question_loader_test.php b/question/tests/random_question_loader_test.php index 1931b5a3892..a977bbbae9d 100644 --- a/question/tests/random_question_loader_test.php +++ b/question/tests/random_question_loader_test.php @@ -120,6 +120,58 @@ final class random_question_loader_test extends \advanced_testcase { $this->assertNull($loader->get_next_filtered_question_id($filters)); } + public function test_draft_questions_not_returned(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + + // Create a question in draft state. + $category = $questiongenerator->create_question_category(); + $questiongenerator->create_question('shortanswer', null, + ['category' => $category->id, 'status' => question_version_status::QUESTION_STATUS_DRAFT]); + + // Try to a random question from that category - should not be one. + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'jointype' => condition::JOINTYPE_DEFAULT, + 'values' => [$category->id], + 'filteroptions' => ['includesubcategories' => false], + ], + ], + ]; + $loader = new random_question_loader(new qubaid_list([])); + $this->assertNull($loader->get_next_filtered_question_id($filtercondition)); + } + + public function test_questions_with_later_draft_version_is_returned(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); + + // Create a question in draft state. + $category = $questiongenerator->create_question_category(); + $question = $questiongenerator->create_question('shortanswer', null, + ['questiontext' => 'V1', 'category' => $category->id]); + $questiongenerator->update_question($question, null, + ['questiontext' => 'V2', 'status' => question_version_status::QUESTION_STATUS_DRAFT]); + + // Try to a random question from that category - should get V1. + $filtercondition = [ + 'filter' => [ + 'category' => [ + 'jointype' => condition::JOINTYPE_DEFAULT, + 'values' => [$category->id], + 'filteroptions' => ['includesubcategories' => false], + ], + ], + ]; + $loader = new random_question_loader(new qubaid_list([])); + $this->assertEquals($question->id, $loader->get_next_filtered_question_id($filtercondition)); + } + public function test_one_question_category_returns_that_q_then_null(): void { $this->resetAfterTest(); $generator = $this->getDataGenerator()->get_plugin_generator('core_question');