From ff4d5f3caa5425f863b8cd415a16b9c4c395dd08 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Wed, 27 Mar 2024 11:49:06 +0800 Subject: [PATCH 1/4] MDL-81327 core: Remove unnecessary table filter The \Traversable interface does not define the `valid` method. Furthermore, the `valid` method actually checks that there is a _next_ value, which requires the value already be fetched and waiting. This is not the case for all Iterators. For example the CallbackFilterIterator does not load the initial value until it is requested. It is completely unnecessary to do this check anyway as an invalid Iterator will just not return any values. --- lib/tablelib.php | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/tablelib.php b/lib/tablelib.php index 06612d7991b..204b850861c 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -2067,6 +2067,8 @@ class table_sql extends flexible_table { } /** + * Build the table from the fetched data. + * * Take the data returned from the db_query and go through all the rows * processing each col using either col_{columnname} method or other_cols * method or if other_cols returns NULL then put the data straight into the @@ -2075,18 +2077,13 @@ class table_sql extends flexible_table { * After calling this function, don't forget to call close_recordset. */ public function build_table() { - - if ($this->rawdata instanceof \Traversable && !$this->rawdata->valid()) { - return; - } if (!$this->rawdata) { return; } foreach ($this->rawdata as $row) { $formattedrow = $this->format_row($row); - $this->add_data_keyed($formattedrow, - $this->get_row_class($row)); + $this->add_data_keyed($formattedrow, $this->get_row_class($row)); } } From f67fa427bdcf7eeef2a63daccd3f67136d1d6438 Mon Sep 17 00:00:00 2001 From: Laurent David Date: Thu, 28 Mar 2024 10:05:57 +0100 Subject: [PATCH 2/4] MDL-81327 report_log: Fix col_fullusername * Avoid calling get_user_fullname too many times and deal with the case that $event->userid is empty --- report/log/classes/table_log.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/report/log/classes/table_log.php b/report/log/classes/table_log.php index 4cb1da64d1b..4e9baf587b5 100644 --- a/report/log/classes/table_log.php +++ b/report/log/classes/table_log.php @@ -25,6 +25,7 @@ defined('MOODLE_INTERNAL') || die; global $CFG; require_once($CFG->libdir . '/tablelib.php'); + /** * Table log class for displaying logs. * @@ -33,7 +34,6 @@ require_once($CFG->libdir . '/tablelib.php'); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class report_log_table_log extends table_sql { - /** @var array list of user fullnames shown in report */ private $userfullnames = array(); @@ -155,20 +155,22 @@ class report_log_table_log extends table_sql { /** * Generate the username column. * - * @param stdClass $event event data. + * @param \core\event\base $event event data. * @return string HTML for the username column */ public function col_fullnameuser($event) { // Get extra event data for origin and realuserid. $logextra = $event->get_logextra(); + $eventusername = $event->userid ? $this->get_user_fullname($event->userid) : false; + // Add username who did the action. if (!empty($logextra['realuserid'])) { $a = new stdClass(); if (!$a->realusername = $this->get_user_fullname($logextra['realuserid'])) { $a->realusername = '-'; } - if (!$a->asusername = $this->get_user_fullname($event->userid)) { + if (!$a->asusername = $eventusername) { $a->asusername = '-'; } if (empty($this->download)) { @@ -182,13 +184,13 @@ class report_log_table_log extends table_sql { } $username = get_string('eventloggedas', 'report_log', $a); - } else if (!empty($event->userid) && $username = $this->get_user_fullname($event->userid)) { + } else if ($eventusername) { if (empty($this->download)) { $params = array('id' => $event->userid); if ($event->courseid) { $params['course'] = $event->courseid; } - $username = html_writer::link(new moodle_url('/user/view.php', $params), $username); + $username = html_writer::link(new moodle_url('/user/view.php', $params), $eventusername); } } else { $username = '-'; From 61282aa69271e566b83431abc418637a25c1888a Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Wed, 27 Mar 2024 11:51:30 +0800 Subject: [PATCH 3/4] MDL-81327 report_log: Make group filtering logstore agnostic The original implementation of group filtering introduced in MDL-80565 assumed that the log table existed in Moodle's own database. This is not the case of the database logstore, or any similar logstore implemetning the database \core\log\sql_reader interface. Furthermore this check was also applying the SQL when the user had the `accessallgroups` capability, or when the course was not in SEPARATE groups mode (no groupmode and/or visible groups). Co-authored: Laurent David --- lib/classes/report_helper.php | 69 +++++++++++++++ report/log/classes/table_log.php | 128 ++++++--------------------- report/log/tests/renderable_test.php | 95 ++++++++++---------- 3 files changed, 145 insertions(+), 147 deletions(-) diff --git a/lib/classes/report_helper.php b/lib/classes/report_helper.php index d84e0a0f144..52cf9f5639f 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', ], ]; } From e5f200616e1dc53f56c6643c965a2d198064b94a Mon Sep 17 00:00:00 2001 From: Laurent David Date: Thu, 28 Mar 2024 18:09:39 +0100 Subject: [PATCH 4/4] MDL-81327 report_loglive: Apply the same fixes to loglive * Additional fix: take into account selected group (filter) --- report/loglive/classes/table_log.php | 54 ++++++++++++++----------- report/loglive/tests/table_log_test.php | 5 ++- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/report/loglive/classes/table_log.php b/report/loglive/classes/table_log.php index 296849f5d07..89e30fd2811 100644 --- a/report/loglive/classes/table_log.php +++ b/report/loglive/classes/table_log.php @@ -47,6 +47,9 @@ class report_loglive_table_log extends table_sql { /** @var stdClass filters parameters */ protected $filterparams; + /** @var int[] A list of users to filter by */ + private ?array $lateuseridfilter = null; + /** * Sets up the table_log parameters. * @@ -298,33 +301,23 @@ 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 $USER; - $joins = array(); - $params = array(); + $joins = []; + $params = []; - // Set up filtering. if (!empty($this->filterparams->courseid)) { $joins[] = "courseid = :courseid"; $params['courseid'] = $this->filterparams->courseid; - // 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) || 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); - $joins[] = "userid IN ($groupmembersql)"; - $params = array_merge($params, $groupmemberparams); } + // Getting all members of a group. + [ + 'joins' => $groupjoins, + 'params' => $groupparams, + 'useridfilter' => $this->lateuseridfilter, + ] = \core\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"; $params['date'] = $this->filterparams->date; @@ -339,8 +332,23 @@ class report_loglive_table_log extends table_sql { $total = $this->filterparams->logreader->get_events_select_count($selector, $params); $this->pagesize($pagesize, $total); - $this->rawdata = $this->filterparams->logreader->get_events_select($selector, $params, $this->filterparams->orderby, - $this->get_page_start(), $this->get_page_size()); + + $this->rawdata = + array_filter( + $this->filterparams->logreader->get_events_select( + $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) { diff --git a/report/loglive/tests/table_log_test.php b/report/loglive/tests/table_log_test.php index f411eb36a48..9f5dac1a14e 100644 --- a/report/loglive/tests/table_log_test.php +++ b/report/loglive/tests/table_log_test.php @@ -303,7 +303,10 @@ class table_log_test extends advanced_testcase { $this->setUser($currentuser->id); $store->flush(); $table->query_db(100); - $filteredevents = array_filter($table->rawdata, fn($event) => get_class($event) === \core\event\course_viewed::class); + $filteredevents = + array_filter( + $table->rawdata, fn($event) => get_class($event) === \core\event\course_viewed::class + ); $usernames = array_map( function($event) { $user = core_user::get_user($event->userid, '*', MUST_EXIST);