From 4f97ea8856d84d2ee8be127cfcc54f801ab5c2cd Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Thu, 15 Nov 2018 13:33:43 +0100 Subject: [PATCH] MDL-64024 message: Fix lib::get_messages to get only individual messages From 3.6 users can have also group conversations, so it's needed to review the messages_get_messages function to make sure only returns the individual messages between 2 users. --- message/lib.php | 26 +++++++--- message/tests/messagelib_test.php | 79 +++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 6 deletions(-) diff --git a/message/lib.php b/message/lib.php index eb0dffbbd21..577ad530f9d 100644 --- a/message/lib.php +++ b/message/lib.php @@ -525,7 +525,8 @@ function translate_message_default_setting($plugindefault, $processorname) { /** * Get messages sent or/and received by the specified users. - * Please note that this function return deleted messages too. + * Please note that this function return deleted messages too. Besides, only individual conversation messages + * are returned to maintain backwards compatibility. * * @param int $useridto the user id who received the message * @param int $useridfrom the user id who sent the message. -10 or -20 for no-reply or support user @@ -581,8 +582,9 @@ function message_get_messages($useridto, $useridfrom = 0, $notifications = -1, $ $messagejoinsql WHERE mr.useridfrom = ? AND mr.useridfrom != mcm.userid - AND u.deleted = 0 "; - $messageparams = array_merge($messagejoinparams, [$useridfrom]); + AND u.deleted = 0 + AND mc.type = ? "; + $messageparams = array_merge($messagejoinparams, [$useridfrom, \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL]); // Create the notifications query and params. $notificationsql .= "INNER JOIN {user} u @@ -598,11 +600,23 @@ function message_get_messages($useridto, $useridfrom = 0, $notifications = -1, $ $messagejoinsql WHERE mcm.userid = ? AND mr.useridfrom != mcm.userid - AND u.deleted = 0 "; - $messageparams = array_merge($messagejoinparams, [$useridto]); - if (!empty($useridfrom)) { + AND u.deleted = 0 + AND mc.type = ? "; + $messageparams = array_merge($messagejoinparams, [$useridto, \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL]); + + // If we're dealing with messages only and both useridto and useridfrom are set, + // try to get a conversation between the users. Break early if we can't find one. + if (!empty($useridfrom) && $notifications == 0) { $messagesql .= " AND mr.useridfrom = ? "; $messageparams[] = $useridfrom; + + // There should be an individual conversation between the users. If not, we can return early. + $conversationid = \core_message\api::get_conversation_between_users([$useridto, $useridfrom]); + if (empty($conversationid)) { + return []; + } + $messagesql .= " AND mc.id = ? "; + $messageparams[] = $conversationid; } // Create the notifications query and params. diff --git a/message/tests/messagelib_test.php b/message/tests/messagelib_test.php index dbc7a0befbd..c77781b798f 100644 --- a/message/tests/messagelib_test.php +++ b/message/tests/messagelib_test.php @@ -28,6 +28,8 @@ defined('MOODLE_INTERNAL') || die(); global $CFG; require_once($CFG->dirroot . '/message/lib.php'); +use \core_message\tests\helper as testhelper; + /** * Test api's in message lib. * @@ -406,4 +408,81 @@ class core_message_messagelib_testcase extends advanced_testcase { $this->assertCount(1, message_search_users(0, 'user1')); $this->assertCount(2, message_search_users(0, 'user')); } + + /** + * Test message_get_messages. + */ + public function test_message_get_messages() { + $this->resetAfterTest(true); + + // Set this user as the admin. + $this->setAdminUser(); + + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + $user3 = self::getDataGenerator()->create_user(); + + \core_message\api::add_contact($user1->id, $user2->id); + \core_message\api::add_contact($user1->id, $user3->id); + + // Create some individual conversations. + $ic1 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, + [$user1->id, $user2->id]); + $ic2 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, + [$user1->id, $user3->id]); + + // Send some messages to individual conversations. + $im1 = testhelper::send_fake_message_to_conversation($user1, $ic1->id, 'Message 1'); + $im2 = testhelper::send_fake_message_to_conversation($user2, $ic1->id, 'Message 2'); + $im3 = testhelper::send_fake_message_to_conversation($user1, $ic1->id, 'Message 3'); + $im4 = testhelper::send_fake_message_to_conversation($user1, $ic2->id, 'Message 4'); + + // Retrieve all messages sent from user1 to user2. + $lastmessages = message_get_messages($user2->id, $user1->id, 0, false); + $this->assertCount(2, $lastmessages); + $this->assertArrayHasKey($im1, $lastmessages); + $this->assertArrayHasKey($im3, $lastmessages); + + // Create some group conversations. + $gc1 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, + [$user1->id, $user2->id, $user3->id], 'Group chat'); + + // Send some messages to group conversations. + $gm1 = testhelper::send_fake_message_to_conversation($user1, $gc1->id, 'Group message 1'); + + // Retrieve all messages sent from user1 to user2 (the result should be the same as before, because only individual + // conversations should be considered by the message_get_messages function). + $lastmessages = message_get_messages($user2->id, $user1->id, 0, false); + $this->assertCount(2, $lastmessages); + $this->assertArrayHasKey($im1, $lastmessages); + $this->assertArrayHasKey($im3, $lastmessages); + } + + /** + * Test message_get_messages with only group conversations between users. + */ + public function test_message_get_messages_only_group_conversations() { + $this->resetAfterTest(true); + + // Set this user as the admin. + $this->setAdminUser(); + + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + $user3 = self::getDataGenerator()->create_user(); + + // Create some group conversations. + $gc1 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, + [$user1->id, $user2->id, $user3->id], 'Group chat'); + + // Send some messages to group conversations. + $gm1 = testhelper::send_fake_message_to_conversation($user1, $gc1->id, 'Group message 1'); + $gm2 = testhelper::send_fake_message_to_conversation($user2, $gc1->id, 'Group message 2'); + + // Retrieve all messages sent from user1 to user2. There shouldn't be messages, because only individual + // conversations should be considered by the message_get_messages function. + $lastmessages = message_get_messages($user2->id, $user1->id, 0, false); + $this->assertCount(0, $lastmessages); + } + }