From 510ede569890929cada39a4c7992d5491283a026 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 15 May 2019 18:15:56 +0800 Subject: [PATCH] MDL-65566 core_message: fix bug allowing duplicate unique conversations Individual and self conversations may exist once only, so we need to enforce this in the creation method. --- message/classes/api.php | 6 ++++++ message/tests/api_test.php | 27 +++++++++++++++++++++++++++ message/tests/externallib_test.php | 14 +++++++++++++- 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/message/classes/api.php b/message/classes/api.php index 42bfb62217c..1af93dc3168 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -2528,6 +2528,12 @@ class api { $conversation->convhash = null; if ($type == self::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL || $type == self::MESSAGE_CONVERSATION_TYPE_SELF) { $conversation->convhash = helper::get_conversation_hash($userids); + + // Don't blindly create a conversation between 2 users if there is already one present - return that. + // This stops us making duplicate self and individual conversations, which is invalid. + if ($record = $DB->get_record('message_conversations', ['convhash' => $conversation->convhash])) { + return $record; + } } $conversation->component = $component; $conversation->itemtype = $itemtype; diff --git a/message/tests/api_test.php b/message/tests/api_test.php index 8d926458507..235ad5fa14c 100644 --- a/message/tests/api_test.php +++ b/message/tests/api_test.php @@ -2266,6 +2266,22 @@ class core_message_api_testcase extends core_message_messagelib_testcase { ); } + /** + * Test that creation can't create the same conversation twice for 1:1 conversations. + */ + public function test_create_conversation_duplicate_conversations() { + global $DB; + $user1 = $this::getDataGenerator()->create_user(); + + \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_SELF, [$user1->id]); + \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_SELF, [$user1->id]); + + $convhash = \core_message\helper::get_conversation_hash([$user1->id]); + $countconversations = $DB->count_records('message_conversations', ['convhash' => $convhash]); + $this->assertEquals(1, $countconversations); + $this->assertNotEmpty($conversation = \core_message\api::get_self_conversation($user1->id)); + } + /** * Test get_conversations with a mixture of messages. * @@ -6717,6 +6733,17 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $messageids[] = testhelper::send_fake_message_to_conversation($userfrom, $conversation->id); } + // Remove the self conversations created by the generator, + // so we can choose to set that ourself and honour the original intention of the test. + $userids = array_map(function($userindex) use ($users) { + return $users[$userindex]->id; + }, $config['users']); + foreach ($userids as $userid) { + if ($conversation->type == \core_message\api::MESSAGE_CONVERSATION_TYPE_SELF) { + \core_message\api::unset_favourite_conversation($conversation->id, $userid); + } + } + foreach ($config['favourites'] as $userfromindex) { $userfrom = $users[$userfromindex]; $usercontext = \context_user::instance($userfrom->id); diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index e13591efe12..207eea78be7 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -6523,13 +6523,14 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $user1 = self::getDataGenerator()->create_user(); $user2 = self::getDataGenerator()->create_user(); + $user3 = self::getDataGenerator()->create_user(); // Some random conversation. $otherconversation = \core_message\api::create_conversation( \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, [ $user1->id, - $user2->id, + $user3->id, ] ); @@ -7039,6 +7040,17 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $messageids[] = testhelper::send_fake_message_to_conversation($userfrom, $conversation->id); } + // Remove the self conversations created by the generator, + // so we can choose to set that ourself and honour the original intention of the test. + $userids = array_map(function($userindex) use ($users) { + return $users[$userindex]->id; + }, $config['users']); + foreach ($userids as $userid) { + if ($conversation->type == \core_message\api::MESSAGE_CONVERSATION_TYPE_SELF) { + \core_message\api::unset_favourite_conversation($conversation->id, $userid); + } + } + foreach ($config['favourites'] as $userfromindex) { $userfrom = $users[$userfromindex]; $usercontext = \context_user::instance($userfrom->id); -- 2.43.0