diff --git a/lib/classes/report_helper.php b/lib/classes/report_helper.php index e8cb401459c..05a0b26a9a5 100644 --- a/lib/classes/report_helper.php +++ b/lib/classes/report_helper.php @@ -24,7 +24,9 @@ */ namespace core; +use context_course; use moodle_url; +use stdClass; /** * A helper class with static methods to help report plugins @@ -110,4 +112,71 @@ class report_helper { } $USER->course_last_report[$id] = $url; } + + /** + * Retrieve the right SQL / params for the group filter depending on the filterparams, course and group settings. + * + * Addionnaly, it will return the list of users visible by the current user so + * it can be used to filter out records that are not visible. This is mainly + * because we cannot use joins as the log tables can be in two different databases. + * + * @param stdClass $filterparams + * @return array + */ + public static function get_group_filter(stdClass $filterparams): array { + global $DB, $USER; + $useridfilter = null; + // First and just in case we are in separate group, just set the $useridfilter to the list + // of users visible by this user. + $courseid = $filterparams->courseid ?? SITEID; + $courseid = $courseid ?: SITEID; // Make sure that if courseid is set to 0 we use SITEID. + $course = get_course($courseid); + $groupmode = groups_get_course_groupmode($course); + $groupid = $filterparams->groupid ?? 0; + if ($groupmode == SEPARATEGROUPS || $groupid) { + $context = context_course::instance($courseid); + if ($groupid) { + $cgroups = [(int) $groupid]; + } else { + $cgroups = groups_get_all_groups( + $courseid, + has_capability('moodle/site:accessallgroups', $context) ? 0 : $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)) { + $cgroups[] = USERSWITHOUTGROUP; + } + } + // If that's the case, limit the users to be in the groups only, defined by the filter. + [$groupmembersql, $groupmemberparams] = groups_get_members_ids_sql($cgroups, $context); + $groupusers = $DB->get_fieldset_sql($groupmembersql, $groupmemberparams); + $useridfilter = array_fill_keys($groupusers, true); + } + $joins = []; + $params = []; + if (empty($filterparams->userid)) { + if ($groupid) { + if ($thisgroupusers = groups_get_members($groupid)) { + [$sql, $sqlfilterparams] = $DB->get_in_or_equal( + array_keys($thisgroupusers), + SQL_PARAMS_NAMED, + ); + $joins[] = "userid {$sql}"; + $params = $sqlfilterparams; + } else { + $joins[] = 'userid = 0'; // No users in groups, so we want something that will always be false. + } + } + } else { + $joins[] = "userid = :userid"; + $params['userid'] = $filterparams->userid; + } + + return [ + 'joins' => $joins, + 'params' => $params, + 'useridfilter' => $useridfilter, + ]; + } } diff --git a/report/log/classes/table_log.php b/report/log/classes/table_log.php index 4e9baf587b5..bbd45b4cab4 100644 --- a/report/log/classes/table_log.php +++ b/report/log/classes/table_log.php @@ -22,6 +22,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core\report_helper; + defined('MOODLE_INTERNAL') || die; global $CFG; require_once($CFG->libdir . '/tablelib.php'); @@ -43,6 +45,9 @@ class report_log_table_log extends table_sql { /** @var stdClass filters parameters */ private $filterparams; + /** @var int[] A list of users to filter by */ + private ?array $lateuseridfilter = null; + /** * Sets up the table_log parameters. * @@ -414,12 +419,7 @@ class report_log_table_log extends table_sql { // If we filter by userid and module id we also need to filter by crud and edulevel to ensure DB index is engaged. $useextendeddbindex = !empty($this->filterparams->userid) && !empty($this->filterparams->modid); - $groupid = 0; if (!empty($this->filterparams->courseid) && $this->filterparams->courseid != SITEID) { - if (!empty($this->filterparams->groupid)) { - $groupid = $this->filterparams->groupid; - } - $joins[] = "courseid = :courseid"; $params['courseid'] = $this->filterparams->courseid; } @@ -441,38 +441,14 @@ class report_log_table_log extends table_sql { } // Getting all members of a group. - if (empty($this->filterparams->userid)) { - if ($groupid) { - if ($gusers = groups_get_members($groupid)) { - $gusers = array_keys($gusers); - $joins[] = 'userid IN (' . implode(',', $gusers) . ')'; - } else { - $joins[] = 'userid = 0'; // No users in groups, so we want something that will always be false. - } - } else { - // No group selected and we are not filtering by user, so we want all users that are visible to the current user. - // If we are in a course, then let's check what logs we can see. - $course = get_course($this->filterparams->courseid); - $groupmode = groups_get_course_groupmode($course); - $context = context_course::instance($this->filterparams->courseid); - $userid = 0; - if ($groupmode == SEPARATEGROUPS && !has_capability('moodle/site:accessallgroups', $context)) { - $userid = $USER->id; - } - $cgroups = groups_get_all_groups($this->filterparams->courseid, $userid); - $cgroups = array_keys($cgroups); - if ($groupmode != SEPARATEGROUPS || has_capability('moodle/site:accessallgroups', $context)) { - $cgroups[] = USERSWITHOUTGROUP; - } - // If that's the case, limit the users to be in the groups only, defined by the filter. - [$groupmembersql, $groupmemberparams] = groups_get_members_ids_sql($cgroups, $context); - $joins[] = "userid IN ($groupmembersql)"; - $params = array_merge($params, $groupmemberparams); - } - } else { - $joins[] = "userid = :userid"; - $params['userid'] = $this->filterparams->userid; - } + [ + 'joins' => $groupjoins, + 'params' => $groupparams, + 'useridfilter' => $this->lateuseridfilter, + ] = report_helper::get_group_filter($this->filterparams); + + $joins = array_merge($joins, $groupjoins); + $params = array_merge($params, $groupparams); if (!empty($this->filterparams->date)) { $joins[] = "timecreated > :date AND timecreated < :enddate"; @@ -524,24 +500,29 @@ class report_log_table_log extends table_sql { $this->pageable(false); } - // Get the users and course data. - $this->rawdata = $this->filterparams->logreader->get_events_select_iterator($selector, $params, - $this->filterparams->orderby, $this->get_page_start(), $this->get_page_size()); - - // Update list of users which will be displayed on log page. - $this->update_users_used(); - // Get the events. Same query than before; even if it is not likely, logs from new users // may be added since last query so we will need to work around later to prevent problems. // In almost most of the cases this will be better than having two opened recordsets. - $this->rawdata = $this->filterparams->logreader->get_events_select_iterator($selector, $params, - $this->filterparams->orderby, $this->get_page_start(), $this->get_page_size()); + $this->rawdata = new \CallbackFilterIterator( + $this->filterparams->logreader->get_events_select_iterator( + $selector, + $params, + $this->filterparams->orderby, + $this->get_page_start(), + $this->get_page_size(), + ), + function ($event) { + if ($this->lateuseridfilter === null) { + return true; + } + return isset($this->lateuseridfilter[$event->userid]); + }, + ); // Set initial bars. if ($useinitialsbar && !$this->is_downloading()) { $this->initialbars($total > $pagesize); } - } /** @@ -553,57 +534,6 @@ class report_log_table_log extends table_sql { * @deprecated since Moodle 2.9 MDL-48595 - please do not use this function any more. */ public function update_users_and_courses_used() { - throw new coding_exception('update_users_and_courses_used() can not be used any more, please use update_users_used() instead.'); - } - - /** - * Helper function to create list of user fullnames shown in log report. - * - * This will update $this->userfullnames array with userfullname, - * which will be used to render logs in table. - * - * @since Moodle 2.9 - * @return void - */ - protected function update_users_used() { - global $DB; - - $this->userfullnames = array(); - $userids = array(); - - // For each event cache full username. - // Get list of userids which will be shown in log report. - foreach ($this->rawdata as $event) { - $logextra = $event->get_logextra(); - if (!empty($event->userid) && empty($userids[$event->userid])) { - $userids[$event->userid] = $event->userid; - } - if (!empty($logextra['realuserid']) && empty($userids[$logextra['realuserid']])) { - $userids[$logextra['realuserid']] = $logextra['realuserid']; - } - if (!empty($event->relateduserid) && empty($userids[$event->relateduserid])) { - $userids[$event->relateduserid] = $event->relateduserid; - } - } - $this->rawdata->close(); - - // Get user fullname and put that in return list. - if (!empty($userids)) { - list($usql, $uparams) = $DB->get_in_or_equal($userids); - $userfieldsapi = \core_user\fields::for_name(); - $users = $DB->get_records_sql("SELECT id," . $userfieldsapi->get_sql('', false, '', '', false)->selects . - " FROM {user} WHERE id " . $usql, - $uparams); - foreach ($users as $userid => $user) { - $this->userfullnames[$userid] = fullname($user, has_capability('moodle/site:viewfullnames', $this->get_context())); - unset($userids[$userid]); - } - - // We fill the array with false values for the users that don't exist anymore - // in the database so we don't need to query the db again later. - foreach ($userids as $userid) { - $this->userfullnames[$userid] = false; - } - } + throw new coding_exception('update_users_and_courses_used() can not be used any more.'); } } diff --git a/report/log/tests/renderable_test.php b/report/log/tests/renderable_test.php index 5dd39cee406..8afe9e42813 100644 --- a/report/log/tests/renderable_test.php +++ b/report/log/tests/renderable_test.php @@ -55,6 +55,7 @@ class renderable_test extends \advanced_testcase { 'teacher0' => ['group0'], 'teacher1' => ['group1'], 'teacher2' => ['group0', 'group1'], + 'teacher3' => [], ], // Make editingteacher also member of group1. 'editingteacher' => [ @@ -96,19 +97,39 @@ class renderable_test extends \advanced_testcase { 'separategroups: student 1' => [ self::COURSE_SEPARATE_GROUP, 'student1', - // All users in group 1. + // All users in group1. [ - 'student1', 'student2', 'teacher1', 'teacher2', 'editingteacher1', - 'editingteacher2', + 'student1', 'student2', + 'teacher1', 'teacher2', + 'editingteacher1', 'editingteacher2', + ], + ], + 'separategroups: student 2' => [ + self::COURSE_SEPARATE_GROUP, + 'student2', + // All users in group0 and group1. + [ + 'student0', 'student1', 'student2', + 'teacher0', 'teacher1', 'teacher2', + 'editingteacher0', 'editingteacher1', 'editingteacher2', + ], + ], + 'separategroups: student3' => [ + self::COURSE_SEPARATE_GROUP, + 'student3', + // Student 3 is not in any group so should only see user without a group. + [ + 'student3', + 'teacher3', ], ], 'separategroups: editing teacher 0' => [ self::COURSE_SEPARATE_GROUP, 'editingteacher0', - // All users (including student3 who is not in a group). + // All users including student 3 as we can see all users (event the one not in a group). [ 'student0', 'student1', 'student2', 'student3', - 'teacher0', 'teacher1', 'teacher2', + 'teacher0', 'teacher1', 'teacher2', 'teacher3', 'editingteacher0', 'editingteacher1', 'editingteacher2', ], ], @@ -125,28 +146,26 @@ class renderable_test extends \advanced_testcase { 'separategroups: teacher 2' => [ self::COURSE_SEPARATE_GROUP, 'teacher2', - // All users in group 0 and 1. + // All users in group0 and group1. [ 'student0', 'student1', 'student2', 'teacher0', 'teacher1', 'teacher2', 'editingteacher0', 'editingteacher1', 'editingteacher2', ], ], - 'separategroups: teacher 2 with group0 selected' => [ + 'separategroups: teacher 3' => [ self::COURSE_SEPARATE_GROUP, - 'teacher2', - // All users in group 0. + 'teacher3', + // Teacher 3 is not in any group so should only see user without a group. [ - 'student0', 'student2', - 'teacher0', 'teacher2', - 'editingteacher0', 'editingteacher2', + 'student3', + 'teacher3', ], - 'group0', ], - 'separategroups: teacher 2 with group1 selected' => [ + 'separategroups: teacher 2 with group set to group 1' => [ self::COURSE_SEPARATE_GROUP, 'teacher2', - // All users in group 1. + // All users in group1. [ 'student1', 'student2', 'teacher1', 'teacher2', @@ -154,34 +173,13 @@ class renderable_test extends \advanced_testcase { ], 'group1', ], - 'visiblegroup: teacher 0 with group1 selected' => [ - self::COURSE_VISIBLE_GROUP, - 'teacher2', - // All users in group 1. - [ - 'student1', 'student2', - 'teacher1', 'teacher2', - 'editingteacher1', 'editingteacher2', - ], - 'group1', - ], - 'visiblegroup: teacher 0 without group selected' => [ - self::COURSE_VISIBLE_GROUP, - 'teacher2', - // All users. - [ - 'student0', 'student1', 'student2', 'student3', - 'teacher0', 'teacher1', 'teacher2', - 'editingteacher0', 'editingteacher1', 'editingteacher2', - ], - ], 'visiblegroup: editing teacher' => [ self::COURSE_VISIBLE_GROUP, 'editingteacher0', // All users. [ 'student0', 'student1', 'student2', 'student3', - 'teacher0', 'teacher1', 'teacher2', + 'teacher0', 'teacher1', 'teacher2', 'teacher3', 'editingteacher0', 'editingteacher1', 'editingteacher2', ], ], @@ -191,39 +189,40 @@ class renderable_test extends \advanced_testcase { // All users. [ 'student0', 'student1', 'student2', 'student3', - 'teacher0', 'teacher1', 'teacher2', + 'teacher0', 'teacher1', 'teacher2', 'teacher3', 'editingteacher0', 'editingteacher1', 'editingteacher2', ], ], - 'nogroup: teacher 0' => [ + 'visiblegroup: teacher 0' => [ self::COURSE_VISIBLE_GROUP, 'teacher2', // All users. [ 'student0', 'student1', 'student2', 'student3', - 'teacher0', 'teacher1', 'teacher2', + 'teacher0', 'teacher1', 'teacher2', 'teacher3', 'editingteacher0', 'editingteacher1', 'editingteacher2', ], ], - 'nogroup: editing teacher 0' => [ + 'visiblegroup: editing teacher 0' => [ self::COURSE_VISIBLE_GROUP, 'editingteacher0', // All users. [ 'student0', 'student1', 'student2', 'student3', - 'teacher0', 'teacher1', 'teacher2', + 'teacher0', 'teacher1', 'teacher2', 'teacher3', 'editingteacher0', 'editingteacher1', 'editingteacher2', ], ], - 'nogroup: student' => [ + 'visiblegroup: teacher 2 with group set to group 1' => [ self::COURSE_VISIBLE_GROUP, - 'student0', - // All users. + 'teacher2', + // All users in group1. [ - 'student0', 'student1', 'student2', 'student3', - 'teacher0', 'teacher1', 'teacher2', - 'editingteacher0', 'editingteacher1', 'editingteacher2', + 'student1', 'student2', + 'teacher1', 'teacher2', + 'editingteacher1', 'editingteacher2', ], + 'group1', ], ]; }