diff --git a/public/question/classes/local/bank/question_bank_helper.php b/public/question/classes/local/bank/question_bank_helper.php index 477b4e82d2d..2df94efc8e5 100644 --- a/public/question/classes/local/bank/question_bank_helper.php +++ b/public/question/classes/local/bank/question_bank_helper.php @@ -19,6 +19,7 @@ namespace core_question\local\bank; use cm_info; use context; use context_course; +use core\context_helper; use core\task\manager; use moodle_url; use stdClass; @@ -227,6 +228,16 @@ class question_bank_helper { $pluginssql = []; $params = []; + if ($getcategories || !empty($havingcap)) { + $contextselect = ', ' . context_helper::get_preload_record_columns_sql('c'); + $contextsql = ' JOIN {context} c ON c.instanceid = cm.id AND c.contextlevel = ' . CONTEXT_MODULE . ' '; + $contextgroupby = ', ' . implode(', ', array_keys(context_helper::get_preload_record_columns('c'))); + } else { + $contextselect = ''; + $contextsql = ''; + $contextgroupby = ''; + } + // Build the SELECT portion of the SQL and include question category joins as required. if ($getcategories) { $concat = $DB->sql_concat('qc.id', @@ -237,8 +248,7 @@ class question_bank_helper { ); $groupconcat = $DB->sql_group_concat($concat, self::CATEGORY_SEPARATOR); $select = "SELECT cm.id, cm.course, {$groupconcat} AS cats"; - $catsql = ' JOIN {context} c ON c.instanceid = cm.id AND c.contextlevel = ' . CONTEXT_MODULE . - ' JOIN {question_categories} qc ON qc.contextid = c.id AND qc.parent <> 0'; + $catsql = ' JOIN {question_categories} qc ON qc.contextid = c.id AND qc.parent <> 0'; } else { $select = 'SELECT cm.id, cm.course'; $catsql = ''; @@ -296,22 +306,27 @@ class question_bank_helper { $orderbysql = ''; } - $sql = "{$select} + $sql = "{$select} {$contextselect} FROM {course_modules} cm JOIN {modules} m ON m.id = cm.module {$pluginssql} + {$contextsql} {$catsql} WHERE 1=1 {$notincoursesql} {$incoursesql} - GROUP BY cm.id, cm.course + GROUP BY cm.id, cm.course {$contextgroupby} {$orderbysql}"; - $rs = $DB->get_recordset_sql($sql, $params, limitnum: $limit); + $limitforsql = $limit !== 0 && !empty($havingcap) ? 0 : $limit; + $rs = $DB->get_recordset_sql($sql, $params, limitnum: $limitforsql); $banks = []; foreach ($rs as $cm) { // If capabilities have been supplied as a method argument then ensure the viewing user has at least one of those // capabilities on the module itself. if (!empty($havingcap)) { + // We can preload because we made sure that in case of capabilities being passed we have the context joined in the + // SQL. + context_helper::preload_from_record($cm); $context = \context_module::instance($cm->id); if (!(new question_edit_contexts($context))->have_one_cap($havingcap)) { continue; @@ -319,6 +334,9 @@ class question_bank_helper { } // Populate the raw record. $banks[] = self::get_formatted_bank($cm, $currentbankid, filtercontext: $filtercontext); + if (!empty($limit) && count($banks) === $limit) { + break; + } } $rs->close(); diff --git a/public/question/tests/local/bank/question_bank_helper_test.php b/public/question/tests/local/bank/question_bank_helper_test.php index 5a40d7bdcb6..dbf363caae8 100644 --- a/public/question/tests/local/bank/question_bank_helper_test.php +++ b/public/question/tests/local/bank/question_bank_helper_test.php @@ -160,6 +160,54 @@ final class question_bank_helper_test extends \advanced_testcase { $this->assertEquals(1, $count); } + /** + * Tests if applying the limit and capability checks are interacting properly. + * + * @covers ::get_activity_instances_with_shareable_questions + * @covers ::get_activity_instances_with_private_questions + */ + public function test_get_instances_with_limit_and_capabilities(): void { + global $DB; + $this->resetAfterTest(); + $course = $this->getDataGenerator()->create_course(); + $teacher = self::getDataGenerator()->create_user(); + self::setUser($teacher); + $editingteacherroleid = $DB->get_record('role', ['shortname' => 'editingteacher'])->id; + + $sharedmodgen = self::getDataGenerator()->get_plugin_generator('mod_qbank'); + // Create 20 question banks, and give the teacher permission to edit only in the last 5. + for ($i = 0; $i < 20; $i++) { + $sharedmod = $sharedmodgen->create_instance(['course' => $course]); + if ($i >= 15) { + role_assign($editingteacherroleid, $teacher->id, \context_module::instance($sharedmod->cmid)); + } + } + + // We now have created 20 banks. If the limit is below 20, we have to make sure that the code does NOT first apply a limit + // of, for example, 15 and check capabilities afterward. This would mean we end up returning 0 qbanks. + $sharedbanks = question_bank_helper::get_activity_instances_with_shareable_questions( + havingcap: ['moodle/question:add'], + limit: 15 + ); + $this->assertCount(5, $sharedbanks); + + // On the other hand, check if the limit parameter works at all and is being applied correctly. + $sharedbanks = question_bank_helper::get_activity_instances_with_shareable_questions( + havingcap: ['moodle/question:add'], + limit: 2 + ); + $this->assertCount(2, $sharedbanks); + + $sharedbanks = question_bank_helper::get_activity_instances_with_shareable_questions(limit: 10); + $this->assertCount(10, $sharedbanks); + + $sharedbanks = question_bank_helper::get_activity_instances_with_shareable_questions(limit: 30); + $this->assertCount(20, $sharedbanks); + + $sharedbanks = question_bank_helper::get_activity_instances_with_shareable_questions(limit: 0); + $this->assertCount(20, $sharedbanks); + } + /** * We should be able to filter sharable question bank instances by name. *