From 6ba656e2f58d7bebbbea6947218b7159e10ef10d Mon Sep 17 00:00:00 2001 From: sam marshall Date: Tue, 29 Oct 2024 10:51:21 +0000 Subject: [PATCH] MDL-83584 report_log: Group filter can get list of all users in system --- lib/classes/report_helper.php | 15 ++-- lib/tests/report_helper_test.php | 136 +++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 8 deletions(-) diff --git a/lib/classes/report_helper.php b/lib/classes/report_helper.php index 0b9c187f22f..0fe81ee2667 100644 --- a/lib/classes/report_helper.php +++ b/lib/classes/report_helper.php @@ -133,18 +133,17 @@ class report_helper { $course = get_course($courseid); $groupmode = groups_get_course_groupmode($course); $groupid = $filterparams->groupid ?? 0; - if ($groupmode == SEPARATEGROUPS || $groupid) { - $context = context_course::instance($courseid); + $context = context_course::instance($courseid); + if ($groupid || ($groupmode == SEPARATEGROUPS && !has_capability('moodle/site:accessallgroups', $context))) { if ($groupid) { $cgroups = [(int) $groupid]; } else { - $cgroups = groups_get_all_groups( - $courseid, - has_capability('moodle/site:accessallgroups', $context) ? 0 : $USER->id - ); + $cgroups = groups_get_all_groups($courseid, $USER->id); $cgroups = array_keys($cgroups); - // If that's the case, limit the users to be in the groups only, defined by the filter. - if (has_capability('moodle/site:accessallgroups', $context) || empty($cgroups)) { + // If you are not in any groups you can still view users without group. This may + // perform poorly because it will list all users in the entire system who do not + // belong to a group on this course. + if (empty($cgroups)) { $cgroups[] = USERSWITHOUTGROUP; } } diff --git a/lib/tests/report_helper_test.php b/lib/tests/report_helper_test.php index f6cab8c7655..aaf00c7c0af 100644 --- a/lib/tests/report_helper_test.php +++ b/lib/tests/report_helper_test.php @@ -30,8 +30,13 @@ use core\report_helper; /** * Tests the functions for report_helper class. + * + * @covers \core\report_helper */ class report_helper_test extends \advanced_testcase { + /** @var int[] Array of created user ids */ + protected array $userids; + /** * Data provider for testing selected report for same and different courses * @@ -75,4 +80,135 @@ class report_helper_test extends \advanced_testcase { $this->assertEquals($USER->course_last_report[$courseid2], $url2); } + + /** + * Tests {@see report_helper::get_group_filter()}. + */ + public function test_get_group_filter(): void { + $this->resetAfterTest(); + + // Create some test course, groups, and users. + $generator = self::getDataGenerator(); + + $vgcourse = $generator->create_course(['groupmode' => VISIBLEGROUPS]); + $sgcourse = $generator->create_course(['groupmode' => SEPARATEGROUPS]); + + $vg1 = $generator->create_group(['courseid' => $vgcourse->id]); + $vg2 = $generator->create_group(['courseid' => $vgcourse->id]); + $sg1 = $generator->create_group(['courseid' => $sgcourse->id]); + $sg2 = $generator->create_group(['courseid' => $sgcourse->id]); + + $this->userids = []; + for ($i = 0; $i < 10; $i++) { + $this->userids[$i] = $generator->create_user()->id; + $generator->enrol_user($this->userids[$i], ($i < 5) ? $vgcourse->id : $sgcourse->id, 'student'); + } + + groups_add_member($vg1, $this->userids[0]); + groups_add_member($vg1, $this->userids[1]); + groups_add_member($vg2, $this->userids[0]); + groups_add_member($vg2, $this->userids[2]); + groups_add_member($vg2, $this->userids[3]); + + groups_add_member($sg1, $this->userids[5]); + groups_add_member($sg1, $this->userids[6]); + groups_add_member($sg2, $this->userids[5]); + groups_add_member($sg2, $this->userids[7]); + groups_add_member($sg2, $this->userids[8]); + + // Teacher user has access all groups. + $teacher = $generator->create_user(); + $generator->enrol_user($teacher->id, $vgcourse->id, 'editingteacher'); + $generator->enrol_user($teacher->id, $sgcourse->id, 'editingteacher'); + + // With specified groups on either course (does not matter who user is). + $this->assert_group_filter([0, 1], ['courseid' => $vgcourse->id, 'groupid' => $vg1->id]); + $this->assert_group_filter([0, 2, 3], ['courseid' => $vgcourse->id, 'groupid' => $vg2->id]); + $this->assert_group_filter([5, 6], ['courseid' => $sgcourse->id, 'groupid' => $sg1->id]); + $this->assert_group_filter([5, 7, 8], ['courseid' => $sgcourse->id, 'groupid' => $sg2->id]); + + // With specified group and user. + $this->assert_group_filter([2], [ + 'courseid' => $vgcourse->id, + 'groupid' => $vg2->id, + 'userid' => $this->userids[2], + ]); + $this->assert_group_filter([6], [ + 'courseid' => $sgcourse->id, + 'groupid' => $sg2->id, + 'userid' => $this->userids[6], + ]); + + // No restrictions, user belongs to a group or to both groups on VG course. + $this->setUser($this->userids[1]); + $all = array_keys($this->userids); + $this->assert_group_filter($all, ['courseid' => $vgcourse->id]); + $this->setUser($this->userids[0]); + $this->assert_group_filter($all, ['courseid' => $vgcourse->id]); + + // No restrictions, user belongs to a group or to both groups on SG course. + $this->setUser($this->userids[6]); + $this->assert_group_filter([5, 6], ['courseid' => $sgcourse->id]); + $this->setUser($this->userids[5]); + $this->assert_group_filter([5, 6, 7, 8], ['courseid' => $sgcourse->id]); + + // No restrictions, user has access all groups on either course. + $this->setUser($teacher); + $this->assert_group_filter($all, ['courseid' => $vgcourse->id]); + $this->assert_group_filter($all, ['courseid' => $sgcourse->id]); + + // There was a performance issue for users with access all groups where it listed all users + // in the system in the 'filter' list, now it doesn't. + $this->assertNull(report_helper::get_group_filter( + (object)['courseid' => $sgcourse->id], + )['useridfilter']); + + // Specified group even if you have AAG. + $this->assert_group_filter([0, 1], ['courseid' => $vgcourse->id, 'groupid' => $vg1->id]); + $this->assert_group_filter([5, 6], ['courseid' => $sgcourse->id, 'groupid' => $sg1->id]); + + // No restrictions, user does not belong to a group on course. Makes no difference in VG. + $this->setUser($this->userids[5]); + $this->assert_group_filter($all, ['courseid' => $vgcourse->id]); + + // In SG user can now view all users across system who are not in a group on course. + // Strange but true. + $this->setUser($this->userids[0]); + $this->assert_group_filter([0, 1, 2, 3, 4, 9], ['courseid' => $sgcourse->id]); + } + + /** + * Calls {@see report_helper::get_group_filter()} and checks which of the users created by this + * unit test are returned. + * + * @param int[] $expecteduserindexes Expected user indexes + * @param array $filterparams Array of filter parameters to pass to get_group_filter + */ + protected function assert_group_filter(array $expecteduserindexes, array $filterparams): void { + global $DB; + + $result = report_helper::get_group_filter((object)$filterparams); + + // Combine the joins (if any). 'TRUE' is not allowed in SQL Server, you must use '1 = 1'. + $where = '1 = 1'; + foreach ($result['joins'] as $join) { + $where .= ' AND ' . $join; + } + + // The joins use field 'userid' so we make a subselect table with that field name. + $userids = $DB->get_fieldset_sql(" + SELECT userid + FROM (SELECT id AS userid FROM {user}) userdata + WHERE $where", $result['params']); + + if ($result['useridfilter'] !== null) { + $userids = array_filter($userids, fn($userid) => array_key_exists($userid, $result['useridfilter'])); + } + + // Convert user ids to expected indexes, exclude any results not in our test user list, and sort. + $indexes = array_map(fn($userid) => array_search($userid, $this->userids), $userids); + $indexes = array_filter($indexes, fn($index) => $index !== false); + sort($indexes); + $this->assertEquals($expecteduserindexes, $indexes); + } }