MDL-61967 core_user: Allow filtering by No groups on participants page
authorSara Arjona <sara@moodle.com>
Fri, 13 Jul 2018 09:28:59 +0000 (11:28 +0200)
committerSara Arjona <sara@moodle.com>
Mon, 24 Sep 2018 16:33:45 +0000 (18:33 +0200)
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.

lib/enrollib.php
lib/grouplib.php
lib/tests/accesslib_test.php
lib/tests/grouplib_test.php
lib/upgrade.txt
user/classes/participants_table.php
user/index.php
user/lib.php
user/renderer.php
user/tests/behat/filter_participants.feature
user/tests/userlib_test.php

index 2df4d50..4ed18bb 100644 (file)
@@ -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).
index 336e372..687eef1 100644 (file)
@@ -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.
@@ -976,15 +981,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);
 }
@@ -992,20 +1002,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');
+    }
+
+    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, '', $param);
+    return new \core\dml\sql_join($join, $where, $param);
 }
 
 /**
index 3ff6192..f8b9a86 100644 (file)
@@ -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(
index f9bfbee..ccf66c7 100644 (file)
@@ -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() {
index 70b6a59..e4e9d5b 100644 (file)
@@ -121,6 +121,9 @@ information provided here is intended especially for developers.
   - phone1
   - phone2
   - address
+* 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 ===
 
index 8f999f5..d686118 100644 (file)
@@ -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.
index f36491c..fccd1df 100644 (file)
@@ -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);
index bb1d75a..e0605a9 100644 (file)
@@ -1272,7 +1272,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.
@@ -1464,7 +1464,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.
@@ -1488,7 +1488,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.
index 35711ce..1e8461f 100644 (file)
@@ -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);
index 968fbbd..f0097f2 100644 (file)
@@ -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                | idnumber | country | city   | maildisplay |
       | student1 | Student   | 1        | student1@example.com | SID1     |         | SCITY1 | 0           |
@@ -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 "<filter1>" item in the autocomplete list
+    Then I should see "<expected1>" in the "participants" "table"
+    And I should see "<expected2>" in the "participants" "table"
+    And I should see "<expected3>" in the "participants" "table"
+    And I should not see "<notexpected1>" in the "participants" "table"
+    And I should not see "<notexpected2>" 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"
index b7f6d6c..8516c0b 100644 (file)
@@ -940,6 +940,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));
     }
 
     /**