MDL-81951 communication_matrix: Fix suspended users ignored bug

.upgradenotes/MDL-81951-2024060308093446.yml

@@ -0,0 +1,7 @@
+issueNumber: MDL-81951
+notes:
+  core_communication:
+    - message: >-
+        The get_enrolled_users_for_course() method now accepts an additional
+        argument that can filter only active enrolments.
+      type: changed

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;

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,
         );
     }
 

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) {
+                $muid = matrix_user_manager::get_matrixid_from_moodle($userid);
-                    unset($newuserpowerlevels[$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(),

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.
      */