Merge branch 'MDL-64167-master-2' of https://github.com/snake/moodle

2018-11-26 09:07:38 +08:00
parent 599703e83d 7f6f45c460
commit 8dbaf173b9
4 changed files with 145 additions and 8 deletions
@@ -82,11 +82,14 @@ class manager {
        $localisedeventdata = clone $eventdata;

        // Get user records for all members of the conversation.
+        // We must fetch distinct users, because it's possible for a user to message themselves via bulk user actions.
+        // In such cases, there will be 2 records referring to the same user.
        $sql = "SELECT u.*
-                  FROM {message_conversation_members} mcm
-                  JOIN {user} u
-                    ON (mcm.conversationid = :convid AND u.id = mcm.userid)
-              ORDER BY u.id desc";
+                  FROM {user} u
+                 WHERE u.id IN (
+                          SELECT mcm.userid FROM {message_conversation_members} mcm
+                           WHERE mcm.conversationid = :convid
+                 )";
        $members = $DB->get_records_sql($sql, ['convid' => $eventdata->convid]);
        if (empty($members)) {
            throw new \moodle_exception("Conversation has no members or does not exist.");
@@ -586,6 +586,7 @@ class api {
        $conversationset = $DB->get_recordset_sql($sql, $params, $limitfrom, $limitnum);

        $conversations = [];
+        $selfconversations = []; // Used to track legacy conversations with one's self (both conv members the same user).
        $members = [];
        $individualmembers = [];
        $groupmembers = [];
@@ -613,6 +614,9 @@ class api {
        //
        // For 'individual' type conversations between 2 users, regardless of who sent the last message,
        // we want the details of the other member in the conversation (i.e. not the current user).
+        // The only exception to the 'not the current user' rule is for 'self' conversations - a legacy construct in which a user
+        // can message themselves via user bulk actions. Subsequently, there are 2 records for the same user created in the members
+        // table.
        //
        // For 'group' type conversations, we want the details of the member who sent the last message, if there is one.
        // This can be the current user or another group member, but for groups without messages, this will be empty.
@@ -654,6 +658,23 @@ class api {
                $members[$member->conversationid][$member->userid] = $member->userid;
                $individualmembers[$member->userid] = $member->userid;
            }
+
+            // Self conversations: If any of the individual conversations which were missing members are still missing members,
+            // we know these must be 'self' conversations. This is a legacy scenario, created via user bulk actions.
+            // In such cases, the member returned should be the current user.
+            //
+            // NOTE: Currently, these conversations are not returned by this method, however,
+            // identifying them is important for future reference.
+            foreach ($individualconversations as $indconvid) {
+                if (empty($members[$indconvid])) {
+                    // Keep track of the self conversation (for future use).
+                    $selfconversations[$indconvid] = $indconvid;
+
+                    // Set the member to the current user.
+                    $members[$indconvid][$userid] = $userid;
+                    $individualmembers[$userid] = $userid;
+                }
+            }
        }

        // We could fail early here if we're sure that:
@@ -702,7 +723,7 @@ class api {
        // MEMBER COUNT.
        $cids = array_column($conversations, 'id');
        list ($cidinsql, $cidinparams) = $DB->get_in_or_equal($cids, SQL_PARAMS_NAMED, 'convid');
-        $membercountsql = "SELECT conversationid, count(id) AS membercount
+        $membercountsql = "SELECT conversationid, count(DISTINCT userid) AS membercount
                             FROM {message_conversation_members} mcm
                            WHERE mcm.conversationid $cidinsql
                         GROUP BY mcm.conversationid";
@@ -738,6 +759,11 @@ class api {
                continue;
            }

+            // Exclude 'self' conversations for now.
+            if (isset($selfconversations[$conversation->id])) {
+                continue;
+            }
+
            $conv = new \stdClass();
            $conv->id = $conversation->id;
            $conv->name = $conversation->conversationname;
@@ -1497,6 +1523,7 @@ class api {
        // Some restrictions we need to be aware of:
        // - Individual conversations containing soft-deleted user must be counted.
        // - Individual conversations containing only deleted messages must NOT be counted.
+        // - Individual conversations which are legacy 'self' conversations (2 members, both the same user) must NOT be counted.
        // - Group conversations with 0 messages must be counted.
        // - Linked conversations which are disabled (enabled = 0) must NOT be counted.
        // - Any type of conversation can be included in the favourites count, however, the type counts and the favourites count
@@ -1512,6 +1539,17 @@ class api {
                  FROM {message_conversations} mc
            INNER JOIN {message_conversation_members} mcm
                    ON mcm.conversationid = mc.id
+            INNER JOIN (
+                              SELECT mcm.conversationid, count(distinct mcm.userid) as membercount
+                                FROM {message_conversation_members} mcm
+                               WHERE mcm.conversationid IN (
+                                        SELECT DISTINCT conversationid
+                                          FROM {message_conversation_members} mcm2
+                                         WHERE userid = :userid5
+                                     )
+                            GROUP BY mcm.conversationid
+                       ) uniquemembercount
+                    ON uniquemembercount.conversationid = mc.id
             LEFT JOIN (
                              SELECT m.conversationid as convid, MAX(m.timecreated) as maxtime
                                FROM {messages} m
@@ -1527,7 +1565,10 @@ class api {
               $favsql
                 WHERE mcm.userid = :userid3
                   AND mc.enabled = :enabled
-                   AND ((mc.type = :individualtype AND maxvisibleconvmessage.convid IS NOT NULL) OR (mc.type = :grouptype))
+                   AND (
+                          (mc.type = :individualtype AND maxvisibleconvmessage.convid IS NOT NULL AND membercount > 1) OR
+                          (mc.type = :grouptype)
+                       )
              GROUP BY mc.type, fav.itemtype
              ORDER BY mc.type ASC";

@@ -1536,6 +1577,7 @@ class api {
            'userid2' => $userid,
            'userid3' => $userid,
            'userid4' => $userid,
+            'userid5' => $userid,
            'action' => self::MESSAGE_ACTION_DELETED,
            'enabled' => self::MESSAGE_CONVERSATION_ENABLED,
            'individualtype' => self::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
@@ -1364,6 +1364,31 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
        $conversations = \core_message\api::get_conversations($user1->id, 0, 20, 0);
    }

+    /**
+     * Tests retrieving conversations when a legacy 'self' conversation exists.
+     */
+    public function test_get_conversations_legacy_self_conversations() {
+        global $DB;
+
+        // Create a legacy conversation between one user and themself.
+        $user1 = self::getDataGenerator()->create_user();
+        $conversation = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
+            [$user1->id, $user1->id]);
+        testhelper::send_fake_message_to_conversation($user1, $conversation->id, 'Test message to self!');
+
+        // Verify we are in a 'self' conversation state.
+        $members = $DB->get_records('message_conversation_members', ['conversationid' => $conversation->id]);
+        $this->assertCount(2, $members);
+        $member = array_pop($members);
+        $this->assertEquals($user1->id, $member->userid);
+        $member = array_pop($members);
+        $this->assertEquals($user1->id, $member->userid);
+
+        // Verify this conversation is not returned by the method.
+        $conversations = \core_message\api::get_conversations($user1->id);
+        $this->assertCount(0, $conversations);
+    }
+
    /**
     * Tests retrieving conversations when a conversation contains a deleted user.
     */
@@ -5681,7 +5706,7 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
    public function test_get_conversation_counts_test_cases() {
        $typeindividual = \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL;
        $typegroup = \core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP;
-        list($user1, $user2, $user3, $user4, $user5, $user6, $user7) = [0, 1, 2, 3, 4, 5, 6];
+        list($user1, $user2, $user3, $user4, $user5, $user6, $user7, $user8) = [0, 1, 2, 3, 4, 5, 6, 7];
        $conversations = [
            [
                'type' => $typeindividual,
@@ -5711,6 +5736,13 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
                'favourites' => [$user6],
                'enabled' => false
            ],
+            [
+                'type' => $typeindividual,
+                'users' => [$user8, $user8],
+                'messages' => [$user8, $user8],
+                'favourites' => [],
+                'enabled' => null // Individual conversations cannot be disabled.
+            ],
        ];

        return [
@@ -5901,6 +5933,17 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
                ]],
                'deletedusers' => []
            ],
+            'Conversation with self' => [
+                'conversationConfigs' => $conversations,
+                'deletemessagesuser' => null,
+                'deletemessages' => [],
+                'arguments' => [$user8],
+                'expected' => ['favourites' => 0, 'types' => [
+                    \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL => 0,
+                    \core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP => 0
+                ]],
+                'deletedusers' => []
+            ],
        ];
    }

@@ -5931,6 +5974,7 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
            $generator->create_user(),
            $generator->create_user(),
            $generator->create_user(),
+            $generator->create_user(),
            $generator->create_user()
        ];

@@ -5289,6 +5289,35 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
        core_message_external::get_conversations($user1->id, 0, 20, 0);
    }

+    /**
+     * Tests retrieving conversations when a legacy 'self' conversation exists.
+     */
+    public function test_get_conversations_legacy_self_conversations() {
+        global $DB;
+        $this->resetAfterTest();
+
+        // Create a legacy conversation between one user and themself.
+        $user1 = self::getDataGenerator()->create_user();
+        $conversation = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
+            [$user1->id, $user1->id]);
+        testhelper::send_fake_message_to_conversation($user1, $conversation->id, 'Test message to self!');
+
+        // Verify we are in a 'self' conversation state.
+        $members = $DB->get_records('message_conversation_members', ['conversationid' => $conversation->id]);
+        $this->assertCount(2, $members);
+        $member = array_pop($members);
+        $this->assertEquals($user1->id, $member->userid);
+        $member = array_pop($members);
+        $this->assertEquals($user1->id, $member->userid);
+
+        // Verify this conversation is not returned by the method.
+        $this->setUser($user1);
+        $result = core_message_external::get_conversations($user1->id, 0, 20);
+        $result = external_api::clean_returnvalue(core_message_external::get_conversations_returns(), $result);
+        $conversations = $result['conversations'];
+        $this->assertCount(0, $conversations);
+    }
+
    /**
     * Tests retrieving conversations when a conversation contains a deleted user.
     */
@@ -6109,7 +6138,7 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
    public function test_get_conversation_counts_test_cases() {
        $typeindividual = \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL;
        $typegroup = \core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP;
-        list($user1, $user2, $user3, $user4, $user5, $user6, $user7) = [0, 1, 2, 3, 4, 5, 6];
+        list($user1, $user2, $user3, $user4, $user5, $user6, $user7, $user8) = [0, 1, 2, 3, 4, 5, 6, 7];
        $conversations = [
            [
                'type' => $typeindividual,
@@ -6139,6 +6168,13 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
                'favourites' => [$user6],
                'enabled' => false
            ],
+            [
+                'type' => $typeindividual,
+                'users' => [$user8, $user8],
+                'messages' => [$user8, $user8],
+                'favourites' => [],
+                'enabled' => null // Individual conversations cannot be disabled.
+            ],
        ];

        return [
@@ -6329,6 +6365,17 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
                ]],
                'deletedusers' => []
            ],
+            'Conversation with self' => [
+                'conversationConfigs' => $conversations,
+                'deletemessagesuser' => null,
+                'deletemessages' => [],
+                'arguments' => [$user8],
+                'expected' => ['favourites' => 0, 'types' => [
+                    \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL => 0,
+                    \core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP => 0
+                ]],
+                'deletedusers' => []
+            ],
        ];
    }

@@ -6360,6 +6407,7 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
            $generator->create_user(),
            $generator->create_user(),
            $generator->create_user(),
+            $generator->create_user(),
            $generator->create_user()
        ];