mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-04-14 04:52:36 +02:00
Code Issues Projects Releases Wiki Activity

MDL-81327 report_log: Make group filtering logstore agnostic

Browse Source 
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 <laurent.david@moodle.com>
This commit is contained in:
Andrew Nicols 2024-03-27 11:51:30 +08:00 committed by Laurent David
parent b418eff39d
commit a1ee9cdbdb
3 changed files with 145 additions and 147 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
lib/classes/report_helper.php
View File

@ -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,
];
}
}

128
report/log/classes/table_log.php
View File

@ -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.');
}
}

95
report/log/tests/renderable_test.php
View File

@ -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',
],
];
}