From 9cfb533e2d79748e312bb2a6ab6e353eff0f02ec Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Fri, 13 Jul 2018 11:28:59 +0200 Subject: [PATCH] MDL-63485 core_user: Allow filtering by No groups on participants page A new optional parameter $context has been added to the core_group::groups_get_members_join() function. Besides, some core_group methods now accept -1 (USERSWITHOUTHGROUP) for the groupid field. Backport of MDL-61967. --- lib/enrollib.php | 11 +- lib/grouplib.php | 46 +++++-- lib/tests/accesslib_test.php | 34 ++++++ lib/tests/grouplib_test.php | 121 +++++++++++++++++-- lib/upgrade.txt | 6 + user/classes/participants_table.php | 2 +- user/index.php | 2 +- user/lib.php | 6 +- user/renderer.php | 5 + user/tests/behat/filter_participants.feature | 30 +++++ user/tests/userlib_test.php | 6 + 11 files changed, 244 insertions(+), 25 deletions(-) diff --git a/lib/enrollib.php b/lib/enrollib.php index 8a1c7c0eb61..4a714fb15c0 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -1290,7 +1290,7 @@ 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, otherwise the group id + * @param int $group optional, 0 indicates no current group and USERSWITHOUTGROUP users without any group; otherwise the group id * @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. @@ -1315,9 +1315,12 @@ function get_enrolled_with_capabilities_join(context $context, $prefix = '', $ca } if ($group) { - $groupjoin = groups_get_members_join($group, $uid); + $groupjoin = groups_get_members_join($group, $uid, $context); $joins[] = $groupjoin->joins; $params = array_merge($params, $groupjoin->params); + if (!empty($groupjoin->wheres)) { + $wheres[] = $groupjoin->wheres; + } } $joins = implode("\n", $joins); @@ -1335,7 +1338,7 @@ function get_enrolled_with_capabilities_join(context $context, $prefix = '', $ca * * @param context $context * @param string $withcapability - * @param int $groupid 0 means ignore groups, any other value limits the result by group id + * @param int $groupid 0 means ignore groups, USERSWITHOUTGROUP without any group and any other value limits the result by group id * @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. @@ -1461,7 +1464,7 @@ function get_enrolled_join(context $context, $useridcolumn, $onlyactive = false, * * @param context $context * @param string $withcapability - * @param int $groupid 0 means ignore groups, any other value limits the result by group id + * @param int $groupid 0 means ignore groups, USERSWITHOUTGROUP without any group and any other value limits the result by group id * @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). diff --git a/lib/grouplib.php b/lib/grouplib.php index 469a0128699..cbaebb8898c 100644 --- a/lib/grouplib.php +++ b/lib/grouplib.php @@ -38,6 +38,11 @@ define('SEPARATEGROUPS', 1); */ define('VISIBLEGROUPS', 2); +/** + * This is for filtering users without any group. + */ +define('USERSWITHOUTGROUP', -1); + /** * Determines if a group with a given groupid exists. @@ -1122,15 +1127,20 @@ function groups_group_visible($groupid, $course, $cm = null, $userid = null) { * Get sql and parameters that will return user ids for a group * * @param int $groupid + * @param context $context Course context or a context within a course. Mandatory when $groupid = USERSWITHOUTGROUP * @return array($sql, $params) + * @throws coding_exception if empty or invalid context submitted when $groupid = USERSWITHOUTGROUP */ -function groups_get_members_ids_sql($groupid) { - $groupjoin = groups_get_members_join($groupid, 'u.id'); +function groups_get_members_ids_sql($groupid, context $context = null) { + $groupjoin = groups_get_members_join($groupid, 'u.id', $context); $sql = "SELECT DISTINCT u.id FROM {user} u $groupjoin->joins WHERE u.deleted = 0"; + if (!empty($groupjoin->wheres)) { + $sql .= ' AND '. $groupjoin->wheres; + } return array($sql, $groupjoin->params); } @@ -1138,20 +1148,42 @@ function groups_get_members_ids_sql($groupid) { /** * Get sql join to return users in a group * - * @param int $groupid + * @param int $groupid The groupid, 0 means all groups and USERSWITHOUTGROUP no group * @param string $useridcolumn The column of the user id from the calling SQL, e.g. u.id + * @param context $context Course context or a context within a course. Mandatory when $groupid = USERSWITHOUTGROUP * @return \core\dml\sql_join Contains joins, wheres, params + * @throws coding_exception if empty or invalid context submitted when $groupid = USERSWITHOUTGROUP */ -function groups_get_members_join($groupid, $useridcolumn) { +function groups_get_members_join($groupid, $useridcolumn, context $context = null) { // Use unique prefix just in case somebody makes some SQL magic with the result. static $i = 0; $i++; $prefix = 'gm' . $i . '_'; - $join = "JOIN {groups_members} {$prefix}gm ON ({$prefix}gm.userid = $useridcolumn AND {$prefix}gm.groupid = :{$prefix}gmid)"; - $param = array("{$prefix}gmid" => $groupid); + $coursecontext = (!empty($context)) ? $context->get_course_context() : null; + if ($groupid == USERSWITHOUTGROUP && empty($coursecontext)) { + // Throw an exception if $context is empty or invalid because it's needed to get the users without any group. + throw new coding_exception('Missing or wrong $context parameter in an attempt to get members without any group'); + } - return new \core\dml\sql_join($join, '', $param); + if ($groupid == USERSWITHOUTGROUP) { + // Get members without any group. + $join = "LEFT JOIN ( + SELECT g.courseid, m.groupid, m.userid + FROM {groups_members} m + JOIN {groups} g ON g.id = m.groupid + ) {$prefix}gm ON ({$prefix}gm.userid = $useridcolumn AND {$prefix}gm.courseid = :{$prefix}gcourseid)"; + $where = "{$prefix}gm.userid IS NULL"; + $param = array("{$prefix}gcourseid" => $coursecontext->instanceid); + } else { + // Get members of defined groupid. + $join = "JOIN {groups_members} {$prefix}gm + ON ({$prefix}gm.userid = $useridcolumn AND {$prefix}gm.groupid = :{$prefix}gmid)"; + $where = ''; + $param = array("{$prefix}gmid" => $groupid); + } + + return new \core\dml\sql_join($join, $where, $param); } /** diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php index 2f9e8738368..6e26531653d 100644 --- a/lib/tests/accesslib_test.php +++ b/lib/tests/accesslib_test.php @@ -2335,6 +2335,40 @@ class core_accesslib_testcase extends advanced_testcase { } + /** + * Test that enrolled users SQL does not return any values for users + * without a group when $context is not a valid course context. + */ + public function test_get_enrolled_sql_userswithoutgroup() { + global $DB; + + $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); + + $group = $this->getDataGenerator()->create_group(array('courseid' => $course->id)); + groups_add_member($group, $user1); + + $enrolled = get_enrolled_users($coursecontext); + $this->assertCount(2, $enrolled); + + // Get users without any group on the course context. + $enrolledwithoutgroup = get_enrolled_users($coursecontext, '', USERSWITHOUTGROUP); + $this->assertCount(1, $enrolledwithoutgroup); + $this->assertFalse(isset($enrolledwithoutgroup[$user1->id])); + + // Get users without any group on the system context (it should throw an exception). + $this->expectException('coding_exception'); + get_enrolled_users($systemcontext, '', USERSWITHOUTGROUP); + } + public function get_enrolled_sql_provider() { return array( array( diff --git a/lib/tests/grouplib_test.php b/lib/tests/grouplib_test.php index 36413270b9d..5cc0b17ee25 100644 --- a/lib/tests/grouplib_test.php +++ b/lib/tests/grouplib_test.php @@ -176,7 +176,6 @@ class core_grouplib_testcase extends advanced_testcase { $this->assertEquals($grouping, groups_get_grouping_by_idnumber($course->id, $idnumber2)); } - public function test_groups_get_members_ids_sql() { global $DB; @@ -185,7 +184,9 @@ class core_grouplib_testcase extends advanced_testcase { $generator = $this->getDataGenerator(); $course = $generator->create_course(); - $student = $generator->create_user(); + $coursecontext = context_course::instance($course->id); + $student1 = $generator->create_user(); + $student2 = $generator->create_user(); $plugin = enrol_get_plugin('manual'); $role = $DB->get_record('role', array('shortname' => 'student')); $group = $generator->create_group(array('courseid' => $course->id)); @@ -196,20 +197,122 @@ class core_grouplib_testcase extends advanced_testcase { $this->assertNotEquals($instance, false); - // Enrol the user in the course. - $plugin->enrol_user($instance, $student->id, $role->id); + // Enrol users in the course. + $plugin->enrol_user($instance, $student1->id, $role->id); + $plugin->enrol_user($instance, $student2->id, $role->id); - list($sql, $params) = groups_get_members_ids_sql($group->id, true); + list($sql, $params) = groups_get_members_ids_sql($group->id); // Test an empty group. $users = $DB->get_records_sql($sql, $params); - - $this->assertFalse(array_key_exists($student->id, $users)); - groups_add_member($group->id, $student->id); + $this->assertFalse(array_key_exists($student1->id, $users)); // Test with a group member. + groups_add_member($group->id, $student1->id); $users = $DB->get_records_sql($sql, $params); - $this->assertTrue(array_key_exists($student->id, $users)); + $this->assertTrue(array_key_exists($student1->id, $users)); + } + + public function test_groups_get_members_ids_sql_valid_context() { + global $DB; + + $this->resetAfterTest(true); + + $generator = $this->getDataGenerator(); + + $course = $generator->create_course(); + $coursecontext = context_course::instance($course->id); + $student1 = $generator->create_user(); + $student2 = $generator->create_user(); + $plugin = enrol_get_plugin('manual'); + $role = $DB->get_record('role', array('shortname' => 'student')); + $group = $generator->create_group(array('courseid' => $course->id)); + $instance = $DB->get_record('enrol', array( + 'courseid' => $course->id, + 'enrol' => 'manual', + )); + + $this->assertNotEquals($instance, false); + + // Enrol users in the course. + $plugin->enrol_user($instance, $student1->id, $role->id); + $plugin->enrol_user($instance, $student2->id, $role->id); + + // Add student1 to the group. + groups_add_member($group->id, $student1->id); + + // Test with members at any group and with a valid $context. + list($sql, $params) = groups_get_members_ids_sql(USERSWITHOUTGROUP, $coursecontext); + $users = $DB->get_records_sql($sql, $params); + $this->assertFalse(array_key_exists($student1->id, $users)); + $this->assertTrue(array_key_exists($student2->id, $users)); + } + + public function test_groups_get_members_ids_sql_empty_context() { + global $DB; + + $this->resetAfterTest(true); + + $generator = $this->getDataGenerator(); + + $course = $generator->create_course(); + $coursecontext = context_course::instance($course->id); + $student1 = $generator->create_user(); + $student2 = $generator->create_user(); + $plugin = enrol_get_plugin('manual'); + $role = $DB->get_record('role', array('shortname' => 'student')); + $group = $generator->create_group(array('courseid' => $course->id)); + $instance = $DB->get_record('enrol', array( + 'courseid' => $course->id, + 'enrol' => 'manual', + )); + + $this->assertNotEquals($instance, false); + + // Enrol users in the course. + $plugin->enrol_user($instance, $student1->id, $role->id); + $plugin->enrol_user($instance, $student2->id, $role->id); + + // Add student1 to the group. + groups_add_member($group->id, $student1->id); + + // Test with members at any group and without the $context. + $this->expectException('coding_exception'); + list($sql, $params) = groups_get_members_ids_sql(USERSWITHOUTGROUP); + } + + public function test_groups_get_members_ids_sql_invalid_context() { + global $DB; + + $this->resetAfterTest(true); + + $generator = $this->getDataGenerator(); + + $course = $generator->create_course(); + $coursecontext = context_course::instance($course->id); + $student1 = $generator->create_user(); + $student2 = $generator->create_user(); + $plugin = enrol_get_plugin('manual'); + $role = $DB->get_record('role', array('shortname' => 'student')); + $group = $generator->create_group(array('courseid' => $course->id)); + $instance = $DB->get_record('enrol', array( + 'courseid' => $course->id, + 'enrol' => 'manual', + )); + + $this->assertNotEquals($instance, false); + + // Enrol users in the course. + $plugin->enrol_user($instance, $student1->id, $role->id); + $plugin->enrol_user($instance, $student2->id, $role->id); + + // Add student1 to the group. + groups_add_member($group->id, $student1->id); + + // Test with members at any group and with an invalid $context. + $syscontext = context_system::instance(); + $this->expectException('coding_exception'); + list($sql, $params) = groups_get_members_ids_sql(USERSWITHOUTGROUP, $syscontext); } public function test_groups_get_group_by_name() { diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 3d33af04334..f2468daed0b 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -1,6 +1,12 @@ This files describes API changes in core libraries and APIs, information provided here is intended especially for developers. +=== 3.5.4 === + +* New optional parameter $context for the groups_get_members_join() function and ability to filter users that are not members of +any group. Besides, groups_get_members_ids_sql, get_enrolled_sql and get_enrolled_users now accepts -1 (USERSWITHOUTGROUP) for +the groupid field. + === 3.5.2 === * Custom AJAX handlers for the form autocomplete fields can now optionally return string in their processResults() diff --git a/user/classes/participants_table.php b/user/classes/participants_table.php index 8f999f51b47..d6861180e55 100644 --- a/user/classes/participants_table.php +++ b/user/classes/participants_table.php @@ -136,7 +136,7 @@ class participants_table extends \table_sql { * Sets up the table. * * @param int $courseid - * @param int|false $currentgroup False if groups not used, int if groups used, 0 for all groups. + * @param int|false $currentgroup False if groups not used, int if groups used, 0 all groups, USERSWITHOUTGROUP for no group * @param int $accesssince The time the user last accessed the site * @param int $roleid The role we are including, 0 means all enrolled users * @param int $enrolid The applied filter for the user enrolment ID. diff --git a/user/index.php b/user/index.php index f36491cef55..fccd1df64ac 100644 --- a/user/index.php +++ b/user/index.php @@ -196,7 +196,7 @@ if (!empty($groupid)) { } } -if ($groupid && ($course->groupmode != SEPARATEGROUPS || $canaccessallgroups)) { +if ($groupid > 0 && ($course->groupmode != SEPARATEGROUPS || $canaccessallgroups)) { $grouprenderer = $PAGE->get_renderer('core_group'); $groupdetailpage = new \core_group\output\group_details($groupid); echo $grouprenderer->group_details($groupdetailpage); diff --git a/user/lib.php b/user/lib.php index dc6102003b6..7bb4f5f4070 100644 --- a/user/lib.php +++ b/user/lib.php @@ -1245,7 +1245,7 @@ function user_get_tagged_users($tag, $exclusivemode = false, $fromctx = 0, $ctx * Returns the SQL used by the participants table. * * @param int $courseid The course id - * @param int $groupid The groupid, 0 means all groups + * @param int $groupid The groupid, 0 means all groups and USERSWITHOUTGROUP no group * @param int $accesssince The time since last access, 0 means any time * @param int $roleid The role id, 0 means all roles * @param int $enrolid The enrolment id, 0 means all enrolment methods will be returned. @@ -1416,7 +1416,7 @@ function user_get_participants_sql($courseid, $groupid = 0, $accesssince = 0, $r * Returns the total number of participants for a given course. * * @param int $courseid The course id - * @param int $groupid The groupid, 0 means all groups + * @param int $groupid The groupid, 0 means all groups and USERSWITHOUTGROUP no group * @param int $accesssince The time since last access, 0 means any time * @param int $roleid The role id, 0 means all roles * @param int $enrolid The applied filter for the user enrolment ID. @@ -1440,7 +1440,7 @@ function user_get_total_participants($courseid, $groupid = 0, $accesssince = 0, * Returns the participants for a given course. * * @param int $courseid The course id - * @param int $groupid The group id + * @param int $groupid The groupid, 0 means all groups and USERSWITHOUTGROUP no group * @param int $accesssince The time since last access * @param int $roleid The role id * @param int $enrolid The applied filter for the user enrolment ID. diff --git a/user/renderer.php b/user/renderer.php index 35711ce11e0..1e8461f695d 100644 --- a/user/renderer.php +++ b/user/renderer.php @@ -145,6 +145,11 @@ class core_user_renderer extends plugin_renderer_base { if (has_capability('moodle/site:accessallgroups', $context) || $course->groupmode != SEPARATEGROUPS) { // List all groups if the user can access all groups, or we are in visible group mode or no groups mode. $groups = $manager->get_all_groups(); + if (!empty($groups)) { + // Add 'No group' option, to enable filtering users without any group. + $nogroup[USERSWITHOUTGROUP] = (object)['name' => get_string('nogroup', 'group')]; + $groups = $nogroup + $groups; + } } else { // Otherwise, just list the groups the user belongs to. $groups = groups_get_all_groups($course->id, $USER->id); diff --git a/user/tests/behat/filter_participants.feature b/user/tests/behat/filter_participants.feature index 073ce49108f..ee8796576ea 100644 --- a/user/tests/behat/filter_participants.feature +++ b/user/tests/behat/filter_participants.feature @@ -9,6 +9,7 @@ Feature: Course participants can be filtered | fullname | shortname | groupmode | | Course 1 | C1 | 1 | | Course 2 | C2 | 0 | + | Course 3 | C3 | 0 | And the following "users" exist: | username | firstname | lastname | email | | student1 | Student | 1 | student1@example.com | @@ -25,17 +26,26 @@ Feature: Course participants can be filtered | student1 | C2 | student | 0 | | | student2 | C2 | student | 0 | | | student3 | C2 | student | 0 | | + | student1 | C3 | student | 0 | | + | student2 | C3 | student | 0 | | + | student3 | C3 | student | 0 | | | teacher1 | C1 | editingteacher | 0 | | | teacher1 | C2 | editingteacher | 0 | | + | teacher1 | C3 | editingteacher | 0 | | And the following "groups" exist: | name | course | idnumber | | Group 1 | C1 | G1 | | Group 2 | C1 | G2 | + | Group A | C3 | GA | + | Group B | C3 | GB | And the following "group members" exist: | user | group | | student2 | G1 | | student2 | G2 | | student3 | G2 | + | student1 | GA | + | student2 | GA | + | student2 | GB | @javascript Scenario: No filters applied @@ -62,12 +72,32 @@ Feature: Course participants can be filtered # Note the 'XX-IGNORE-XX' elements are for when there is less than 2 'not expected' items. Examples: | filter1 | expected1 | expected2 | expected3 | notexpected1 | notexpected2 | + | Group: No group | Student 1 | Student 4 | Teacher 1 | Student 2 | Student 3 | | Group: Group 1 | Student 2 | | | Student 1 | Student 3 | | Group: Group 2 | Student 2 | Student 3 | | Student 1 | XX-IGNORE-XX | | Role: Teacher | Teacher 1 | | | Student 1 | Student 2 | | Status: Active | Teacher 1 | Student 1 | Student 3 | Student 2 | Student 4 | | Status: Inactive | Student 2 | Student 4 | | Teacher 1 | Student 1 | + @javascript + Scenario Outline: Filter users which are group members in several courses + Given I log in as "teacher1" + And I am on "Course 3" course homepage + And I navigate to course participants + When I open the autocomplete suggestions list + And I click on "" item in the autocomplete list + Then I should see "" in the "participants" "table" + And I should see "" in the "participants" "table" + And I should see "" in the "participants" "table" + And I should not see "" in the "participants" "table" + And I should not see "" in the "participants" "table" + # Note the 'XX-IGNORE-XX' elements are for when there is less than 2 'not expected' items. + Examples: + | filter1 | expected1 | expected2 | expected3 | notexpected1 | notexpected2 | + | Group: No group | Student 3 | | | Student 1 | Student 2 | + | Group: Group A | Student 1 | Student 2 | | Student 3 | XX-IGNORE-XX | + | Group: Group B | Student 2 | | | Student 1 | Student 3 | + @javascript Scenario: Multiple filters applied Given I log in as "teacher1" diff --git a/user/tests/userlib_test.php b/user/tests/userlib_test.php index 5391ab897c3..bb4bb45f8f9 100644 --- a/user/tests/userlib_test.php +++ b/user/tests/userlib_test.php @@ -939,6 +939,12 @@ class core_userliblib_testcase extends advanced_testcase { $this->assertEquals($student1->id, $userset->current()->id); $this->assertEquals(1, iterator_count($userset)); + + // Search for users without any group. + $userset = user_get_participants($course->id, USERSWITHOUTGROUP, 0, $roleids['student'], 0, -1, ''); + + $this->assertEquals($student3->id, $userset->current()->id); + $this->assertEquals(1, iterator_count($userset)); } /**