diff --git a/message/classes/api.php b/message/classes/api.php index 43fa325726f..7885baa702f 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -561,10 +561,11 @@ class api { $conversationset = $DB->get_recordset_sql($sql, $params, $limitfrom, $limitnum); $conversations = []; - $uniquemembers = []; $members = []; + $individualmembers = []; + $groupmembers = []; foreach ($conversationset as $conversation) { - $conversations[] = $conversation; + $conversations[$conversation->id] = $conversation; $members[$conversation->id] = []; } $conversationset->close(); @@ -600,7 +601,7 @@ class api { if ($conversation->conversationtype == self::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL) { if (!is_null($conversation->useridfrom) && $conversation->useridfrom != $userid) { $members[$conversation->id][$conversation->useridfrom] = $conversation->useridfrom; - $uniquemembers[$conversation->useridfrom] = $conversation->useridfrom; + $individualmembers[$conversation->useridfrom] = $conversation->useridfrom; } else { $individualconversations[] = $conversation->id; } @@ -608,7 +609,7 @@ class api { // If we have a recent message, the sender is our member. if (!is_null($conversation->useridfrom)) { $members[$conversation->id][$conversation->useridfrom] = $conversation->useridfrom; - $uniquemembers[$conversation->useridfrom] = $conversation->useridfrom; + $groupmembers[$conversation->useridfrom] = $conversation->useridfrom; } } } @@ -626,10 +627,9 @@ class api { foreach ($conversationmembers as $mid => $member) { $members[$member->conversationid][$member->userid] = $member->userid; - $uniquemembers[$member->userid] = $member->userid; + $individualmembers[$member->userid] = $member->userid; } } - $memberids = array_values($uniquemembers); // We could fail early here if we're sure that: // a) we have no otherusers for all the conversations (users may have been deleted) @@ -639,8 +639,17 @@ class api { // needs to be done in a separate query to avoid doing a join on the messages tables and the user // tables because on large sites these tables are massive which results in extremely slow // performance (typically due to join buffer exhaustion). - if (!empty($memberids)) { - $memberinfo = helper::get_member_info($userid, $memberids); + if (!empty($individualmembers) || !empty($groupmembers)) { + // Now, we want to remove any duplicates from the group members array. For individual members we will + // be doing a more extensive call as we want their contact requests as well as privacy information, + // which is not necessary for group conversations. + $diffgroupmembers = array_diff($groupmembers, $individualmembers); + + $individualmemberinfo = helper::get_member_info($userid, $individualmembers, true, true); + $groupmemberinfo = helper::get_member_info($userid, $diffgroupmembers); + + // Don't use array_merge, as we lose array keys. + $memberinfo = $individualmemberinfo + $groupmemberinfo; // Update the members array with the member information. $deletedmembers = []; @@ -651,7 +660,15 @@ class api { if ($memberinfo[$memberid]->isdeleted) { $deletedmembers[$convid][] = $memberid; } - $members[$convid][$key] = $memberinfo[$memberid]; + + $members[$convid][$key] = clone $memberinfo[$memberid]; + + if ($conversations[$convid]->conversationtype == self::MESSAGE_CONVERSATION_TYPE_GROUP) { + // Remove data we don't need for group. + $members[$convid][$key]->requirescontact = null; + $members[$convid][$key]->canmessage = null; + $members[$convid][$key]->contactrequests = []; + } } } } @@ -2490,25 +2507,7 @@ class api { if ($members = $DB->get_records('message_conversation_members', ['conversationid' => $conversationid], 'timecreated ASC, id ASC', 'userid', $limitfrom, $limitnum)) { $userids = array_keys($members); - $members = helper::get_member_info($userid, $userids); - - // Check if we want to include contact requests as well. - if ($includecontactrequests) { - list($useridsql, $usersparams) = $DB->get_in_or_equal($userids); - - $wheresql = "(userid $useridsql OR requesteduserid $useridsql)"; - if ($contactrequests = $DB->get_records_select('message_contact_requests', $wheresql, - array_merge($usersparams, $usersparams), 'timecreated ASC, id ASC')) { - foreach ($contactrequests as $contactrequest) { - if (isset($members[$contactrequest->userid])) { - $members[$contactrequest->userid]->contactrequests[] = $contactrequest; - } - if (isset($members[$contactrequest->requesteduserid])) { - $members[$contactrequest->requesteduserid]->contactrequests[] = $contactrequest; - } - } - } - } + $members = helper::get_member_info($userid, $userids, $includecontactrequests); return $members; } diff --git a/message/classes/helper.php b/message/classes/helper.php index 732268434ab..5af85f18277 100644 --- a/message/classes/helper.php +++ b/message/classes/helper.php @@ -482,13 +482,22 @@ class helper { * * @param int $referenceuserid the id of the user which check contact and blocked status. * @param array $userids + * @param bool $includecontactrequests Do we want to include contact requests with this data? + * @param bool $includeprivacyinfo Do we want to include whether the user can message another, and if the user + * requires a contact. * @return array the array of objects containing member info, indexed by userid. * @throws \coding_exception * @throws \dml_exception */ - public static function get_member_info(int $referenceuserid, array $userids) : array { + public static function get_member_info(int $referenceuserid, array $userids, bool $includecontactrequests = false, + bool $includeprivacyinfo = false) : array { global $DB, $PAGE; + // Prevent exception being thrown when array is empty. + if (empty($userids)) { + return []; + } + list($useridsql, $usersparams) = $DB->get_in_or_equal($userids); $userfields = \user_picture::fields('u', array('lastaccess')); $userssql = "SELECT $userfields, u.deleted, mc.id AS contactid, mub.id AS blockedid @@ -525,8 +534,46 @@ class helper { $data->isdeleted = ($member->deleted) ? true : false; + $data->requirescontact = null; + $data->canmessage = null; + if ($includeprivacyinfo) { + $privacysetting = api::get_user_privacy_messaging_preference($member->id); + $data->requirescontact = $privacysetting == api::MESSAGE_PRIVACY_ONLYCONTACTS; + + $recipient = new \stdClass(); + $recipient->id = $member->id; + + $sender = new \stdClass(); + $sender->id = $referenceuserid; + + $data->canmessage = api::can_post_message($recipient, $sender); + } + + // Populate the contact requests, even if we don't need them. + $data->contactrequests = []; + $members[$data->id] = $data; } + + // Check if we want to include contact requests as well. + if (!empty($members) && $includecontactrequests) { + list($useridsql, $usersparams) = $DB->get_in_or_equal($userids); + + $wheresql = "(userid $useridsql AND requesteduserid = ?) OR (userid = ? AND requesteduserid $useridsql)"; + $params = array_merge($usersparams, [$referenceuserid, $referenceuserid], $usersparams); + if ($contactrequests = $DB->get_records_select('message_contact_requests', $wheresql, $params, + 'timecreated ASC, id ASC')) { + foreach ($contactrequests as $contactrequest) { + if (isset($members[$contactrequest->userid])) { + $members[$contactrequest->userid]->contactrequests[] = $contactrequest; + } + if (isset($members[$contactrequest->requesteduserid])) { + $members[$contactrequest->requesteduserid]->contactrequests[] = $contactrequest; + } + } + } + } + return $members; } diff --git a/message/externallib.php b/message/externallib.php index af3740a426d..8a126bb26ca 100644 --- a/message/externallib.php +++ b/message/externallib.php @@ -964,7 +964,7 @@ class core_message_external extends external_api { 'unreadcount' => new external_value(PARAM_INT, 'The number of unread messages in this conversation', VALUE_DEFAULT, null), 'members' => new external_multiple_structure( - self::get_conversation_member_structure() + self::get_conversation_member_structure(true) ), 'messages' => new external_multiple_structure( self::get_conversation_message_structure() @@ -992,6 +992,8 @@ class core_message_external extends external_api { 'showonlinestatus' => new external_value(PARAM_BOOL, 'Show the user\'s online status?'), 'isblocked' => new external_value(PARAM_BOOL, 'If the user has been blocked'), 'iscontact' => new external_value(PARAM_BOOL, 'Is the user a contact?'), + 'canmessage' => new external_value(PARAM_BOOL, 'If the user can be messaged'), + 'requirescontact' => new external_value(PARAM_BOOL, 'If the user requires to be contacts'), ]; if ($includecontactrequests) { diff --git a/message/tests/api_test.php b/message/tests/api_test.php index dceda6f79cb..29be0fa2f57 100644 --- a/message/tests/api_test.php +++ b/message/tests/api_test.php @@ -1089,6 +1089,9 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertObjectHasAttribute('showonlinestatus', $member); $this->assertObjectHasAttribute('isblocked', $member); $this->assertObjectHasAttribute('iscontact', $member); + $this->assertObjectHasAttribute('canmessage', $member); + $this->assertObjectHasAttribute('requirescontact', $member); + $this->assertObjectHasAttribute('contactrequests', $member); } $this->assertObjectHasAttribute('messages', $conv); foreach ($conv->messages as $message) { @@ -1322,6 +1325,65 @@ class core_message_api_testcase extends core_message_messagelib_testcase { } } + /** + * Test verifying get_conversations when there are users in a group and/or individual conversation. The reason this + * test is performed is because we do not need as much data for group conversations (saving DB calls), so we want + * to confirm this happens. + */ + public function test_get_conversations_user_in_group_and_individual_chat() { + $this->resetAfterTest(); + + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + $user3 = self::getDataGenerator()->create_user(); + + $conversation = \core_message\api::create_conversation( + \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, + [ + $user1->id, + $user2->id + ], + 'Individual conversation' + ); + + testhelper::send_fake_message_to_conversation($user1, $conversation->id); + + $conversation = \core_message\api::create_conversation( + \core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, + [ + $user1->id, + $user2->id, + ], + 'Group conversation' + ); + + testhelper::send_fake_message_to_conversation($user1, $conversation->id); + + \core_message\api::create_contact_request($user1->id, $user2->id); + \core_message\api::create_contact_request($user1->id, $user3->id); + + $conversations = \core_message\api::get_conversations($user2->id); + + $groupconversation = array_shift($conversations); + $individualconversation = array_shift($conversations); + + $this->assertEquals('Group conversation', $groupconversation->name); + $this->assertEquals('Individual conversation', $individualconversation->name); + + $this->assertCount(1, $groupconversation->members); + $this->assertCount(1, $individualconversation->members); + + $groupmember = reset($groupconversation->members); + $this->assertNull($groupmember->requirescontact); + $this->assertNull($groupmember->canmessage); + $this->assertEmpty($groupmember->contactrequests); + + $individualmember = reset($individualconversation->members); + $this->assertNotNull($individualmember->requirescontact); + $this->assertNotNull($individualmember->canmessage); + $this->assertNotEmpty($individualmember->contactrequests); + } + /** * Test verifying that group linked conversations are returned and contain a subname matching the course name. */ @@ -4450,7 +4512,8 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertEquals(true, $member1->showonlinestatus); $this->assertEquals(false, $member1->iscontact); $this->assertEquals(false, $member1->isblocked); - $this->assertObjectNotHasAttribute('contactrequests', $member1); + $this->assertObjectHasAttribute('contactrequests', $member1); + $this->assertEmpty($member1->contactrequests); $this->assertEquals($user2->id, $member2->id); $this->assertEquals(fullname($user2), $member2->fullname); @@ -4458,7 +4521,8 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertEquals(true, $member2->showonlinestatus); $this->assertEquals(true, $member2->iscontact); $this->assertEquals(false, $member2->isblocked); - $this->assertObjectNotHasAttribute('contactrequests', $member2); + $this->assertObjectHasAttribute('contactrequests', $member2); + $this->assertEmpty($member2->contactrequests); $this->assertEquals($user3->id, $member3->id); $this->assertEquals(fullname($user3), $member3->fullname); @@ -4466,7 +4530,8 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertEquals(true, $member3->showonlinestatus); $this->assertEquals(false, $member3->iscontact); $this->assertEquals(true, $member3->isblocked); - $this->assertObjectNotHasAttribute('contactrequests', $member3); + $this->assertObjectHasAttribute('contactrequests', $member3); + $this->assertEmpty($member3->contactrequests); } /** @@ -4521,7 +4586,7 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertEquals(true, $member1->showonlinestatus); $this->assertEquals(false, $member1->iscontact); $this->assertEquals(false, $member1->isblocked); - $this->assertCount(3, $member1->contactrequests); + $this->assertCount(2, $member1->contactrequests); $this->assertEquals($user2->id, $member2->id); $this->assertEquals(fullname($user2), $member2->fullname); @@ -4529,7 +4594,7 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertEquals(true, $member2->showonlinestatus); $this->assertEquals(true, $member2->iscontact); $this->assertEquals(false, $member2->isblocked); - $this->assertCount(2, $member2->contactrequests); + $this->assertCount(1, $member2->contactrequests); $this->assertEquals($user3->id, $member3->id); $this->assertEquals(fullname($user3), $member3->fullname); @@ -4537,12 +4602,11 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertEquals(true, $member3->showonlinestatus); $this->assertEquals(false, $member3->iscontact); $this->assertEquals(true, $member3->isblocked); - $this->assertCount(2, $member3->contactrequests); + $this->assertCount(1, $member3->contactrequests); // Confirm the contact requests are OK. $request1 = array_shift($member1->contactrequests); $request2 = array_shift($member1->contactrequests); - $request3 = array_shift($member1->contactrequests); $this->assertEquals($user1->id, $request1->userid); $this->assertEquals($user2->id, $request1->requesteduserid); @@ -4550,26 +4614,15 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertEquals($user1->id, $request2->userid); $this->assertEquals($user3->id, $request2->requesteduserid); - $this->assertEquals($user1->id, $request3->userid); - $this->assertEquals($user4->id, $request3->requesteduserid); - $request1 = array_shift($member2->contactrequests); - $request2 = array_shift($member2->contactrequests); $this->assertEquals($user1->id, $request1->userid); $this->assertEquals($user2->id, $request1->requesteduserid); - $this->assertEquals($user2->id, $request2->userid); - $this->assertEquals($user3->id, $request2->requesteduserid); - $request1 = array_shift($member3->contactrequests); - $request2 = array_shift($member3->contactrequests); $this->assertEquals($user1->id, $request1->userid); $this->assertEquals($user3->id, $request1->requesteduserid); - - $this->assertEquals($user2->id, $request2->userid); - $this->assertEquals($user3->id, $request2->requesteduserid); } /** diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index 0781adc416f..8c9678ce0ae 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -4781,6 +4781,9 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertArrayHasKey('showonlinestatus', $member); $this->assertArrayHasKey('isblocked', $member); $this->assertArrayHasKey('iscontact', $member); + $this->assertArrayHasKey('canmessage', $member); + $this->assertArrayHasKey('requirescontact', $member); + $this->assertArrayHasKey('contactrequests', $member); } $this->assertArrayHasKey('messages', $conv); foreach ($conv['messages'] as $message) { @@ -5076,6 +5079,68 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals($groupimageurl, $conversations[0]['imageurl']); } + /** + * Test verifying get_conversations when there are users in a group and/or individual conversation. The reason this + * test is performed is because we do not need as much data for group conversations (saving DB calls), so we want + * to confirm this happens. + */ + public function test_get_conversations_user_in_group_and_individual_chat() { + $this->resetAfterTest(); + + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + $user3 = self::getDataGenerator()->create_user(); + + $conversation = \core_message\api::create_conversation( + \core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, + [ + $user1->id, + $user2->id + ], + 'Individual conversation' + ); + + testhelper::send_fake_message_to_conversation($user1, $conversation->id); + + $conversation = \core_message\api::create_conversation( + \core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, + [ + $user1->id, + $user2->id, + ], + 'Group conversation' + ); + + testhelper::send_fake_message_to_conversation($user1, $conversation->id); + + \core_message\api::create_contact_request($user1->id, $user2->id); + \core_message\api::create_contact_request($user1->id, $user3->id); + + $this->setUser($user2); + $result = core_message_external::get_conversations($user2->id); + $result = external_api::clean_returnvalue(core_message_external::get_conversations_returns(), $result); + $conversations = $result['conversations']; + + $groupconversation = array_shift($conversations); + $individualconversation = array_shift($conversations); + + $this->assertEquals('Group conversation', $groupconversation['name']); + $this->assertEquals('Individual conversation', $individualconversation['name']); + + $this->assertCount(1, $groupconversation['members']); + $this->assertCount(1, $individualconversation['members']); + + $groupmember = reset($groupconversation['members']); + $this->assertNull($groupmember['requirescontact']); + $this->assertNull($groupmember['canmessage']); + $this->assertEmpty($groupmember['contactrequests']); + + $individualmember = reset($individualconversation['members']); + $this->assertNotNull($individualmember['requirescontact']); + $this->assertNotNull($individualmember['canmessage']); + $this->assertNotEmpty($individualmember['contactrequests']); + } + /** * Test returning members in a conversation with no contact requests. */ @@ -5161,7 +5226,8 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals(true, $member1->showonlinestatus); $this->assertEquals(false, $member1->iscontact); $this->assertEquals(false, $member1->isblocked); - $this->assertObjectNotHasAttribute('contactrequests', $member1); + $this->assertObjectHasAttribute('contactrequests', $member1); + $this->assertEmpty($member1->contactrequests); $this->assertEquals($user2->id, $member2->id); $this->assertEquals(fullname($user2), $member2->fullname); @@ -5169,7 +5235,8 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals(true, $member2->showonlinestatus); $this->assertEquals(true, $member2->iscontact); $this->assertEquals(false, $member2->isblocked); - $this->assertObjectNotHasAttribute('contactrequests', $member2); + $this->assertObjectHasAttribute('contactrequests', $member2); + $this->assertEmpty($member2->contactrequests); $this->assertEquals($user3->id, $member3->id); $this->assertEquals(fullname($user3), $member3->fullname); @@ -5177,7 +5244,8 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals(true, $member3->showonlinestatus); $this->assertEquals(false, $member3->iscontact); $this->assertEquals(true, $member3->isblocked); - $this->assertObjectNotHasAttribute('contactrequests', $member3); + $this->assertObjectHasAttribute('contactrequests', $member3); + $this->assertEmpty($member3->contactrequests); } /** @@ -5236,7 +5304,7 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals(true, $member1->showonlinestatus); $this->assertEquals(false, $member1->iscontact); $this->assertEquals(false, $member1->isblocked); - $this->assertCount(3, $member1->contactrequests); + $this->assertCount(2, $member1->contactrequests); $this->assertEquals($user2->id, $member2->id); $this->assertEquals(fullname($user2), $member2->fullname); @@ -5244,7 +5312,7 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals(true, $member2->showonlinestatus); $this->assertEquals(true, $member2->iscontact); $this->assertEquals(false, $member2->isblocked); - $this->assertCount(2, $member2->contactrequests); + $this->assertCount(1, $member2->contactrequests); $this->assertEquals($user3->id, $member3->id); $this->assertEquals(fullname($user3), $member3->fullname); @@ -5252,12 +5320,11 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals(true, $member3->showonlinestatus); $this->assertEquals(false, $member3->iscontact); $this->assertEquals(true, $member3->isblocked); - $this->assertCount(2, $member3->contactrequests); + $this->assertCount(1, $member3->contactrequests); // Confirm the contact requests are OK. $request1 = array_shift($member1->contactrequests); $request2 = array_shift($member1->contactrequests); - $request3 = array_shift($member1->contactrequests); $this->assertEquals($user1->id, $request1->userid); $this->assertEquals($user2->id, $request1->requesteduserid); @@ -5265,26 +5332,15 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals($user1->id, $request2->userid); $this->assertEquals($user3->id, $request2->requesteduserid); - $this->assertEquals($user1->id, $request3->userid); - $this->assertEquals($user4->id, $request3->requesteduserid); - $request1 = array_shift($member2->contactrequests); - $request2 = array_shift($member2->contactrequests); $this->assertEquals($user1->id, $request1->userid); $this->assertEquals($user2->id, $request1->requesteduserid); - $this->assertEquals($user2->id, $request2->userid); - $this->assertEquals($user3->id, $request2->requesteduserid); - $request1 = array_shift($member3->contactrequests); - $request2 = array_shift($member3->contactrequests); $this->assertEquals($user1->id, $request1->userid); $this->assertEquals($user3->id, $request1->requesteduserid); - - $this->assertEquals($user2->id, $request2->userid); - $this->assertEquals($user3->id, $request2->requesteduserid); } /**