From edd9c78756fe6300b113300aca0721c549bd2d82 Mon Sep 17 00:00:00 2001 From: Laurent David Date: Tue, 3 Jun 2025 13:22:03 +0200 Subject: [PATCH] MDL-84464 report_log: Hide report for teachers without groups * Teachers without groups in SEPARATEGROUP mode should not see the report log page nor access it --- lib/classes/report_helper.php | 40 +++++ lib/tests/report_helper_test.php | 145 +++++++++++++++++- lib/upgrade.txt | 5 + report/log/index.php | 11 +- report/log/lib.php | 12 +- report/log/tests/behat/behat_report_log.php | 51 ++++++ .../log/tests/behat/group_report_log.feature | 95 ++++++++++++ report/loglive/index.php | 21 +-- 8 files changed, 365 insertions(+), 15 deletions(-) create mode 100644 report/log/tests/behat/behat_report_log.php create mode 100644 report/log/tests/behat/group_report_log.feature diff --git a/lib/classes/report_helper.php b/lib/classes/report_helper.php index f458b5b3611..4029fccd0aa 100644 --- a/lib/classes/report_helper.php +++ b/lib/classes/report_helper.php @@ -103,4 +103,44 @@ class report_helper { } $USER->course_last_report[$id] = $url; } + + /** + * Check if the user is in a valid group for the course (i.e. if the user is in a group in SEPARATEGROUPS mode) + * + * @param context $context context for the course or module: if context is a course context, the course group mode is used, + * if it is a module context, the module effective group mode is used (combined with the current user). + * @param int|null $userid user id to check, if null the current user is used + * @return bool true if the user is in a valid group (i.e. belongs to a group in SEPARATEGROUPS MODE), false otherwise + */ + public static function has_valid_group(\context $context, ?int $userid = null): bool { + global $USER; + + $userid = $userid ?? $USER->id; + + if ($context instanceof \context_course) { + $courseid = $context->instanceid; + $course = get_course($courseid); + $groupmode = $course->groupmode; + } else if ($context instanceof \context_module) { + $courseid = $context->get_course_context()->instanceid; + $modinfo = get_fast_modinfo($courseid); + $cm = $modinfo->get_cm($context->instanceid); + $groupmode = $cm->effectivegroupmode; + } else { + return true; // No groups in system context. + } + + if ($groupmode != SEPARATEGROUPS) { + return true; // No groups or visible all groups. + } + + if (!has_capability('moodle/site:accessallgroups', $context, $userid)) { + $usergroups = groups_get_all_groups($courseid, $userid); + if (empty($usergroups)) { + return false; + } + } + + return true; + } } diff --git a/lib/tests/report_helper_test.php b/lib/tests/report_helper_test.php index 37612756faf..0ebc8ebdbd4 100644 --- a/lib/tests/report_helper_test.php +++ b/lib/tests/report_helper_test.php @@ -26,7 +26,6 @@ namespace core; use moodle_url; -use core\report_helper; /** * Tests the functions for report_helper class. @@ -113,4 +112,148 @@ final class report_helper_test extends \advanced_testcase { $this->assertStringContainsString($loglive, $output); $this->assertStringContainsString($participation, $output); } + + /** + * Tests {@see report_helper::has_valid_group()}. + * + * @param int $groupmode Group mode for the course + * @param string $username Username of the user to check + * @param array $expected Expected result of the check, with 3 boolean values depending on the context: + * - Course context + * - Module context + * - System context + * + * @covers \core\report_helper::has_valid_group + * @dataProvider has_valid_group_provider + */ + public function test_has_valid_group(int $groupmode, string $username, array $expected): void { + $this->resetAfterTest(); + + // Create some test course, groups, and users. + $generator = self::getDataGenerator(); + $course = $generator->create_course(['groupmode' => $groupmode, 'groupmodeforce' => 1]); + $assign = $generator->create_module('assign', ['course' => $course->id]); + $g1 = $generator->create_group(['courseid' => $course->id]); + + $this->userids = []; + $data = [ + 's1' => ['role' => 'student', 'group' => $g1->id], + 's2' => ['role' => 'student', 'group' => null], + 't1' => ['role' => 'teacher', 'group' => $g1->id], + 't2' => ['role' => 'teacher', 'group' => null], + 'et1' => ['role' => 'editingteacher', 'group' => null], + ]; + foreach ($data as $key => $value) { + ['group' => $groupid, 'role' => $role] = $value; + $this->userids[$key] = $generator->create_user(['username' => $key]); + $generator->enrol_user($this->userids[$key]->id, $course->id, $role); + if ($groupid) { + groups_add_member($groupid, $this->userids[$key]->id); + } + } + $coursecontext = \context_course::instance($course->id); + [$course, $cm] = get_course_and_cm_from_instance($assign->id, 'assign'); + $modulecontext = \context_module::instance($cm->id); + [$hasvalidgroupcourse, $hasvalidgroupmodule, $hasvalidgroupsystem] = $expected; + $this->assertEquals( + $hasvalidgroupcourse, + report_helper::has_valid_group($coursecontext, $this->userids[$username]->id), + "Failed for user $username in course context" + ); + $this->assertEquals( + $hasvalidgroupmodule, + report_helper::has_valid_group($modulecontext, $this->userids[$username]->id), + 'Failed for user ' . $username . ' in module context' + ); + $this->assertEquals( + $hasvalidgroupsystem, + report_helper::has_valid_group(\context_system::instance(), $this->userids[$username]->id), + 'Failed for user ' . $username . ' in system context' + ); + } + + /** + * Data provider for test_has_valid_group. + * + * @return array + */ + public static function has_valid_group_provider(): array { + return [ + 'student 1 - g1 - separate group' => [ + 'groupmode' => SEPARATEGROUPS, + 'username' => 's1', + 'expected' => [true, true, true], + ], + 'student 2 - no group - separate group' => [ + 'groupmode' => SEPARATEGROUPS, + 'username' => 's2', + 'expected' => [false, false, true], + ], + 'teacher 1 - g1 - separate group' => [ + 'groupmode' => SEPARATEGROUPS, + 'username' => 't1', + 'expected' => [true, true, true], + ], + 'teacher 2 - no group - separate group' => [ + 'groupmode' => SEPARATEGROUPS, + 'username' => 't2', + 'expected' => [false, false, true], + ], + 'editing teacher - no group - separate group' => [ + 'groupmode' => SEPARATEGROUPS, + 'username' => 'et1', + 'expected' => [true, true, true], + ], + 'student 1 - g1 - no group' => [ + 'groupmode' => NOGROUPS, + 'username' => 's1', + 'expected' => [true, true, true], + ], + 'student 2 - no group - no group' => [ + 'groupmode' => NOGROUPS, + 'username' => 's2', + 'expected' => [true, true, true], + ], + 'teacher 1 - g1 - no group' => [ + 'groupmode' => NOGROUPS, + 'username' => 't1', + 'expected' => [true, true, true], + ], + 'teacher 2 - no group - no group' => [ + 'groupmode' => NOGROUPS, + 'username' => 't2', + 'expected' => [true, true, true], + ], + 'editing teacher - no group - no group' => [ + 'groupmode' => NOGROUPS, + 'username' => 'et1', + 'expected' => [true, true, true], + ], + 'student 1 - g1 - visible group' => [ + 'groupmode' => VISIBLEGROUPS, + 'username' => 's1', + 'expected' => [true, true, true], + ], + 'student 2 - no group - visible group' => [ + 'groupmode' => VISIBLEGROUPS, + 'username' => 's2', + 'expected' => [true, true, true], + ], + 'teacher 1 - g1 - visible group' => [ + 'groupmode' => VISIBLEGROUPS, + 'username' => 't1', + 'expected' => [true, true, true], + ], + 'teacher 2 - no group - visible group' => [ + 'groupmode' => VISIBLEGROUPS, + 'username' => 't2', + 'expected' => [true, true, true], + ], + 'editing teacher - visible group - no group' => [ + 'groupmode' => VISIBLEGROUPS, + 'username' => 'et1', + 'expected' => [true, true, true], + ], + ]; + } } diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 434ddb15b96..c4e8df824b1 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -1,6 +1,11 @@ This files describes API changes in core libraries and APIs, information provided here is intended especially for developers. +=== 4.1.20 === + +* Add a new method has_valid_group in \core\report_helper that will return true or false depending if the user has a valid group. This is mainly + false in case the user is not in any group in SEPARATEGROUPS. Used in report_log and report_loglive. + === 4.1.18 === * A new method, `core_text::trim_ctrl_chars()`, has been introduced to clean control characters from text. This ensures cleaner input handling and prevents issues caused by invisible or non-printable characters diff --git a/report/log/index.php b/report/log/index.php index e3c7187efc2..f556e64a6fd 100644 --- a/report/log/index.php +++ b/report/log/index.php @@ -150,11 +150,18 @@ if ($course->id == $SITE->id) { $PAGE->set_heading($course->fullname); } +$output = $PAGE->get_renderer('report_log'); +if (!report_helper::has_valid_group($context)) { + echo $output->header(); + echo $output->notification(get_string('notingroup')); + echo $output->footer(); + exit(); +} + $reportlog = new report_log_renderable($logreader, $course, $user, $modid, $modaction, $group, $edulevel, $showcourses, $showusers, $chooselog, true, $url, $date, $logformat, $page, $perpage, 'timecreated DESC', $origin); -$readers = $reportlog->get_readers(); -$output = $PAGE->get_renderer('report_log'); +$readers = $reportlog->get_readers(); if (empty($readers)) { echo $output->header(); echo $output->heading(get_string('nologreaderenabled', 'report_log')); diff --git a/report/log/lib.php b/report/log/lib.php index 4d0fbe4b262..16789cbf3a2 100644 --- a/report/log/lib.php +++ b/report/log/lib.php @@ -24,6 +24,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core\report_helper; + defined('MOODLE_INTERNAL') || die; /** @@ -34,7 +36,10 @@ defined('MOODLE_INTERNAL') || die; * @param stdClass $context The context of the course */ function report_log_extend_navigation_course($navigation, $course, $context) { - if (has_capability('report/log:view', $context)) { + if ( + has_capability('report/log:view', $context) + && report_helper::has_valid_group($context) + ) { $url = new moodle_url('/report/log/index.php', array('id'=>$course->id)); $navigation->add(get_string('pluginname', 'report_log'), $url, navigation_node::TYPE_SETTING, null, null, new pix_icon('i/report', '')); } @@ -124,7 +129,10 @@ function report_log_can_access_user_report($user, $course) { * @param stdClass $cm */ function report_log_extend_navigation_module($navigation, $cm) { - if (has_capability('report/log:view', context_course::instance($cm->course))) { + if ( + has_capability('report/log:view', context_course::instance($cm->course)) + && report_helper::has_valid_group(context_module::instance($cm->id)) + ) { $url = new moodle_url('/report/log/index.php', array('chooselog'=>'1','id'=>$cm->course,'modid'=>$cm->id)); $navigation->add(get_string('logs'), $url, navigation_node::TYPE_SETTING, null, 'logreport', new pix_icon('i/report', '')) ->set_show_in_secondary_navigation(false); diff --git a/report/log/tests/behat/behat_report_log.php b/report/log/tests/behat/behat_report_log.php new file mode 100644 index 00000000000..f19ad082a89 --- /dev/null +++ b/report/log/tests/behat/behat_report_log.php @@ -0,0 +1,51 @@ +. + +require_once(__DIR__ . '/../../../../lib/behat/behat_base.php'); + +/** + * Step definition for report_log behat tests. + * + * @package report_log + * @category test + * @copyright 2025 Laurent David + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class behat_report_log extends behat_base { + /** + * Convert page names to URLs for steps like 'When I am on the "[identifier]" "[page type]" page'. + * + * Recognised page names are: + * | pagetype | name meaning | description | + * | logs | Course name | The course report logs page | + * + * @param string $page identifies which type of page this is, e.g. 'Logs'. + * @param string $identifier identifies the particular page, e.g. 'C1'. + * @return moodle_url the corresponding URL. + * @throws Exception with a meaningful error message if the specified page cannot be found. + */ + protected function resolve_page_instance_url(string $page, string $identifier): moodle_url { + switch (strtolower($page)) { + case 'logs': + $courseid = $this->get_course_id($identifier); + return new moodle_url('/report/log/index.php', [ + 'id' => $courseid, + ]); + default: + throw new Exception("Unrecognised page type '{$page}'"); + } + } +} diff --git a/report/log/tests/behat/group_report_log.feature b/report/log/tests/behat/group_report_log.feature new file mode 100644 index 00000000000..6a6aeec9f4f --- /dev/null +++ b/report/log/tests/behat/group_report_log.feature @@ -0,0 +1,95 @@ +@report @report_log +Feature: In a course with group mode, I can view the group report log page or not + depending on the group I am in. + + Background: + Given the following "courses" exist: + | fullname | shortname | category | groupmode | + | Course separate group | C1 | 0 | 1 | + | Course visible group | C2 | 0 | 2 | + | Course no group | C3 | 0 | 0 | + And the following "users" exist: + | username | firstname | lastname | email | + | student1 | Student | 1 | student1@example.com | + | student2 | Student | 2 | student2@example.com | + | teacher1 | Teacher | 1 | teacher1@example.com | + | teacher2 | Teacher | 2 | teacher2@example.com | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | teacher | + | teacher2 | C1 | teacher | + | student1 | C1 | student | + | student2 | C1 | student | + | teacher1 | C2 | teacher | + | teacher2 | C2 | teacher | + | student1 | C2 | student | + | student2 | C2 | student | + | teacher1 | C3 | teacher | + | teacher2 | C3 | teacher | + | student1 | C3 | student | + | student2 | C3 | student | + And the following "groups" exist: + | course | name | idnumber | + | C1 | Group C1.1 | group11 | + | C2 | Group C2.1 | group21 | + | C3 | Group C3.1 | group31 | + And the following "group members" exist: + | group | user | + | group11 | student1 | + | group11 | teacher1 | + | group21 | student1 | + | group21 | teacher1 | + | group31 | student1 | + | group31 | teacher1 | + And the following "activities" exist: + | activity | name | intro | course | idnumber | + | page | Page11 | Page11 | C1 | page1 | + | page | Page21 | Page21 | C2 | page1 | + | page | Page31 | Page31 | C3 | page1 | + # Generate logs for the pages. + And I am on the "Page11" "page activity" page logged in as student1 + And I am on "Course separate group" course homepage + And I log out + And I am on the "Page11" "page activity" page logged in as student2 + And I am on "Course separate group" course homepage + And I log out + And I am on the "Page21" "page activity" page logged in as student1 + And I am on "Course visible group" course homepage + And I log out + And I am on the "Page21" "page activity" page logged in as student2 + And I am on "Course visible group" course homepage + And I log out + And I am on the "Page31" "page activity" page logged in as student1 + And I am on "Course no group" course homepage + And I log out + And I am on the "Page31" "page activity" page logged in as student2 + And I am on "Course no group" course homepage + And I log out + + Scenario Outline: As a user in a course, I can view a link to the report logs if I am in the right group. + Given I log in as "" + And I am on "" course homepage + When I navigate to "Reports" in current page administration + And "Logs" "link" + And I log out + Examples: + | course | user | shouldexist | + | Course separate group | teacher1 | should exist | + | Course separate group | teacher2 | should not exist | + | Course visible group | teacher1 | should exist | + | Course visible group | teacher2 | should exist | + | Course no group | teacher1 | should exist | + | Course no group | teacher2 | should exist | + + Scenario Outline: As a non editing teacher not in a group, I can not view the report logs. + Given I log in as "" + When I am on the "" "report_log > Logs" page + Then I "you need to be part of a group to see this page." + Examples: + | course | user | shouldsee | + | Course separate group | teacher1 | should not see | + | Course separate group | teacher2 | should see | + | Course visible group | teacher1 | should not see | + | Course visible group | teacher2 | should not see | + | Course no group | teacher1 | should not see | + | Course no group | teacher2 | should not see | \ No newline at end of file diff --git a/report/loglive/index.php b/report/loglive/index.php index ffe45342e80..4875b68bfa7 100644 --- a/report/loglive/index.php +++ b/report/loglive/index.php @@ -29,6 +29,7 @@ use core\report_helper; require('../../config.php'); require_once($CFG->libdir.'/adminlib.php'); require_once($CFG->dirroot.'/course/lib.php'); +global $USER, $SITE, $PAGE; $id = optional_param('id', 0, PARAM_INT); $page = optional_param('page', 0, PARAM_INT); @@ -60,23 +61,23 @@ $url = new moodle_url("/report/loglive/index.php", $params); $PAGE->set_url($url); $PAGE->set_pagelayout('report'); +$PAGE->set_context($context); +$strlivelogs = get_string('livelogs', 'report_loglive'); +$PAGE->set_title("$coursename: $strlivelogs"); +$output = $PAGE->get_renderer('report_loglive'); +echo $output->header(); +if (!report_helper::has_valid_group($context)) { + echo $output->notification(get_string('notingroup')); + echo $output->footer(); + exit(); +} $renderable = new report_loglive_renderable($logreader, $id, $url, 0, $page); $refresh = $renderable->get_refresh_rate(); $logreader = $renderable->selectedlogreader; - -$strlivelogs = get_string('livelogs', 'report_loglive'); $strupdatesevery = get_string('updatesevery', 'moodle', $refresh); - - -$PAGE->set_url($url); -$PAGE->set_context($context); -$PAGE->set_title("$coursename: $strlivelogs"); $PAGE->set_heading($coursename); -$output = $PAGE->get_renderer('report_loglive'); -echo $output->header(); - // Print selector dropdown. $pluginname = get_string('pluginname', 'report_loglive'); report_helper::print_report_selector($pluginname);