MDL-64024 message: Fix lib::get_messages to get only individual messages
authorSara Arjona <sara@moodle.com>
Thu, 15 Nov 2018 12:33:43 +0000 (13:33 +0100)
committerJake Dallimore <jake@moodle.com>
Tue, 20 Nov 2018 02:44:27 +0000 (10:44 +0800)
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
message/tests/messagelib_test.php

index eb0dffb..577ad53 100644 (file)
@@ -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.
index dbc7a0b..c77781b 100644 (file)
@@ -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);
+    }
+
 }