diff --git a/communication/classes/helper.php b/communication/classes/helper.php index 1d7bf47e4fe..524f4044a16 100644 --- a/communication/classes/helper.php +++ b/communication/classes/helper.php @@ -324,13 +324,14 @@ class helper { * Get the enrolled users for course. * * @param stdClass $course The course object. + * @param bool $onlyactive Only enrolments that are active (e.g. not suspended). * @return array */ - public static function get_enrolled_users_for_course(stdClass $course): array { + public static function get_enrolled_users_for_course(stdClass $course, bool $onlyactive = true): array { global $CFG; require_once($CFG->libdir . '/enrollib.php'); return array_column( - enrol_get_course_users(courseid: $course->id), + enrol_get_course_users(courseid: $course->id, onlyactive: $onlyactive), 'id', ); } @@ -520,11 +521,15 @@ class helper { ); foreach ($coursegroups as $coursegroup) { - $groupuserstoadd = array_column( + $groupusers = array_column( groups_get_members(groupid: $coursegroup->id), 'id', ); + // Filter out users who are not active in this course. + $enrolledusers = self::get_enrolled_users_for_course(course: $course); + $groupuserstoadd = array_intersect($groupusers, $enrolledusers); + foreach ($allaccessgroupusers as $allaccessgroupuser) { if (!in_array($allaccessgroupuser, $groupuserstoadd, true)) { $groupuserstoadd[] = $allaccessgroupuser; diff --git a/communication/classes/hook_listener.php b/communication/classes/hook_listener.php index cacb35b8c89..67f3d2dcf97 100644 --- a/communication/classes/hook_listener.php +++ b/communication/classes/hook_listener.php @@ -214,8 +214,13 @@ class hook_listener { groupid: $group->id, context: $context, ); + + // Filter out users who are not active in this course. + $enrolledusers = helper::get_enrolled_users_for_course($course, true); + $userids = array_intersect($hook->userids, $enrolledusers); + $communication->add_members_to_room( - userids: $hook->userids, + userids: $userids, ); } diff --git a/communication/provider/matrix/classes/communication_feature.php b/communication/provider/matrix/classes/communication_feature.php index d43889d57a3..98d372dc715 100644 --- a/communication/provider/matrix/classes/communication_feature.php +++ b/communication/provider/matrix/classes/communication_feature.php @@ -629,8 +629,9 @@ class communication_feature implements if (!empty($forceremoval)) { // Remove the users from the power levels if they are not admins. foreach ($forceremoval as $userid) { - if ($newuserpowerlevels < matrix_constants::POWER_LEVEL_MAXIMUM) { - unset($newuserpowerlevels[$userid]); + $muid = matrix_user_manager::get_matrixid_from_moodle($userid); + if (isset($newuserpowerlevels[$muid]) && $newuserpowerlevels[$muid] < matrix_constants::POWER_LEVEL_MAXIMUM) { + unset($newuserpowerlevels[$muid]); } } } @@ -640,7 +641,6 @@ class communication_feature implements return; } - // Update the power levels for the room. $this->matrixapi->update_room_power_levels( roomid: $this->get_room_id(), diff --git a/communication/tests/hook_listener_test.php b/communication/tests/hook_listener_test.php index 7c6530a8105..d9a43127a59 100644 --- a/communication/tests/hook_listener_test.php +++ b/communication/tests/hook_listener_test.php @@ -153,6 +153,173 @@ class hook_listener_test extends \advanced_testcase { $this->assertCount(1, $adhoctask); } + /** + * Test inactive users are not included when being mapped to a new communication instance. + */ + public function test_inactive_users_are_not_mapped_to_new_communication(): void { + // Create a course without a communication provider set. + $course = $this->getDataGenerator()->create_course(); + + // Enrol some users that are both active and inactive (suspended). + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + $user3 = $this->getDataGenerator()->create_user(); + $user4 = $this->getDataGenerator()->create_user(); + + $this->getDataGenerator()->enrol_user( + userid: $user1->id, + courseid: $course->id, + roleidorshortname: 'teacher', + ); + + $this->getDataGenerator()->enrol_user( + userid: $user2->id, + courseid: $course->id, + roleidorshortname: 'student', + ); + + $this->getDataGenerator()->enrol_user( + userid: $user3->id, + courseid: $course->id, + roleidorshortname: 'teacher', + status: ENROL_USER_SUSPENDED, + ); + + $this->getDataGenerator()->enrol_user( + userid: $user4->id, + courseid: $course->id, + roleidorshortname: 'student', + status: ENROL_USER_SUSPENDED, + ); + + // Set Matrix as the communication provider and update. + $course->selectedcommunication = 'communication_matrix'; + $course->communication_matrixroomname = 'testroom'; + update_course($course); + + helper::update_course_communication_instance( + course: $course, + changesincoursecat: false, + ); + + // Load the communication instance and check that only the 2 active users are returned. + $communication = helper::load_by_course( + courseid: $course->id, + context: \context_course::instance($course->id), + ); + + $userids = $communication->get_processor()->get_all_userids_for_instance(); + + $this->assertEquals( + expected: 2, + actual: count($userids), + ); + + $this->assertContains( + needle: $user1->id, + haystack: $userids, + ); + + $this->assertContains( + needle: $user2->id, + haystack: $userids, + ); + } + + /** + * Test inactive users are not included when being mapped to a new communication instance using groups. + */ + public function test_inactive_users_are_not_mapped_to_new_group_communication(): void { + // Create a course without a communication provider set. + $course = $this->getDataGenerator()->create_course( + options: ['groupmode' => SEPARATEGROUPS], + ); + + // Enrol some users that are both active and inactive (suspended). + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + $user3 = $this->getDataGenerator()->create_user(); + $user4 = $this->getDataGenerator()->create_user(); + + $this->getDataGenerator()->enrol_user( + userid: $user1->id, + courseid: $course->id, + roleidorshortname: 'teacher', + ); + + $this->getDataGenerator()->enrol_user( + userid: $user2->id, + courseid: $course->id, + roleidorshortname: 'student', + ); + + $this->getDataGenerator()->enrol_user( + userid: $user3->id, + courseid: $course->id, + roleidorshortname: 'teacher', + status: ENROL_USER_SUSPENDED, + ); + + $this->getDataGenerator()->enrol_user( + userid: $user4->id, + courseid: $course->id, + roleidorshortname: 'student', + status: ENROL_USER_SUSPENDED, + ); + + // Create a group and add all users to it. + $group = $this->getDataGenerator()->create_group(['courseid' => $course->id]); + groups_add_member( + grouporid: $group, + userorid: $user1, + ); + groups_add_member( + grouporid: $group, + userorid: $user2, + ); + groups_add_member( + grouporid: $group, + userorid: $user3, + ); + groups_add_member( + grouporid: $group, + userorid: $user4, + ); + + // Set Matrix as the communication provider and update. + $course->selectedcommunication = 'communication_matrix'; + $course->communication_matrixroomname = 'testroom'; + update_course($course); + + helper::update_group_communication_instances_for_course( + course: $course, + provider: 'communication_matrix', + ); + + // Load the communication instance and check that only the 2 active users are returned. + $communication = helper::load_by_group( + groupid: $group->id, + context: \context_course::instance($course->id), + ); + + $userids = $communication->get_processor()->get_all_userids_for_instance(); + + $this->assertEquals( + expected: 2, + actual: count($userids), + ); + + $this->assertContains( + needle: $user1->id, + haystack: $userids, + ); + + $this->assertContains( + needle: $user2->id, + haystack: $userids, + ); + } + /** * Test add_members_to_group_room. */ diff --git a/communication/upgrade.txt b/communication/upgrade.txt index a9e78cbbfbe..6cc0f2ca263 100644 --- a/communication/upgrade.txt +++ b/communication/upgrade.txt @@ -1,2 +1,6 @@ This file describes API changes in /communication/* Information provided here is intended especially for developers. + +=== 4.4.2 === + +* The get_enrolled_users_for_course() method now accepts an additional argument that can filter only active enrolments.