From e86ead1d9187e11f3d033a452dcfc3bfac9e4bcc Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 20 Oct 2020 17:55:35 +0100 Subject: [PATCH] MDL-45242 Course: Participants list supports custom profile fields --- user/classes/table/participants.php | 3 +- user/classes/table/participants_search.php | 39 +++++--- user/tests/behat/filter_participants.feature | 98 +++++++++---------- user/tests/table/participants_search_test.php | 75 +++++++++++++- 4 files changed, 145 insertions(+), 70 deletions(-) diff --git a/user/classes/table/participants.php b/user/classes/table/participants.php index 95c57587e95..09823d73a59 100644 --- a/user/classes/table/participants.php +++ b/user/classes/table/participants.php @@ -141,8 +141,7 @@ class participants extends \table_sql implements dynamic_table { $headers[] = get_string('fullname'); $columns[] = 'fullname'; - // TODO Does not support custom user profile fields (MDL-70456). - $extrafields = \core\user_fields::get_identity_fields($this->context, false); + $extrafields = \core\user_fields::get_identity_fields($this->context); foreach ($extrafields as $field) { $headers[] = \core\user_fields::get_display_name($field); $columns[] = $field; diff --git a/user/classes/table/participants_search.php b/user/classes/table/participants_search.php index 8e2c11c27a7..754c42ec0ba 100644 --- a/user/classes/table/participants_search.php +++ b/user/classes/table/participants_search.php @@ -30,7 +30,7 @@ use core_table\local\filter\filterset; use core_user; use moodle_recordset; use stdClass; -use user_picture; +use core\user_fields; defined('MOODLE_INTERNAL') || die; @@ -77,8 +77,7 @@ class participants_search { $this->context = $context; $this->filterset = $filterset; - // TODO Does not support custom user profile fields (MDL-70456). - $this->userfields = \core\user_fields::get_identity_fields($this->context, false); + $this->userfields = user_fields::get_identity_fields($this->context); } /** @@ -193,7 +192,15 @@ class participants_search { 'params' => $params, ] = $this->get_enrolled_sql(); - $userfieldssql = user_picture::fields('u', $this->userfields); + // Get the fields for all contexts because there is a special case later where it allows + // matches of fields you can't access if they are on your own account. + $userfields = user_fields::for_identity(null)->with_userpic(); + ['selects' => $userfieldssql, 'joins' => $userfieldsjoin, 'params' => $userfieldsparams, 'mappings' => $mappings] = + (array)$userfields->get_sql('u', true); + if ($userfieldsjoin) { + $outerjoins[] = $userfieldsjoin; + $params = array_merge($params, $userfieldsparams); + } // Include any compulsory enrolment SQL (eg capability related filtering that must be applied). if (!empty($esqlforced)) { @@ -207,12 +214,12 @@ class participants_search { } if ($isfrontpage) { - $outerselect = "SELECT {$userfieldssql}, u.lastaccess"; + $outerselect = "SELECT u.lastaccess $userfieldssql"; if ($accesssince) { $wheres[] = user_get_user_lastaccess_sql($accesssince, 'u', $matchaccesssince); } } else { - $outerselect = "SELECT {$userfieldssql}, COALESCE(ul.timeaccess, 0) AS lastaccess"; + $outerselect = "SELECT COALESCE(ul.timeaccess, 0) AS lastaccess $userfieldssql"; // Not everybody has accessed the course yet. $outerjoins[] = 'LEFT JOIN {user_lastaccess} ul ON (ul.userid = u.id AND ul.courseid = :courseid2)'; $params['courseid2'] = $this->course->id; @@ -255,7 +262,7 @@ class participants_search { [ 'where' => $keywordswhere, 'params' => $keywordsparams, - ] = $this->get_keywords_search_sql(); + ] = $this->get_keywords_search_sql($mappings); if (!empty($keywordswhere)) { $wheres[] = $keywordswhere; @@ -873,9 +880,10 @@ class participants_search { /** * Prepare SQL where clause and associated parameters for any keyword searches being performed. * + * @param array $mappings Array of field mappings (fieldname => SQL code for the value) * @return array SQL query data in the format ['where' => '', 'params' => []]. */ - protected function get_keywords_search_sql(): array { + protected function get_keywords_search_sql(array $mappings): array { global $CFG, $DB, $USER; $keywords = []; @@ -964,24 +972,25 @@ class participants_search { $conditions[] = $idnumber; // Search all user identify fields. - // TODO Does not support custom user profile fields (MDL-70456). - $extrasearchfields = \core\user_fields::get_identity_fields(null, false); - foreach ($extrasearchfields as $extrasearchfield) { + $extrasearchfields = user_fields::get_identity_fields(null); + foreach ($extrasearchfields as $fieldindex => $extrasearchfield) { if (in_array($extrasearchfield, ['email', 'idnumber', 'country'])) { // Already covered above. Search by country not supported. continue; } - $param = $searchkey3 . $extrasearchfield; - $condition = $DB->sql_like($extrasearchfield, ':' . $param, false, false); + // The param must be short (max 32 characters) so don't include field name. + $param = $searchkey3 . '_ident' . $fieldindex; + $fieldsql = $mappings[$extrasearchfield]; + $condition = $DB->sql_like($fieldsql, ':' . $param, false, false); $params[$param] = "%$keyword%"; if ($notjoin) { - $condition = "($extrasearchfield IS NOT NULL AND {$condition})"; + $condition = "($fieldsql IS NOT NULL AND {$condition})"; } if (!in_array($extrasearchfield, $this->userfields)) { // User cannot see this field, but allow match if their own account. - $userid3 = 'userid' . $index . '3' . $extrasearchfield; + $userid3 = 'userid' . $index . '3_ident' . $fieldindex; $condition = "(". $condition . " AND u.id = :$userid3)"; $params[$userid3] = $USER->id; } diff --git a/user/tests/behat/filter_participants.feature b/user/tests/behat/filter_participants.feature index a7d6715fc6d..730b58b2d54 100644 --- a/user/tests/behat/filter_participants.feature +++ b/user/tests/behat/filter_participants.feature @@ -10,14 +10,17 @@ Feature: Course participants can be filtered | Course 1 | C1 | 1 | ##5 months ago## | | Course 2 | C2 | 0 | ##4 months ago## | | Course 3 | C3 | 0 | ##3 months ago## | + And the following "custom profile fields" exist: + | datatype | shortname | name | + | text | frog | Favourite frog | And the following "users" exist: - | username | firstname | lastname | email | idnumber | country | city | maildisplay | - | student1 | Student | 1 | student1@example.com | SID1 | | SCITY1 | 0 | - | student2 | Student | 2 | student2@example.com | SID2 | GB | SCITY2 | 1 | - | student3 | Student | 3 | student3@example.com | SID3 | AU | SCITY3 | 0 | - | student4 | Student | 4 | student4@moodle.com | SID4 | AT | SCITY4 | 0 | - | student5 | Trendy | Learnson | trendy@learnson.com | SID5 | AU | SCITY5 | 0 | - | patricia | Patricia | Pea | patricia.pea1@example.org | TID1 | US | TCITY1 | 0 | + | username | firstname | lastname | email | idnumber | country | city | maildisplay | profile_field_frog | + | student1 | Student | 1 | student1@example.com | SID1 | | SCITY1 | 0 | Kermit | + | student2 | Student | 2 | student2@example.com | SID2 | GB | SCITY2 | 1 | Mr Toad | + | student3 | Student | 3 | student3@example.com | SID3 | AU | SCITY3 | 0 | | + | student4 | Student | 4 | student4@moodle.com | SID4 | AT | SCITY4 | 0 | | + | student5 | Trendy | Learnson | trendy@learnson.com | SID5 | AU | SCITY5 | 0 | | + | patricia | Patricia | Pea | patricia.pea1@example.org | TID1 | US | TCITY1 | 0 | | And the following "course enrolments" exist: | user | course | role | status | timeend | | student1 | C1 | student | 0 | | @@ -58,8 +61,7 @@ Feature: Course participants can be filtered @javascript Scenario: No filters applied - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants Then I should see "Student 1" in the "participants" "table" And I should see "Student 2" in the "participants" "table" @@ -68,8 +70,7 @@ Feature: Course participants can be filtered @javascript Scenario Outline: Filter users for a course with a single value - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants And I set the field "Match" in the "Filter 1" "fieldset" to "" And I set the field "type" in the "Filter 1" "fieldset" to "" @@ -99,8 +100,7 @@ Feature: Course participants can be filtered @javascript Scenario Outline: Filter users for a course with multiple values for a single filter - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants And I set the field "Match" in the "Filter 1" "fieldset" to "" And I set the field "type" in the "Filter 1" "fieldset" to "" @@ -121,8 +121,7 @@ Feature: Course participants can be filtered @javascript Scenario Outline: Filter users which are group members in several courses - Given I log in as "patricia" - And I am on "Course 3" course homepage + Given I am on the "C3" "Course" page logged in as "patricia" And I navigate to course participants And I set the field "type" in the "Filter 1" "fieldset" to "" And I set the field "Type or select..." in the "Filter 1" "fieldset" to "" @@ -141,8 +140,7 @@ Feature: Course participants can be filtered @javascript Scenario: In separate groups mode, a student in a single group can only view and filter by users in their own group - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants # Unsuspend student 2 for to improve coverage of this test. @@ -201,8 +199,7 @@ Feature: Course participants can be filtered @javascript Scenario: In separate groups mode, a student in multiple groups can only view and filter by users in their own groups - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants # Unsuspend student 2 for to improve coverage of this test. @@ -265,8 +262,7 @@ Feature: Course participants can be filtered @javascript Scenario: Filter users who have no role in a course - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants # Remove the user role. @@ -292,8 +288,7 @@ Feature: Course participants can be filtered @javascript Scenario: Multiple filters applied (All filterset match type) - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants # Match Any: @@ -535,8 +530,7 @@ Feature: Course participants can be filtered @javascript Scenario: Filter match by one or more keywords and modified match types - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants # Match: @@ -610,8 +604,7 @@ Feature: Course participants can be filtered @javascript Scenario: Reorder users without losing filter - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants When I set the field "type" in the "Filter 1" "fieldset" to "Roles" @@ -633,8 +626,7 @@ Feature: Course participants can be filtered @javascript Scenario: Only possible to add filter rows for the number of filters available - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants When I set the field "type" in the "Filter 1" "fieldset" to "Keyword" And I click on "Add condition" "button" @@ -652,8 +644,7 @@ Feature: Course participants can be filtered @javascript Scenario: Rendering filter options for teachers in a course that don't support groups - Given I log in as "patricia" - And I am on "Course 2" course homepage + Given I am on the "C2" "Course" page logged in as "patricia" When I navigate to course participants Then I should see "Roles" in the "type" "field" And I should see "Enrolment methods" in the "type" "field" @@ -661,8 +652,7 @@ Feature: Course participants can be filtered @javascript Scenario: Rendering filter options for students who have limited privileges - Given I log in as "student1" - And I am on "Course 2" course homepage + Given I am on the "C2" "Course" page logged in as "student1" When I navigate to course participants Then I should see "Roles" in the "type" "field" But I should not see "Status" in the "type" "field" @@ -670,10 +660,9 @@ Feature: Course participants can be filtered @javascript Scenario: Filter by user identity fields - Given I log in as "patricia" - And the following config values are set as admin: + Given the following config values are set as admin: | showuseridentity | idnumber,email,city,country | - And I am on "Course 1" course homepage + And I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants # Search by email (only) - should only see visible email + own. @@ -750,8 +739,7 @@ Feature: Course participants can be filtered | showuseridentity | idnumber,email,city,country | And I log out - And I log in as "patricia" - And I am on "Course 1" course homepage + And I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants # Match: @@ -820,8 +808,7 @@ Feature: Course participants can be filtered # Keyword Any ["@example.com"]. # Set the Roles to "All" ["Student"]. - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants And I set the field "Match" in the "Filter 1" "fieldset" to "All" And I set the field "type" in the "Filter 1" "fieldset" to "Roles" @@ -859,8 +846,7 @@ Feature: Course participants can be filtered # Keyword Any ["@example.com"]. # Set the Roles to "All" ["Student"]. - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants When I set the field "Match" in the "Filter 1" "fieldset" to "All" And I set the field "type" in the "Filter 1" "fieldset" to "Roles" @@ -895,8 +881,7 @@ Feature: Course participants can be filtered # Match None: # Keyword Any ["@example.com"]; and # Roles All ["Teacher"]. - Given I log in as "patricia" - And I am on "Course 1" course homepage + Given I am on the "C1" "Course" page logged in as "patricia" And I navigate to course participants # Set the Keyword to "Any" ["@example.com"] @@ -952,8 +937,7 @@ Feature: Course participants can be filtered # Match: # No filters; and # First initial "T". - Given I log in as "patricia" - And I am on "Course 2" course homepage + Given I am on the "C2" "Course" page logged in as "patricia" And I navigate to course participants And I should see "Student 1" in the "participants" "table" And I should see "Student 2" in the "participants" "table" @@ -972,8 +956,7 @@ Feature: Course participants can be filtered # Match: # No filters; and # Last initial "L". - Given I log in as "patricia" - And I am on "Course 2" course homepage + Given I am on the "C2" "Course" page logged in as "patricia" And I navigate to course participants And I should see "Student 1" in the "participants" "table" And I should see "Student 2" in the "participants" "table" @@ -993,8 +976,7 @@ Feature: Course participants can be filtered # No filters; and # First initial "T"; and # Last initial "L". - Given I log in as "patricia" - And I am on "Course 2" course homepage + Given I am on the "C2" "Course" page logged in as "patricia" And I navigate to course participants And I should see "Student 1" in the "participants" "table" And I should see "Student 2" in the "participants" "table" @@ -1014,8 +996,7 @@ Feature: Course participants can be filtered # Match: # Roles All ["Teacher"]; and # First initial "T". - Given I log in as "patricia" - And I am on "Course 2" course homepage + Given I am on the "C2" "Course" page logged in as "patricia" And I navigate to course participants And I should see "Student 1" in the "participants" "table" And I should see "Student 2" in the "participants" "table" @@ -1036,3 +1017,16 @@ Feature: Course participants can be filtered And I should not see "Student 2" in the "participants" "table" And I should not see "Student 3" in the "participants" "table" And I should not see "Patricia Pea" in the "participants" "table" + + @javascript @frogfrog + Scenario: Filtering works correctly with custom profile fields + Given the following config values are set as admin: + | showuseridentity | email,profile_field_frog | + And I am on the "C2" "Course" page logged in as "patricia" + And I navigate to course participants + And I set the field "type" in the "Filter 1" "fieldset" to "Keyword" + And I set the field "Type..." to "Kermit" + And I press enter + And I click on "Apply filters" "button" + Then I should see "Student 1" in the "participants" "table" + And I should not see "Student 2" in the "participants" "table" diff --git a/user/tests/table/participants_search_test.php b/user/tests/table/participants_search_test.php index 89f720e39e5..c95cd085845 100644 --- a/user/tests/table/participants_search_test.php +++ b/user/tests/table/participants_search_test.php @@ -765,13 +765,22 @@ class participants_search_test extends advanced_testcase { * @param int $jointype The join type to use when combining filter values * @param int $count The expected count * @param array $expectedusers + * @param string $asuser If non-blank, uses that user account (for identify field permission checks) * @dataProvider keywords_provider */ - public function test_keywords_filter(array $usersdata, array $keywords, int $jointype, int $count, array $expectedusers): void { + public function test_keywords_filter(array $usersdata, array $keywords, int $jointype, int $count, + array $expectedusers, string $asuser): void { + global $DB; + $course = $this->getDataGenerator()->create_course(); $coursecontext = context_course::instance($course->id); $users = []; + // Create the custom user profile field and put it into showuseridentity. + $this->getDataGenerator()->create_custom_profile_field( + ['datatype' => 'text', 'shortname' => 'frog', 'name' => 'Fave frog']); + set_config('showuseridentity', 'email,profile_field_frog'); + foreach ($usersdata as $username => $userdata) { // Prevent randomly generated field values that may cause false fails. $userdata['firstnamephonetic'] = $userdata['firstnamephonetic'] ?? $userdata['firstname']; @@ -801,6 +810,10 @@ class participants_search_test extends advanced_testcase { } $keywordfilter->set_join_type($jointype); + if ($asuser) { + $this->setUser($DB->get_record('user', ['username' => $asuser])); + } + // Run the search. $search = new participants_search($course, $coursecontext, $filterset); $rs = $search->get_participants(); @@ -835,6 +848,7 @@ class participants_search_test extends advanced_testcase { 'alternatename' => 'Babs', 'firstnamephonetic' => 'Barbra', 'lastnamephonetic' => 'Benit', + 'profile_field_frog' => 'Kermit', ], 'colin.carnforth' => [ 'firstname' => 'Colin', @@ -845,6 +859,7 @@ class participants_search_test extends advanced_testcase { 'firstname' => 'Anthony', 'lastname' => 'Rogers', 'lastnamephonetic' => 'Rowjours', + 'profile_field_frog' => 'Mr Toad', ], 'sarah.rester' => [ 'firstname' => 'Sarah', @@ -958,6 +973,23 @@ class participants_search_test extends advanced_testcase { 'tony.rogers', ], ], + 'ANY: Filter on custom profile field' => (object) [ + 'keywords' => ['Kermit', 'Mr Toad'], + 'jointype' => filter::JOINTYPE_ANY, + 'count' => 2, + 'expectedusers' => [ + 'barbara.bennett', + 'tony.rogers', + ], + 'asuser' => 'admin' + ], + 'ANY: Filter on custom profile field (no permissions)' => (object) [ + 'keywords' => ['Kermit', 'Mr Toad'], + 'jointype' => filter::JOINTYPE_ANY, + 'count' => 0, + 'expectedusers' => [], + 'asuser' => 'barbara.bennett' + ], // Tests for jointype: ALL. 'ALL: No filter' => (object) [ @@ -1065,6 +1097,22 @@ class participants_search_test extends advanced_testcase { 'barbara.bennett', ], ], + 'ALL: Filter on custom profile field' => (object) [ + 'keywords' => ['Kermit', 'Kermi'], + 'jointype' => filter::JOINTYPE_ALL, + 'count' => 1, + 'expectedusers' => [ + 'barbara.bennett', + ], + 'asuser' => 'admin', + ], + 'ALL: Filter on custom profile field (no permissions)' => (object) [ + 'keywords' => ['Kermit', 'Kermi'], + 'jointype' => filter::JOINTYPE_ALL, + 'count' => 0, + 'expectedusers' => [], + 'asuser' => 'barbara.bennett', + ], // Tests for jointype: NONE. 'NONE: No filter' => (object) [ @@ -1205,6 +1253,30 @@ class participants_search_test extends advanced_testcase { 'sarah.rester', ], ], + 'NONE: Filter on custom profile field' => (object) [ + 'keywords' => ['Kermit', 'Mr Toad'], + 'jointype' => filter::JOINTYPE_NONE, + 'count' => 3, + 'expectedusers' => [ + 'adam.ant', + 'colin.carnforth', + 'sarah.rester', + ], + 'asuser' => 'admin', + ], + 'NONE: Filter on custom profile field (no permissions)' => (object) [ + 'keywords' => ['Kermit', 'Mr Toad'], + 'jointype' => filter::JOINTYPE_NONE, + 'count' => 5, + 'expectedusers' => [ + 'adam.ant', + 'barbara.bennett', + 'colin.carnforth', + 'tony.rogers', + 'sarah.rester', + ], + 'asuser' => 'barbara.bennett', + ], ], ], ]; @@ -1218,6 +1290,7 @@ class participants_search_test extends advanced_testcase { 'jointype' => $expectdata->jointype, 'count' => $expectdata->count, 'expectedusers' => $expectdata->expectedusers, + 'asuser' => $expectdata->asuser ?? '' ]; } }