diff --git a/mod/quiz/classes/question/bank/custom_view.php b/mod/quiz/classes/question/bank/custom_view.php index f0354eaf381..c396c784207 100644 --- a/mod/quiz/classes/question/bank/custom_view.php +++ b/mod/quiz/classes/question/bank/custom_view.php @@ -246,9 +246,16 @@ class custom_view extends \core_question\local\bank\view { FROM {question_versions} v JOIN {question_bank_entries} be ON be.id = v.questionbankentryid - WHERE be.id = qbe.id)'; - $onlyready = '((' . "qv.status = '" . question_version_status::QUESTION_STATUS_READY . "'" .'))'; - $this->sqlparams = []; + WHERE be.id = qbe.id AND v.status <> :substatus)'; + + // An additional condition is required in the subquery to account for scenarios + // where the latest version is hidden. This ensures we retrieve the previous + // "Ready" version instead of the hidden latest version. + $onlyready = '((qv.status = :status))'; + $this->sqlparams = [ + 'status' => question_version_status::QUESTION_STATUS_READY, + 'substatus' => question_version_status::QUESTION_STATUS_HIDDEN, + ]; $conditions = []; foreach ($this->searchconditions as $searchcondition) { if ($searchcondition->where()) { diff --git a/mod/quiz/classes/question/bank/filter/custom_category_condition_helper.php b/mod/quiz/classes/question/bank/filter/custom_category_condition_helper.php index 5164f0f52bb..59dd3e45a05 100644 --- a/mod/quiz/classes/question/bank/filter/custom_category_condition_helper.php +++ b/mod/quiz/classes/question/bank/filter/custom_category_condition_helper.php @@ -102,8 +102,11 @@ class custom_category_condition_helper extends \qbank_managecategories\helper { bool $top = false, int $showallversions = 0): array { global $DB; $topwhere = $top ? '' : 'AND c.parent <> 0'; - $statuscondition = "AND qv.status = '". question_version_status::QUESTION_STATUS_READY . "' "; - + $statuscondition = "AND qv.status = :status"; + $params = [ + 'status' => question_version_status::QUESTION_STATUS_READY, + 'substatus' => question_version_status::QUESTION_STATUS_HIDDEN, + ]; $sql = "SELECT c.*, (SELECT COUNT(1) FROM {question} q @@ -116,7 +119,7 @@ class custom_category_condition_helper extends \qbank_managecategories\helper { OR (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) + WHERE be.id = qbe.id AND v.status <> :substatus) ) ) ) AS questioncount @@ -124,6 +127,6 @@ class custom_category_condition_helper extends \qbank_managecategories\helper { WHERE c.contextid IN ($contexts) $topwhere ORDER BY $sortorder"; - return $DB->get_records_sql($sql); + return $DB->get_records_sql($sql, $params); } } diff --git a/mod/quiz/tests/quiz_question_bank_view_test.php b/mod/quiz/tests/quiz_question_bank_view_test.php index 29ad473b522..ad9066ccd63 100644 --- a/mod/quiz/tests/quiz_question_bank_view_test.php +++ b/mod/quiz/tests/quiz_question_bank_view_test.php @@ -31,7 +31,7 @@ require_once($CFG->dirroot . '/question/editlib.php'); * @category test * @copyright 2018 the Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - * @covers \core_question\local\bank\view + * @covers \mod_quiz\question\bank\custom_view */ final class quiz_question_bank_view_test extends \advanced_testcase { @@ -82,6 +82,59 @@ final class quiz_question_bank_view_test extends \advanced_testcase { $this->assertFalse($cache->has($questiondata->id)); } + public function test_viewing_question_bank_should_not_load_hidden_question(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + $generator = $this->getDataGenerator(); + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $generator->get_plugin_generator('core_question'); + + // Create a course and a quiz. + $course = $generator->create_course(); + $quiz = $this->getDataGenerator()->create_module('quiz', ['course' => $course->id]); + $context = \context_module::instance($quiz->cmid); + $cm = get_coursemodule_from_instance('quiz', $quiz->id); + + // Create a question in the default category. + $contexts = new question_edit_contexts($context); + question_make_default_categories($contexts->all()); + $cat = question_get_default_category($context->id); + $question = $questiongenerator->create_question('numerical', null, + ['name' => 'Example question', 'category' => $cat->id]); + + // Create another version. + $newversion = $questiongenerator->update_question($question, null, ['name' => 'This is the latest version']); + + // Add them to the quiz. + quiz_add_quiz_question($newversion->id, $quiz); + // Generate the view. + $params = [ + 'qpage' => 0, + 'qperpage' => 20, + 'cat' => $cat->id . ',' . $context->id, + 'recurse' => false, + 'showhidden' => false, + 'qbshowtext' => false, + 'tabname' => 'editq', + ]; + $extraparams = ['cmid' => $cm->id]; + $view = new custom_view($contexts, new \moodle_url('/'), $course, $cm, $params, $extraparams); + ob_start(); + $view->display(); + $html = ob_get_clean(); + // Verify the output should included the latest version. + $this->assertStringContainsString('This is the latest version', $html); + $this->assertStringNotContainsString('Example question', $html); + // Delete the latest version. + question_delete_question($newversion->id); + // Verify the output should display the old version with status ready. + ob_start(); + $view->display(); + $html = ob_get_clean(); + $this->assertStringContainsString('Example question', $html); + $this->assertStringNotContainsString('This is the latest version', $html); + } + public function test_viewing_question_bank_when_paging_out_of_limit(): void { $this->resetAfterTest(); $this->setAdminUser(); diff --git a/question/bank/managecategories/classes/helper.php b/question/bank/managecategories/classes/helper.php index cbe0bfc4545..f590da5a549 100644 --- a/question/bank/managecategories/classes/helper.php +++ b/question/bank/managecategories/classes/helper.php @@ -308,9 +308,11 @@ class helper { ): array { global $DB; $topwhere = $top ? '' : 'AND c.parent <> 0'; - $statuscondition = "AND (qv.status = '" . question_version_status::QUESTION_STATUS_READY . "' " . - " OR qv.status = '" . question_version_status::QUESTION_STATUS_DRAFT . "' )"; - + $statuscondition = "AND qv.status <> :status"; + $params = [ + 'status' => question_version_status::QUESTION_STATUS_HIDDEN, + 'substatus' => question_version_status::QUESTION_STATUS_HIDDEN, + ]; $sql = "SELECT c.*, (SELECT COUNT(1) FROM {question} q @@ -323,7 +325,7 @@ class helper { OR (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) + WHERE be.id = qbe.id AND v.status <> :substatus) ) ) ) AS questioncount @@ -331,7 +333,7 @@ class helper { WHERE c.contextid IN ($contexts) $topwhere ORDER BY $sortorder"; - return $DB->get_records_sql($sql); + return $DB->get_records_sql($sql, $params); } /** diff --git a/question/bank/managecategories/tests/helper_test.php b/question/bank/managecategories/tests/helper_test.php index ed8be52e7ce..169f59b30fa 100644 --- a/question/bank/managecategories/tests/helper_test.php +++ b/question/bank/managecategories/tests/helper_test.php @@ -295,4 +295,34 @@ final class helper_test extends manage_category_test_base { $this->assertCount($count + 1, $categorycontext); } } + + /** + * Test that get_categories_for_contexts function returns the correct question count number. + * + * @covers ::get_categories_for_contexts + */ + public function test_question_category_question_count(): void { + global $DB; + // Create quiz. + $quiz = $this->quiz; + // Create category 1 and one hidden question. + $qcat = $this->qgenerator->create_question_category(['contextid' => $this->context->id]); + $q1 = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcat->id]); + $DB->set_field('question_versions', 'status', 'hidden', ['questionid' => $q1->id]); + + $contexts = new \core_question\local\bank\question_edit_contexts(\context_module::instance($quiz->cmid)); + $contexts = $contexts->having_cap('moodle/question:add'); + foreach ($contexts as $context) { + $contextslist[] = $context->id; + } + $contextslist = join(', ', $contextslist); + // Verify we have 0 question in category since it is hidden. + $categorycontexts = helper::get_categories_for_contexts($contextslist); + $this->assertEquals(0, reset($categorycontexts)->questioncount); + // Add an extra question. + $this->qgenerator->create_question('shortanswer', null, ['category' => $qcat->id]); + $categorycontexts = helper::get_categories_for_contexts($contextslist); + // Verify we have 1 question in category. + $this->assertEquals(1, reset($categorycontexts)->questioncount); + } } diff --git a/question/classes/local/bank/view.php b/question/classes/local/bank/view.php index 8ee30c9f1ba..ff5f50851f8 100644 --- a/question/classes/local/bank/view.php +++ b/question/classes/local/bank/view.php @@ -746,16 +746,16 @@ class view { [$colname, $subsort] = $this->parse_subsort($sortname); $sorts[] = $this->requiredcolumns[$colname]->sort_expression($sortorder == SORT_DESC, $subsort); } - - // Build the where clause. - $latestversion = '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)'; $this->sqlparams = []; $conditions = []; + $showhiddenquestion = true; + // Build the where clause. foreach ($this->searchconditions as $searchcondition) { + // TODO: Looking at the contents of params like this is not great. It is just a short-term fix. + // This will be solved properly when MDL-84433 is done. + if (array_key_exists('hidden_condition', $searchcondition->params())) { + $showhiddenquestion = false; + } if ($searchcondition->where()) { $conditions[] = '((' . $searchcondition->where() .'))'; } @@ -763,6 +763,18 @@ class view { $this->sqlparams = array_merge($this->sqlparams, $searchcondition->params()); } } + $extracondition = ''; + if (!$showhiddenquestion) { + // If Show hidden question option is off, then we need get the latest version that is not hidden. + $extracondition = ' AND v.status <> :hiddenstatus'; + $this->sqlparams = array_merge($this->sqlparams, ['hiddenstatus' => question_version_status::QUESTION_STATUS_HIDDEN]); + } + $latestversion = "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 $extracondition)"; + // Get higher level filter condition. $jointype = isset($this->pagevars['jointype']) ? (int)$this->pagevars['jointype'] : condition::JOINTYPE_DEFAULT; $nonecondition = ($jointype === datafilter::JOINTYPE_NONE) ? ' NOT ' : ''; diff --git a/question/tests/question_bank_view_test.php b/question/tests/question_bank_view_test.php new file mode 100644 index 00000000000..03cc0ab4542 --- /dev/null +++ b/question/tests/question_bank_view_test.php @@ -0,0 +1,108 @@ +. + +namespace core_question; + +use core_question\local\bank\question_edit_contexts; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/question/editlib.php'); + +/** + * Unit tests for the question own question bank view class. + * + * @package core_question + * @copyright 2024 the Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core_question\local\bank\view + */ +final class question_bank_view_test extends \advanced_testcase { + + public function test_viewing_question_bank_should_not_load_hidden_question(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + $generator = $this->getDataGenerator(); + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $generator->get_plugin_generator('core_question'); + + // Create a course and a quiz. + $course = $generator->create_course(); + $quiz = $this->getDataGenerator()->create_module('quiz', ['course' => $course->id]); + $context = \context_module::instance($quiz->cmid); + $cm = get_coursemodule_from_instance('quiz', $quiz->id); + + // Create a question in the default category. + $contexts = new question_edit_contexts($context); + question_make_default_categories($contexts->all()); + $cat = question_get_default_category($context->id); + $question = $questiongenerator->create_question('numerical', null, + ['name' => 'Example question', 'category' => $cat->id]); + // Create another version. + $newversion = $questiongenerator->update_question($question, null, ['name' => 'This is the latest version']); + // Add them to the quiz. + quiz_add_quiz_question($newversion->id, $quiz); + // Generate the view. + $params = [ + 'qpage' => 0, + 'qperpage' => 20, + 'cat' => $cat->id . ',' . $context->id, + 'recurse' => false, + 'qbshowtext' => false, + 'tabname' => 'editq', + ]; + $extraparams = ['cmid' => $cm->id]; + $view = new \core_question\local\bank\view($contexts, new \moodle_url('/'), $course, $cm, $params, $extraparams); + ob_start(); + $view->display(); + $html = ob_get_clean(); + // Verify the output should included the latest version. + $this->assertStringContainsString('This is the latest version', $html); + $this->assertStringNotContainsString('Example question', $html); + // Delete the latest version. + question_delete_question($newversion->id); + + // Verify the output should display the old version with status ready. + ob_start(); + $view->display(); + $html = ob_get_clean(); + $this->assertStringContainsString('Example question', $html); + $this->assertStringNotContainsString('This is the latest version', $html); + + // Use show hidden question filter. + $params['filter'] = [ + 'category' => [ + 'name' => 'category', + 'jointype' => 1, + 'values' => [$cat->id], + 'filteroptions' => [], + ], + 'hidden' => [ + 'name' => 'hidden', + 'jointype' => 1, + 'values' => [1], + 'filteroptions' => [], + ], + ]; + $view = new \core_question\local\bank\view($contexts, new \moodle_url('/'), $course, $cm, $params, $extraparams); + ob_start(); + $view->display(); + $html = ob_get_clean(); + $this->assertStringContainsString('This is the latest version', $html); + $this->assertStringNotContainsString('Example question', $html); + } +}