From 329a9e97d294a0e7004a6911ca1e5abce1d03358 Mon Sep 17 00:00:00 2001 From: Michael Hawkins Date: Wed, 4 Oct 2023 18:26:02 +0800 Subject: [PATCH 1/2] MDL-79573 communication: Support new records for each provider type Previously, we replaced information in the same communication record in the database, now there will be a unique record per provider. This ensures when switching between different providers, the correct actions such as room member management can be actioned for the correct provider. This also includes the required updates for the instances to no longer assume a provider must be enabled to be relevant. Specifying a provider is optional to fetch a specific provider instance, the default will use the currently enabled provider. --- communication/classes/api.php | 220 +++++++++--------- communication/classes/processor.php | 37 +-- communication/configure.php | 1 + .../tests/communication_feature_test.php | 4 +- .../matrix/tests/matrix_test_helper_trait.php | 4 +- communication/tests/api_test.php | 24 +- communication/tests/processor_test.php | 33 ++- course/lib.php | 93 ++++++-- 8 files changed, 258 insertions(+), 158 deletions(-) diff --git a/communication/classes/api.php b/communication/classes/api.php index d49c0054327..3a36e3e1059 100644 --- a/communication/classes/api.php +++ b/communication/classes/api.php @@ -56,6 +56,7 @@ class api { * @param string $component The component of the item for the instance * @param string $instancetype The type of the item for the instance * @param int $instanceid The id of the instance + * @param string|null $provider The provider type - if null will load for this context's active provider. * */ private function __construct( @@ -63,12 +64,14 @@ class api { private string $component, private string $instancetype, private int $instanceid, + private ?string $provider = null, ) { $this->communication = processor::load_by_instance( context: $context, component: $component, instancetype: $instancetype, instanceid: $instanceid, + provider: $provider, ); } @@ -79,6 +82,7 @@ class api { * @param string $component The component of the item for the instance * @param string $instancetype The type of the item for the instance * @param int $instanceid The id of the instance + * @param string|null $provider The provider type - if null will load for this context's active provider. * @return api */ public static function load_by_instance( @@ -86,12 +90,14 @@ class api { string $component, string $instancetype, int $instanceid, + ?string $provider = null, ): self { return new self( context: $context, component: $component, instancetype: $instancetype, instanceid: $instanceid, + provider: $provider, ); } @@ -104,6 +110,7 @@ class api { component: $this->component, instancetype: $this->instancetype, instanceid: $this->instanceid, + provider: $this->provider, ); } @@ -246,31 +253,33 @@ class api { * @param string $provider The provider name */ public function form_definition_for_provider(\MoodleQuickForm $mform, string $provider = processor::PROVIDER_NONE): void { - if ($provider !== processor::PROVIDER_NONE) { - // Room name for the communication provider. - $mform->insertElementBefore( - $mform->createElement( - 'text', - 'communicationroomname', - get_string('communicationroomname', 'communication'), - 'maxlength="100" size="20"' - ), - 'addcommunicationoptionshere' - ); - $mform->setType('communicationroomname', PARAM_TEXT); - - $mform->insertElementBefore( - $mform->createElement( - 'static', - 'communicationroomnameinfo', - '', - get_string('communicationroomnameinfo', 'communication'), - ), - 'addcommunicationoptionshere', - ); - - processor::set_provider_specific_form_definition($provider, $mform); + if ($provider === processor::PROVIDER_NONE) { + return; } + + // Room name for the communication provider. + $mform->insertElementBefore( + $mform->createElement( + 'text', + 'communicationroomname', + get_string('communicationroomname', 'communication'), + 'maxlength="100" size="20"' + ), + 'addcommunicationoptionshere' + ); + $mform->setType('communicationroomname', PARAM_TEXT); + + $mform->insertElementBefore( + $mform->createElement( + 'static', + 'communicationroomnameinfo', + '', + get_string('communicationroomnameinfo', 'communication'), + ), + 'addcommunicationoptionshere', + ); + + processor::set_provider_specific_form_definition($provider, $mform); } /** @@ -396,120 +405,105 @@ class api { * Create a communication ad-hoc task for create operation. * This method will add a task to the queue to create the room. * - * @param string $selectedcommunication The selected communication provider * @param string $communicationroomname The communication room name * @param null|\stored_file $avatar The stored file for the avatar * @param \stdClass|null $instance The actual instance object */ public function create_and_configure_room( - string $selectedcommunication, string $communicationroomname, ?\stored_file $avatar = null, ?\stdClass $instance = null, ): void { - if ($selectedcommunication !== processor::PROVIDER_NONE && $selectedcommunication !== '') { - // Create communication record. - $this->communication = processor::create_instance( - context: $this->context, - provider: $selectedcommunication, - instanceid: $this->instanceid, - component: $this->component, - instancetype: $this->instancetype, - roomname: $communicationroomname, - ); - - // Update provider record from form data. - if ($instance !== null) { - $this->communication->get_form_provider()->save_form_data($instance); - } - - // Set the avatar. - if (!empty($avatar)) { - $this->set_avatar($avatar); - } - - // Add ad-hoc task to create the provider room. - create_and_configure_room_task::queue( - $this->communication, - ); + if ($this->provider === processor::PROVIDER_NONE || $this->provider === '') { + return; } + + // Create communication record. + $this->communication = processor::create_instance( + context: $this->context, + provider: $this->provider, + instanceid: $this->instanceid, + component: $this->component, + instancetype: $this->instancetype, + roomname: $communicationroomname, + ); + + // Update provider record from form data. + if ($instance !== null) { + $this->communication->get_form_provider()->save_form_data($instance); + } + + // Set the avatar. + if (!empty($avatar)) { + $this->set_avatar($avatar); + } + + // Add ad-hoc task to create the provider room. + create_and_configure_room_task::queue( + $this->communication, + ); } /** * Create a communication ad-hoc task for update operation. * This method will add a task to the queue to update the room. * - * @param string $selectedprovider The selected communication provider - * @param string $communicationroomname The communication room name + * @param null|int $active The selected active state of the provider + * @param null|string $communicationroomname The communication room name * @param null|\stored_file $avatar The stored file for the avatar * @param \stdClass|null $instance The actual instance object */ public function update_room( - ?string $selectedprovider = null, + ?int $active = null, ?string $communicationroomname = null, ?\stored_file $avatar = null, ?\stdClass $instance = null, ): void { - // Existing object found, let's update the communication record and associated actions. - if ($this->communication !== null) { - // Get the previous data to compare for update. - $previousprovider = $this->communication->get_provider(); - if ($previousprovider === $selectedprovider) { - // If the provider is the same, unset it. - $selectedprovider = null; - } - - $previousroomname = $this->communication->get_room_name(); - if ($previousroomname === $communicationroomname) { - // If the room name is the same, we don't need to update the room. - $communicationroomname = null; - } - - if ($selectedprovider !== null || $communicationroomname !== null) { - // Something to update. Update communication record. - $this->communication->update_instance( - provider: $selectedprovider, - roomname: $communicationroomname, - ); - } - - // Reload so the currently selected provider is used. - $this->reload(); - - // Update provider record from form data. - if ($instance !== null) { - $this->communication->get_form_provider()->save_form_data($instance); - } - - // Update the avatar. - // If the value is `null`, then unset the avatar. - $this->set_avatar($avatar); - - // If the provider is none, we don't need to do anything from room point of view. - if ($this->communication->get_provider() === processor::PROVIDER_NONE) { - return; - } - - // Add ad-hoc task to update the provider room if the room name changed. - // TODO add efficiency considering dynamic fields. - if ( - $previousprovider === $selectedprovider - ) { - update_room_task::queue( - $this->communication, - ); - } else if ( - $previousprovider !== $selectedprovider - ) { - // Add ad-hoc task to create the provider room. - create_and_configure_room_task::queue( - $this->communication, - ); - } - } else { - // The instance didn't have any communication record, so create one. - $this->create_and_configure_room($selectedprovider, $communicationroomname, $avatar, $instance); + // If the provider is none, we don't need to do anything from room point of view. + if ($this->communication->get_provider() === processor::PROVIDER_NONE) { + return; } + + $roomnamechange = null; + $activestatuschange = null; + + // Check if the room name is being changed. + if ( + $communicationroomname !== null && + $communicationroomname !== $this->communication->get_room_name() + ) { + $roomnamechange = $communicationroomname; + } + + // Check if the active status of the provider is being changed. + if ( + $active !== null && + $active !== $this->communication->is_instance_active() + ) { + $activestatuschange = $active; + } + + if ($roomnamechange !== null || $activestatuschange !== null) { + $this->communication->update_instance( + active: $active, + roomname: $communicationroomname, + ); + } + + // Update provider record from form data. + if ($instance !== null) { + $this->communication->get_form_provider()->save_form_data($instance); + } + + // Update the avatar. + // If the value is `null`, then unset the avatar. + $this->set_avatar($avatar); + + // Always queue a room update, even if none of the above standard fields have changed. + // It is possible for providers to have custom fields that have been updated. + update_room_task::queue( + $this->communication, + ); } /** diff --git a/communication/classes/processor.php b/communication/classes/processor.php index e1bd735c10e..cf244706459 100644 --- a/communication/classes/processor.php +++ b/communication/classes/processor.php @@ -109,22 +109,17 @@ class processor { /** * Update the communication instance with any changes. * - * @param null|string $provider The communication provider + * @param null|int $active Active state of the instance (processor::PROVIDER_ACTIVE or processor::PROVIDER_INACTIVE) * @param null|string $roomname The room name */ public function update_instance( - ?string $provider = null, + ?string $active = null, ?string $roomname = null, ): void { global $DB; - if ($provider !== null) { - if ($provider === self::PROVIDER_NONE) { - $this->instancedata->active = self::PROVIDER_INACTIVE; - } else { - $this->instancedata->provider = $provider; - $this->instancedata->active = self::PROVIDER_ACTIVE; - } + if ($active !== null && in_array($active, [self::PROVIDER_ACTIVE, self::PROVIDER_INACTIVE])) { + $this->instancedata->active = $active; } if ($roomname !== null) { @@ -365,24 +360,35 @@ class processor { * @param string $component The component name * @param string $instancetype The instance type * @param int $instanceid The instance id + * @param string|null $provider The provider type - if null will load for this context's active provider. * @return processor|null */ public static function load_by_instance( context $context, string $component, string $instancetype, - int $instanceid + int $instanceid, + ?string $provider = null, ): ?self { global $DB; - $record = $DB->get_record('communication', [ + $params = [ 'contextid' => $context->id, 'instanceid' => $instanceid, 'component' => $component, 'instancetype' => $instancetype, - ]); + ]; + if ($provider === null) { + // Fetch the active provider in this context. + $params['active'] = 1; + } else { + // Fetch a specific provider in this context (which may be inactive). + $params['provider'] = $provider; + } + + $record = $DB->get_record('communication', $params); if ($record && self::is_provider_available($record->provider)) { return new self($record); } @@ -464,15 +470,12 @@ class processor { } /** - * Get communication provider. + * Get communication provider type. * * @return string|null */ public function get_provider(): ?string { - if ((int)$this->instancedata->active === self::PROVIDER_ACTIVE) { - return $this->instancedata->provider; - } - return self::PROVIDER_NONE; + return $this->instancedata->provider; } /** diff --git a/communication/configure.php b/communication/configure.php index 9a9f2266850..b3e16881e5b 100644 --- a/communication/configure.php +++ b/communication/configure.php @@ -52,6 +52,7 @@ $communication = \core_communication\api::load_by_instance( component: $component, instancetype: $instancetype, instanceid: $instanceid, + provider: $selectedcommunication, ); // No communication, no way this form can be used. diff --git a/communication/provider/matrix/tests/communication_feature_test.php b/communication/provider/matrix/tests/communication_feature_test.php index 9aed0670e1c..1d6e431ea61 100644 --- a/communication/provider/matrix/tests/communication_feature_test.php +++ b/communication/provider/matrix/tests/communication_feature_test.php @@ -60,10 +60,10 @@ class communication_feature_test extends \advanced_testcase { component: 'communication_matrix', instancetype: 'example', instanceid: 1, + provider: 'communication_matrix', ); $communication->create_and_configure_room( - selectedcommunication: 'communication_matrix', communicationroomname: 'Room name', instance: (object) [ 'matrixroomtopic' => 'A fun topic', @@ -526,10 +526,10 @@ class communication_feature_test extends \advanced_testcase { component: $component, instancetype: $itemtype, instanceid: $itemid, + provider: 'communication_matrix', ); $communication->create_and_configure_room( - selectedcommunication: 'communication_matrix', communicationroomname: $roomname ?? 'Room name', avatar: $roomavatar, instance: (object) [ diff --git a/communication/provider/matrix/tests/matrix_test_helper_trait.php b/communication/provider/matrix/tests/matrix_test_helper_trait.php index 7a09a188592..c121e38292d 100644 --- a/communication/provider/matrix/tests/matrix_test_helper_trait.php +++ b/communication/provider/matrix/tests/matrix_test_helper_trait.php @@ -253,7 +253,7 @@ trait matrix_test_helper_trait { * @return api */ protected function create_matrix_room( - ?string $component = 'communication_matrix', + ?string $component = 'core_course', ?string $itemtype = 'example', ?int $itemid = 1, ?string $roomname = null, @@ -269,10 +269,10 @@ trait matrix_test_helper_trait { component: $component, instancetype: $itemtype, instanceid: $itemid, + provider: 'communication_matrix', ); $communication->create_and_configure_room( - selectedcommunication: 'communication_matrix', communicationroomname: $roomname ?? 'Room name', avatar: $roomavatar, instance: (object) [ diff --git a/communication/tests/api_test.php b/communication/tests/api_test.php index 8f976d6a77f..7a9dc913b8a 100644 --- a/communication/tests/api_test.php +++ b/communication/tests/api_test.php @@ -118,9 +118,10 @@ class api_test extends \advanced_testcase { component: 'core_course', instancetype: 'coursecommunication', instanceid: $course->id, + provider: $selectedcommunication, ); - $communication->create_and_configure_room($selectedcommunication, $communicationroomname, $avatar); + $communication->create_and_configure_room($communicationroomname, $avatar); // Reload the communication processor. $communicationprocessor = processor::load_by_instance( @@ -153,8 +154,9 @@ class api_test extends \advanced_testcase { component: 'core_course', instancetype: 'coursecommunication', instanceid: $course->id, + provider: $selectedcommunication, ); - $communication->create_and_configure_room($selectedcommunication, $communicationroomname); + $communication->create_and_configure_room($communicationroomname); // Test the tasks added. $adhoctask = \core\task\manager::get_adhoc_tasks('\\core_communication\\task\\create_and_configure_room_task'); @@ -213,7 +215,7 @@ class api_test extends \advanced_testcase { instancetype: 'coursecommunication', instanceid: $course->id, ); - $communication->update_room($selectedcommunication, $communicationroomname); + $communication->update_room(processor::PROVIDER_ACTIVE, $communicationroomname); // Test the communication record exists. $communicationprocessor = processor::load_by_instance( @@ -225,6 +227,22 @@ class api_test extends \advanced_testcase { $this->assertEquals($communicationroomname, $communicationprocessor->get_room_name()); $this->assertEquals($selectedcommunication, $communicationprocessor->get_provider()); + $this->assertTrue($communicationprocessor->is_instance_active()); + + $communication->update_room(processor::PROVIDER_INACTIVE, $communicationroomname); + + // Test updating active state. + $communicationprocessor = processor::load_by_instance( + context: \core\context\course::instance($course->id), + component: 'core_course', + instancetype: 'coursecommunication', + instanceid: $course->id, + provider: $selectedcommunication, + ); + + $this->assertEquals($communicationroomname, $communicationprocessor->get_room_name()); + $this->assertEquals($selectedcommunication, $communicationprocessor->get_provider()); + $this->assertFalse($communicationprocessor->is_instance_active()); } /** diff --git a/communication/tests/processor_test.php b/communication/tests/processor_test.php index fd427eb680c..29c6344726a 100644 --- a/communication/tests/processor_test.php +++ b/communication/tests/processor_test.php @@ -127,7 +127,7 @@ class processor_test extends \advanced_testcase { $selectedcommunication = 'none'; $communicationroomname = 'communicationroomedited'; - $communicationprocessor->update_instance($selectedcommunication, $communicationroomname); + $communicationprocessor->update_instance(processor::PROVIDER_INACTIVE, $communicationroomname); // Now test the record against the database. $communicationrecord = $DB->get_record('communication', [ @@ -291,7 +291,7 @@ class processor_test extends \advanced_testcase { $course = $this->get_course(); $context = \core\context\course::instance($course->id); - // Test the communication record exists. + // Test the communication record exists when fetching the active provider. $communicationprocessor = processor::load_by_instance( context: $context, component: 'core_course', @@ -300,6 +300,29 @@ class processor_test extends \advanced_testcase { ); $this->assertEquals('communication_matrix', $communicationprocessor->get_provider()); + + // Test the communication record exists when specifying the provider. + $communicationprocessor = processor::load_by_instance( + context: $context, + component: 'core_course', + instancetype: 'coursecommunication', + instanceid: $course->id, + provider: 'communication_matrix', + ); + + $this->assertEquals('communication_matrix', $communicationprocessor->get_provider()); + + // Test the communication record exists when the provider is not active. + $communicationprocessor->update_instance(processor::PROVIDER_INACTIVE); + $communicationprocessor = processor::load_by_instance( + context: $context, + component: 'core_course', + instancetype: 'coursecommunication', + instanceid: $course->id, + provider: 'communication_matrix', + ); + + $this->assertEquals('communication_matrix', $communicationprocessor->get_provider()); } /** @@ -427,8 +450,9 @@ class processor_test extends \advanced_testcase { component: 'core_course', instancetype: 'coursecommunication', instanceid: $course->id, + provider: $selectedcommunication, ); - $communication->create_and_configure_room($selectedcommunication, $communicationroomname, $avatar); + $communication->create_and_configure_room($communicationroomname, $avatar); $communicationprocessor = processor::load_by_instance( context: \core\context\course::instance($course->id), @@ -498,8 +522,9 @@ class processor_test extends \advanced_testcase { component: $component, instancetype: $instancetype, instanceid: $course->id, + provider: $selectedcommunication, ); - $communication->create_and_configure_room($selectedcommunication, $communicationroomname); + $communication->create_and_configure_room($communicationroomname); $communication->add_members_to_room([$user1, $user2]); // Now remove user1 from the room. diff --git a/course/lib.php b/course/lib.php index 3cfb4f6a6d2..50dcb32f870 100644 --- a/course/lib.php +++ b/course/lib.php @@ -2292,9 +2292,9 @@ function create_course($data, $editoroptions = NULL) { component: 'core_course', instancetype: 'coursecommunication', instanceid: $course->id, + provider: $provider, ); $communication->create_and_configure_room( - $provider, $communicationroomname, $courseimage ?: null, $data, @@ -2461,38 +2461,97 @@ function update_course($data, $editoroptions = NULL) { $enrolledusers[] = $user->id; } + // Existing communication provider. $communication = \core_communication\api::load_by_instance( context: $context, component: 'core_course', instancetype: 'coursecommunication', instanceid: $data->id, ); + $existingprovider = $communication->get_provider(); + $addusersrequired = false; + $enablenewprovider = false; + $instanceexists = true; - $addafterupdate = false; - if ($provider !== $communication->get_provider()) { - // If provider set to none, remove all the members. - if ($provider === 'none') { + // Action required changes if provider has changed. + if ($provider !== $existingprovider) { + // Provider changed, flag new one to be enabled. + $enablenewprovider = true; + + // If provider set to none, remove all the members from previous provider. + if ($provider === 'none' && $existingprovider !== '') { $communication->remove_members_from_room($enrolledusers); } else if ( - // If previous provider was not none and current provider is not none, but a different provider, remove members. - $communication->get_provider() !== '' && - $communication->get_provider() !== 'none' && - $provider !== $communication->get_provider() + // If previous provider was not none and current provider is not none, + // remove members from previous provider. + $existingprovider !== '' && + $existingprovider !== 'none' ) { $communication->remove_members_from_room($enrolledusers); - $addafterupdate = true; + $addusersrequired = true; } else if ( - // If previous provider was none and current provider is not none, but a different provider, remove members. - ($communication->get_provider() === '' || $communication->get_provider() === 'none') && - $provider !== $communication->get_provider() + // If previous provider was none and current provider is not none, + // remove members from previous provider. + ($existingprovider === '' || $existingprovider === 'none') ) { - $addafterupdate = true; + $addusersrequired = true; + } + + // Disable previous provider, if one was enabled. + if ($existingprovider !== '' && $existingprovider !== 'none') { + $communication->update_room( + active: \core_communication\processor::PROVIDER_INACTIVE, + ); + } + + // Switch to the newly selected provider so it can be updated. + if ($provider !== 'none') { + $communication = \core_communication\api::load_by_instance( + context: $context, + component: 'core_course', + instancetype: 'coursecommunication', + instanceid: $data->id, + provider: $provider, + ); + + // Create it if it does not exist. + if ($communication->get_provider() === '') { + $communication->create_and_configure_room( + communicationroomname: $communicationroomname, + avatar: $courseimage, + instance: $data + ); + + $communication = \core_communication\api::load_by_instance( + context: $context, + component: 'core_course', + instancetype: 'coursecommunication', + instanceid: $data->id, + provider: $provider, + ); + + $addusersrequired = true; + $instanceexists = false; + } } } - $communication->update_room($provider, $communicationroomname, $courseimage, $data); - if ($addafterupdate) { - $communication->add_members_to_room($enrolledusers, false); + if ($provider !== 'none' && $instanceexists) { + // Update the currently enabled provider's room data. + // Newly created providers do not need to run this, the create process handles it. + $communication->update_room( + active: $enablenewprovider ? \core_communication\processor::PROVIDER_ACTIVE : null, + communicationroomname: $communicationroomname, + avatar: $courseimage, + instance: $data, + ); + } + + // Complete room membership tasks if required. + // Newly created providers complete the user mapping but do not queue the task + // (it will be handled by the room creation task). + if ($addusersrequired) { + $communication->add_members_to_room($enrolledusers, $instanceexists); } } From acc48db38624fcf99467335502e84c1686d206d5 Mon Sep 17 00:00:00 2001 From: Michael Hawkins Date: Fri, 6 Oct 2023 11:10:09 +0800 Subject: [PATCH 2/2] MDL-79573 communication: Load form data for the correct provider fields --- communication/classes/form/configure_form.php | 1 + 1 file changed, 1 insertion(+) diff --git a/communication/classes/form/configure_form.php b/communication/classes/form/configure_form.php index fc317909e88..bcd8a31ad8e 100644 --- a/communication/classes/form/configure_form.php +++ b/communication/classes/form/configure_form.php @@ -88,6 +88,7 @@ class configure_form extends \moodleform { component: $component, instancetype: $instancetype, instanceid: $instanceid, + provider: $this->_customdata['selectedcommunication'], ); $this->communication->form_definition($mform); $this->communication->set_data($instancedata);