From ad5de40c497b07399d96a93653c6cc1da439ff7d Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 23 Nov 2018 10:25:46 +0800 Subject: [PATCH 1/4] MDL-64167 core_message: fix for sending bulk messages from site admin This ensures the following: - An admin/manager can send a message to all users, including themself, without any errors. --- lib/classes/message/manager.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/classes/message/manager.php b/lib/classes/message/manager.php index 95bf8c7facf..774a5907180 100644 --- a/lib/classes/message/manager.php +++ b/lib/classes/message/manager.php @@ -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."); From cb38961988473decaca3a18a10b42a8aad0dcb5c Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 23 Nov 2018 12:28:06 +0800 Subject: [PATCH 2/4] MDL-64167 core_message: get_conversations() handles self conversations Those individual conversations created with one's self (via admin user bulk actions) are now supported in get_conversations(). These had two records with the same userid in the message_conversation_members table. The following adjustments have been made to accomodate these: - Member count adjusted to read 1, not 2 for 'self' conversations. - Member information for the current user now returned for 'self' conversations. - The method now tracks 'self' conversations via $selfconversations. --- message/classes/api.php | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/message/classes/api.php b/message/classes/api.php index d5792239e13..776ca9248c2 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -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"; From 425f5adcca26d41799244f9b1a479aed8490286c Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 23 Nov 2018 12:39:23 +0800 Subject: [PATCH 3/4] MDL-64167 core_message: get_conversations() excludes self conversations The UI can't handle these, so for now, let's not return them. --- message/classes/api.php | 5 +++++ message/tests/api_test.php | 25 +++++++++++++++++++++++++ message/tests/externallib_test.php | 29 +++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/message/classes/api.php b/message/classes/api.php index 776ca9248c2..b4422be3435 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -759,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; diff --git a/message/tests/api_test.php b/message/tests/api_test.php index 6f8d2669998..65dfb4cd2b0 100644 --- a/message/tests/api_test.php +++ b/message/tests/api_test.php @@ -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. */ diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index 2d7dea85ac5..cb1959e4920 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -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. */ From 7f6f45c460da39f0aeef4f794d30fdb341ee613b Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 23 Nov 2018 13:45:09 +0800 Subject: [PATCH 4/4] MDL-64167 core_message: fix counts to exclude 'self' conversations --- message/classes/api.php | 18 +++++++++++++++++- message/tests/api_test.php | 21 ++++++++++++++++++++- message/tests/externallib_test.php | 21 ++++++++++++++++++++- 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/message/classes/api.php b/message/classes/api.php index b4422be3435..19a2fa68ceb 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -1523,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 @@ -1538,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 @@ -1553,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"; @@ -1562,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, diff --git a/message/tests/api_test.php b/message/tests/api_test.php index 65dfb4cd2b0..5370fc3b8c2 100644 --- a/message/tests/api_test.php +++ b/message/tests/api_test.php @@ -5706,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, @@ -5736,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 [ @@ -5926,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' => [] + ], ]; } @@ -5956,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() ]; diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index cb1959e4920..fbfeb35fc94 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -6138,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, @@ -6168,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 [ @@ -6358,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' => [] + ], ]; } @@ -6389,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() ];