Merge branch 'MDL-81333-main' of https://github.com/davewoloszyn/moodle

communication/classes/helper.php

@@ -424,11 +424,9 @@ class helper {
             $provider = $coursecommunication->get_provider();
         }
 
-        // This nasty logic is here because of hide course doesn't pass anything in the data object.
-        if (!empty($course->communicationroomname)) {
-            $coursecommunicationroomname = $course->communicationroomname;
-        } else {
-            $coursecommunicationroomname = $course->fullname ?? get_course($course->id)->fullname;
+        // Determine the communication room name if none was provided and add it to the course data.
+        if (empty($course->communicationroomname)) {
+            $course->communicationroomname = $course->fullname ?? get_course($course->id)->fullname;
         }
 
         // List of enrolled users for course communication.
@@ -460,7 +458,7 @@ class helper {
             $communication->configure_room_and_membership_by_provider(
                 provider: $provider,
                 instance: $course,
-                communicationroomname: $coursecommunicationroomname,
+                communicationroomname: $course->communicationroomname,
                 users: $enrolledusers,
                 instanceimage: $courseimage,
             );
@@ -481,7 +479,7 @@ class helper {
             if ($communication->get_processor() === null) {
                 // If a course communication instance is not created, create one.
                 $communication->create_and_configure_room(
-                    communicationroomname: $coursecommunicationroomname,
+                    communicationroomname: $course->communicationroomname,
                     avatar: $courseimage,
                     instance: $course,
                     queue: false,
@@ -493,7 +491,7 @@ class helper {
                 // If provider is none, then we will make the room inactive, otherwise always active in group mode.
                 $communication->update_room(
                     active: $provider === processor::PROVIDER_NONE ? processor::PROVIDER_INACTIVE : processor::PROVIDER_ACTIVE,
-                    communicationroomname: $coursecommunicationroomname,
+                    communicationroomname: $course->communicationroomname,
                     avatar: $courseimage,
                     instance: $course,
                     queue: false,
@@ -536,12 +534,31 @@ class helper {
                 groupid: $coursegroup->id,
                 context: $coursecontext,
             );
+
+            $communicationroomname = self::format_group_room_name(
+                baseroomname: $course->communicationroomname,
+                groupname: $coursegroup->name,
+            );
+
             $communication->configure_room_and_membership_by_provider(
                 provider: $provider,
                 instance: $course,
-                communicationroomname: $coursegroup->name,
+                communicationroomname: $communicationroomname,
                 users: $groupuserstoadd,
             );
         }
     }
+
+    /**
+     * Format a group communication room name with the following syntax: 'Group A (Course 1)'.
+     *
+     * @param string $baseroomname The base room name.
+     * @param string $groupname The group name.
+     */
+    public static function format_group_room_name(
+        string $baseroomname,
+        string $groupname
+    ): string {
+        return "{$groupname} ({$baseroomname})";
+    }
 }

communication/classes/hook_listener.php

@@ -86,6 +86,11 @@ class hook_listener {
             context: $coursecontext,
         );
 
+        // Check we have communication correctly set up before proceeding.
+        if ($coursecommunication->get_processor() === null) {
+            return;
+        }
+
         $communication = api::load_by_instance(
             context: $coursecontext,
             component: constants::GROUP_COMMUNICATION_COMPONENT,
@@ -94,8 +99,13 @@ class hook_listener {
             provider: $coursecommunication->get_provider(),
         );
 
+        $communicationroomname = helper::format_group_room_name(
+            baseroomname: $coursecommunication->get_room_name(),
+            groupname: $group->name,
+        );
+
         $communication->create_and_configure_room(
-            communicationroomname: $group->name,
+            communicationroomname: $communicationroomname,
             instance: $course,
         );
 
@@ -134,14 +144,25 @@ class hook_listener {
             context: $coursecontext,
         );
 
+        // Get the course communication instance so we can extract the base room name.
+        $coursecommunication = helper::load_by_course(
+            courseid: $course->id,
+            context: $coursecontext,
+        );
+
+        $communicationroomname = helper::format_group_room_name(
+            baseroomname: $coursecommunication->get_room_name(),
+            groupname: $group->name,
+        );
+
         // If the name didn't change, then we don't need to update the room.
-        if ($group->name === $communication->get_room_name()) {
+        if ($communicationroomname === $communication->get_room_name()) {
             return;
         }
 
         $communication->update_room(
             active: processor::PROVIDER_ACTIVE,
-            communicationroomname: $group->name,
+            communicationroomname: $communicationroomname,
             instance: $course,
         );
     }

communication/tests/helper_test.php

@@ -220,4 +220,17 @@ class helper_test extends \advanced_testcase {
             memberaction: 'a_funny_action',
         );
     }
+
+    /**
+     * Test format_group_room_name.
+     *
+     * @covers ::format_group_room_name
+     */
+    public function test_format_group_room_name(): void {
+        $baseroomname = 'Course A';
+        $groupname = 'Group 1';
+        $formattedroomname = helper::format_group_room_name($baseroomname, $groupname);
+        // Check the room name is formatted as expected.
+        $this->assertEquals('Group 1 (Course A)', $formattedroomname);
+    }
 }

communication/tests/hook_listener_test.php

@@ -56,6 +56,7 @@ class hook_listener_test extends \advanced_testcase {
         global $DB;
 
         $course = $this->get_course(
+            roomname: 'Test room name',
             extrafields: ['groupmode' => SEPARATEGROUPS],
         );
         $coursecontext = \context_course::instance(courseid: $course->id);
@@ -132,8 +133,7 @@ class hook_listener_test extends \advanced_testcase {
         $this->assertCount(0, $adhoctask);
 
         // Now change the group name.
-        $changedgroupname = 'Changedgroupname';
-        $group->name = $changedgroupname;
+        $group->name = 'Changed group name';
         groups_update_group($group);
 
         // Now one task should be there to update the group room name.
@@ -142,7 +142,7 @@ class hook_listener_test extends \advanced_testcase {
 
         $groupcommunication->reload();
         $this->assertEquals(
-            expected: $changedgroupname,
+            expected: 'Changed group name (Test room name)',
             actual: $groupcommunication->get_processor()->get_room_name(),
         );