From ebeaa4b3f2c85386b3f57ce90e255666daa1c831 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 2 Apr 2021 18:01:21 -0700 Subject: [PATCH] MDL-71261 mod_quiz: Quiz user override should only get enrolled users Also update similar code in mod_assign to use the improved APIs. Co-Authored-By: Tim Hunt --- lib/enrollib.php | 26 +++---- lib/tests/accesslib_test.php | 74 ++++++++++++++++++- lib/upgrade.txt | 11 +++ mod/assign/override_form.php | 23 ++---- mod/lesson/override_form.php | 23 ++---- mod/quiz/override_form.php | 37 +++------- .../tests/behat/quiz_user_override.feature | 22 ++++++ 7 files changed, 144 insertions(+), 72 deletions(-) diff --git a/lib/enrollib.php b/lib/enrollib.php index 8f1cb302cac..31e3735de44 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -1453,13 +1453,13 @@ function is_enrolled(context $context, $user = null, $withcapability = '', $only * @param string|array $capability optional, may include a capability name, or array of names. * If an array is provided then this is the equivalent of a logical 'OR', * i.e. the user needs to have one of these capabilities. - * @param int $group optional, 0 indicates no current group and USERSWITHOUTGROUP users without any group; otherwise the group id + * @param int|array $groupids The groupids, 0 or [] means all groups and USERSWITHOUTGROUP no group * @param bool $onlyactive consider only active enrolments in enabled plugins and time restrictions * @param bool $onlysuspended inverse of onlyactive, consider only suspended enrolments * @param int $enrolid The enrolment ID. If not 0, only users enrolled using this enrolment method will be returned. * @return \core\dml\sql_join Contains joins, wheres, params and cannotmatchanyrows */ -function get_enrolled_with_capabilities_join(context $context, $prefix = '', $capability = '', $group = 0, +function get_enrolled_with_capabilities_join(context $context, $prefix = '', $capability = '', $groupids = 0, $onlyactive = false, $onlysuspended = false, $enrolid = 0) { $uid = $prefix . 'u.id'; $joins = array(); @@ -1480,8 +1480,8 @@ function get_enrolled_with_capabilities_join(context $context, $prefix = '', $ca $cannotmatchanyrows = $cannotmatchanyrows || $capjoin->cannotmatchanyrows; } - if ($group) { - $groupjoin = groups_get_members_join($group, $uid, $context); + if ($groupids) { + $groupjoin = groups_get_members_join($groupids, $uid, $context); $joins[] = $groupjoin->joins; $params = array_merge($params, $groupjoin->params); if (!empty($groupjoin->wheres)) { @@ -1505,13 +1505,13 @@ function get_enrolled_with_capabilities_join(context $context, $prefix = '', $ca * * @param context $context * @param string $withcapability - * @param int $groupid 0 means ignore groups, USERSWITHOUTGROUP without any group and any other value limits the result by group id + * @param int|array $groupids The groupids, 0 or [] means all groups and USERSWITHOUTGROUP no group * @param bool $onlyactive consider only active enrolments in enabled plugins and time restrictions * @param bool $onlysuspended inverse of onlyactive, consider only suspended enrolments * @param int $enrolid The enrolment ID. If not 0, only users enrolled using this enrolment method will be returned. * @return array list($sql, $params) */ -function get_enrolled_sql(context $context, $withcapability = '', $groupid = 0, $onlyactive = false, $onlysuspended = false, +function get_enrolled_sql(context $context, $withcapability = '', $groupids = 0, $onlyactive = false, $onlysuspended = false, $enrolid = 0) { // Use unique prefix just in case somebody makes some SQL magic with the result. @@ -1520,7 +1520,7 @@ function get_enrolled_sql(context $context, $withcapability = '', $groupid = 0, $prefix = 'eu' . $i . '_'; $capjoin = get_enrolled_with_capabilities_join( - $context, $prefix, $withcapability, $groupid, $onlyactive, $onlysuspended, $enrolid); + $context, $prefix, $withcapability, $groupids, $onlyactive, $onlysuspended, $enrolid); $sql = "SELECT DISTINCT {$prefix}u.id FROM {user} {$prefix}u @@ -1631,7 +1631,7 @@ function get_enrolled_join(context $context, $useridcolumn, $onlyactive = false, * * @param context $context * @param string $withcapability - * @param int $groupid 0 means ignore groups, USERSWITHOUTGROUP without any group and any other value limits the result by group id + * @param int|array $groupids The groupids, 0 or [] means all groups and USERSWITHOUTGROUP no group * @param string $userfields requested user record fields * @param string $orderby * @param int $limitfrom return a subset of records, starting at this point (optional, required if $limitnum is set). @@ -1639,11 +1639,11 @@ function get_enrolled_join(context $context, $useridcolumn, $onlyactive = false, * @param bool $onlyactive consider only active enrolments in enabled plugins and time restrictions * @return array of user records */ -function get_enrolled_users(context $context, $withcapability = '', $groupid = 0, $userfields = 'u.*', $orderby = null, +function get_enrolled_users(context $context, $withcapability = '', $groupids = 0, $userfields = 'u.*', $orderby = null, $limitfrom = 0, $limitnum = 0, $onlyactive = false) { global $DB; - list($esql, $params) = get_enrolled_sql($context, $withcapability, $groupid, $onlyactive); + list($esql, $params) = get_enrolled_sql($context, $withcapability, $groupids, $onlyactive); $sql = "SELECT $userfields FROM {user} u JOIN ($esql) je ON je.id = u.id @@ -1665,15 +1665,15 @@ function get_enrolled_users(context $context, $withcapability = '', $groupid = 0 * * @param context $context * @param string $withcapability - * @param int $groupid 0 means ignore groups, any other value limits the result by group id + * @param int|array $groupids The groupids, 0 or [] means all groups and USERSWITHOUTGROUP no group * @param bool $onlyactive consider only active enrolments in enabled plugins and time restrictions * @return int number of users enrolled into course */ -function count_enrolled_users(context $context, $withcapability = '', $groupid = 0, $onlyactive = false) { +function count_enrolled_users(context $context, $withcapability = '', $groupids = 0, $onlyactive = false) { global $DB; $capjoin = get_enrolled_with_capabilities_join( - $context, '', $withcapability, $groupid, $onlyactive); + $context, '', $withcapability, $groupids, $onlyactive); $sql = "SELECT COUNT(DISTINCT u.id) FROM {user} u diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php index 1a5448d4c09..5ee21daa3f4 100644 --- a/lib/tests/accesslib_test.php +++ b/lib/tests/accesslib_test.php @@ -2745,7 +2745,13 @@ class accesslib_test extends advanced_testcase { * Test that enrolled users SQL does not return any values for users in * other courses. * + * * @covers ::get_enrolled_users + * @covers ::get_enrolled_sql + * @covers ::get_enrolled_with_capabilities_join + * @covers ::get_enrolled_join + * @covers ::get_with_capability_join + * @covers ::groups_get_members_join * @covers ::get_suspended_userids */ public function test_get_enrolled_sql_different_course() { @@ -2778,7 +2784,13 @@ class accesslib_test extends advanced_testcase { * Test that enrolled users SQL does not return any values for role * assignments without an enrolment. * + * * @covers ::get_enrolled_users + * @covers ::get_enrolled_sql + * @covers ::get_enrolled_with_capabilities_join + * @covers ::get_enrolled_join + * @covers ::get_with_capability_join + * @covers ::groups_get_members_join * @covers ::get_suspended_userids */ public function test_get_enrolled_sql_role_only() { @@ -2810,6 +2822,11 @@ class accesslib_test extends advanced_testcase { * Test that multiple enrolments for the same user are counted correctly. * * @covers ::get_enrolled_users + * @covers ::get_enrolled_sql + * @covers ::get_enrolled_with_capabilities_join + * @covers ::get_enrolled_join + * @covers ::get_with_capability_join + * @covers ::groups_get_members_join * @covers ::get_suspended_userids */ public function test_get_enrolled_sql_multiple_enrolments() { @@ -2857,11 +2874,66 @@ class accesslib_test extends advanced_testcase { } + /** + * Test that enrolled users returns only users in those groups that are + * specified. + * + * @covers ::get_enrolled_users + * @covers ::get_enrolled_sql + * @covers ::get_enrolled_with_capabilities_join + * @covers ::get_enrolled_join + * @covers ::get_with_capability_join + * @covers ::groups_get_members_join + * @covers ::get_suspended_userids + */ + public function test_get_enrolled_sql_userswithgroups() { + $this->resetAfterTest(); + + $systemcontext = context_system::instance(); + $course = $this->getDataGenerator()->create_course(); + $coursecontext = context_course::instance($course->id); + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + + $this->getDataGenerator()->enrol_user($user1->id, $course->id); + $this->getDataGenerator()->enrol_user($user2->id, $course->id); + + $group1 = $this->getDataGenerator()->create_group(['courseid' => $course->id]); + groups_add_member($group1, $user1); + $group2 = $this->getDataGenerator()->create_group(['courseid' => $course->id]); + groups_add_member($group2, $user2); + + // Get user from group 1. + $group1users = get_enrolled_users($coursecontext, '', $group1->id); + $this->assertCount(1, $group1users); + $this->assertArrayHasKey($user1->id, $group1users); + $this->assertEquals(1, count_enrolled_users($coursecontext, '', $group1->id)); + + // Get user from group 2. + $group2users = get_enrolled_users($coursecontext, '', $group2->id); + $this->assertCount(1, $group2users); + $this->assertArrayHasKey($user2->id, $group2users); + $this->assertEquals(1, count_enrolled_users($coursecontext, '', $group2->id)); + + // Get users from multiple groups. + $groupusers = get_enrolled_users($coursecontext, '', [$group1->id, $group2->id]); + $this->assertCount(2, $groupusers); + $this->assertArrayHasKey($user1->id, $groupusers); + $this->assertArrayHasKey($user2->id, $groupusers); + $this->assertEquals(2, count_enrolled_users($coursecontext, '', [$group1->id, $group2->id])); + } + /** * Test that enrolled users SQL does not return any values for users * without a group when $context is not a valid course context. * * @covers ::get_enrolled_users + * @covers ::get_enrolled_sql + * @covers ::get_enrolled_with_capabilities_join + * @covers ::get_enrolled_join + * @covers ::get_with_capability_join + * @covers ::groups_get_members_join + * @covers ::get_suspended_userids */ public function test_get_enrolled_sql_userswithoutgroup() { global $DB; @@ -2880,7 +2952,7 @@ class accesslib_test extends advanced_testcase { $group = $this->getDataGenerator()->create_group(array('courseid' => $course->id)); groups_add_member($group, $user1); - $enrolled = get_enrolled_users($coursecontext); + $enrolled = get_enrolled_users($coursecontext); $this->assertCount(2, $enrolled); // Get users without any group on the course context. diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 8a2a696670f..87a9f4d63a0 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -1,5 +1,15 @@ This files describes API changes in core libraries and APIs, information provided here is intended especially for developers. + +=== 4.2 === + +* In enrollib.php, the methods get_enrolled_with_capabilities_join, get_enrolled_sql, get_enrolled_users and + count_enrolled_users used to only be able to accept a single group id, even though internally, they used + groups_get_members_join which could also accept an array of groups, or the constant USERSWITHOUTGROUP. + This has now been made consistent. These enrol methods now accept all the group-related options that + groups_get_members_join can handle. + + === 4.1 === * HTMLPurifier has been upgraded to the latest version - 4.16.0 @@ -696,6 +706,7 @@ filepath because some components, such as mod_page or mod_resource, add the revi call the above function because the file is sent by a third party library, then you should add the attribute data-double-submit-protection="off" to your form. + === 3.7 === * Nodes in the navigation api can have labels for each group. See set/get_collectionlabel(). diff --git a/mod/assign/override_form.php b/mod/assign/override_form.php index cce7bf41a62..537c311b7bb 100644 --- a/mod/assign/override_form.php +++ b/mod/assign/override_form.php @@ -157,23 +157,14 @@ class assign_override_form extends moodleform { // Get the list of appropriate users, depending on whether and how groups are used. $userfieldsapi = \core_user\fields::for_name(); - if ($accessallgroups) { - $users = get_enrolled_users($this->context, '', 0, - 'u.id, u.email, ' . $userfieldsapi->get_sql('u', false, '', '', false)->selects, $sort); - } else if ($groups = groups_get_activity_allowed_groups($cm)) { - $enrolledjoin = get_enrolled_join($this->context, 'u.id'); - $userfields = 'u.id, u.email, ' . $userfieldsapi->get_sql('u', false, '', '', false)->selects; - list($ingroupsql, $ingroupparams) = $DB->get_in_or_equal(array_keys($groups), SQL_PARAMS_NAMED); - $params = $enrolledjoin->params + $ingroupparams; - $sql = "SELECT $userfields - FROM {user} u - JOIN {groups_members} gm ON gm.userid = u.id - {$enrolledjoin->joins} - WHERE gm.groupid $ingroupsql - AND {$enrolledjoin->wheres} - ORDER BY $sort"; - $users = $DB->get_records_sql($sql, $params); + $userfields = 'u.id, u.email, ' . $userfieldsapi->get_sql('u', false, '', '', false)->selects; + $groupids = 0; + if (!$accessallgroups) { + $groups = groups_get_activity_allowed_groups($cm); + $groupids = array_keys($groups); } + $users = get_enrolled_users($this->context, '', + $groupids, $userfields, $sort); // Filter users based on any fixed restrictions (groups, profile). $info = new \core_availability\info_module($cm); diff --git a/mod/lesson/override_form.php b/mod/lesson/override_form.php index 7ee2b31b22d..de62efb7064 100644 --- a/mod/lesson/override_form.php +++ b/mod/lesson/override_form.php @@ -141,23 +141,14 @@ class lesson_override_form extends moodleform { // Get the list of appropriate users, depending on whether and how groups are used. $userfieldsapi = \core_user\fields::for_name(); - if ($accessallgroups) { - $users = get_enrolled_users($this->context, '', 0, - 'u.id, u.email, ' . $userfieldsapi->get_sql('u', false, '', '', false)->selects, $sort); - } else if ($groups = groups_get_activity_allowed_groups($cm)) { - $enrolledjoin = get_enrolled_join($this->context, 'u.id'); - $userfields = 'u.id, u.email, ' . $userfieldsapi->get_sql('u', false, '', '', false)->selects; - list($ingroupsql, $ingroupparams) = $DB->get_in_or_equal(array_keys($groups), SQL_PARAMS_NAMED); - $params = $enrolledjoin->params + $ingroupparams; - $sql = "SELECT $userfields - FROM {user} u - JOIN {groups_members} gm ON gm.userid = u.id - {$enrolledjoin->joins} - WHERE gm.groupid $ingroupsql - AND {$enrolledjoin->wheres} - ORDER BY $sort"; - $users = $DB->get_records_sql($sql, $params); + $userfields = 'u.id, u.email, ' . $userfieldsapi->get_sql('u', false, '', '', false)->selects; + $groupids = 0; + if (!$accessallgroups) { + $groups = groups_get_activity_allowed_groups($cm); + $groupids = array_keys($groups); } + $users = get_enrolled_users($this->context, '', + $groupids, $userfields, $sort); // Filter users based on any fixed restrictions (groups, profile). $info = new \core_availability\info_module($cm); diff --git a/mod/quiz/override_form.php b/mod/quiz/override_form.php index c44a4da696d..04f33033bbf 100644 --- a/mod/quiz/override_form.php +++ b/mod/quiz/override_form.php @@ -135,43 +135,28 @@ class quiz_override_form extends moodleform { $mform->freeze('userid'); } else { // Prepare the list of users. - - // Get the list of appropriate users, depending on whether and how groups are used. - $userfieldsql = $userfieldsapi->get_sql('u', true, '', '', false); - $capabilityjoin = get_with_capability_join($this->context, 'mod/quiz:attempt', 'u.id'); - list($sort, $sortparams) = users_order_by_sql('u', null, - $this->context, $userfieldsql->mappings); - - $groupjoin = ''; - $groupparams = []; + $groupids = 0; if (!$accessallgroups) { $groups = groups_get_activity_allowed_groups($cm); - list($grouptest, $groupparams) = $DB->get_in_or_equal( - array_keys($groups), SQL_PARAMS_NAMED, 'grp'); - $groupjoin = "JOIN (SELECT DISTINCT userid - FROM {groups_members} - WHERE groupid $grouptest - ) gm ON gm.userid = u.id"; - } - - $capabilitywhere = ''; - if ($capabilityjoin->wheres) { - $capabilitywhere = 'AND ' . $capabilityjoin->wheres; + $groupids = array_keys($groups); } + $enrolledjoin = get_enrolled_with_capabilities_join( + $this->context, '', 'mod/quiz:attempt', $groupids, true); + $userfieldsql = $userfieldsapi->get_sql('u', true, '', '', false); + list($sort, $sortparams) = users_order_by_sql('u', null, + $this->context, $userfieldsql->mappings); $users = $DB->get_records_sql(" SELECT $userfieldsql->selects FROM {user} u + $enrolledjoin->joins + $userfieldsql->joins LEFT JOIN {quiz_overrides} existingoverride ON existingoverride.userid = u.id AND existingoverride.quiz = :quizid - $capabilityjoin->joins - $groupjoin - $userfieldsql->joins WHERE existingoverride.id IS NULL - $capabilitywhere + AND $enrolledjoin->wheres ORDER BY $sort - ", array_merge(['quizid' => $this->quiz->id], $capabilityjoin->params, - $groupparams, $userfieldsql->params, $sortparams)); + ", array_merge(['quizid' => $this->quiz->id], $userfieldsql->params, $enrolledjoin->params, $sortparams)); // Filter users based on any fixed restrictions (groups, profile). $info = new \core_availability\info_module($cm); diff --git a/mod/quiz/tests/behat/quiz_user_override.feature b/mod/quiz/tests/behat/quiz_user_override.feature index 4ac10f0608d..25d702825b9 100644 --- a/mod/quiz/tests/behat/quiz_user_override.feature +++ b/mod/quiz/tests/behat/quiz_user_override.feature @@ -196,3 +196,25 @@ Feature: Quiz user override | quiz | Other quiz | C2 | quiz2 | When I am on the "Other quiz" "mod_quiz > User overrides" page logged in as "admin" Then the "Add user override" "button" should be disabled + + @javascript + Scenario: Should see only enrolled users in user selector + Given the following "users" exist: + | username | firstname | lastname | email | + | manager | Max | Manager | man@example.com | + And the following "role assigns" exist: + | user | role | contextlevel | reference | + | manager | manager | System | | + And the following "activities" exist: + | activity | name | course | idnumber | groupmode | + | quiz | Test quiz | C1 | quiz1 | 1 | + And I log in as "admin" + And I set the following system permissions of "Manager" role: + | capability | permission | + | mod/quiz:attempt | Allow | + And I log out + When I am on the "Test quiz" "mod_quiz > User overrides" page logged in as "teacher" + And I press "Add user override" + And I click on "Override user" "field" + And I type "Max Manager" + Then I should see "No suggestions"