From 958da5b67ec00bf1c716515ef7416286f41f9899 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Tue, 6 Sep 2022 09:14:24 +0100 Subject: [PATCH] MDL-68093 groups: Add visibility and participation settings These new settings are designed to enchance user privacy surrounding groups. They allow groups to be configured so that users outside the group cannot see the group, so that users in the group cannot see each other, or so that users cannot see the group at all, even if they are in it. This avoids issues where a group may be assigned based on sensitive personal information (such as a person requiring special arrangements due to a disability). By default, groups are visible to all and available for participation in activities, which maintains the current behaviour. For performance, a new cache has been added to track the number of groups on a course that are not visible to non-members. This allows us to revert to the existing behaviour if the new features are not being used at all on a course, and only apply the new visibility conditions if they are. Users who have the moodle/course:viewhiddengroups capability should be concious of exposing hidden groups when showing their screen to other users. The "Switch role to..." feature can be used to show a course page on screen without exposing private availability conditions, for example. The changes cover several specific areas: * grouplib functions, which most code should use to get lists of groups and members (this includes the participants page). * Activities supporting group overrides will not allow overrides for groups that are hidden from all users. * Activities supporting separate/visible groups modes will only allow groups with the new "participation" flag enabled to be selected. * Group messaging will be disabled for groups where members cannot see each other, or cannot see the group at all. --- backup/moodle2/backup_stepslib.php | 2 +- group/autogroup.php | 1 + group/classes/visibility.php | 172 +++++++ group/externallib.php | 86 +++- group/group_form.php | 29 ++ group/lib.php | 19 + .../tests/behat/backup_restore_groups.feature | 48 ++ group/tests/behat/private_groups.feature | 129 ++++++ group/tests/behat/update_groups.feature | 17 + group/tests/externallib_test.php | 147 ++++++ group/upgrade.txt | 16 + lang/en/cache.php | 1 + lang/en/group.php | 27 ++ lang/en/role.php | 1 + lib/db/access.php | 11 + lib/db/caches.php | 8 + lib/db/install.xml | 4 +- lib/db/upgrade.php | 20 + lib/grouplib.php | 251 +++++++++-- lib/testing/generator/data_generator.php | 8 + lib/tests/grouplib_test.php | 418 ++++++++++++++++++ lib/upgrade.txt | 11 + .../tests/behat/group_conversation.feature | 15 + mod/assign/override_form.php | 4 +- .../tests/behat/assign_group_override.feature | 20 + mod/lesson/override_form.php | 4 +- .../tests/behat/lesson_group_override.feature | 20 + mod/quiz/classes/form/edit_override_form.php | 4 +- .../tests/behat/quiz_group_override.feature | 20 + version.php | 2 +- 30 files changed, 1470 insertions(+), 45 deletions(-) create mode 100644 group/classes/visibility.php create mode 100644 group/tests/behat/backup_restore_groups.feature create mode 100644 group/tests/behat/private_groups.feature diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php index 2c94486690ee9..a09c1dc0741f1 100644 --- a/backup/moodle2/backup_stepslib.php +++ b/backup/moodle2/backup_stepslib.php @@ -1341,7 +1341,7 @@ protected function define_structure() { $group = new backup_nested_element('group', array('id'), array( 'name', 'idnumber', 'description', 'descriptionformat', 'enrolmentkey', - 'picture', 'timecreated', 'timemodified')); + 'picture', 'visibility', 'participation', 'timecreated', 'timemodified')); $members = new backup_nested_element('group_members'); diff --git a/group/autogroup.php b/group/autogroup.php index 5b85e56e54a6a..1c55d4a4d0eb8 100644 --- a/group/autogroup.php +++ b/group/autogroup.php @@ -234,6 +234,7 @@ $newgroup->courseid = $data->courseid; $newgroup->name = $group['name']; $newgroup->enablemessaging = $data->enablemessaging ?? 0; + $newgroup->visibility = GROUPS_VISIBILITY_ALL; $groupid = groups_create_group($newgroup); $createdgroups[] = $groupid; foreach($group['members'] as $user) { diff --git a/group/classes/visibility.php b/group/classes/visibility.php new file mode 100644 index 0000000000000..bd032bf80a0e6 --- /dev/null +++ b/group/classes/visibility.php @@ -0,0 +1,172 @@ +. + +/** + * Group visibility methods + * + * @package core_group + * @copyright 2022 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace core_group; + +/** + * Group visibility methods. + */ +class visibility { + + /** + * Store the number groups with visibility other than ALL on the course. + * + * @param int $courseid Course ID to update the cache for. + * @param \cache|null $cache Existing cache instance. If null, once will be created. + * @return void + * @throws \dml_exception + */ + public static function update_hiddengroups_cache(int $courseid, ?\cache $cache = null): void { + global $DB; + if (!$cache) { + $cache = \cache::make('core', 'coursehiddengroups'); + } + $hiddengroups = $DB->count_records_select('groups', 'courseid = ? AND visibility != ?', + [$courseid, GROUPS_VISIBILITY_ALL]); + $cache->set($courseid, $hiddengroups); + } + + /** + * Return whether a course currently had hidden groups. + * + * This can be used as a shortcut to decide whether visibility restrictions need to be applied. If this returns false, + * we may be able to use cached data, or do a much simpler query. + * + * @param int $courseid + * @return bool + * @throws \coding_exception + * @throws \dml_exception + */ + public static function course_has_hidden_groups(int $courseid): bool { + $cache = \cache::make('core', 'coursehiddengroups'); + $hiddengroups = $cache->get($courseid); + if ($hiddengroups === false) { + self::update_hiddengroups_cache($courseid, $cache); + $cache->get($courseid); + } + return $hiddengroups > 0; + } + + /** + * Can the current user view all the groups on the course? + * + * Returns true if there are no groups on the course with visibility != ALL, + * or if the user has viewhiddengroups. + * + * This is useful for deciding whether we need to perform additional visibility checkes + * such as the sql_* methods of this class. + * + * @param int $courseid + * @return bool + */ + public static function can_view_all_groups(int $courseid): bool { + $viewhidden = has_capability('moodle/course:viewhiddengroups', \context_course::instance($courseid)); + $hashidden = self::course_has_hidden_groups($courseid); + return $viewhidden || !$hashidden; + } + + /** + * Return SQL conditions for determining whether a user can see a group and its memberships. + * + * @param int $userid + * @param string $groupsalias The SQL alias being used for the groups table. + * @param string $groupsmembersalias The SQL alias being used for the groups_members table. + * @return array [$where, $params] + */ + public static function sql_group_visibility_where(int $userid, + string $groupsalias = 'g', string $groupsmembersalias = 'gm'): array { + global $USER; + // Apply visibility restrictions. + // Everyone can see who is in groups with ALL visibility. + $where = "({$groupsalias}.visibility = :all"; + $params['all'] = GROUPS_VISIBILITY_ALL; + if ($userid == $USER->id) { + // If the user is looking at their own groups, they can see those with MEMBERS or OWN visibility. + $where .= " OR {$groupsalias}.visibility IN (:members, :own)"; + $params['members'] = GROUPS_VISIBILITY_MEMBERS; + $params['own'] = GROUPS_VISIBILITY_OWN; + } else { + list($memberssql, $membersparams) = self::sql_members_visibility_condition($groupsalias, $groupsmembersalias); + // If someone else's groups, they can see those with MEMBERS visibilty, only if they are a member too. + $where .= " OR ($memberssql)"; + $params = array_merge($params, $membersparams); + } + $where .= ")"; + return [$where, $params]; + } + + /** + * Return SQL conditions for determining whether a user can see a group's members. + * + * @param string $groupsalias The SQL alias being used for the groups table. + * @param string $groupsmembersalias The SQL alias being used for the groups_members table. + * @param string $useralias The SQL alias being used for the user table. + * @return array [$where, $params] + */ + public static function sql_member_visibility_where(string $groupsalias = 'g', + string $groupsmembersalias = 'gm', string $useralias = 'u'): array { + global $USER; + + list($memberssql, $membersparams) = self::sql_members_visibility_condition($groupsalias, $groupsmembersalias); + + $where = " AND ( + {$groupsalias}.visibility = :all + OR ($memberssql) + OR ({$groupsalias}.visibility = :own AND {$useralias}.id = :currentuser2) + )"; + $params = [ + 'all' => GROUPS_VISIBILITY_ALL, + 'own' => GROUPS_VISIBILITY_OWN, + 'currentuser2' => $USER->id, + ]; + $params = array_merge($params, $membersparams); + return [$where, $params]; + } + + /** + * Return a condition to check if a user can view a group because it has MEMBERS visibility and they are a member. + * + * @param string $groupsalias The SQL alias being used for the groups table. + * @param string $groupsmembersalias The SQL alias being used for the groups_members table. + * @return array [$sql, $params] + */ + protected static function sql_members_visibility_condition(string $groupsalias = 'g', + string $groupsmembersalias = 'gm'): array { + global $USER; + $sql = "{$groupsalias}.visibility = :members + AND ( + SELECT gm2.id + FROM {groups_members} gm2 + WHERE gm2.groupid = {$groupsmembersalias}.groupid + AND gm2.userid = :currentuser + ) IS NOT NULL"; + $params = [ + 'members' => GROUPS_VISIBILITY_MEMBERS, + 'currentuser' => $USER->id + ]; + + return [$sql, $params]; + } +} diff --git a/group/externallib.php b/group/externallib.php index 1e25db334a56e..9d258195392d1 100644 --- a/group/externallib.php +++ b/group/externallib.php @@ -22,6 +22,7 @@ use core_external\external_value; use core_external\external_warnings; use core_external\util; +use core_group\visibility; /** * Group external functions @@ -34,6 +35,26 @@ */ class core_group_external extends external_api { + + /** + * Validate visibility. + * + * @param int $visibility Visibility string, must one of the visibility class constants. + * @throws invalid_parameter_exception if visibility is not an allowed value. + */ + protected static function validate_visibility(int $visibility): void { + $allowed = [ + GROUPS_VISIBILITY_ALL, + GROUPS_VISIBILITY_MEMBERS, + GROUPS_VISIBILITY_OWN, + GROUPS_VISIBILITY_NONE, + ]; + if (!array_key_exists($visibility, $allowed)) { + throw new invalid_parameter_exception('Invalid group visibility provided. Must be one of ' + . join(',', $allowed)); + } + } + /** * Returns description of method parameters * @@ -51,7 +72,14 @@ public static function create_groups_parameters() { 'description' => new external_value(PARAM_RAW, 'group description text'), 'descriptionformat' => new external_format_value('description', VALUE_DEFAULT), 'enrolmentkey' => new external_value(PARAM_RAW, 'group enrol secret phrase', VALUE_OPTIONAL), - 'idnumber' => new external_value(PARAM_RAW, 'id number', VALUE_OPTIONAL) + 'idnumber' => new external_value(PARAM_RAW, 'id number', VALUE_OPTIONAL), + 'visibility' => new external_value(PARAM_INT, + 'group visibility mode. 0 = Visible to all. 1 = Visible to members. ' + . '2 = See own membership. 3 = Membership is hidden. default: 0', + VALUE_DEFAULT, 0), + 'participation' => new external_value(PARAM_BOOL, + 'activity participation enabled? Only for "all" and "members" visibility. Default true.', + VALUE_DEFAULT, true), ) ), 'List of group object. A group has a courseid, a name, a description and an enrolment key.' ) @@ -101,6 +129,9 @@ public static function create_groups($groups) { // Validate format. $group->descriptionformat = util::validate_format($group->descriptionformat); + // Validate visibility. + self::validate_visibility($group->visibility); + // finally create the group $group->id = groups_create_group($group, false); if (!isset($group->enrolmentkey)) { @@ -134,7 +165,11 @@ public static function create_groups_returns() { 'description' => new external_value(PARAM_RAW, 'group description text'), 'descriptionformat' => new external_format_value('description'), 'enrolmentkey' => new external_value(PARAM_RAW, 'group enrol secret phrase'), - 'idnumber' => new external_value(PARAM_RAW, 'id number') + 'idnumber' => new external_value(PARAM_RAW, 'id number'), + 'visibility' => new external_value(PARAM_INT, + 'group visibility mode. 0 = Visible to all. 1 = Visible to members. 2 = See own membership. ' + . '3 = Membership is hidden.'), + 'participation' => new external_value(PARAM_BOOL, 'participation mode'), ) ), 'List of group object. A group has an id, a courseid, a name, a description and an enrolment key.' ); @@ -168,7 +203,8 @@ public static function get_groups($groupids) { $groups = array(); foreach ($params['groupids'] as $groupid) { // validate params - $group = groups_get_group($groupid, 'id, courseid, name, idnumber, description, descriptionformat, enrolmentkey', MUST_EXIST); + $group = groups_get_group($groupid, 'id, courseid, name, idnumber, description, descriptionformat, enrolmentkey, ' + . 'visibility, participation', MUST_EXIST); // now security checks $context = context_course::instance($group->courseid, IGNORE_MISSING); @@ -208,7 +244,11 @@ public static function get_groups_returns() { 'description' => new external_value(PARAM_RAW, 'group description text'), 'descriptionformat' => new external_format_value('description'), 'enrolmentkey' => new external_value(PARAM_RAW, 'group enrol secret phrase'), - 'idnumber' => new external_value(PARAM_RAW, 'id number') + 'idnumber' => new external_value(PARAM_RAW, 'id number'), + 'visibility' => new external_value(PARAM_INT, + 'group visibility mode. 0 = Visible to all. 1 = Visible to members. 2 = See own membership. ' + . '3 = Membership is hidden.'), + 'participation' => new external_value(PARAM_BOOL, 'participation mode'), ) ) ); @@ -251,7 +291,8 @@ public static function get_course_groups($courseid) { require_capability('moodle/course:managegroups', $context); $gs = groups_get_all_groups($params['courseid'], 0, 0, - 'g.id, g.courseid, g.name, g.idnumber, g.description, g.descriptionformat, g.enrolmentkey'); + 'g.id, g.courseid, g.name, g.idnumber, g.description, g.descriptionformat, g.enrolmentkey, ' + . 'g.visibility, g.participation'); $groups = array(); foreach ($gs as $group) { @@ -280,7 +321,11 @@ public static function get_course_groups_returns() { 'description' => new external_value(PARAM_RAW, 'group description text'), 'descriptionformat' => new external_format_value('description'), 'enrolmentkey' => new external_value(PARAM_RAW, 'group enrol secret phrase'), - 'idnumber' => new external_value(PARAM_RAW, 'id number') + 'idnumber' => new external_value(PARAM_RAW, 'id number'), + 'visibility' => new external_value(PARAM_INT, + 'group visibility mode. 0 = Visible to all. 1 = Visible to members. 2 = See own membership. ' + . '3 = Membership is hidden.'), + 'participation' => new external_value(PARAM_BOOL, 'participation mode'), ) ) ); @@ -1499,7 +1544,12 @@ public static function update_groups_parameters() { 'description' => new external_value(PARAM_RAW, 'group description text', VALUE_OPTIONAL), 'descriptionformat' => new external_format_value('description', VALUE_DEFAULT), 'enrolmentkey' => new external_value(PARAM_RAW, 'group enrol secret phrase', VALUE_OPTIONAL), - 'idnumber' => new external_value(PARAM_RAW, 'id number', VALUE_OPTIONAL) + 'idnumber' => new external_value(PARAM_RAW, 'id number', VALUE_OPTIONAL), + 'visibility' => new external_value(PARAM_TEXT, + 'group visibility mode. 0 = Visible to all. 1 = Visible to members. ' + . '2 = See own membership. 3 = Membership is hidden.', VALUE_OPTIONAL), + 'participation' => new external_value(PARAM_BOOL, + 'activity participation enabled? Only for "all" and "members" visibility', VALUE_OPTIONAL), ) ), 'List of group objects. A group is found by the id, then all other details provided will be updated.' ) @@ -1523,13 +1573,13 @@ public static function update_groups($groups) { $transaction = $DB->start_delegated_transaction(); foreach ($params['groups'] as $group) { - $group = (object)$group; + $group = (object) $group; if (trim($group->name) == '') { throw new invalid_parameter_exception('Invalid group name'); } - if (! $currentgroup = $DB->get_record('groups', array('id' => $group->id))) { + if (!$currentgroup = $DB->get_record('groups', array('id' => $group->id))) { throw new invalid_parameter_exception("Group $group->id does not exist"); } @@ -1539,6 +1589,24 @@ public static function update_groups($groups) { throw new invalid_parameter_exception('A different group with the same name already exists in the course'); } + if (isset($group->visibility) || isset($group->participation)) { + $hasmembers = $DB->record_exists('groups_members', ['groupid' => $group->id]); + if (isset($group->visibility)) { + // Validate visibility. + self::validate_visibility($group->visibility); + if ($hasmembers && $group->visibility != $currentgroup->visibility) { + throw new invalid_parameter_exception( + 'The visibility of this group cannot be changed as it currently has members.'); + } + } else { + $group->visibility = $currentgroup->visibility; + } + if (isset($group->participation) && $hasmembers && $group->participation != $currentgroup->participation) { + throw new invalid_parameter_exception( + 'The participation mode of this group cannot be changed as it currently has members.'); + } + } + $group->courseid = $currentgroup->courseid; // Now security checks. diff --git a/group/group_form.php b/group/group_form.php index 15b638870deb1..563cec6f627a8 100644 --- a/group/group_form.php +++ b/group/group_form.php @@ -25,6 +25,8 @@ defined('MOODLE_INTERNAL') || die; +use core_group\visibility; + require_once($CFG->dirroot.'/lib/formslib.php'); /** @@ -66,10 +68,27 @@ function definition () { $mform->addHelpButton('enrolmentkey', 'enrolmentkey', 'group'); $mform->setType('enrolmentkey', PARAM_RAW); + $visibilityoptions = [ + GROUPS_VISIBILITY_ALL => get_string('visibilityall', 'group'), + GROUPS_VISIBILITY_MEMBERS => get_string('visibilitymembers', 'group'), + GROUPS_VISIBILITY_OWN => get_string('visibilityown', 'group'), + GROUPS_VISIBILITY_NONE => get_string('visibilitynone', 'group') + ]; + $mform->addElement('select', 'visibility', get_string('visibility', 'group'), $visibilityoptions); + $mform->addHelpButton('visibility', 'visibility', 'group'); + $mform->setType('visibility', PARAM_INT); + + $mform->addElement('advcheckbox', 'participation', '', get_string('participation', 'group')); + $mform->addHelpButton('participation', 'participation', 'group'); + $mform->setType('participation', PARAM_BOOL); + $mform->setDefault('participation', 1); + $mform->disabledIf('participation', 'visibility', 'in', [GROUPS_VISIBILITY_OWN, GROUPS_VISIBILITY_NONE]); + // Group conversation messaging. if (\core_message\api::can_create_group_conversation($USER->id, $coursecontext)) { $mform->addElement('selectyesno', 'enablemessaging', get_string('enablemessaging', 'group')); $mform->addHelpButton('enablemessaging', 'enablemessaging', 'group'); + $mform->disabledIf('enablemessaging', 'visibility', 'in', [GROUPS_VISIBILITY_OWN, GROUPS_VISIBILITY_NONE]); } $mform->addElement('static', 'currentpicture', get_string('currentpicture')); @@ -124,6 +143,16 @@ public function definition_after_data() { } } + if ($DB->record_exists('groups_members', ['groupid' => $groupid])) { + // If the group has members, lock visibility and participation fields. + /** @var MoodleQuickForm_select $visibility */ + $visibility = $mform->getElement('visibility'); + $visibility->freeze(); + /** @var MoodleQuickForm_advcheckbox $participation */ + $participation = $mform->getElement('participation'); + $participation->freeze(); + } + } /** diff --git a/group/lib.php b/group/lib.php index c898e3ea5b847..197a2f5a84fb6 100644 --- a/group/lib.php +++ b/group/lib.php @@ -260,6 +260,13 @@ function groups_create_group($data, $editform = false, $editoroptions = false) { } } + $data->visibility ??= GROUPS_VISIBILITY_ALL; + + if (!in_array($data->visibility, [GROUPS_VISIBILITY_ALL, GROUPS_VISIBILITY_MEMBERS])) { + $data->participation = false; + $data->enablemessaging = false; + } + if ($editform and $editoroptions) { $data->description = $data->description_editor['text']; $data->descriptionformat = $data->description_editor['format']; @@ -285,6 +292,8 @@ function groups_create_group($data, $editform = false, $editoroptions = false) { // Invalidate the grouping cache for the course cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($course->id)); + // Rebuild the coursehiddengroups cache for the course. + \core_group\visibility::update_hiddengroups_cache($course->id); // Group conversation messaging. if (\core_message\api::can_create_group_conversation($USER->id, $context)) { @@ -423,6 +432,10 @@ function groups_update_group($data, $editform = false, $editoroptions = false) { throw new moodle_exception('idnumbertaken'); } } + if (isset($data->visibility) && !in_array($data->visibility, [GROUPS_VISIBILITY_ALL, GROUPS_VISIBILITY_MEMBERS])) { + $data->participation = false; + $data->enablemessaging = false; + } if ($editform and $editoroptions) { $data = file_postupdate_standard_editor($data, 'description', $editoroptions, $context, 'group', 'description', $data->id); @@ -432,6 +445,8 @@ function groups_update_group($data, $editform = false, $editoroptions = false) { // Invalidate the group data. cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($data->courseid)); + // Rebuild the coursehiddengroups cache for the course. + \core_group\visibility::update_hiddengroups_cache($data->courseid); $group = $DB->get_record('groups', array('id'=>$data->id)); @@ -575,6 +590,8 @@ function groups_delete_group($grouporid) { cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($group->courseid)); // Purge the group and grouping cache for users. cache_helper::purge_by_definition('core', 'user_group_groupings'); + // Rebuild the coursehiddengroups cache for the course. + \core_group\visibility::update_hiddengroups_cache($group->courseid); // Trigger group event. $params = array( @@ -723,6 +740,8 @@ function groups_delete_groups($courseid, $showfeedback=false) { cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($courseid)); // Purge the group and grouping cache for users. cache_helper::purge_by_definition('core', 'user_group_groupings'); + // Rebuild the coursehiddengroups cache for the course. + \core_group\visibility::update_hiddengroups_cache($courseid); if ($showfeedback) { echo $OUTPUT->notification(get_string('deleted').' - '.get_string('groups', 'group'), 'notifysuccess'); diff --git a/group/tests/behat/backup_restore_groups.feature b/group/tests/behat/backup_restore_groups.feature new file mode 100644 index 0000000000000..fd5e776ce6ed3 --- /dev/null +++ b/group/tests/behat/backup_restore_groups.feature @@ -0,0 +1,48 @@ +@core @core_group +Feature: Backup and restore a course containing groups + In order to transfer groups to another course + As a teacher + I want to backup and restore a course retaining the groups + + Background: + Given the following "courses" exist: + | fullname | shortname | format | enablecompletion | numsections | + | Course 1 | C1 | topics | 1 | 3 | + And the following "users" exist: + | username | firstname | lastname | + | teacher1 | Teacher | Teacher | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + And the following "groups" exist: + | name | course | idnumber | visibility | participation | + | Visible to all/Participation | C1 | VP | 0 | 1 | + | Visible to members/Participation | C1 | MP | 1 | 1 | + | See own membership | C1 | O | 2 | 0 | + | Not visible | C1 | N | 3 | 0 | + | Visible to all/Non-Participation | C1 | VN | 0 | 0 | + | Visible to members/Non-Participation | C1 | MN | 1 | 0 | + And I log in as "admin" + And I backup "Course 1" course using this options: + | Confirmation | Filename | test_backup.mbz | + And I restore "test_backup.mbz" backup into a new course using this options: + | Schema | Course name | Restored course | + + @javascript + Scenario Outline: Check restored groups + Given I am on the "Restored course copy 1" "groups" page logged in as teacher1 + When I set the field "Groups" to "" + And I press "Edit group settings" + Then the following fields match these values: + | Group ID number | | + | Group visibility | | + | Allow activity participation | | + + Examples: + | group | idnumber | visibility | participation | + | Visible to all/Participation | VP | 0 | 1 | + | Visible to members/Participation | MP | 1 | 1 | + | See own membership | O | 2 | 0 | + | Not visible | N | 3 | 0 | + | Visible to all/Non-Participation | VN | 0 | 0 | + | Visible to members/Non-Participation | MN | 1 | 0 | diff --git a/group/tests/behat/private_groups.feature b/group/tests/behat/private_groups.feature new file mode 100644 index 0000000000000..ee2fb28605ca9 --- /dev/null +++ b/group/tests/behat/private_groups.feature @@ -0,0 +1,129 @@ +@core @core_group +Feature: Private groups + As a teacher + In order to organise students into groups while protecting their privacy + I want to define groups that are not visible to all students + + Background: + Given the following "courses" exist: + | fullname | shortname | format | enablecompletion | numsections | + | Course 1 | C1 | topics | 1 | 3 | + And the following "users" exist: + | username | firstname | lastname | + | teacher1 | Teacher | Teacher | + | student1 | Student | 1 | + | student2 | Student | 2 | + | student3 | Student | 3 | + | student4 | Student | 4 | + | student5 | Student | 5 | + | student6 | Student | 6 | + | student7 | Student | 7 | + | student8 | Student | 8 | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + | student1 | C1 | student | + | student2 | C1 | student | + | student3 | C1 | student | + | student4 | C1 | student | + | student5 | C1 | student | + | student6 | C1 | student | + | student7 | C1 | student | + | student8 | C1 | student | + And the following "groups" exist: + | name | course | idnumber | visibility | participation | + | Visible to all/Participation | C1 | VP | 0 | 1 | + | Visible to members/Participation | C1 | MP | 1 | 1 | + | See own membership | C1 | O | 2 | 0 | + | Not visible | C1 | N | 3 | 0 | + | Visible to all/Non-Participation | C1 | VN | 0 | 0 | + | Visible to members/Non-Participation | C1 | MN | 1 | 0 | + And the following "group members" exist: + | user | group | + | student1 | VP | + | student1 | VN | + | student2 | MP | + | student2 | MN | + | student3 | O | + | student4 | N | + | student5 | VP | + | student5 | VN | + | student6 | MP | + | student6 | MN | + | student7 | O | + | student8 | N | + + Scenario: Participants in "Visible to all" groups see their membership and other members: + Given I log in as "student1" + And I am on "Course 1" course homepage + When I follow "Participants" + Then the following should exist in the "participants" table: + | First name / Surname | Groups | + | Student 1 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 2 | No groups | + | Student 3 | No groups | + | Student 4 | No groups | + | Student 5 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 6 | No groups | + | Student 7 | No groups | + | Student 8 | No groups | + + Scenario: Participants in "Visible to members" groups see their membership and other members, plus "Visible to all" + Given I log in as "student2" + And I am on "Course 1" course homepage + When I follow "Participants" + Then the following should exist in the "participants" table: + | First name / Surname | Groups | + | Student 1 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 2 | Visible to members/Non-Participation, Visible to members/Participation | + | Student 3 | No groups | + | Student 4 | No groups | + | Student 5 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 6 | Visible to members/Non-Participation, Visible to members/Participation | + | Student 7 | No groups | + | Student 8 | No groups | + + Scenario: Participants in "See own membership" groups see their membership but not other members, plus "Visible to all" + Given I log in as "student3" + And I am on "Course 1" course homepage + When I follow "Participants" + Then the following should exist in the "participants" table: + | First name / Surname | Groups | + | Student 1 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 2 | No groups | + | Student 3 | See own membership | + | Student 4 | No groups | + | Student 5 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 6 | No groups | + | Student 7 | No groups | + | Student 8 | No groups | + + Scenario: Participants in "Not visible" groups do not see that group, do see "Visible to all" + Given I log in as "student4" + And I am on "Course 1" course homepage + When I follow "Participants" + Then the following should exist in the "participants" table: + | First name / Surname | Groups | + | Student 1 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 2 | No groups | + | Student 3 | No groups | + | Student 4 | No groups | + | Student 5 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 6 | No groups | + | Student 7 | No groups | + | Student 8 | No groups | + + Scenario: View participants list as a teacher: + Given I log in as "teacher1" + And I am on "Course 1" course homepage + When I follow "Participants" + Then the following should exist in the "participants" table: + | First name / Surname | Groups | + | Student 1 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 2 | Visible to members/Non-Participation, Visible to members/Participation | + | Student 3 | See own membership | + | Student 4 | Not visible | + | Student 5 | Visible to all/Non-Participation, Visible to all/Participation | + | Student 6 | Visible to members/Non-Participation, Visible to members/Participation | + | Student 7 | See own membership | + | Student 8 | Not visible | diff --git a/group/tests/behat/update_groups.feature b/group/tests/behat/update_groups.feature index b3b6bfb4d1af3..dc8e0aaf948e9 100644 --- a/group/tests/behat/update_groups.feature +++ b/group/tests/behat/update_groups.feature @@ -142,3 +142,20 @@ Feature: Automatic updating of groups and groupings | Enrolment key | Abcdef-1 | And I press "Save changes" And I should not see "This enrolment key is already used for another group." + + @javascript + Scenario: Visibility and Participation settings are locked once a group has members + Given I set the field "groups" to "Group (with ID)" + And I press "Edit group settings" + And "visibility" "select" should exist + And the field "Group visibility" matches value "Visible to all" + And the "participation" "checkbox" should be enabled + And the field "Allow activity participation" matches value "1" + When the following "group members" exist: + | user | group | + | teacher1 | An ID | + And I reload the page + Then "visibility" "select" should not exist + And "Visible to all" "text" should exist + And the "participation" "checkbox" should be disabled + And the field "Allow activity participation" matches value "1" diff --git a/group/tests/externallib_test.php b/group/tests/externallib_test.php index 719db86305035..b19ebc0d752d9 100644 --- a/group/tests/externallib_test.php +++ b/group/tests/externallib_test.php @@ -60,6 +60,8 @@ public function test_create_groups() { $group2['courseid'] = $course->id; $group2['name'] = 'Group Test 2'; $group2['description'] = 'Group Test 2 description'; + $group2['visibility'] = GROUPS_VISIBILITY_MEMBERS; + $group2['participation'] = false; $group3 = array(); $group3['courseid'] = $course->id; $group3['name'] = 'Group Test 3'; @@ -92,10 +94,15 @@ public function test_create_groups() { $this->assertEquals($dbgroup->descriptionformat, $group1['descriptionformat']); $this->assertEquals($dbgroup->enrolmentkey, $group1['enrolmentkey']); $this->assertEquals($dbgroup->idnumber, $group1['idnumber']); + // The visibility and participation attributes were not specified, so should match the default values. + $groupvisibility = GROUPS_VISIBILITY_ALL; + $groupparticipation = true; break; case $group2['name']: $groupdescription = $group2['description']; $groupcourseid = $group2['courseid']; + $groupvisibility = $group2['visibility']; + $groupparticipation = $group2['participation']; break; default: throw new \moodle_exception('unknowgroupname'); @@ -103,6 +110,8 @@ public function test_create_groups() { } $this->assertEquals($dbgroup->description, $groupdescription); $this->assertEquals($dbgroup->courseid, $groupcourseid); + $this->assertEquals($dbgroup->visibility, $groupvisibility); + $this->assertEquals($dbgroup->participation, $groupparticipation); } try { @@ -120,6 +129,33 @@ public function test_create_groups() { $froups = core_group_external::create_groups(array($group4)); } + /** + * Test that creating a group with an invalid visibility value throws an exception. + * + * @covers \core_group_external::create_groups + * @return void + */ + public function test_create_group_invalid_visibility(): void { + $this->resetAfterTest(true); + + $course = self::getDataGenerator()->create_course(); + + $group1 = array(); + $group1['courseid'] = $course->id; + $group1['name'] = 'Group Test 1'; + $group1['description'] = 'Group Test 1 description'; + $group1['visibility'] = 1000; + + // Set the required capabilities by the external function. + $context = \context_course::instance($course->id); + $roleid = $this->assignUserCapability('moodle/course:managegroups', $context->id); + $this->assignUserCapability('moodle/course:view', $context->id, $roleid); + + // Call the external function. + $this->expectException('invalid_parameter_exception'); + core_group_external::create_groups([$group1]); + } + /** * Test update_groups */ @@ -161,6 +197,7 @@ public function test_update_groups() { $group1data['idnumber'] = 'CHANGED'; core_group_external::update_groups(array($group1data)); $group2data['description'] = 'Group Test 2 description CHANGED'; + $group2data['visibility'] = GROUPS_VISIBILITY_MEMBERS; core_group_external::update_groups(array($group2data)); foreach ([$group1, $group2] as $group) { @@ -169,16 +206,20 @@ public function test_update_groups() { case $group1data['name']: $this->assertEquals($dbgroup->idnumber, $group1data['idnumber']); $groupdescription = $group1data['description']; + // Visibility was not specified, so should match the default value. + $groupvisibility = GROUPS_VISIBILITY_ALL; break; case $group2data['name']: $this->assertEquals($dbgroup->idnumber, $group2data['idnumber']); $groupdescription = $group2data['description']; + $groupvisibility = $group2data['visibility']; break; default: throw new \moodle_exception('unknowngroupname'); break; } $this->assertEquals($dbgroup->description, $groupdescription); + $this->assertEquals($dbgroup->visibility, $groupvisibility); } // Taken idnumber exception. @@ -199,6 +240,103 @@ public function test_update_groups() { $groups = core_group_external::update_groups(array($group1data)); } + /** + * Test an exception is thrown when an invalid visibility value is passed in an update. + * + * @covers \core_group_external::update_groups + * @return void + */ + public function test_update_groups_invalid_visibility(): void { + $this->resetAfterTest(true); + + $course = self::getDataGenerator()->create_course(); + + $group1data = array(); + $group1data['courseid'] = $course->id; + $group1data['name'] = 'Group Test 1'; + + // Set the required capabilities by the external function. + $context = \context_course::instance($course->id); + $roleid = $this->assignUserCapability('moodle/course:managegroups', $context->id); + $this->assignUserCapability('moodle/course:view', $context->id, $roleid); + + // Create the test group. + $group1 = self::getDataGenerator()->create_group($group1data); + + $group1data['id'] = $group1->id; + unset($group1data['courseid']); + $group1data['visibility'] = 1000; + + $this->expectException('invalid_parameter_exception'); + core_group_external::update_groups(array($group1data)); + } + + /** + * Attempting to change the visibility of a group with members should throw an exception. + * + * @covers \core_group_external::update_groups + * @return void + */ + public function test_update_groups_visibility_with_members(): void { + $this->resetAfterTest(true); + + $course = self::getDataGenerator()->create_course(); + + $group1data = array(); + $group1data['courseid'] = $course->id; + $group1data['name'] = 'Group Test 1'; + + // Set the required capabilities by the external function. + $context = \context_course::instance($course->id); + $roleid = $this->assignUserCapability('moodle/course:managegroups', $context->id); + $this->assignUserCapability('moodle/course:view', $context->id, $roleid); + + // Create the test group and add a member. + $group1 = self::getDataGenerator()->create_group($group1data); + $user1 = self::getDataGenerator()->create_and_enrol($course); + self::getDataGenerator()->create_group_member(['userid' => $user1->id, 'groupid' => $group1->id]); + + $group1data['id'] = $group1->id; + unset($group1data['courseid']); + $group1data['visibility'] = GROUPS_VISIBILITY_MEMBERS; + + $this->expectExceptionMessage('The visibility of this group cannot be changed as it currently has members.'); + core_group_external::update_groups(array($group1data)); + } + + /** + * Attempting to change the participation field of a group with members should throw an exception. + * + * @covers \core_group_external::update_groups + * @return void + */ + public function test_update_groups_participation_with_members(): void { + $this->resetAfterTest(true); + + $course = self::getDataGenerator()->create_course(); + + $group1data = array(); + $group1data['courseid'] = $course->id; + $group1data['name'] = 'Group Test 1'; + + // Set the required capabilities by the external function. + $context = \context_course::instance($course->id); + $roleid = $this->assignUserCapability('moodle/course:managegroups', $context->id); + $this->assignUserCapability('moodle/course:view', $context->id, $roleid); + + // Create the test group and add a member. + $group1 = self::getDataGenerator()->create_group($group1data); + $user1 = self::getDataGenerator()->create_and_enrol($course); + self::getDataGenerator()->create_group_member(['userid' => $user1->id, 'groupid' => $group1->id]); + + $group1data['id'] = $group1->id; + unset($group1data['courseid']); + $group1data['participation'] = false; + + $this->expectExceptionMessage('The participation mode of this group cannot be changed as it currently has members.'); + core_group_external::update_groups(array($group1data)); + } + /** * Test get_groups */ @@ -219,6 +357,8 @@ public function test_get_groups() { $group2data['courseid'] = $course->id; $group2data['name'] = 'Group Test 2'; $group2data['description'] = 'Group Test 2 description'; + $group2data['visibility'] = GROUPS_VISIBILITY_MEMBERS; + $group2data['participation'] = false; $group1 = self::getDataGenerator()->create_group($group1data); $group2 = self::getDataGenerator()->create_group($group2data); @@ -241,6 +381,9 @@ public function test_get_groups() { case $group1->name: $groupdescription = $group1->description; $groupcourseid = $group1->courseid; + // The visibility and participation attributes were not specified, so should match the default values. + $groupvisibility = GROUPS_VISIBILITY_ALL; + $groupparticipation = true; $this->assertEquals($dbgroup->descriptionformat, $group1->descriptionformat); $this->assertEquals($dbgroup->enrolmentkey, $group1->enrolmentkey); $this->assertEquals($dbgroup->idnumber, $group1->idnumber); @@ -248,6 +391,8 @@ public function test_get_groups() { case $group2->name: $groupdescription = $group2->description; $groupcourseid = $group2->courseid; + $groupvisibility = $group2->visibility; + $groupparticipation = $group2->participation; break; default: throw new \moodle_exception('unknowgroupname'); @@ -255,6 +400,8 @@ public function test_get_groups() { } $this->assertEquals($dbgroup->description, $groupdescription); $this->assertEquals($dbgroup->courseid, $groupcourseid); + $this->assertEquals($dbgroup->visibility, $groupvisibility); + $this->assertEquals($dbgroup->participation, $groupparticipation); } // Call without required capability diff --git a/group/upgrade.txt b/group/upgrade.txt index 894ffc4efbc87..9d25ad0d45e4a 100644 --- a/group/upgrade.txt +++ b/group/upgrade.txt @@ -1,6 +1,22 @@ This files describes API changes in /group/*, information provided here is intended especially for developers. +=== 4.2 === +* `\core_group\visibility` class added to support new `visibility` field in group records. This holds the visibility constants + and helper functions for applying visibility restrictions when querying groups or group members in the database. +* Changes to the group form to support visibility features: + - New `visibility` field. + - New `participation` field. + - `participation` and `enablemessaging` fields are disabled (default: false) when `visibility` is set + to `visibility::OWN` or `visibility::NONE`. +* The following externallib functions now accept `visibility` and `participation` as optional parameters: + - create_groups() + - update_groups() +* The following externallib functions now also return `visibility` and `participation` fields in their responses: + - create_groups() + - get_groups() + - get_course_groups() + === 3.11 === * The groups do not support 'hidepicture' any more, and so the column 'hidepicture' diff --git a/lang/en/cache.php b/lang/en/cache.php index 30ecda933a777..aa644ce4beba7 100644 --- a/lang/en/cache.php +++ b/lang/en/cache.php @@ -51,6 +51,7 @@ $string['cachedef_coursecattree'] = 'Course categories tree'; $string['cachedef_coursecompletion'] = 'Course completion status'; $string['cachedef_coursecontacts'] = 'List of course contacts'; +$string['cachedef_coursehiddengroups'] = 'Number of groups on a course with restricted visibility'; $string['cachedef_coursemodinfo'] = 'Accumulated information about modules and sections for each course'; $string['cachedef_courseeditorstate'] = 'Session course state cache keys to detect course changes in the frontend'; $string['cachedef_course_image'] = 'Course images'; diff --git a/lang/en/group.php b/lang/en/group.php index f93966a7950c2..645b8e55f212c 100644 --- a/lang/en/group.php +++ b/lang/en/group.php @@ -174,6 +174,14 @@ $string['mygroups'] = 'My groups'; $string['othergroups'] = 'Other groups'; $string['overview'] = 'Overview'; +$string['participation'] = 'Allow activity participation'; +$string['participation_help'] = 'If enabled, members can select this group when participating in an activity using Separate Groups +or Visible Groups mode. + +This setting is only applicable if the Group visibility is set to "Visible to all" or "Visible to members". Participation is +disabled otherwise. + +This setting cannot be edited once a group has members.'; $string['potentialmembers'] = 'Potential members: {$a}'; $string['potentialmembs'] = 'Potential members'; $string['printerfriendly'] = 'Printer-friendly display'; @@ -198,6 +206,25 @@ $string['usercount'] = 'User count'; $string['usercounttotal'] = 'User count ({$a})'; $string['usergroupmembership'] = 'Selected user\'s membership:'; +$string['visibility'] = 'Group visibility'; +$string['visibility_help'] = 'Controls the visibility of membership to this group. + +If "Visible to all" is set, all users can see when a user is a member of this group (default). + +If "Visible to members" is set, only members of this group can see when another user is a member. + +If "See own membership" is set, users can see that they are in this group, but cannot see that other users are members of the group. + +If "Membership is hidden" is set, users cannot see that they or anyone else are members of the group. + +Users with moodle/course:viewhiddengroups will always be able to see group membership. + +This setting cannot be edited once a group has members. +'; +$string['visibilityall'] = 'Visible to all'; +$string['visibilitymembers'] = 'Visible to members'; +$string['visibilityown'] = 'See own membership'; +$string['visibilitynone'] = 'Membership is hidden'; $string['memberofgroup'] = 'Group member of: {$a}'; // Deprecated since Moodle 3.11. diff --git a/lang/en/role.php b/lang/en/role.php index 4b366744316a7..884b247e4683c 100644 --- a/lang/en/role.php +++ b/lang/en/role.php @@ -204,6 +204,7 @@ $string['course:viewcoursegrades'] = 'View course grades'; $string['course:viewhiddenactivities'] = 'View hidden activities'; $string['course:viewhiddencourses'] = 'View hidden courses'; +$string['course:viewhiddengroups'] = 'View hidden groups'; $string['course:viewhiddensections'] = 'View hidden sections'; $string['course:viewhiddenuserfields'] = 'View hidden user fields'; $string['course:viewparticipants'] = 'View participants'; diff --git a/lib/db/access.php b/lib/db/access.php index ef1e11b74f23c..1d54febf4dce3 100644 --- a/lib/db/access.php +++ b/lib/db/access.php @@ -1156,6 +1156,17 @@ ) ), + 'moodle/course:viewhiddengroups' => array( + 'riskbitmask' => RISK_PERSONAL, + 'captype' => 'READ', + 'contextlevel' => CONTEXT_COURSE, + 'archetypes' => array( + 'teacher' => CAP_ALLOW, + 'editingteacher' => CAP_ALLOW, + 'manager' => CAP_ALLOW + ) + ), + 'moodle/course:reset' => array( 'riskbitmask' => RISK_DATALOSS, diff --git a/lib/db/caches.php b/lib/db/caches.php index 7a473be767054..8d0c2be2d7482 100644 --- a/lib/db/caches.php +++ b/lib/db/caches.php @@ -119,6 +119,14 @@ 'staticaccelerationsize' => 2, // The original cache used 1, we've increased that to two. ), + // Whether a course currently has hidden groups. + 'coursehiddengroups' => array( + 'mode' => cache_store::MODE_APPLICATION, + 'simplekeys' => true, // The course id the groupings exist for. + 'simpledata' => true, // Booleans. + 'staticacceleration' => true, // Likely there will be a couple of calls to this. + ), + // Used to cache calendar subscriptions. 'calendar_subscriptions' => array( 'mode' => cache_store::MODE_APPLICATION, diff --git a/lib/db/install.xml b/lib/db/install.xml index 617dd9da70d0d..d67a2c6f55560 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -1,5 +1,5 @@ - @@ -2322,6 +2322,8 @@ + + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 583ef1cda6a38..78f4b207c6be7 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -3118,5 +3118,25 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2023030300.03); } + if ($oldversion < 2023031000.01) + // Define field id to be added to groups. + $table = new xmldb_table('groups'); + $field = new xmldb_field('visibility', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '0', 'picture'); + + // Conditionally launch add field visibility. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + // Define field participation to be added to groups. + $field = new xmldb_field('participation', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '1', 'visibility'); + + // Conditionally launch add field participation. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + upgrade_main_savepoint(true, 2023031000.01); + } + return true; } diff --git a/lib/grouplib.php b/lib/grouplib.php index bee8583d979a6..94730d3eed7fd 100644 --- a/lib/grouplib.php +++ b/lib/grouplib.php @@ -58,6 +58,26 @@ */ define('GROUPS_JOIN_ALL', 2); +/** + * All users can see this group and its members. + */ +define('GROUPS_VISIBILITY_ALL', 0); + +/** + * Members of this group can see this group and other members. + */ +define('GROUPS_VISIBILITY_MEMBERS', 1); + +/** + * Members of this group can see the group and their own membership, but not each other's membership + */ +define('GROUPS_VISIBILITY_OWN', 2); + +/** + * No-one can see this group or its members. Members of the group will not know they are in the group. + */ +define('GROUPS_VISIBILITY_NONE', 3); + /** * Determines if a group with a given groupid exists. * @@ -213,6 +233,9 @@ function groups_get_grouping($groupingid, $fields='*', $strictness=IGNORE_MISSIN /** * Gets array of all groups in a specified course (subject to the conditions imposed by the other arguments). * + * If a user does not have moodle/course:viewhiddengroups, the list of groups and members will be restricted based on the + * visibility setting of each group. + * * @category group * @param int $courseid The id of the course. * @param int|int[] $userid optional user id or array of ids, returns only groups continaing one or more of those users. @@ -223,11 +246,12 @@ function groups_get_grouping($groupingid, $fields='*', $strictness=IGNORE_MISSIN * @param bool $withmembers if true return an extra field members (int[]) which is the list of userids that * are members of each group. For this to work, g.id (or g.*) must be included in $fields. * In this case, the final results will always be an array indexed by group id. + * @param bool $participationonly Only return groups where the participation field is true. * @return array returns an array of the group objects (unless you have done something very weird * with the $fields option). */ -function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.*', $withmembers=false) { - global $DB; +function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.*', $withmembers=false, $participationonly = false) { + global $DB, $USER; // We need to check that we each field in the fields list belongs to the group table and that it has not being // aliased. If its something else we need to avoid the cache and run the query as who knows whats going on. @@ -249,7 +273,7 @@ function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.* } } - if (empty($userid) && $knownfields && !$withmembers) { + if (empty($userid) && $knownfields && !$withmembers && \core_group\visibility::can_view_all_groups($courseid)) { // We can use the cache. $data = groups_get_course_data($courseid); if (empty($groupingid)) { @@ -266,6 +290,9 @@ function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.* } } } + if ($participationonly) { + $groups = array_filter($groups, fn($group) => $group->participation); + } // Yay! We could use the cache. One more query saved. return $groups; } @@ -289,14 +316,44 @@ function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.* array_unshift($params, $courseid); + $visibilityfrom = ''; + $visibilitywhere = ''; + $viewhidden = has_capability('moodle/course:viewhiddengroups', context_course::instance($courseid)); + if (!$viewhidden) { + // Apply group visibility restrictions. Only return groups where visibility is ALL, or the current user is a member and the + // visibility is MEMBERS or OWN. + $userids = []; + if (empty($userid)) { + $userids = [$USER->id]; + $visibilityfrom = "LEFT JOIN {groups_members} gm ON gm.groupid = g.id AND gm.userid = ?"; + } + [$insql, $inparams] = $DB->get_in_or_equal([GROUPS_VISIBILITY_MEMBERS, GROUPS_VISIBILITY_OWN]); + $visibilitywhere = "AND (g.visibility = ? OR (g.visibility $insql AND gm.id IS NOT NULL))"; + $params = array_merge( + $userids, + $params, + [GROUPS_VISIBILITY_ALL], + $inparams + ); + } + + $participationwhere = ''; + if ($participationonly) { + $participationwhere = "AND g.participation = ?"; + $params = array_merge($params, [1]); + } + $results = $DB->get_records_sql(" SELECT $fields FROM {groups} g $userfrom $groupingfrom + $visibilityfrom WHERE g.courseid = ? $userwhere $groupingwhere + $visibilitywhere + $participationwhere ORDER BY g.name ASC", $params); if (!$withmembers) { @@ -312,7 +369,44 @@ function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.* $groups[$row->id] = $row; $groups[$row->id]->members = []; } - $groupmembers = $DB->get_records_list('groups_members', 'groupid', array_keys($groups)); + + $gmvisibilityfrom = ''; + $gmvisibilitywhere = ''; + $gmvisibilityparams = []; + if (!$viewhidden) { + // Only return membership records where visibility is ALL, visibility is MEMBERS and the current user is a member, + // or visibility is OWN and the record is for the current user. + $gmvisibilityfrom = " + JOIN {groups} g ON gm.groupid = g.id + "; + $gmvisibilitywhere = " + AND (g.visibility = ? + OR (g.visibility = ? + AND g.id IN (SELECT gm2.groupid FROM {groups_members} gm2 WHERE gm2.groupid = g.id AND gm2.userid = ?)) + OR (g.visibility = ? + AND gm.userid = ?))"; + $gmvisibilityparams = [ + GROUPS_VISIBILITY_ALL, + GROUPS_VISIBILITY_MEMBERS, + $USER->id, + GROUPS_VISIBILITY_OWN, + $USER->id + ]; + } + + $groupmembers = []; + if (!empty($groups)) { + [$gmin, $gmparams] = $DB->get_in_or_equal(array_keys($groups)); + $params = array_merge($gmparams, $gmvisibilityparams); + $gmsql = " + SELECT gm.* + FROM {groups_members} gm + $gmvisibilityfrom + WHERE gm.groupid $gmin + $gmvisibilitywhere"; + $groupmembers = $DB->get_records_sql($gmsql, $params); + } + foreach ($groupmembers as $gm) { $groups[$gm->groupid]->members[$gm->userid] = $gm->userid; } @@ -328,12 +422,23 @@ function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.* */ function groups_get_my_groups() { global $DB, $USER; + + $params = ['userid' => $USER->id]; + + $viewhidden = has_capability('moodle/course:viewhiddengroups', context_system::instance()); + $visibilitywhere = ''; + if (!$viewhidden) { + $params['novisibility'] = GROUPS_VISIBILITY_NONE; + $visibilitywhere = 'AND g.visibility != :novisibility'; + } + return $DB->get_records_sql("SELECT * FROM {groups_members} gm JOIN {groups} g ON g.id = gm.groupid - WHERE gm.userid = ? - ORDER BY name ASC", array($USER->id)); + WHERE gm.userid = :userid + $visibilitywhere + ORDER BY name ASC", $params); } /** @@ -347,23 +452,44 @@ function groups_get_my_groups() { function groups_get_user_groups($courseid, $userid=0) { global $USER, $DB; + if (empty($courseid)) { + return ['0' => []]; + } + if (empty($userid)) { $userid = $USER->id; } + $usergroups = false; + $viewhidden = has_capability('moodle/course:viewhiddengroups', context_course::instance($courseid)); + $viewall = \core_group\visibility::can_view_all_groups($courseid); + $cache = cache::make('core', 'user_group_groupings'); - // Try to retrieve group ids from the cache. - $usergroups = $cache->get($userid); + if ($viewall) { + // Try to retrieve group ids from the cache. + $usergroups = $cache->get($userid); + } if ($usergroups === false) { + $sql = "SELECT g.id, g.courseid, gg.groupingid FROM {groups} g JOIN {groups_members} gm ON gm.groupid = g.id LEFT JOIN {groupings_groups} gg ON gg.groupid = g.id - WHERE gm.userid = ?"; + WHERE gm.userid = :userid"; + + $params = ['userid' => $userid]; - $rs = $DB->get_recordset_sql($sql, array($userid)); + if (!$viewhidden) { + // Apply visibility restrictions. + // Everyone can see who is in groups with ALL visibility. + list($visibilitywhere, $visibilityparams) = \core_group\visibility::sql_group_visibility_where($userid); + $sql .= " AND " . $visibilitywhere; + $params = array_merge($params, $visibilityparams); + } + + $rs = $DB->get_recordset_sql($sql, $params); $usergroups = array(); $allgroups = array(); @@ -390,8 +516,10 @@ function groups_get_user_groups($courseid, $userid=0) { $usergroups[$cid]['0'] = array_keys($allgroups[$cid]); // All user groups in the course. } - // Cache the data. - $cache->set($userid, $usergroups); + if ($viewall) { + // Cache the data, if we got the full list of groups. + $cache->set($userid, $usergroups); + } } if (array_key_exists($courseid, $usergroups)) { @@ -432,7 +560,28 @@ function groups_is_member($groupid, $userid=null) { $userid = $USER->id; } - return $DB->record_exists('groups_members', array('groupid'=>$groupid, 'userid'=>$userid)); + $courseid = $DB->get_field('groups', 'courseid', ['id' => $groupid]); + if (!$courseid) { + return false; + } + + if (\core_group\visibility::can_view_all_groups($courseid)) { + return $DB->record_exists('groups_members', ['groupid' => $groupid, 'userid' => $userid]); + } + + $sql = "SELECT * + FROM {groups_members} gm + JOIN {groups} g ON gm.groupid = g.id + WHERE g.id = :groupid + AND gm.userid = :userid"; + $params = ['groupid' => $groupid, 'userid' => $userid]; + + list($visibilitywhere, $visibilityparams) = \core_group\visibility::sql_group_visibility_where($userid); + + $sql .= " AND " . $visibilitywhere; + $params = array_merge($params, $visibilityparams); + + return $DB->record_exists_sql($sql, $params); } /** @@ -489,12 +638,34 @@ function groups_has_membership($cm, $userid=null) { * group or false if no users or an error returned. */ function groups_get_members($groupid, $fields='u.*', $sort='lastname ASC') { - global $DB; + global $DB, $USER; - return $DB->get_records_sql("SELECT $fields - FROM {user} u, {groups_members} gm - WHERE u.id = gm.userid AND gm.groupid = ? - ORDER BY $sort", array($groupid)); + if (empty($groupid)) { + return []; + } + + $courseid = $DB->get_field('groups', 'courseid', ['id' => $groupid]); + + $select = "SELECT $fields"; + $from = "FROM {user} u + JOIN {groups_members} gm ON gm.userid = u.id"; + $where = "WHERE gm.groupid = :groupid"; + $order = "ORDER BY $sort"; + + $params = ['groupid' => $groupid]; + + if (!\core_group\visibility::can_view_all_groups($courseid)) { + $from .= " JOIN {groups} g ON g.id = gm.groupid"; + // Can view memberships of visibility is ALL, visibility is MEMBERS and current user is a member, + // or visibility is OWN and this is their membership. + list($visibilitywhere, $visibilityparams) = \core_group\visibility::sql_member_visibility_where(); + $params = array_merge($params, $visibilityparams); + $where .= $visibilitywhere; + } + + $sql = implode(PHP_EOL, [$select, $from, $where, $order]); + + return $DB->get_records_sql($sql, $params); } @@ -778,11 +949,12 @@ function groups_print_activity_menu($cm, $urlroot, $return=false, $hideallpartic $usergroups = array(); if ($groupmode == VISIBLEGROUPS or $aag) { - $allowedgroups = groups_get_all_groups($cm->course, 0, $cm->groupingid); // any group in grouping + $allowedgroups = groups_get_all_groups($cm->course, 0, $cm->groupingid, 'g.*', false, true); // Any group in grouping. // Get user's own groups and put to the top. - $usergroups = groups_get_all_groups($cm->course, $USER->id, $cm->groupingid); + $usergroups = groups_get_all_groups($cm->course, $USER->id, $cm->groupingid, 'g.*', false, true); } else { - $allowedgroups = groups_get_all_groups($cm->course, $USER->id, $cm->groupingid); // only assigned groups + // Only assigned groups. + $allowedgroups = groups_get_all_groups($cm->course, $USER->id, $cm->groupingid, 'g.*', false, true); } $activegroup = groups_get_activity_group($cm, true, $allowedgroups); @@ -900,9 +1072,9 @@ function groups_get_activity_group($cm, $update=false, $allowedgroups=null) { if (!is_array($allowedgroups)) { if ($groupmode == VISIBLEGROUPS or $groupmode === 'aag') { - $allowedgroups = groups_get_all_groups($cm->course, 0, $cm->groupingid); + $allowedgroups = groups_get_all_groups($cm->course, 0, $cm->groupingid, 'g.*', false, true); } else { - $allowedgroups = groups_get_all_groups($cm->course, $USER->id, $cm->groupingid); + $allowedgroups = groups_get_all_groups($cm->course, $USER->id, $cm->groupingid, 'g.*', false, true); } } @@ -951,10 +1123,10 @@ function groups_get_activity_allowed_groups($cm,$userid=0) { // then they can access all groups for the activity... $context = context_module::instance($cm->id); if ($groupmode == VISIBLEGROUPS or has_capability('moodle/site:accessallgroups', $context, $userid)) { - return groups_get_all_groups($cm->course, 0, $cm->groupingid); + return groups_get_all_groups($cm->course, 0, $cm->groupingid, 'g.*', false, true); } else { // ...otherwise they can only access groups they belong to - return groups_get_all_groups($cm->course, $userid, $cm->groupingid); + return groups_get_all_groups($cm->course, $userid, $cm->groupingid, 'g.*', false, true); } } @@ -1210,7 +1382,8 @@ function _group_verify_activegroup($courseid, $groupmode, $groupingid, array $al $SESSION->activegroup[$courseid][$groupmode][$groupingid] = 0; // all groups by default if user has accessallgroups } else if ($allowedgroups) { - if ($groupmode != SEPARATEGROUPS and $mygroups = groups_get_all_groups($courseid, $USER->id, $groupingid)) { + if ($groupmode != SEPARATEGROUPS + && $mygroups = groups_get_all_groups($courseid, $USER->id, $groupingid, 'g.*', false, true)) { $firstgroup = reset($mygroups); } else { $firstgroup = reset($allowedgroups); @@ -1359,11 +1532,29 @@ function groups_get_groups_members($groupsids, $extrafields=null, $sort='lastnam $userfieldsapi = \core_user\fields::for_userpic()->including(...($extrafields ?? [])); $userfields = $userfieldsapi->get_sql('u', false, '', '', false)->selects; - list($insql, $params) = $DB->get_in_or_equal($groupsids); + list($insql, $params) = $DB->get_in_or_equal($groupsids, SQL_PARAMS_NAMED); + + $courseids = $DB->get_fieldset_sql("SELECT DISTINCT courseid FROM {groups} WHERE id $insql", $params); + + if (count($courseids) > 1) { + // Groups from multiple courses. Have to check permission in system context. + $context = context_system::instance(); + } else { + $courseid = reset($courseids); + $context = context_course::instance($courseid); + } + + $visibilitywhere = ''; + if (!has_capability('moodle/course:viewhiddengroups', $context)) { + list($visibilitywhere, $visibilityparams) = \core_group\visibility::sql_member_visibility_where(); + $params = array_merge($params, $visibilityparams); + } return $DB->get_records_sql("SELECT $userfields - FROM {user} u, {groups_members} gm - WHERE u.id = gm.userid AND gm.groupid $insql + FROM {user} u + JOIN {groups_members} gm ON u.id = gm.userid + JOIN {groups} g ON g.id = gm.groupid + WHERE gm.groupid $insql $visibilitywhere GROUP BY $userfields ORDER BY $sort", $params); } @@ -1380,7 +1571,7 @@ function groups_get_activity_shared_group_members($cm, $userid = null) { global $USER; if (empty($userid)) { - $userid = $USER; + $userid = $USER->id; } $groupsids = array_keys(groups_get_activity_allowed_groups($cm, $userid)); diff --git a/lib/testing/generator/data_generator.php b/lib/testing/generator/data_generator.php index 92e5e79e02b4f..0c426d1fb503d 100644 --- a/lib/testing/generator/data_generator.php +++ b/lib/testing/generator/data_generator.php @@ -541,6 +541,14 @@ public function create_group($record) { $record['descriptionformat'] = FORMAT_MOODLE; } + if (!isset($record['visibility'])) { + $record['visibility'] = GROUPS_VISIBILITY_ALL; + } + + if (!isset($record['participation'])) { + $record['participation'] = true; + } + $id = groups_create_group((object)$record); // Allow tests to set group pictures. diff --git a/lib/tests/grouplib_test.php b/lib/tests/grouplib_test.php index f5a2bedc2329a..364f866073872 100644 --- a/lib/tests/grouplib_test.php +++ b/lib/tests/grouplib_test.php @@ -16,6 +16,8 @@ namespace core; +use core_group\visibility; + /** * Unit tests for lib/grouplib.php * @@ -1909,4 +1911,420 @@ public function test_groups_get_activity_shared_group_members() { $this->assertCount(2, $members); // Now I see members of group 3. $this->assertEqualsCanonicalizing([$user1->id, $user3->id], array_keys($members)); } + + /** + * Generate a set of groups with different visibility levels and users to assign to them. + * + * @return array + */ + protected function create_groups_with_visibilty(): array { + $this->resetAfterTest(true); + $generator = $this->getDataGenerator(); + + // Create courses. + $course = $generator->create_course(); + + // Create users. + $users = [ + 1 => $generator->create_user(), + 2 => $generator->create_user(), + 3 => $generator->create_user(), + 4 => $generator->create_user(), + 5 => $generator->create_user(), + ]; + + // Enrol users. + $generator->enrol_user($users[1]->id, $course->id); + $generator->enrol_user($users[2]->id, $course->id); + $generator->enrol_user($users[3]->id, $course->id); + $generator->enrol_user($users[4]->id, $course->id); + $generator->enrol_user($users[5]->id, $course->id, 'editingteacher'); + + // Create groups. + $groups = [ + 'all' => $generator->create_group(['courseid' => $course->id, 'visibility' => GROUPS_VISIBILITY_ALL]), + 'members' => $generator->create_group(['courseid' => $course->id, 'visibility' => GROUPS_VISIBILITY_MEMBERS]), + 'own' => $generator->create_group(['courseid' => $course->id, 'visibility' => GROUPS_VISIBILITY_OWN]), + 'none' => $generator->create_group(['courseid' => $course->id, 'visibility' => GROUPS_VISIBILITY_NONE]), + ]; + + return [ + $users, + $groups, + $course, + ]; + } + + /** + * Tests getting groups and group members based on visibility settings. + * + * This also covers the groupdata cache, since calls without $withmembers = true use the cache. + * + * @covers \groups_get_all_groups() + */ + public function test_get_all_groups_with_visibility() { + list($users, $groups, $course) = $this->create_groups_with_visibilty(); + + // Assign users to groups. + $generator = $this->getDataGenerator(); + $generator->create_group_member(['groupid' => $groups['all']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[2]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[3]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[4]->id]); + + $this->setUser($users[1]); + $groups1 = groups_get_all_groups($course->id); + // User1 is in groups all, members and own, and can see them. + $this->assertArrayHasKey($groups['all']->id, $groups1); + $this->assertArrayHasKey($groups['members']->id, $groups1); + $this->assertArrayHasKey($groups['own']->id, $groups1); + $this->assertArrayNotHasKey($groups['none']->id, $groups1); + // User1 can see members of groups all and members, but only themselves in group own. + $groupsmembers1 = groups_get_all_groups($course->id, 0, 0, 'g.*', true); + $this->assertArrayHasKey($users[1]->id, $groupsmembers1[$groups['all']->id]->members); + $this->assertArrayHasKey($users[1]->id, $groupsmembers1[$groups['members']->id]->members); + $this->assertArrayHasKey($users[2]->id, $groupsmembers1[$groups['members']->id]->members); + $this->assertArrayHasKey($users[1]->id, $groupsmembers1[$groups['own']->id]->members); + $this->assertArrayNotHasKey($users[3]->id, $groupsmembers1[$groups['own']->id]->members); + + $this->setUser($users[2]); + $groups2 = groups_get_all_groups($course->id); + // User2 is in group members, and can see group all as well. + $this->assertArrayHasKey($groups['all']->id, $groups2); + $this->assertArrayHasKey($groups['members']->id, $groups2); + $this->assertArrayNotHasKey($groups['own']->id, $groups2); + $this->assertArrayNotHasKey($groups['none']->id, $groups2); + // User2 can see members of groups all and members. + $groupsmembers2 = groups_get_all_groups($course->id, 0, 0, 'g.*', true); + $this->assertArrayHasKey($users[1]->id, $groupsmembers2[$groups['all']->id]->members); + $this->assertArrayHasKey($users[1]->id, $groupsmembers2[$groups['members']->id]->members); + $this->assertArrayHasKey($users[2]->id, $groupsmembers2[$groups['members']->id]->members); + + $this->setUser($users[3]); + $groups3 = groups_get_all_groups($course->id); + // User3 is in group own, and can see group all as well. + $this->assertArrayHasKey($groups['all']->id, $groups3); + $this->assertArrayNotHasKey($groups['members']->id, $groups3); + $this->assertArrayHasKey($groups['own']->id, $groups3); + $this->assertArrayNotHasKey($groups['none']->id, $groups3); + $groupsmembers3 = groups_get_all_groups($course->id, 0, 0, 'g.*', true); + // User3 can see members of group all, but only themselves in group own. + $this->assertArrayHasKey($users[1]->id, $groupsmembers3[$groups['all']->id]->members); + $this->assertArrayHasKey($users[3]->id, $groupsmembers3[$groups['own']->id]->members); + $this->assertArrayNotHasKey($users[1]->id, $groupsmembers3[$groups['own']->id]->members); + + $this->setUser($users[4]); + $groups4 = groups_get_all_groups($course->id); + // User4 can see group all and its members. They are in group none but cannot see it. + $this->assertArrayHasKey($groups['all']->id, $groups4); + $this->assertArrayNotHasKey($groups['members']->id, $groups4); + $this->assertArrayNotHasKey($groups['own']->id, $groups4); + $this->assertArrayNotHasKey($groups['none']->id, $groups4); + // User4 can see members of group all. + $groupsmembers4 = groups_get_all_groups($course->id, 0, 0, 'g.*', true); + $this->assertArrayHasKey($users[1]->id, $groupsmembers4[$groups['all']->id]->members); + + $this->setUser($users[5]); + $groups5 = groups_get_all_groups($course->id); + // User5 is has viewallgroups, so can see all groups. + $this->assertArrayHasKey($groups['all']->id, $groups5); + $this->assertArrayHasKey($groups['members']->id, $groups5); + $this->assertArrayHasKey($groups['own']->id, $groups5); + $this->assertArrayHasKey($groups['none']->id, $groups5); + // User5 is has viewallgroups, so can see all members. + $groupsmembers5 = groups_get_all_groups($course->id, 0, 0, 'g.*', true); + $this->assertArrayHasKey($users[1]->id, $groupsmembers5[$groups['all']->id]->members); + $this->assertArrayHasKey($users[1]->id, $groupsmembers5[$groups['members']->id]->members); + $this->assertArrayHasKey($users[2]->id, $groupsmembers5[$groups['members']->id]->members); + $this->assertArrayHasKey($users[1]->id, $groupsmembers5[$groups['own']->id]->members); + $this->assertArrayHasKey($users[3]->id, $groupsmembers5[$groups['own']->id]->members); + $this->assertArrayHasKey($users[4]->id, $groupsmembers5[$groups['none']->id]->members); + + } + + /** + * Tests getting groups the current user is a member of, with visibility settings applied. + * + * @covers \groups_get_my_groups() + */ + public function test_get_my_groups_with_visibility() { + list($users, $groups) = $this->create_groups_with_visibilty(); + + // Assign users to groups. + $generator = $this->getDataGenerator(); + $generator->create_group_member(['groupid' => $groups['all']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['all']->id, 'userid' => $users[5]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[5]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[5]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[5]->id]); + + $generator->role_assign('editingteacher', $users[5]->id, \context_system::instance()); + + // User can see all groups they are in, except group with visibility::NONE. + $this->setUser($users[1]); + $groups1 = groups_get_my_groups(); + $this->assertCount(3, $groups1); + $groupids1 = array_map(function($groupmember) { + return $groupmember->groupid; + }, $groups1); + sort($groupids1); + $this->assertEquals([$groups['all']->id, $groups['members']->id, $groups['own']->id], $groupids1); + + $this->setUser($users[5]); + $groups2 = groups_get_my_groups(); + $this->assertCount(4, $groups2); + $groupids2 = array_map(function($groupmember) { + return $groupmember->groupid; + }, $groups2); + sort($groupids2); + $this->assertEquals([$groups['all']->id, $groups['members']->id, $groups['own']->id, $groups['none']->id], $groupids2); + } + + /** + * Tests getting groups a user is a member of, with visibility settings applied. + * + * @covers \groups_get_user_groups() + */ + public function test_get_user_groups_with_visibility() { + list($users, $groups, $course) = $this->create_groups_with_visibilty(); + + // Assign users to groups. + $generator = $this->getDataGenerator(); + $generator->create_group_member(['groupid' => $groups['all']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[2]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[3]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[4]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[1]->id]); + + // Run as unprivileged user. + $this->setUser($users[1]); + // Own groups - should see all groups except group with visibility::NONE. + $usergroups1 = groups_get_user_groups($course->id, $users[1]->id); + $this->assertEquals([$groups['all']->id, $groups['members']->id, $groups['own']->id], $usergroups1[0]); + // Fellow member of a group with visiblity::MEMBERS. Should see that group. + $usergroups2 = groups_get_user_groups($course->id, $users[2]->id); + $this->assertEquals([$groups['members']->id], $usergroups2[0]); + // Fellow member of a group with visiblity::OWN. Should not see that group. + $usergroups3 = groups_get_user_groups($course->id, $users[3]->id); + $this->assertEmpty($usergroups3[0]); + // Fellow member of a group with visiblity::NONE. Should not see that group. + $usergroups4 = groups_get_user_groups($course->id, $users[4]->id); + $this->assertEmpty($usergroups4[0]); + + // Run as a user with viewhiddengroups. Should see all group memberships for each member. + $this->setUser($users[5]); + $usergroups1 = groups_get_user_groups($course->id, $users[1]->id); + $this->assertEquals([$groups['all']->id, $groups['members']->id, $groups['own']->id, $groups['none']->id], $usergroups1[0]); + // Fellow member of a group with visiblity::MEMBERS. Should see that group. + $usergroups2 = groups_get_user_groups($course->id, $users[2]->id); + $this->assertEquals([$groups['members']->id], $usergroups2[0]); + // Fellow member of a group with visiblity::OWN. Should not see that group. + $usergroups3 = groups_get_user_groups($course->id, $users[3]->id); + $this->assertEquals([$groups['own']->id], $usergroups3[0]); + // Fellow member of a group with visiblity::NONE. Should not see that group. + $usergroups4 = groups_get_user_groups($course->id, $users[4]->id); + $this->assertEquals([$groups['none']->id], $usergroups4[0]); + + } + + /** + * Test groups_is_member() using groups with different visibility settings. + * + * @covers \groups_is_member() + */ + public function test_groups_is_member_with_visibility(): void { + list($users, $groups) = $this->create_groups_with_visibilty(); + + // Assign users to groups. + $generator = $this->getDataGenerator(); + $generator->create_group_member(['groupid' => $groups['all']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[2]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[3]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[4]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[1]->id]); + + // Run as unprivileged user. + $this->setUser($users[1]); + $this->assertTrue(groups_is_member($groups['all']->id, $users[1]->id)); // All can see members. + $this->assertTrue(groups_is_member($groups['members']->id, $users[1]->id)); // Can see members. + $this->assertTrue(groups_is_member($groups['own']->id, $users[1]->id)); // Can see own membership. + $this->assertFalse(groups_is_member($groups['none']->id, $users[1]->id)); // Cannot see group. + + $this->assertFalse(groups_is_member($groups['all']->id, $users[2]->id)); // Not a member. + $this->assertTrue(groups_is_member($groups['members']->id, $users[2]->id)); // Can see other members. + $this->assertFalse(groups_is_member($groups['own']->id, $users[3]->id)); // Can only see own membership, not others. + $this->assertFalse(groups_is_member($groups['none']->id, $users[4]->id)); // Cannot see group. + + // Run as a user not in group 1 or 2. + $this->setUser($users[3]); + $this->assertTrue(groups_is_member($groups['all']->id, $users[1]->id)); // All can see members. + $this->assertFalse(groups_is_member($groups['members']->id, $users[2]->id)); // Cannot see members of the group. + + // Run as a user with viewhiddengroups. Should be able to see memberships that exist in any group. + $this->setUser($users[5]); + $this->assertTrue(groups_is_member($groups['all']->id, $users[1]->id)); + $this->assertTrue(groups_is_member($groups['members']->id, $users[1]->id)); + $this->assertTrue(groups_is_member($groups['own']->id, $users[1]->id)); + $this->assertTrue(groups_is_member($groups['none']->id, $users[1]->id)); + + $this->assertFalse(groups_is_member($groups['all']->id, $users[2]->id)); // Not a member. + $this->assertTrue(groups_is_member($groups['members']->id, $users[2]->id)); + $this->assertTrue(groups_is_member($groups['own']->id, $users[3]->id)); + $this->assertTrue(groups_is_member($groups['none']->id, $users[4]->id)); + } + + /** + * Test groups_get_members() using groups with different visibility settings. + * + * @covers \groups_get_members() + */ + public function test_groups_get_members_with_visibility(): void { + list($users, $groups) = $this->create_groups_with_visibilty(); + + // Assign users to groups. + $generator = $this->getDataGenerator(); + $generator->create_group_member(['groupid' => $groups['all']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[2]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[3]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[4]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[1]->id]); + + // Run as unprivileged user. + $this->setUser($users[1]); + $this->assertEquals([$users[1]->id], + array_keys(groups_get_members($groups['all']->id, 'u.id', 'u.id'))); // All can see members. + $this->assertEquals([$users[1]->id, $users[2]->id], + array_keys(groups_get_members($groups['members']->id, 'u.id', 'u.id'))); // Can see members. + $this->assertEquals([$users[1]->id], + array_keys(groups_get_members($groups['own']->id, 'u.id', 'u.id'))); // Can see own membership. + $this->assertEquals([], array_keys(groups_get_members($groups['none']->id, 'u.id', 'u.id'))); // Cannot see group. + + // Run as a user not in group 1 or 2. + $this->setUser($users[3]); + $this->assertEquals([$users[1]->id], + array_keys(groups_get_members($groups['all']->id, 'u.id', 'u.id'))); // All can see members. + $this->assertEquals([], array_keys(groups_get_members($groups['members']->id, 'u.id', 'u.id'))); // Cannot see members. + + // Run as a user with viewhiddengroups. Should be able to see memberships that exist in any group. + $this->setUser($users[5]); + $this->assertEquals([$users[1]->id], array_keys(groups_get_members($groups['all']->id, 'u.id', 'u.id'))); + $this->assertEquals([$users[1]->id, $users[2]->id], + array_keys(groups_get_members($groups['members']->id, 'u.id', 'u.id'))); + $this->assertEquals([$users[1]->id, $users[3]->id], + array_keys(groups_get_members($groups['own']->id, 'u.id', 'u.id'))); + $this->assertEquals([$users[1]->id, $users[4]->id], + array_keys(groups_get_members($groups['none']->id, 'u.id', 'u.id'))); + } + + /** + * Test groups_get_groups_members() using groups with different visibility settings. + * + * @covers \groups_get_groups_members() + */ + public function test_groups_get_groups_members_with_visibility(): void { + list($users, $groups) = $this->create_groups_with_visibilty(); + + // Assign users to groups. + $generator = $this->getDataGenerator(); + $generator->create_group_member(['groupid' => $groups['all']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['members']->id, 'userid' => $users[2]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[1]->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $users[3]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[4]->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $users[1]->id]); + + $groupids = [$groups['all']->id, $groups['members']->id, $groups['own']->id, $groups['none']->id]; + + $this->setUser($users[1]); + // Can see self in group1/3, other users in group2. + $this->assertEquals([$users[1]->id, $users[2]->id], array_keys(groups_get_groups_members($groupids, null, 'id ASC'))); + + $this->setUser($users[2]); + // Can see self in group2, user1 from group1/2. + $this->assertEquals([$users[1]->id, $users[2]->id], array_keys(groups_get_groups_members($groupids, null, 'id ASC'))); + + $this->setUser($users[3]); + // Can see self in group3, user1 from group1. + $this->assertEquals([$users[1]->id, $users[3]->id], array_keys(groups_get_groups_members($groupids, null, 'id ASC'))); + + $this->setUser($users[4]); + // Can see user1 from group1, cannot see self in group4. + $this->assertEquals([$users[1]->id], array_keys(groups_get_groups_members($groupids, null, 'id ASC'))); + + $this->setUser($users[5]); + // Can see all users from all groups. + $this->assertEquals([$users[1]->id, $users[2]->id, $users[3]->id, $users[4]->id], + array_keys(groups_get_groups_members($groupids, null, 'id ASC'))); + } + + /** + * Only groups with participation == true should be returned for an activity. + * + * @covers \groups_get_activity_allowed_groups() + * @return void + * @throws \coding_exception + */ + public function test_groups_get_activity_allowed_groups(): void { + $this->resetAfterTest(true); + $generator = $this->getDataGenerator(); + + // Create courses. + $course = $generator->create_course(); + + // Create user. + $user = $generator->create_user(); + + // Enrol user. + $generator->enrol_user($user->id, $course->id); + + // Create groups. + $groups = [ + 'all-p' => $generator->create_group(['courseid' => $course->id, 'visibility' => GROUPS_VISIBILITY_ALL]), + 'members-p' => $generator->create_group(['courseid' => $course->id, 'visibility' => GROUPS_VISIBILITY_MEMBERS]), + 'all-n' => $generator->create_group([ + 'courseid' => $course->id, + 'visibility' => GROUPS_VISIBILITY_ALL, + 'participation' => false + ]), + 'members-n' => $generator->create_group([ + 'courseid' => $course->id, + 'visibility' => GROUPS_VISIBILITY_MEMBERS, + 'participation' => false + ]), + 'own' => $generator->create_group(['courseid' => $course->id, 'visibility' => GROUPS_VISIBILITY_OWN]), + 'none' => $generator->create_group(['courseid' => $course->id, 'visibility' => GROUPS_VISIBILITY_NONE]), + ]; + // Add user to all groups. + $generator->create_group_member(['groupid' => $groups['all-p']->id, 'userid' => $user->id]); + $generator->create_group_member(['groupid' => $groups['members-p']->id, 'userid' => $user->id]); + $generator->create_group_member(['groupid' => $groups['all-n']->id, 'userid' => $user->id]); + $generator->create_group_member(['groupid' => $groups['members-n']->id, 'userid' => $user->id]); + $generator->create_group_member(['groupid' => $groups['own']->id, 'userid' => $user->id]); + $generator->create_group_member(['groupid' => $groups['none']->id, 'userid' => $user->id]); + + $module = $generator->create_module('forum', ['course' => $course->id]); + $cm = get_fast_modinfo($course)->get_cm($module->cmid); + + $activitygroups = groups_get_activity_allowed_groups($cm, $user->id); + + $this->assertContains((int)$groups['all-p']->id, array_keys($activitygroups)); + $this->assertContains((int)$groups['members-p']->id, array_keys($activitygroups)); + $this->assertNotContains((int)$groups['all-n']->id, array_keys($activitygroups)); + $this->assertNotContains((int)$groups['members-n']->id, array_keys($activitygroups)); + $this->assertNotContains((int)$groups['own']->id, array_keys($activitygroups)); + $this->assertNotContains((int)$groups['none']->id, array_keys($activitygroups)); + + } } diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 5d85b1306e474..6338b524bc6cc 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -79,6 +79,17 @@ information provided here is intended especially for developers. Course formats using components will be allowed to use one level indentation only. * The method `flexible_table::set_columnsattributes` now can be used with 'class' key to add custom classes to the DOM. * The editor_tinymce plugin has been removed from core. +* Support for serving of AMD modules built in really old versions of Moodle (<= 3.8) has been removed. + Please ensure that your AMD modules have been rebuilt with a supported Moodle version. +* Addition of new 'visibility' and 'participation' fields in the groups table, and 'moodle/group:viewhiddengroups' capability. + The following grouplib functions will now return groups and members based on group visibility and the user's permissions: + - groups_get_all_groups() + - groups_get_user_groups() + - groups_get_my_groups() + - groups_is_member() + - groups_get_members() + - groups_get_groups_members() + groups_print_activity_menu() will now only return groups where particiation == true. === 4.1 === diff --git a/message/tests/behat/group_conversation.feature b/message/tests/behat/group_conversation.feature index 7974763a8f5c7..035bdf1074c73 100644 --- a/message/tests/behat/group_conversation.feature +++ b/message/tests/behat/group_conversation.feature @@ -108,3 +108,18 @@ Feature: Create conversations for course's groups And I open messaging information And "No participants" "core_message > Message member" should not exist And "Student 4" "core_message > Message member" should exist + + Scenario: Disable messaging for private groups + Given the following "groups" exist: + | name | course | idnumber | visibility | enablemessaging | + | Messaging group | C1 | MG | 0 | 1 | + | No messaging group | C1 | NM | 2 | 1 | + And the following "group members" exist: + | user | group | + | student1 | MG | + | student1 | NM | + When I log in as "student1" + And I open messaging + And I open the "Group" conversations list + Then "Messaging group" "core_message > Message" should exist + Then "No messaging group" "core_message > Message" should not exist diff --git a/mod/assign/override_form.php b/mod/assign/override_form.php index 514db23f60385..d0990eb59fb4f 100644 --- a/mod/assign/override_form.php +++ b/mod/assign/override_form.php @@ -128,7 +128,9 @@ protected function definition() { $groupchoices = array(); foreach ($groups as $group) { - $groupchoices[$group->id] = format_string($group->name, true, $this->context); + if ($group->visibility != \core_group\visibility::NONE) { + $groupchoices[$group->id] = format_string($group->name, true, $this->context); + } } unset($groups); diff --git a/mod/assign/tests/behat/assign_group_override.feature b/mod/assign/tests/behat/assign_group_override.feature index 8bce7188733be..5ffffad3c34fa 100644 --- a/mod/assign/tests/behat/assign_group_override.feature +++ b/mod/assign/tests/behat/assign_group_override.feature @@ -237,3 +237,23 @@ Feature: Assign group override And I select "Group overrides" from the "jump" singleselect Then I should see "Group 1" in the ".generaltable" "css_element" And I should not see "Group 2" in the ".generaltable" "css_element" + + Scenario: "Not visible" groups should not be available for group overrides + Given the following "groups" exist: + | name | course | idnumber | visibility | participation | + | Visible to all/Participation | C1 | VP | 0 | 1 | + | Visible to members/Participation | C1 | MP | 1 | 1 | + | See own membership | C1 | O | 2 | 0 | + | Not visible | C1 | N | 3 | 0 | + | Visible to all/Non-Participation | C1 | VN | 0 | 0 | + | Visible to members/Non-Participation | C1 | MN | 1 | 0 | + When I am on the "Test assignment name" Activity page logged in as teacher1 + And I navigate to "Overrides" in current page administration + And I select "Group overrides" from the "jump" singleselect + And I press "Add group override" + Then I should see "Visible to all/Participation" in the "Override group" "select" + And I should see "Visible to all/Non-Participation" in the "Override group" "select" + And I should see "Visible to members" in the "Override group" "select" + And I should see "Visible to members/Non-Participation" in the "Override group" "select" + And I should see "See own membership" in the "Override group" "select" + And I should not see "Not visible" in the "Override group" "select" diff --git a/mod/lesson/override_form.php b/mod/lesson/override_form.php index 4fd54a012f03b..0674a18cd8d16 100644 --- a/mod/lesson/override_form.php +++ b/mod/lesson/override_form.php @@ -113,7 +113,9 @@ protected function definition() { $groupchoices = array(); foreach ($groups as $group) { - $groupchoices[$group->id] = format_string($group->name, true, $this->context); + if ($group->visibility != \core_group\visibility::NONE) { + $groupchoices[$group->id] = format_string($group->name, true, $this->context); + } } unset($groups); diff --git a/mod/lesson/tests/behat/lesson_group_override.feature b/mod/lesson/tests/behat/lesson_group_override.feature index 8b574db9ae40a..338601619b15a 100644 --- a/mod/lesson/tests/behat/lesson_group_override.feature +++ b/mod/lesson/tests/behat/lesson_group_override.feature @@ -369,3 +369,23 @@ Feature: Lesson group override And I select "Group overrides" from the "jump" singleselect Then I should see "Group 1" in the ".generaltable" "css_element" And I should not see "Group 2" in the ".generaltable" "css_element" + + Scenario: "Not visible" groups should not be available for group overrides + Given the following "groups" exist: + | name | course | idnumber | visibility | participation | + | Visible to all/Participation | C1 | VP | 0 | 1 | + | Visible to members/Participation | C1 | MP | 1 | 1 | + | See own membership | C1 | O | 2 | 0 | + | Not visible | C1 | N | 3 | 0 | + | Visible to all/Non-Participation | C1 | VN | 0 | 0 | + | Visible to members/Non-Participation | C1 | MN | 1 | 0 | + When I am on the "lesson1" Activity page logged in as teacher1 + And I navigate to "Overrides" in current page administration + And I select "Group overrides" from the "jump" singleselect + And I follow "Add group override" + Then I should see "Visible to all/Participation" in the "Override group" "select" + And I should see "Visible to all/Non-Participation" in the "Override group" "select" + And I should see "Visible to members" in the "Override group" "select" + And I should see "Visible to members/Non-Participation" in the "Override group" "select" + And I should see "See own membership" in the "Override group" "select" + And I should not see "Not visible" in the "Override group" "select" \ No newline at end of file diff --git a/mod/quiz/classes/form/edit_override_form.php b/mod/quiz/classes/form/edit_override_form.php index 2a21c5be65b3b..712b64ff2ade5 100644 --- a/mod/quiz/classes/form/edit_override_form.php +++ b/mod/quiz/classes/form/edit_override_form.php @@ -113,7 +113,9 @@ protected function definition() { $groupchoices = []; foreach ($groups as $group) { - $groupchoices[$group->id] = format_string($group->name, true, $this->context); + if ($group->visibility != \core_group\visibility::NONE) { + $groupchoices[$group->id] = format_string($group->name, true, $this->context); + } } unset($groups); diff --git a/mod/quiz/tests/behat/quiz_group_override.feature b/mod/quiz/tests/behat/quiz_group_override.feature index 3b65be0fa027e..f657b1951af83 100644 --- a/mod/quiz/tests/behat/quiz_group_override.feature +++ b/mod/quiz/tests/behat/quiz_group_override.feature @@ -117,3 +117,23 @@ Feature: Quiz group override And "Edit" "link" should not exist in the "Group 1" "table_row" And "Copy" "link" should not exist in the "Group 1" "table_row" And "Delete" "link" should not exist in the "Group 1" "table_row" + + Scenario: "Not visible" groups should not be available for group overrides + Given the following "groups" exist: + | name | course | idnumber | visibility | participation | + | Visible to all/Participation | C1 | VP | 0 | 1 | + | Visible to members/Participation | C1 | MP | 1 | 1 | + | See own membership | C1 | O | 2 | 0 | + | Not visible | C1 | N | 3 | 0 | + | Visible to all/Non-Participation | C1 | VN | 0 | 0 | + | Visible to members/Non-Participation | C1 | MN | 1 | 0 | + When I am on the "quiz1" Activity page logged in as teacher1 + And I navigate to "Overrides" in current page administration + And I select "Group overrides" from the "jump" singleselect + And I press "Add group override" + Then I should see "Visible to all/Participation" in the "Override group" "select" + And I should see "Visible to all/Non-Participation" in the "Override group" "select" + And I should see "Visible to members" in the "Override group" "select" + And I should see "Visible to members/Non-Participation" in the "Override group" "select" + And I should see "See own membership" in the "Override group" "select" + And I should not see "Not visible" in the "Override group" "select" \ No newline at end of file diff --git a/version.php b/version.php index f61c6a403a0af..467637cedb462 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2023031000.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2023031000.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '4.2dev (Build: 20230310)'; // Human-friendly version name