From 63b2ceee5f48cde5ddf45e16a7566a2d7f148e6e Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Wed, 6 Dec 2023 13:50:57 +0700 Subject: [PATCH] MDL-41465 reports: In separate group mode, limit to same --- enrol/tests/enrollib_test.php | 37 ++++++ lib/enrollib.php | 14 ++- lib/upgrade.txt | 3 + report/log/classes/renderable.php | 57 ++++++++-- report/log/tests/renderable_test.php | 158 ++++++++++++++++++++++++++ report/loglive/classes/renderable.php | 11 ++ report/loglive/classes/table_log.php | 17 ++- 7 files changed, 283 insertions(+), 14 deletions(-) create mode 100644 report/log/tests/renderable_test.php diff --git a/enrol/tests/enrollib_test.php b/enrol/tests/enrollib_test.php index f033f57e208..92c1255bf38 100644 --- a/enrol/tests/enrollib_test.php +++ b/enrol/tests/enrollib_test.php @@ -1122,6 +1122,43 @@ class enrollib_test extends advanced_testcase { $this->assertCount(1, enrol_get_course_users($course1->id, true)); } + /** + * test_course_users in groups + * + * @covers \enrol_get_course_users() + * @return void + */ + public function test_course_users_in_groups() { + $this->resetAfterTest(); + + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + $user3 = $this->getDataGenerator()->create_user(); + $course = $this->getDataGenerator()->create_course(); + $group1 = $this->getDataGenerator()->create_group(['courseid' => $course->id]); + $group2 = $this->getDataGenerator()->create_group(['courseid' => $course->id]); + + $this->getDataGenerator()->enrol_user($user1->id, $course->id); + $this->getDataGenerator()->enrol_user($user2->id, $course->id); + $this->getDataGenerator()->enrol_user($user3->id, $course->id); + + $this->getDataGenerator()->create_group_member(['groupid' => $group1->id, 'userid' => $user1->id]); + $this->getDataGenerator()->create_group_member(['groupid' => $group2->id, 'userid' => $user1->id]); + $this->getDataGenerator()->create_group_member(['groupid' => $group2->id, 'userid' => $user2->id]); + + $this->assertCount(3, enrol_get_course_users($course->id)); + $this->assertCount(1, enrol_get_course_users($course->id, false, [], [], [$group1->id])); + $this->assertCount(2, enrol_get_course_users($course->id, false, [], [], [$group2->id])); + + $instances = enrol_get_instances($course->id, true); + $manualinstance = reset($instances); + + $manualplugin = enrol_get_plugin('manual'); + $manualplugin->update_user_enrol($manualinstance, $user1->id, ENROL_USER_SUSPENDED); + $this->assertCount(2, enrol_get_course_users($course->id, false, [], [], [$group2->id])); + $this->assertCount(1, enrol_get_course_users($course->id, true, [], [], [$group2->id])); + } + /** * Test count of enrolled users * diff --git a/lib/enrollib.php b/lib/enrollib.php index 30effb35a1e..b6d6f2873a1 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -1769,9 +1769,11 @@ function enrol_get_course_by_user_enrolment_id($ueid) { * @param bool $onlyactive consider only active enrolments in enabled plugins and time restrictions * @param array $usersfilter Limit the results obtained to this list of user ids. $uefilter compatibility not guaranteed. * @param array $uefilter Limit the results obtained to this list of user enrolment ids. $usersfilter compatibility not guaranteed. + * @param array $usergroups Limit the results of users to the ones that belong to one of the submitted group ids. * @return stdClass[] */ -function enrol_get_course_users($courseid = false, $onlyactive = false, $usersfilter = array(), $uefilter = array()) { +function enrol_get_course_users($courseid = false, $onlyactive = false, $usersfilter = [], $uefilter = [], + $usergroups = []) { global $DB; if (!$courseid && !$usersfilter && !$uefilter) { @@ -1814,6 +1816,16 @@ function enrol_get_course_users($courseid = false, $onlyactive = false, $usersfi $params = $params + $ueparams; } + // Only select enrolled users that belong to a specific group(s). + if (!empty($usergroups)) { + $usergroups = array_map(function ($item) { // Sanitize groupid to int to be save for sql. + return (int)$item; + }, $usergroups); + list($ugsql, $ugparams) = $DB->get_in_or_equal($usergroups, SQL_PARAMS_NAMED); + $conditions[] = 'ue.userid IN (SELECT userid FROM {groups_members} WHERE groupid ' . $ugsql . ')'; + $params = $params + $ugparams; + } + return $DB->get_records_sql($sql . ' ' . implode(' AND ', $conditions), $params); } diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 278c98b1dbb..e781d048d98 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -9,6 +9,9 @@ information provided here is intended especially for developers. * course_modinfo now has a purge_course_modules_cache() method, which takes a list of cmids and purges them all in a single cache set. * Behat generators can now implement the function finish_generate_ to detect when the whole list of elements have been generated. +* In enrollib.php, the method enrol_get_course_users() got an optional 5th parameter $usergroups that + defaults to an empty array. Here, user group ids can be provided, to select enrolled users in a course + that are also members of these groups. === 4.1.6 === * \moodle_page::set_title() has been updated to append the site name depending on the value of $CFG->sitenameintitle and whether diff --git a/report/log/classes/renderable.php b/report/log/classes/renderable.php index 76433fa6515..8bf42f521ac 100644 --- a/report/log/classes/renderable.php +++ b/report/log/classes/renderable.php @@ -93,6 +93,9 @@ class report_log_renderable implements renderable { /** @var table_log table log which will be used for rendering logs */ public $tablelog; + /** @var array group ids */ + public $grouplist; + /** * Constructor. * @@ -343,30 +346,40 @@ class report_log_renderable implements renderable { } /** - * Return list of groups. + * Return list of groups that are used in this course. This is done when groups are used in the course + * and the user is allowed to see all groups or groups are visible anyway. If groups are used but the + * mode is separate groups and the user is not allowed to see all groups, the list contains the groups + * only, where the user is member. + * If the course uses no groups, the list is empty. * * @return array list of groups. */ public function get_group_list() { + global $USER; + if ($this->grouplist !== null) { + return $this->grouplist; + } // No groups for system. if (empty($this->course)) { - return array(); + $this->grouplist = []; + return $this->grouplist; } $context = context_course::instance($this->course->id); - $groups = array(); + $this->grouplist = []; $groupmode = groups_get_course_groupmode($this->course); + $cgroups = []; if (($groupmode == VISIBLEGROUPS) || - ($groupmode == SEPARATEGROUPS and has_capability('moodle/site:accessallgroups', $context))) { - // Get all groups. - if ($cgroups = groups_get_all_groups($this->course->id)) { - foreach ($cgroups as $cgroup) { - $groups[$cgroup->id] = $cgroup->name; - } - } + ($groupmode == SEPARATEGROUPS && has_capability('moodle/site:accessallgroups', $context))) { + $cgroups = groups_get_all_groups($this->course->id); + } else if ($groupmode == SEPARATEGROUPS && !has_capability('moodle/site:accessallgroups', $context)) { + $cgroups = groups_get_all_groups($this->course->id, $USER->id); } - return $groups; + foreach ($cgroups as $cgroup) { + $this->grouplist[$cgroup->id] = $cgroup->name; + } + return $this->grouplist; } /** @@ -385,9 +398,29 @@ class report_log_renderable implements renderable { $limitfrom = empty($this->showusers) ? 0 : ''; $limitnum = empty($this->showusers) ? COURSE_MAX_USERS_PER_DROPDOWN + 1 : ''; $userfieldsapi = \core_user\fields::for_name(); - $courseusers = get_enrolled_users($context, '', $this->groupid, 'u.id, ' . + + // Get the groups of that course. + $groups = $this->get_group_list(); + // Check here if we are not in group mode, or in group mode but narrow the group selection + // to the group of the user. + if (empty($groups) || !empty($this->groupid) && isset($groups[(int)$this->groupid])) { + // No groups are used in that course, therefore get all users (maybe limited to one group). + $courseusers = get_enrolled_users($context, '', $this->groupid, 'u.id, ' . $userfieldsapi->get_sql('u', false, '', '', false)->selects, null, $limitfrom, $limitnum); + } else { + // The course uses groups, get the users from these groups. + $groupids = array_keys($groups); + try { + $enrolments = enrol_get_course_users($courseid, false, [], [], $groupids); + $courseusers = []; + foreach ($enrolments as $enrolment) { + $courseusers[$enrolment->id] = $enrolment; + } + } catch (Exception $e) { + $courseusers = []; + } + } if (count($courseusers) < COURSE_MAX_USERS_PER_DROPDOWN && !$this->showusers) { $this->showusers = 1; diff --git a/report/log/tests/renderable_test.php b/report/log/tests/renderable_test.php new file mode 100644 index 00000000000..97e09a58ba3 --- /dev/null +++ b/report/log/tests/renderable_test.php @@ -0,0 +1,158 @@ +. + +namespace report_log; + +/** + * Class report_log\renderable_test to cover functions in \report_log_renderable. + * + * @package report_log + * @copyright 2023 Stephan Robotta + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later. + */ +class renderable_test extends \advanced_testcase { + + /** + * @var [stdClass] The students. + */ + private $student = []; + + /** + * @var [stdClass] The teachers. + */ + private $teacher = []; + + /** + * @var [stdClass] The groups. + */ + private $group = []; + + /** + * @var stdClass The course. + */ + private $course; + + /** + * Set up a course with two groups, three students being each in one of the groups, + * two teachers each in either group while the second teacher is also member of the other group. + * @return void + * @throws \coding_exception + */ + public function setUp(): void { + global $PAGE; + $this->course = $this->getDataGenerator()->create_course(['groupmode' => 1]); + $this->group[] = $this->getDataGenerator()->create_group(['courseid' => $this->course->id]); + $this->group[] = $this->getDataGenerator()->create_group(['courseid' => $this->course->id]); + + for ($i = 0; $i < 3; $i++) { + $this->student[$i] = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($this->student[$i]->id, $this->course->id, 'student'); + $this->getDataGenerator()->create_group_member([ + 'groupid' => $this->group[$i % 2]->id, + 'userid' => $this->student[$i]->id, + ]); + } + for ($i = 0; $i < 2; $i++) { + $this->teacher[$i] = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($this->teacher[$i]->id, $this->course->id, 'editingteacher'); + $this->getDataGenerator()->create_group_member([ + 'groupid' => $this->group[$i]->id, + 'userid' => $this->teacher[$i]->id, + ]); + } + // Make teacher2 also member of group1. + $this->getDataGenerator()->create_group_member([ + 'groupid' => $this->group[0]->id, + 'userid' => $this->teacher[1]->id, + ]); + + $PAGE->set_url('/report/log/index.php?id=' . $this->course->id); + $this->resetAfterTest(); + } + + /** + * Test report_log_renderable::get_user_list(). + * @covers \report_log_renderable::get_user_list + * @return void + */ + public function test_get_user_list() { + // Fetch all users of group 1 and the guest user. + $userids = $this->fetch_users_from_renderable((int)$this->student[0]->id); + $this->assertCount(5, $userids); + $this->assertContains((int)$this->student[0]->id, $userids); // His own group (group 1). + $this->assertNotContains((int)$this->student[1]->id, $userids); // He is in group 2. + $this->assertContains((int)$this->teacher[0]->id, $userids); // He is in group 1. + $this->assertContains((int)$this->teacher[1]->id, $userids); // He is in both groups. + + // Fetch users of all groups and the guest user. The teacher has the capability moodle/site:accessallgroups. + $this->setUser($this->teacher[1]->id); + $renderable = new \report_log_renderable("", (int)$this->course->id, $this->teacher[1]->id); + $users = $renderable->get_user_list(); + $this->assertCount(6, $users); + + // Fetch users of group 2 and the guest user. + $userids = $this->fetch_users_from_renderable((int)$this->student[1]->id); + $this->assertCount( 3, $userids); + $this->assertNotContains((int)$this->student[0]->id, $userids); + $this->assertContains((int)$this->student[1]->id, $userids); + $this->assertNotContains((int)$this->teacher[0]->id, $userids); + $this->assertContains((int)$this->teacher[1]->id, $userids); + + // Fetch users of group 2 and test user as teacher2 but limited to his group. + $userids = $this->fetch_users_from_renderable((int)$this->teacher[1]->id, (int)$this->group[1]->id); + $this->assertCount( 3, $userids); + $this->assertNotContains((int)$this->student[0]->id, $userids); + $this->assertContains((int)$this->student[1]->id, $userids); + $this->assertNotContains((int)$this->teacher[0]->id, $userids); + $this->assertContains((int)$this->teacher[1]->id, $userids); + + } + + /** + * Helper function to return a list of user ids from the renderable object. + * @param int $userid + * @param ?int $groupid + * @return array + */ + protected function fetch_users_from_renderable(int $userid, ?int $groupid = 0): array { + $this->setUser($userid); + $renderable = new \report_log_renderable( + "", (int)$this->course->id, $userid, 0, '', $groupid); + $users = $renderable->get_user_list(); + return \array_keys($users); + } + + /** + * Test report_log_renderable::get_group_list(). + * @covers \report_log_renderable::get_group_list + * @return void + */ + public function test_get_group_list() { + + // The student sees his own group only. + $this->setUser($this->student[0]->id); + $renderable = new \report_log_renderable("", (int)$this->course->id, $this->student[0]->id); + $groups = $renderable->get_group_list(); + $this->assertCount(1, $groups); + + // While the teacher is allowed to see all groups. + $this->setUser($this->teacher[0]->id); + $renderable = new \report_log_renderable("", (int)$this->course->id, $this->teacher[0]->id); + $groups = $renderable->get_group_list(); + $this->assertCount(2, $groups); + + } +} diff --git a/report/loglive/classes/renderable.php b/report/loglive/classes/renderable.php index ff8d4107e06..09031fd4ee1 100644 --- a/report/loglive/classes/renderable.php +++ b/report/loglive/classes/renderable.php @@ -168,6 +168,8 @@ class report_loglive_renderable implements renderable { * @return stdClass filters */ protected function setup_filters() { + global $USER; + $readers = $this->get_readers(); // Set up filters. @@ -178,6 +180,15 @@ class report_loglive_renderable implements renderable { if (!has_capability('moodle/site:viewanonymousevents', $context)) { $filter->anonymous = 0; } + if (groups_get_course_groupmode($this->course) == SEPARATEGROUPS && + !has_capability('moodle/site:accessallgroups', $context)) { + if ($cgroups = groups_get_all_groups($this->course->id, $USER->id)) { + $filter->groups = []; + foreach ($cgroups as $cgroup) { + $filter->groups[] = (int)$cgroup->id; + } + } + } } else { $filter->courseid = 0; } diff --git a/report/loglive/classes/table_log.php b/report/loglive/classes/table_log.php index 68a5bec38b3..1fbb5b0a8c3 100644 --- a/report/loglive/classes/table_log.php +++ b/report/loglive/classes/table_log.php @@ -56,6 +56,7 @@ class report_loglive_table_log extends table_sql { * - int userid: user id * - int|string modid: Module id or "site_errors" to view site errors * - int groupid: Group id + * - array groups: List of group ids * - \core\log\sql_reader logreader: reader from which data will be fetched. * - int edulevel: educational level. * - string action: view action @@ -304,14 +305,29 @@ class report_loglive_table_log extends table_sql { * @param bool $useinitialsbar do you want to use the initials bar. */ public function query_db($pagesize, $useinitialsbar = true) { + global $DB; $joins = array(); $params = array(); // Set up filtering. if (!empty($this->filterparams->courseid)) { + // For a normal course, set the course filter. $joins[] = "courseid = :courseid"; $params['courseid'] = $this->filterparams->courseid; + // If we have a course, then check if the groups filter is set. + if ($this->filterparams->courseid != SITEID && !empty($this->filterparams->groups)) { + // If that's the case, limit the users to be in the groups only, defined by the filter. + $useringroups = []; + foreach ($this->filterparams->groups as $groupid) { + $gusers = groups_get_members($groupid, 'u.id'); + $useringroups = array_merge($useringroups, array_keys($gusers)); + } + $useringroups = array_unique($useringroups); + list($ugsql, $ugparams) = $DB->get_in_or_equal($useringroups, SQL_PARAMS_NAMED); + $joins[] = 'userid ' . $ugsql; + $params = array_merge($params, $ugparams); + } } if (!empty($this->filterparams->date)) { @@ -323,7 +339,6 @@ class report_loglive_table_log extends table_sql { $joins[] = "anonymous = :anon"; $params['anon'] = $this->filterparams->anonymous; } - $selector = implode(' AND ', $joins); $total = $this->filterparams->logreader->get_events_select_count($selector, $params);