From be4d569ad0ee3aafcf4d55982bb5c8d9940af012 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 | 54 ++++++++- .../bank/managecategories/classes/helper.php | 4 +- .../managecategories/tests/helper_test.php | 30 +++++ question/classes/local/bank/view.php | 25 ++-- question/tests/question_bank_view_test.php | 107 ++++++++++++++++++ 7 files changed, 227 insertions(+), 17 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 d3c627d739a..2b731556743 100644 --- a/mod/quiz/classes/question/bank/custom_view.php +++ b/mod/quiz/classes/question/bank/custom_view.php @@ -253,9 +253,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 37d0fec06db..e651b4339bf 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 @@ -105,8 +105,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 @@ -119,7 +122,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 @@ -127,6 +130,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 e1953a4e313..83d529837bd 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 { @@ -81,6 +81,58 @@ 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); + $cat = question_get_default_category($context->id, true); + $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 = ['quizcmid' => $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 850be91a280..bd3a94f8f21 100644 --- a/question/bank/managecategories/classes/helper.php +++ b/question/bank/managecategories/classes/helper.php @@ -322,7 +322,7 @@ class helper { $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 . "' )"; - + $substatuscondition = "AND v.status <> '" . question_version_status::QUESTION_STATUS_HIDDEN . "' "; $sql = "SELECT c.*, (SELECT COUNT(1) FROM {question} q @@ -335,7 +335,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 $substatuscondition) ) ) ) AS questioncount diff --git a/question/bank/managecategories/tests/helper_test.php b/question/bank/managecategories/tests/helper_test.php index cb687f1175a..a10f8f3b7d7 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->create_question_category_for_a_quiz($quiz); + $q1 = $this->create_question_in_a_category('shortanswer', $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->create_question_in_a_category('shortanswer', $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 4dee50d7e3a..e93feff7167 100644 --- a/question/classes/local/bank/view.php +++ b/question/classes/local/bank/view.php @@ -728,16 +728,15 @@ 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) { + // This is nasty hack to check if the filter is using "Show hidden question" option. + if (array_key_exists('hidden_condition', $searchcondition->params())) { + $showhiddenquestion = false; + } if ($searchcondition->where()) { $conditions[] = '((' . $searchcondition->where() .'))'; } @@ -745,6 +744,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..463ed683311 --- /dev/null +++ b/question/tests/question_bank_view_test.php @@ -0,0 +1,107 @@ +. + +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); + $cat = question_get_default_category($context->id, true); + $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); + } +}