From 21253ed62248cd4079dd3b3ad73b6dc160e358ac Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Fri, 29 Nov 2019 07:45:55 +0000 Subject: [PATCH 1/3] MDL-67245 group: display user identity fields for group members. --- group/autogroup.php | 14 ++++++-- group/index.php | 36 ++++++++++++++++---- group/lib.php | 5 +-- group/overview.php | 21 +++++++++--- group/tests/behat/auto_creation.feature | 16 ++++----- group/tests/behat/create_groups.feature | 44 +++++++++++++++++++++---- 6 files changed, 106 insertions(+), 30 deletions(-) diff --git a/group/autogroup.php b/group/autogroup.php index b66ca45cb3a..dd7d582c1b0 100644 --- a/group/autogroup.php +++ b/group/autogroup.php @@ -99,8 +99,9 @@ if ($editform->is_cancelled()) { // Display only active users if the option was selected or they do not have the capability to view suspended users. $onlyactive = !empty($data->includeonlyactiveenrol) || !has_capability('moodle/course:viewsuspendedusers', $context); + $extrafields = get_extra_user_fields($context); $users = groups_get_potential_members($data->courseid, $data->roleid, $source, $orderby, !empty($data->notingroup), - $onlyactive); + $onlyactive, $extrafields); $usercnt = count($users); if ($data->allocateby == 'random') { @@ -183,7 +184,16 @@ if ($editform->is_cancelled()) { if ($data->allocateby != 'no') { $unames = array(); foreach ($group['members'] as $user) { - $unames[] = fullname($user, true); + $fullname = fullname($user, true); + if ($extrafields) { + $extrafieldsdisplay = []; + foreach ($extrafields as $field) { + $extrafieldsdisplay[] = s($user->{$field}); + } + $fullname .= ' (' . implode(', ', $extrafieldsdisplay) . ')'; + } + + $unames[] = $fullname; } $line[] = implode(', ', $unames); $line[] = count($group['members']); diff --git a/group/index.php b/group/index.php index 79c9e21aabd..f589f279b05 100644 --- a/group/index.php +++ b/group/index.php @@ -80,7 +80,11 @@ switch ($action) { case 'ajax_getmembersingroup': $roles = array(); - if ($groupmemberroles = groups_get_members_by_role($groupids[0], $courseid, 'u.id, ' . get_all_user_name_fields(true, 'u'))) { + + $extrafields = get_extra_user_fields($context); + if ($groupmemberroles = groups_get_members_by_role($groupids[0], $courseid, + 'u.id, ' . user_picture::fields('u', $extrafields))) { + foreach($groupmemberroles as $roleid=>$roledata) { $shortroledata = new stdClass(); $shortroledata->name = $roledata->name; @@ -89,6 +93,14 @@ switch ($action) { $shortmember = new stdClass(); $shortmember->id = $member->id; $shortmember->name = fullname($member, true); + if ($extrafields) { + $extrafieldsdisplay = []; + foreach ($extrafields as $field) { + $extrafieldsdisplay[] = s($member->{$field}); + } + $shortmember->name .= ' (' . implode(', ', $extrafieldsdisplay) . ')'; + } + $shortroledata->users[] = $shortmember; } $roles[] = $shortroledata; @@ -188,15 +200,25 @@ if ($groups) { // Get list of group members to render if there is a single selected group. $members = array(); if ($singlegroup) { - $usernamefields = get_all_user_name_fields(true, 'u'); - if ($groupmemberroles = groups_get_members_by_role(reset($groupids), $courseid, 'u.id, ' . $usernamefields)) { + $extrafields = get_extra_user_fields($context); + if ($groupmemberroles = groups_get_members_by_role(reset($groupids), $courseid, + 'u.id, ' . user_picture::fields('u', $extrafields))) { + foreach ($groupmemberroles as $roleid => $roledata) { $users = array(); foreach ($roledata->users as $member) { - $users[] = (object)[ - 'value' => $member->id, - 'text' => fullname($member, true) - ]; + $shortmember = new stdClass(); + $shortmember->value = $member->id; + $shortmember->text = fullname($member, true); + if ($extrafields) { + $extrafieldsdisplay = []; + foreach ($extrafields as $field) { + $extrafieldsdisplay[] = s($member->{$field}); + } + $shortmember->text .= ' (' . implode(', ', $extrafieldsdisplay) . ')'; + } + + $users[] = $shortmember; } $members[] = (object)[ 'role' => s($roledata->name), diff --git a/group/lib.php b/group/lib.php index 42e7fbec1cc..98330166fe5 100644 --- a/group/lib.php +++ b/group/lib.php @@ -785,11 +785,12 @@ function groups_get_possible_roles($context) { * @param string $orderby The column to sort users by * @param int $notingroup restrict to users not in existing groups * @param bool $onlyactiveenrolments restrict to users who have an active enrolment in the course + * @param array $extrafields Extra user fields to return * @return array An array of the users */ function groups_get_potential_members($courseid, $roleid = null, $source = null, $orderby = 'lastname ASC, firstname ASC', - $notingroup = null, $onlyactiveenrolments = false) { + $notingroup = null, $onlyactiveenrolments = false, $extrafields = []) { global $DB; $context = context_course::instance($courseid); @@ -847,7 +848,7 @@ function groups_get_potential_members($courseid, $roleid = null, $source = null, } } - $allusernamefields = get_all_user_name_fields(true, 'u'); + $allusernamefields = user_picture::fields('u', $extrafields); $sql = "SELECT DISTINCT u.id, u.username, $allusernamefields, u.idnumber FROM {user} u JOIN ($esql) e ON e.id = u.id diff --git a/group/overview.php b/group/overview.php index 785cc6356ff..e4202cc8236 100644 --- a/group/overview.php +++ b/group/overview.php @@ -110,7 +110,9 @@ if ($groupingid) { list($sort, $sortparams) = users_order_by_sql('u'); -$allnames = get_all_user_name_fields(true, 'u'); +$extrafields = get_extra_user_fields($context); +$allnames = 'u.id, ' . user_picture::fields('u', $extrafields); + $sql = "SELECT g.id AS groupid, gg.groupingid, u.id AS userid, $allnames, u.idnumber, u.username FROM {groups} g LEFT JOIN {groupings_groups} gg ON g.id = gg.groupid @@ -121,8 +123,9 @@ $sql = "SELECT g.id AS groupid, gg.groupingid, u.id AS userid, $allnames, u.idnu $rs = $DB->get_recordset_sql($sql, array_merge($params, $sortparams)); foreach ($rs as $row) { - $user = new stdClass(); - $user = username_load_fields_from_object($user, $row, null, array('id' => 'userid', 'username', 'idnumber')); + $user = username_load_fields_from_object((object) [], $row, null, + array_merge(['id' => 'userid', 'username', 'idnumber'], $extrafields)); + if (!$row->groupingid) { $row->groupingid = OVERVIEW_GROUPING_GROUP_NO_GROUPING; } @@ -250,7 +253,17 @@ foreach ($members as $gpgid=>$groupdata) { } $fullnames = array(); foreach ($users as $user) { - $fullnames[] = ''.fullname($user, true).''; + $displayname = fullname($user, true); + if ($extrafields) { + $extrafieldsdisplay = []; + foreach ($extrafields as $field) { + $extrafieldsdisplay[] = s($user->{$field}); + } + $displayname .= ' (' . implode(', ', $extrafieldsdisplay) . ')'; + } + + $fullnames[] = html_writer::link(new moodle_url('/user/view.php', ['id' => $user->id, 'course' => $course->id]), + $displayname); } $line[] = implode(', ', $fullnames); $line[] = count($users); diff --git a/group/tests/behat/auto_creation.feature b/group/tests/behat/auto_creation.feature index 54b476329eb..9d8c4deeb49 100644 --- a/group/tests/behat/auto_creation.feature +++ b/group/tests/behat/auto_creation.feature @@ -80,15 +80,13 @@ Feature: Automatic creation of groups | Group/member count | 4 | | Grouping of auto-created groups | New grouping | | Grouping name | Grouping name | + | Allocate members | Alphabetically by last name, first name | And I press "Preview" - Then I should see "Group members" - And I should see "User count" - And I should see "Group A" in the ".generaltable" "css_element" - And I should see "Group B" in the ".generaltable" "css_element" - And I should see "Group C" in the ".generaltable" "css_element" - And I should see "4" in the "Group A" "table_row" - And I should see "4" in the "Group B" "table_row" - And I should see "2" in the "Group C" "table_row" + Then the following should exist in the "generaltable" table: + | Groups (3) | Group members | User count (10) | + | Group A | Student 1 (student1@example.com) | 4 | + | Group B | Student 5 (student5@example.com) | 4 | + | Group C | Student 9 (student9@example.com) | 2 | And I set the field "Prevent last small group" to "1" And I press "Preview" And I should see "Group A" in the ".generaltable" "css_element" @@ -163,7 +161,7 @@ Feature: Automatic creation of groups And I set the field "Auto create based on" to "Members per group" When I set the field "Group/member count" to "11" And I press "Preview" - Then I should see "Suspended student 11" + Then I should see "Suspended student 11 (suspendedstudent11@example.com)" Scenario: Do not display 'Include only active enrolments' if user does not have the 'moodle/course:viewsuspendedusers' capability Given I log out diff --git a/group/tests/behat/create_groups.feature b/group/tests/behat/create_groups.feature index 8cfa041e706..ebf2832d595 100644 --- a/group/tests/behat/create_groups.feature +++ b/group/tests/behat/create_groups.feature @@ -39,13 +39,15 @@ Feature: Organize students into groups And I add "Student 2 (student2@example.com)" user to "Group 2" group members And I add "Student 3 (student3@example.com)" user to "Group 2" group members Then I set the field "groups" to "Group 1 (2)" - And the "members" select box should contain "Student 0" - And the "members" select box should contain "Student 1" - And the "members" select box should not contain "Student 2" + And the "members" select box should contain "Student 0 (student0@example.com)" + And the "members" select box should contain "Student 1 (student1@example.com)" + And the "members" select box should not contain "Student 2 (student2@example.com)" + And the "members" select box should not contain "Student 3 (student3@example.com)" And I set the field "groups" to "Group 2 (2)" - And the "members" select box should contain "Student 2" - And the "members" select box should contain "Student 3" - And the "members" select box should not contain "Student 0" + And the "members" select box should contain "Student 2 (student2@example.com)" + And the "members" select box should contain "Student 3 (student3@example.com)" + And the "members" select box should not contain "Student 0 (student0@example.com)" + And the "members" select box should not contain "Student 1 (student1@example.com)" And I navigate to course participants And I open the autocomplete suggestions list And I click on "Group: Group 1" item in the autocomplete list @@ -59,6 +61,36 @@ Feature: Organize students into groups And I should see "Student 3" And I should not see "Student 0" + @javascript + Scenario: Assign students to groups with site user identity configured + Given the following "courses" exist: + | fullname | shortname | groupmode | + | Course 1 | C1 | 1 | + And the following "users" exist: + | username | firstname | lastname | email | country | + | teacher | Teacher | 1 | teacher@example.com | GB | + | student | Student | 1 | student@example.com | DE | + And the following "course enrolments" exist: + | user | course | role | + | teacher | C1 | editingteacher | + | student | C1 | student | + And the following config values are set as admin: + | showuseridentity | email,country | + And I log in as "teacher" + And I am on "Course 1" course homepage + And I navigate to "Users > Groups" in current page administration + And I press "Create group" + And I set the following fields to these values: + | Group name | Group 1 | + And I press "Save changes" + When I add "Student 1 (student@example.com, DE)" user to "Group 1" group members + And I set the field "groups" to "Group 1 (1)" + Then the "members" select box should contain "Student 1 (student@example.com\, DE)" + # Non-AJAX version of the groups page. + And I press "Add/remove users" + And I press "Back to groups" + And the "members" select box should contain "Student 1 (student@example.com\, DE)" + Scenario: Create groups and groupings without the 'moodle/course:changeidnumber' capability Given the following "courses" exist: | fullname | shortname | category | groupmode | From 6e8ed1a8fbbbf669826ee28932748f0497780fff Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 7 Jan 2020 16:18:56 +0000 Subject: [PATCH 2/3] MDL-67245 group: observe viewfullnames capability. --- group/autogroup.php | 4 ++-- group/classes/output/user_groups_editable.php | 6 ++++-- group/externallib.php | 3 ++- group/index.php | 8 ++++++-- group/overview.php | 3 ++- 5 files changed, 16 insertions(+), 8 deletions(-) diff --git a/group/autogroup.php b/group/autogroup.php index dd7d582c1b0..cc2aeebdf3b 100644 --- a/group/autogroup.php +++ b/group/autogroup.php @@ -172,7 +172,7 @@ if ($editform->is_cancelled()) { $table->width = '90%'; } $table->data = array(); - + $viewfullnames = has_capability('moodle/site:viewfullnames', $context); foreach ($groups as $group) { $line = array(); if (groups_get_group_by_name($courseid, $group['name'])) { @@ -184,7 +184,7 @@ if ($editform->is_cancelled()) { if ($data->allocateby != 'no') { $unames = array(); foreach ($group['members'] as $user) { - $fullname = fullname($user, true); + $fullname = fullname($user, $viewfullnames); if ($extrafields) { $extrafieldsdisplay = []; foreach ($extrafields as $field) { diff --git a/group/classes/output/user_groups_editable.php b/group/classes/output/user_groups_editable.php index 9595a033568..a4a61c50111 100644 --- a/group/classes/output/user_groups_editable.php +++ b/group/classes/output/user_groups_editable.php @@ -77,8 +77,10 @@ class user_groups_editable extends \core\output\inplace_editable { foreach ($coursegroups as $group) { $options[$group->id] = format_string($group->name, true, ['context' => $this->context]); } - $this->edithint = get_string('editusersgroupsa', 'group', fullname($user)); - $this->editlabel = get_string('editusersgroupsa', 'group', fullname($user)); + + $fullname = fullname($user, has_capability('moodle/site:viewfullnames', $this->context)); + $this->edithint = get_string('editusersgroupsa', 'group', $fullname); + $this->editlabel = get_string('editusersgroupsa', 'group', $fullname); $attributes = ['multiple' => true]; $this->set_type_autocomplete($options, $attributes); diff --git a/group/externallib.php b/group/externallib.php index bebede4fd4b..341e72750a8 100644 --- a/group/externallib.php +++ b/group/externallib.php @@ -556,7 +556,8 @@ class core_group_external extends external_api { require_capability('moodle/course:managegroups', $context); if (!groups_remove_member_allowed($group, $user)) { - throw new moodle_exception('errorremovenotpermitted', 'group', '', fullname($user)); + $fullname = fullname($user, has_capability('moodle/site:viewfullnames', $context)); + throw new moodle_exception('errorremovenotpermitted', 'group', '', $fullname); } groups_remove_member($group, $user); } diff --git a/group/index.php b/group/index.php index f589f279b05..e2aa22b35e7 100644 --- a/group/index.php +++ b/group/index.php @@ -85,6 +85,8 @@ switch ($action) { if ($groupmemberroles = groups_get_members_by_role($groupids[0], $courseid, 'u.id, ' . user_picture::fields('u', $extrafields))) { + $viewfullnames = has_capability('moodle/site:viewfullnames', $context); + foreach($groupmemberroles as $roleid=>$roledata) { $shortroledata = new stdClass(); $shortroledata->name = $roledata->name; @@ -92,7 +94,7 @@ switch ($action) { foreach($roledata->users as $member) { $shortmember = new stdClass(); $shortmember->id = $member->id; - $shortmember->name = fullname($member, true); + $shortmember->name = fullname($member, $viewfullnames); if ($extrafields) { $extrafieldsdisplay = []; foreach ($extrafields as $field) { @@ -204,12 +206,14 @@ if ($singlegroup) { if ($groupmemberroles = groups_get_members_by_role(reset($groupids), $courseid, 'u.id, ' . user_picture::fields('u', $extrafields))) { + $viewfullnames = has_capability('moodle/site:viewfullnames', $context); + foreach ($groupmemberroles as $roleid => $roledata) { $users = array(); foreach ($roledata->users as $member) { $shortmember = new stdClass(); $shortmember->value = $member->id; - $shortmember->text = fullname($member, true); + $shortmember->text = fullname($member, $viewfullnames); if ($extrafields) { $extrafieldsdisplay = []; foreach ($extrafields as $field) { diff --git a/group/overview.php b/group/overview.php index e4202cc8236..5c23992cc19 100644 --- a/group/overview.php +++ b/group/overview.php @@ -251,9 +251,10 @@ foreach ($members as $gpgid=>$groupdata) { $line[] = html_writer::tag('span', $name, array('class' => 'group_hoverdescription', 'data-groupid' => $gpid)); $hoverevents[$gpid] = get_string('descriptiona', null, $jsdescription); } + $viewfullnames = has_capability('moodle/site:viewfullnames', $context); $fullnames = array(); foreach ($users as $user) { - $displayname = fullname($user, true); + $displayname = fullname($user, $viewfullnames); if ($extrafields) { $extrafieldsdisplay = []; foreach ($extrafields as $field) { From 7eeea538d2e891d7f8676213e5d1adb7ee3d9177 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 17 Dec 2019 19:17:00 +0000 Subject: [PATCH 3/3] MDL-67245 group: Behat updates to account for member identity fields. --- admin/tool/behat/tests/behat/data_generators.feature | 4 ++-- .../behat/tests/behat/get_and_set_fields.feature | 12 ++++++------ .../tool/uploaduser/tests/behat/upload_users.feature | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/admin/tool/behat/tests/behat/data_generators.feature b/admin/tool/behat/tests/behat/data_generators.feature index f078fabf11f..19644be3a22 100644 --- a/admin/tool/behat/tests/behat/data_generators.feature +++ b/admin/tool/behat/tests/behat/data_generators.feature @@ -259,9 +259,9 @@ Feature: Set up contextual data for tests Then the "groups" select box should contain "Group 1 (1)" And the "groups" select box should contain "Group 2 (1)" And I set the field "groups" to "Group 1 (1)" - And the "members" select box should contain "Student 1" + And the "members" select box should contain "Student 1 (student1@example.com)" And I set the field "groups" to "Group 2 (1)" - And the "members" select box should contain "Student 2" + And the "members" select box should contain "Student 2 (student2@example.com)" Scenario: Add cohorts and cohort members with data generator Given the following "categories" exist: diff --git a/admin/tool/behat/tests/behat/get_and_set_fields.feature b/admin/tool/behat/tests/behat/get_and_set_fields.feature index 9ade5e05930..3f1d88e0751 100644 --- a/admin/tool/behat/tests/behat/get_and_set_fields.feature +++ b/admin/tool/behat/tests/behat/get_and_set_fields.feature @@ -147,13 +147,13 @@ Feature: Verify that all form fields values can be get and set And I navigate to "Users > Groups" in current page administration # Select (multi-select & AJAX) - Checking "I set the field" and "select box should contain". And I set the field "groups" to "Group 2" - And the "members" select box should contain "Student 2" - And the "members" select box should contain "Student 3" - And the "members" select box should not contain "Student 1" + And the "members" select box should contain "Student 2 (s2@example.com)" + And the "members" select box should contain "Student 3 (s3@example.com)" + And the "members" select box should not contain "Student 1 (s1@example.com)" And I set the field "groups" to "Group 1" - And the "members" select box should contain "Student 1" - And the "members" select box should contain "Student 2" - And the "members" select box should not contain "Student 3" + And the "members" select box should contain "Student 1 (s1@example.com)" + And the "members" select box should contain "Student 2 (s2@example.com)" + And the "members" select box should not contain "Student 3 (s3@example.com)" # Checkbox (AJAX) - Checking "I set the field" and "I set the following fields to these values". And I am on "Course 1" course homepage And I add a "Lesson" to section "1" diff --git a/admin/tool/uploaduser/tests/behat/upload_users.feature b/admin/tool/uploaduser/tests/behat/upload_users.feature index a92b633cca5..fc5500217f7 100644 --- a/admin/tool/uploaduser/tests/behat/upload_users.feature +++ b/admin/tool/uploaduser/tests/behat/upload_users.feature @@ -36,7 +36,7 @@ Feature: Upload users And I am on "Maths" course homepage And I navigate to "Users > Groups" in current page administration And I set the field "groups" to "Section 1 (1)" - And the "members" select box should contain "Tom Jones" + And the "members" select box should contain "Tom Jones (jonest@example.com)" @javascript Scenario: Upload users enrolling them on courses and groups applying defaults