From e169cf61889a6b7d0a01e9024c86641da791e3a9 Mon Sep 17 00:00:00 2001 From: David Woloszyn Date: Thu, 4 Apr 2024 13:06:47 +1100 Subject: [PATCH] MDL-81333 core_communication: Group room name formatting --- communication/classes/helper.php | 35 ++++++++++++++++------ communication/classes/hook_listener.php | 27 +++++++++++++++-- communication/tests/helper_test.php | 13 ++++++++ communication/tests/hook_listener_test.php | 6 ++-- 4 files changed, 66 insertions(+), 15 deletions(-) diff --git a/communication/classes/helper.php b/communication/classes/helper.php index f1ea4a99050..bb474fdea45 100644 --- a/communication/classes/helper.php +++ b/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})"; + } } diff --git a/communication/classes/hook_listener.php b/communication/classes/hook_listener.php index 29b03293e0b..7acc0829128 100644 --- a/communication/classes/hook_listener.php +++ b/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, ); } diff --git a/communication/tests/helper_test.php b/communication/tests/helper_test.php index b0a24e40256..f7bcb6dd8ed 100644 --- a/communication/tests/helper_test.php +++ b/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); + } } diff --git a/communication/tests/hook_listener_test.php b/communication/tests/hook_listener_test.php index 81f9225f64a..7c6530a8105 100644 --- a/communication/tests/hook_listener_test.php +++ b/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(), );