From 47ce8f57b71c09ed1e5e74a9ed8200499dc4d77f Mon Sep 17 00:00:00 2001 From: Hieu Nguyen Dang Date: Wed, 3 Jul 2024 15:05:31 +0700 Subject: [PATCH] MDL-79265 question bank: fix which questions are listed with hidden This fixes the bug where if the latest version of the question is hidden, the question disappears, even if older versions exist that are still visible. --- .../classes/question/bank/custom_view.php | 13 ++- .../custom_category_condition_helper.php | 11 +- .../tests/quiz_question_bank_view_test.php | 55 ++++++++- .../bank/managecategories/classes/helper.php | 12 +- .../managecategories/tests/helper_test.php | 30 +++++ question/classes/local/bank/view.php | 26 +++-- question/tests/question_bank_view_test.php | 108 ++++++++++++++++++ 7 files changed, 235 insertions(+), 20 deletions(-) create mode 100644 question/tests/question_bank_view_test.php diff --git a/mod/quiz/classes/question/bank/custom_view.php b/mod/quiz/classes/question/bank/custom_view.php index 572ba8daa1b..601f8125738 100644 --- a/mod/quiz/classes/question/bank/custom_view.php +++ b/mod/quiz/classes/question/bank/custom_view.php @@ -245,9 +245,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 03b241bc149..30d67366986 100644 --- a/question/classes/local/bank/view.php +++ b/question/classes/local/bank/view.php @@ -745,16 +745,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() .'))'; } @@ -762,6 +762,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); + } +}