diff --git a/admin/tests/behat/filter_users.feature b/admin/tests/behat/filter_users.feature index c11b6133e94..85bbf711673 100644 --- a/admin/tests/behat/filter_users.feature +++ b/admin/tests/behat/filter_users.feature @@ -5,12 +5,16 @@ Feature: An administrator can filter user accounts by role, cohort and other pro I need to filter the users account list using different filter Background: - Given the following "users" exist: - | username | firstname | lastname | email | auth | confirmed | lastip | institution | department | - | user1 | User | One | one@example.com | manual | 0 | 127.0.1.1 | moodle | red | - | user2 | User | Two | two@example.com | ldap | 1 | 0.0.0.0 | moodle | blue | - | user3 | User | Three | three@example.com | manual | 1 | 0.0.0.0 | | | - | user4 | User | Four | four@example.com | ldap | 0 | 127.0.1.2 | | | + Given the following "custom profile fields" exist: + | datatype | shortname | name | + | text | frog | Favourite frog | + | text | undead | Type of undead | + And the following "users" exist: + | username | firstname | lastname | email | auth | confirmed | lastip | institution | department | profile_field_frog | profile_field_undead | + | user1 | User | One | one@example.com | manual | 0 | 127.0.1.1 | moodle | red | Kermit | | + | user2 | User | Two | two@example.com | ldap | 1 | 0.0.0.0 | moodle | blue | Mr Toad | Zombie | + | user3 | User | Three | three@example.com | manual | 1 | 0.0.0.0 | | | | | + | user4 | User | Four | four@example.com | ldap | 0 | 127.0.1.2 | | | | | And the following "cohorts" exist: | name | idnumber | | Cohort 1 | CH1 | @@ -116,3 +120,20 @@ Feature: An administrator can filter user accounts by role, cohort and other pro And I press "Add filter" And I should see "User One" And I should not see "User Two" + + Scenario: Filter users by custom profile field (specific or any) + When I set the field "id_profile_fld" to "Favourite frog" + And I set the field "id_profile" to "Kermit" + And I press "Add filter" + Then I should see "User One" + And I should not see "User Two" + And I should not see "User Three" + And I should not see "User Four" + And I press "Remove all filters" + And I set the field "id_profile_fld" to "any field" + And I set the field "id_profile" to "Zombie" + And I press "Add filter" + And I should see "User Two" + And I should not see "User One" + And I should not see "User Three" + And I should not see "User Four" diff --git a/admin/user.php b/admin/user.php index 0f0eec9582b..06ee33bc18e 100644 --- a/admin/user.php +++ b/admin/user.php @@ -183,12 +183,10 @@ // These columns are always shown in the users list. $requiredcolumns = array('city', 'country', 'lastaccess'); // Extra columns containing the extra user fields, excluding the required columns (city and country, to be specific). - // TODO Does not support custom user profile fields (MDL-70456). - $userfields = \core\user_fields::for_identity($context, false)->excluding(...$requiredcolumns); + $userfields = \core\user_fields::for_identity($context, true)->with_name()->excluding(...$requiredcolumns); $extracolumns = $userfields->get_required_fields(); - // Get all user name fields as an array, but with firstname and lastname first. - $allusernamefields = \core\user_fields::get_name_fields(true); - $columns = array_merge($allusernamefields, $extracolumns, $requiredcolumns); + // Get all user name fields as an array. + $columns = array_merge($extracolumns, $requiredcolumns); foreach ($columns as $column) { $string[$column] = \core\user_fields::get_display_name($column); @@ -228,7 +226,7 @@ } // Order in string will ensure that the name columns are in the correct order. - $usernames = order_in_string($allusernamefields, $fullnamesetting); + $usernames = order_in_string($extracolumns, $fullnamesetting); $fullnamedisplay = array(); foreach ($usernames as $name) { // Use the link from $$column for sorting on the user's name. diff --git a/lib/datalib.php b/lib/datalib.php index 68d89f89f8a..64d0c34d358 100644 --- a/lib/datalib.php +++ b/lib/datalib.php @@ -480,7 +480,7 @@ function get_users_listing($sort='lastaccess', $dir='ASC', $page=0, $recordsperp $fullname = $DB->sql_fullname(); - $select = "deleted <> 1 AND id <> :guestid"; + $select = "deleted <> 1 AND u.id <> :guestid"; $params = array('guestid' => $CFG->siteguest); if (!empty($search)) { @@ -503,6 +503,10 @@ function get_users_listing($sort='lastaccess', $dir='ASC', $page=0, $recordsperp } if ($extraselect) { + // The extra WHERE clause may refer to the 'id' column which can now be ambiguous because we + // changed the query to include joins, so replace any 'id' that is on its own (no alias) + // with 'u.id'. + $extraselect = preg_replace('~([ =]|^)id([ =]|$)~', '$1u.id$2', $extraselect); $select .= " AND $extraselect"; $params = $params + (array)$extraparams; } @@ -512,18 +516,21 @@ function get_users_listing($sort='lastaccess', $dir='ASC', $page=0, $recordsperp } // If a context is specified, get extra user fields that the current user - // is supposed to see. - // TODO Does not support custom user profile fields (MDL-70456). - $userfieldsapi = \core\user_fields::for_identity($extracontext, false)->with_name() - ->excluding('id', 'username', 'email', 'firstname', 'lastname', 'city', 'country', - 'lastaccess', 'confirmed', 'mnethostid'); - $extrafields = $userfields->get_sql()->selects; + // is supposed to see, otherwise just get the name fields. + $userfields = \core\user_fields::for_name(); + if ($extracontext) { + $userfields->with_identity($extracontext, true); + } + $userfields->excluding('id', 'username', 'email', 'city', 'country', 'lastaccess', 'confirmed', 'mnethostid'); + ['selects' => $selects, 'joins' => $joins, 'params' => $joinparams] = + (array)$userfields->get_sql('u', true); // warning: will return UNCONFIRMED USERS - return $DB->get_records_sql("SELECT id, username, email, city, country, lastaccess, confirmed, mnethostid, suspended $extrafields - FROM {user} + return $DB->get_records_sql("SELECT u.id, username, email, city, country, lastaccess, confirmed, mnethostid, suspended $selects + FROM {user} u + $joins WHERE $select - $sort", $params, $page, $recordsperpage); + $sort", array_merge($params, $joinparams), $page, $recordsperpage); } diff --git a/lib/tests/datalib_test.php b/lib/tests/datalib_test.php index 76250b27649..8ba4e43e6c1 100644 --- a/lib/tests/datalib_test.php +++ b/lib/tests/datalib_test.php @@ -746,4 +746,100 @@ class core_datalib_testcase extends advanced_testcase { "Please also make sure \$CFG->maxcoursesincategory * MAX_COURSE_CATEGORIES less than max integer. " . "See tracker issues: MDL-25669 and MDL-69573"); } + + /** + * Tests the get_users_listing function. + */ + public function test_get_users_listing(): void { + global $DB; + + $this->resetAfterTest(); + + $generator = $this->getDataGenerator(); + + // Set up profile field. + $generator->create_custom_profile_field(['datatype' => 'text', + 'shortname' => 'specialid', 'name' => 'Special user id']); + + // Set up the show user identity option. + set_config('showuseridentity', 'department,profile_field_specialid'); + + // Get all the existing user ids (we're going to remove these from test results). + $existingids = array_fill_keys($DB->get_fieldset_select('user', 'id', '1 = 1'), true); + + // Create some test user accounts. + $userids = []; + foreach (['a', 'b', 'c', 'd'] as $key) { + $record = [ + 'username' => 'user_' . $key, + 'firstname' => $key . '_first', + 'lastname' => 'last_' . $key, + 'department' => 'department_' . $key, + 'profile_field_specialid' => 'special_' . $key, + 'lastaccess' => ord($key) + ]; + $user = $generator->create_user($record); + $userids[] = $user->id; + } + + // Check default result with no parameters. + $results = get_users_listing(); + $results = array_diff_key($results, $existingids); + + // It should return all the results in order. + $this->assertEquals($userids, array_keys($results)); + + // Results should have some general fields and name fields, check some samples. + $this->assertEquals('user_a', $results[$userids[0]]->username); + $this->assertEquals('user_a@example.com', $results[$userids[0]]->email); + $this->assertEquals(1, $results[$userids[0]]->confirmed); + $this->assertEquals('a_first', $results[$userids[0]]->firstname); + $this->assertObjectHasAttribute('firstnamephonetic', $results[$userids[0]]); + + // Should not have the custom field or department because no context specified. + $this->assertObjectNotHasAttribute('department', $results[$userids[0]]); + $this->assertObjectNotHasAttribute('profile_field_specialid', $results[$userids[0]]); + + // Check sorting. + $results = get_users_listing('username', 'DESC'); + $results = array_diff_key($results, $existingids); + $this->assertEquals([$userids[3], $userids[2], $userids[1], $userids[0]], array_keys($results)); + + // Add the options to showuseridentity and check it returns those fields but only if you + // specify a context AND have permissions. + $results = get_users_listing('lastaccess', 'asc', 0, 0, '', '', '', '', null, + \context_system::instance()); + $this->assertObjectNotHasAttribute('department', $results[$userids[0]]); + $this->assertObjectNotHasAttribute('profile_field_specialid', $results[$userids[0]]); + $this->setAdminUser(); + $results = get_users_listing('lastaccess', 'asc', 0, 0, '', '', '', '', null, + \context_system::instance()); + $this->assertEquals('department_a', $results[$userids[0]]->department); + $this->assertEquals('special_a', $results[$userids[0]]->profile_field_specialid); + + // Check search (full name, email, username). + $results = get_users_listing('lastaccess', 'asc', 0, 0, 'b_first last_b'); + $this->assertEquals([$userids[1]], array_keys($results)); + $results = get_users_listing('lastaccess', 'asc', 0, 0, 'c@example'); + $this->assertEquals([$userids[2]], array_keys($results)); + $results = get_users_listing('lastaccess', 'asc', 0, 0, 'user_d'); + $this->assertEquals([$userids[3]], array_keys($results)); + + // Check first and last initial restriction (all the test ones have same last initial). + $results = get_users_listing('lastaccess', 'asc', 0, 0, '', 'C'); + $this->assertEquals([$userids[2]], array_keys($results)); + $results = get_users_listing('lastaccess', 'asc', 0, 0, '', '', 'L'); + $results = array_diff_key($results, $existingids); + $this->assertEquals($userids, array_keys($results)); + + // Check the extra where clause, either with the 'u.' prefix or not. + $results = get_users_listing('lastaccess', 'asc', 0, 0, '', '', '', 'id IN (:x,:y)', + ['x' => $userids[1], 'y' => $userids[3]]); + $results = array_diff_key($results, $existingids); + $this->assertEquals([$userids[1], $userids[3]], array_keys($results)); + $results = get_users_listing('lastaccess', 'asc', 0, 0, '', '', '', 'u.id IN (:x,:y)', + ['x' => $userids[1], 'y' => $userids[3]]); + $results = array_diff_key($results, $existingids); + $this->assertEquals([$userids[1], $userids[3]], array_keys($results)); + } }