From 14de10c4637a22516fedbc7fdb262ca6cf9734b5 Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Sun, 14 Oct 2018 19:15:30 +0800 Subject: [PATCH 1/7] MDL-63547 core_message: updated message_deleted event to support groups --- lib/classes/event/message_deleted.php | 64 ++++++++++++--------------- lib/upgrade.txt | 3 ++ message/classes/api.php | 27 ++++------- message/tests/events_test.php | 26 +++++------ 4 files changed, 51 insertions(+), 69 deletions(-) diff --git a/lib/classes/event/message_deleted.php b/lib/classes/event/message_deleted.php index b991853b309..ab38448b1db 100644 --- a/lib/classes/event/message_deleted.php +++ b/lib/classes/event/message_deleted.php @@ -33,8 +33,6 @@ defined('MOODLE_INTERNAL') || die(); * Extra information about event. * * - int messageid: the id of the message. - * - int useridfrom: the id of the user who received the message. - * - int useridto: the id of the user who sent the message. * } * * @package core @@ -47,32 +45,22 @@ class message_deleted extends base { /** * Create event using ids. * - * @param int $userfromid the user who the message was from. - * @param int $usertoid the user who the message was sent to. - * @param int $userdeleted the user who deleted it. + * @param int $userid the user who the we are deleting the message for. + * @param int $userdeleting the user who deleted it (it's possible that an admin may delete a message on someones behalf) * @param int $messageid the id of the message that was deleted. - * @param int $muaid The id in the message_user_actions table + * @param int $muaid The id in the message_user_actions table. * @return message_deleted */ - public static function create_from_ids($userfromid, $usertoid, $userdeleted, $messageid, $muaid) { - // Check who was deleting the message. - if ($userdeleted == $userfromid) { - $relateduserid = $usertoid; - } else { - $relateduserid = $userfromid; - } - + public static function create_from_ids(int $userid, int $userdeleting, int $messageid, int $muaid) : message_deleted { // We set the userid to the user who deleted the message, nothing to do // with whether or not they sent or received the message. $event = self::create(array( 'objectid' => $muaid, - 'userid' => $userdeleted, + 'userid' => $userdeleting, 'context' => \context_system::instance(), - 'relateduserid' => $relateduserid, + 'relateduserid' => $userid, 'other' => array( 'messageid' => $messageid, - 'useridfrom' => $userfromid, - 'useridto' => $usertoid ) )); @@ -103,14 +91,28 @@ class message_deleted extends base { * @return string */ public function get_description() { - // Check if the person who deleted the message received or sent it. - if ($this->userid == $this->other['useridto']) { - $str = 'from'; - } else { - $str = 'to'; + // This is for BC when the event used to take this value into account before group conversations. + // We still want the same message to display for older events. + if (isset($this->other['useridto'])) { + // Check if the person who deleted the message received or sent it. + if ($this->userid == $this->other['useridto']) { + $str = 'from'; + } else { + $str = 'to'; + } + + return "The user with id '$this->userid' deleted a message sent $str the user with id '$this->relateduserid'."; } - return "The user with id '$this->userid' deleted a message sent $str the user with id '$this->relateduserid'."; + $messageid = $this->other['messageid']; + + // Check if the user deleting the message was not the actual user we are deleting for. + $str = "The user with id '$this->userid' deleted a message with id '$messageid'"; + if ($this->userid != $this->relateduserid) { + $str .= " for the user with id '$this->relateduserid'"; + } + + return $str; } /** @@ -129,14 +131,6 @@ class message_deleted extends base { if (!isset($this->other['messageid'])) { throw new \coding_exception('The \'messageid\' value must be set in other.'); } - - if (!isset($this->other['useridfrom'])) { - throw new \coding_exception('The \'useridfrom\' value must be set in other.'); - } - - if (!isset($this->other['useridto'])) { - throw new \coding_exception('The \'useridto\' value must be set in other.'); - } } public static function get_objectid_mapping() { @@ -145,9 +139,9 @@ class message_deleted extends base { public static function get_other_mapping() { // Messages are not backed up, so no need to map them on restore. - $othermapped = array(); - $othermapped['useridfrom'] = array('db' => 'user', 'restore' => base::NOT_MAPPED); - $othermapped['useridto'] = array('db' => 'user', 'restore' => base::NOT_MAPPED); + $othermapped = []; + $othermapped['messageid'] = ['db' => 'messages', 'restore' => base::NOT_MAPPED]; + return $othermapped; } } diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 288ed76e1bc..6561f6525cd 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -135,6 +135,9 @@ the groupid field. - message_contact_unblocked The reason for this is because you can now block/unblock users without them necessarily being a contact. These events have been replaced with message_user_blocked and message_user_unblocked respectively. +* The event message_deleted has been changed, it no longer records the value of the 'useridto' due to + the introduction of group messaging. Please, if you have any observers or are triggering this event + in your code you will have to make some changes! === 3.5 === diff --git a/message/classes/api.php b/message/classes/api.php index 8bc93a74288..d8b07950b4e 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -686,13 +686,8 @@ class api { $mua->timecreated = time(); $mua->id = $DB->insert_record('message_user_actions', $mua); - if ($message->useridfrom == $userid) { - $useridto = $otheruserid; - } else { - $useridto = $userid; - } - \core\event\message_deleted::create_from_ids($message->useridfrom, $useridto, - $USER->id, $message->id, $mua->id)->trigger(); + \core\event\message_deleted::create_from_ids($userid, $USER->id, + $message->id, $mua->id)->trigger(); } return true; @@ -1243,17 +1238,11 @@ class api { * @return bool */ public static function delete_message($userid, $messageid) { - global $DB; + global $DB, $USER; - $sql = "SELECT m.id, m.useridfrom, mcm.userid as useridto - FROM {messages} m - INNER JOIN {message_conversations} mc - ON m.conversationid = mc.id - INNER JOIN {message_conversation_members} mcm - ON mcm.conversationid = mc.id - WHERE mcm.userid != m.useridfrom - AND m.id = ?"; - $message = $DB->get_record_sql($sql, [$messageid], MUST_EXIST); + if (!$DB->record_exists('messages', ['id' => $messageid])) { + return false; + } // Check if the user has already deleted this message. if (!$DB->record_exists('message_user_actions', ['userid' => $userid, @@ -1266,8 +1255,8 @@ class api { $mua->id = $DB->insert_record('message_user_actions', $mua); // Trigger event for deleting a message. - \core\event\message_deleted::create_from_ids($message->useridfrom, $message->useridto, - $userid, $message->id, $mua->id)->trigger(); + \core\event\message_deleted::create_from_ids($userid, $USER->id, + $messageid, $mua->id)->trigger(); return true; } diff --git a/message/tests/events_test.php b/message/tests/events_test.php index 25660ce5054..973eddf2a43 100644 --- a/message/tests/events_test.php +++ b/message/tests/events_test.php @@ -274,7 +274,9 @@ class core_message_events_testcase extends core_message_messagelib_testcase { * Test the message deleted event. */ public function test_message_deleted() { - global $DB; + global $DB, $USER; + + $this->setAdminUser(); // Create users to send messages between. $user1 = $this->getDataGenerator()->create_user(); @@ -294,12 +296,12 @@ class core_message_events_testcase extends core_message_messagelib_testcase { // Check that the event data is valid. $this->assertInstanceOf('\core\event\message_deleted', $event); - $this->assertEquals($user1->id, $event->userid); // The user who deleted it. - $this->assertEquals($user2->id, $event->relateduserid); + $this->assertEquals($USER->id, $event->userid); // The user who deleted it. + $this->assertEquals($user1->id, $event->relateduserid); $this->assertEquals($mua->id, $event->objectid); $this->assertEquals($messageid, $event->other['messageid']); - $this->assertEquals($user1->id, $event->other['useridfrom']); - $this->assertEquals($user2->id, $event->other['useridto']); + + $this->setUser($user1); // Create a read message. $messageid = $this->send_fake_message($user1, $user2); @@ -318,12 +320,10 @@ class core_message_events_testcase extends core_message_messagelib_testcase { // Check that the event data is valid. $this->assertInstanceOf('\core\event\message_deleted', $event); - $this->assertEquals($user2->id, $event->userid); - $this->assertEquals($user1->id, $event->relateduserid); + $this->assertEquals($user1->id, $event->userid); + $this->assertEquals($user2->id, $event->relateduserid); $this->assertEquals($mua->id, $event->objectid); $this->assertEquals($messageid, $event->other['messageid']); - $this->assertEquals($user1->id, $event->other['useridfrom']); - $this->assertEquals($user2->id, $event->other['useridto']); } /** @@ -336,7 +336,7 @@ class core_message_events_testcase extends core_message_messagelib_testcase { $user1 = self::getDataGenerator()->create_user(); $user2 = self::getDataGenerator()->create_user(); - // The person doing the search. + // The person doing the deletion. $this->setUser($user1); // Send some messages back and forth. @@ -383,18 +383,14 @@ class core_message_events_testcase extends core_message_messagelib_testcase { // Check that the event data is valid. $i = 1; foreach ($events as $event) { - $useridfromid = ($i % 2 == 0) ? $user2->id : $user1->id; - $useridtoid = ($i % 2 == 0) ? $user1->id : $user2->id; $messageid = $messages[$i - 1]; $this->assertInstanceOf('\core\event\message_deleted', $event); $this->assertEquals($muatest[$messageid]->id, $event->objectid); $this->assertEquals($user1->id, $event->userid); - $this->assertEquals($user2->id, $event->relateduserid); + $this->assertEquals($user1->id, $event->relateduserid); $this->assertEquals($messageid, $event->other['messageid']); - $this->assertEquals($useridfromid, $event->other['useridfrom']); - $this->assertEquals($useridtoid, $event->other['useridto']); $i++; } From 08cb8a34f999830c9722abd928f4c72a4f8b91fd Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Sun, 14 Oct 2018 19:48:13 +0800 Subject: [PATCH 2/7] MDL-63547 core_message: updated api::can_delete_message --- message/classes/api.php | 45 +++++++++++++++++------------- message/tests/api_test.php | 25 +++++++++++++++++ message/tests/externallib_test.php | 2 +- 3 files changed, 51 insertions(+), 21 deletions(-) diff --git a/message/classes/api.php b/message/classes/api.php index d8b07950b4e..4320e25ea89 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -1197,30 +1197,20 @@ class api { public static function can_delete_message($userid, $messageid) { global $DB, $USER; - $sql = "SELECT m.id, m.useridfrom, mcm.userid as useridto - FROM {messages} m - INNER JOIN {message_conversations} mc - ON m.conversationid = mc.id - INNER JOIN {message_conversation_members} mcm - ON mcm.conversationid = mc.id - WHERE mcm.userid != m.useridfrom - AND m.id = ?"; - $message = $DB->get_record_sql($sql, [$messageid], MUST_EXIST); + $systemcontext = \context_system::instance(); - if ($message->useridfrom == $userid) { - $userdeleting = 'useridfrom'; - } else if ($message->useridto == $userid) { - $userdeleting = 'useridto'; - } else { + $conversationid = $DB->get_field('messages', 'conversationid', ['id' => $messageid], MUST_EXIST); + + if (has_capability('moodle/site:deleteanymessage', $systemcontext)) { + return true; + } + + if (!self::is_user_in_conversation($userid, $conversationid)) { return false; } - $systemcontext = \context_system::instance(); - - // Let's check if the user is allowed to delete this message. - if (has_capability('moodle/site:deleteanymessage', $systemcontext) || - ((has_capability('moodle/site:deleteownmessage', $systemcontext) && - $USER->id == $message->$userdeleting))) { + if (has_capability('moodle/site:deleteownmessage', $systemcontext) && + $USER->id == $userid) { return true; } @@ -1601,4 +1591,19 @@ class api { OR (mcr.userid = ? AND mcr.requesteduserid = ?)"; return $DB->record_exists_sql($sql, [$userid, $requesteduserid, $requesteduserid, $userid]); } + + /** + * Checks if a user is already in a conversation. + * + * @param int $userid The id of the user we want to check if they are in a group + * @param int $conversationid The id of the conversation + * @return bool Returns true if a contact request exists, false otherwise + */ + public static function is_user_in_conversation(int $userid, int $conversationid) : bool { + global $DB; + + return $DB->record_exists('message_conversation_members', ['conversationid' => $conversationid, + 'userid' => $userid]); + + } } diff --git a/message/tests/api_test.php b/message/tests/api_test.php index 0278d679352..f9f1cfc15c2 100644 --- a/message/tests/api_test.php +++ b/message/tests/api_test.php @@ -2226,6 +2226,31 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertTrue(\core_message\api::does_contact_request_exist($user2->id, $user1->id)); } + /** + * Test the user in conversation check. + */ + public function test_is_user_in_conversation() { + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + + $conversationid = \core_message\api::create_conversation_between_users([$user1->id, $user2->id]); + + $this->assertTrue(\core_message\api::is_user_in_conversation($user1->id, $conversationid)); + } + + /** + * Test the user in conversation check when they are not. + */ + public function test_is_user_in_conversation_when_not() { + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + $user3 = self::getDataGenerator()->create_user(); + + $conversationid = \core_message\api::create_conversation_between_users([$user1->id, $user2->id]); + + $this->assertFalse(\core_message\api::is_user_in_conversation($user3->id, $conversationid)); + } + /** * Comparison function for sorting contacts. * diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index 784ad149ee2..6b503e5c7de 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -1648,7 +1648,7 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $result = core_message_external::delete_message(-1, $user1->id); $this->fail('Exception expected due invalid messageid.'); } catch (dml_missing_record_exception $e) { - $this->assertEquals('invalidrecordunknown', $e->errorcode); + $this->assertEquals('invalidrecord', $e->errorcode); } // Invalid user. From 263ad98436c27eb36406667159a3c482117f86e2 Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Sun, 14 Oct 2018 14:03:03 +0800 Subject: [PATCH 3/7] MDL-63547 core_message: deprecated api::delete_conversation() --- message/classes/api.php | 23 ++++++++++++++-- message/externallib.php | 5 +++- message/tests/api_test.php | 52 +++++++++++++++++++++++++++++++++++ message/tests/events_test.php | 1 + message/upgrade.txt | 1 + 5 files changed, 78 insertions(+), 4 deletions(-) diff --git a/message/classes/api.php b/message/classes/api.php index 4320e25ea89..f4e260e231a 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -650,13 +650,15 @@ class api { * * This function does not verify any permissions. * + * @deprecated since 3.6 * @param int $userid The user id of who we want to delete the messages for (this may be done by the admin * but will still seem as if it was by the user) * @param int $otheruserid The id of the other user in the conversation * @return bool */ public static function delete_conversation($userid, $otheruserid) { - global $DB, $USER; + debugging('\core_message\api::delete_conversation() is deprecated, please use ' . + '\core_message\api::delete_conversation_by_id() instead.', DEBUG_DEVELOPER); $conversationid = self::get_conversation_between_users([$userid, $otheruserid]); @@ -665,6 +667,23 @@ class api { return true; } + self::delete_conversation_by_id($userid, $conversationid); + + return true; + } + + /** + * Deletes a conversation for a specified user. + * + * This function does not verify any permissions. + * + * @param int $userid The user id of who we want to delete the messages for (this may be done by the admin + * but will still seem as if it was by the user) + * @param int $conversationid The id of the other user in the conversation + */ + public static function delete_conversation_by_id(int $userid, int $conversationid) { + global $DB, $USER; + // Get all messages belonging to this conversation that have not already been deleted by this user. $sql = "SELECT m.* FROM {messages} m @@ -689,8 +708,6 @@ class api { \core\event\message_deleted::create_from_ids($userid, $USER->id, $message->id, $mua->id)->trigger(); } - - return true; } /** diff --git a/message/externallib.php b/message/externallib.php index f678c2f2732..30f056beb89 100644 --- a/message/externallib.php +++ b/message/externallib.php @@ -2588,7 +2588,10 @@ class core_message_external extends external_api { core_user::require_active_user($user); if (\core_message\api::can_delete_conversation($user->id)) { - $status = \core_message\api::delete_conversation($user->id, $otheruserid); + if ($conversationid = \core_message\api::get_conversation_between_users([$userid, $otheruserid])) { + \core_message\api::delete_conversation_by_id($user->id, $conversationid); + } + $status = true; } else { throw new moodle_exception('You do not have permission to delete messages'); } diff --git a/message/tests/api_test.php b/message/tests/api_test.php index f9f1cfc15c2..ed4a27266ae 100644 --- a/message/tests/api_test.php +++ b/message/tests/api_test.php @@ -1181,6 +1181,58 @@ class core_message_api_testcase extends core_message_messagelib_testcase { // Delete the conversation as user 1. \core_message\api::delete_conversation($user1->id, $user2->id); + $this->assertDebuggingCalled(); + + $muas = $DB->get_records('message_user_actions', array(), 'timecreated ASC'); + $this->assertCount(4, $muas); + // Sort by id. + ksort($muas); + + $mua1 = array_shift($muas); + $mua2 = array_shift($muas); + $mua3 = array_shift($muas); + $mua4 = array_shift($muas); + + $this->assertEquals($user1->id, $mua1->userid); + $this->assertEquals($m1id, $mua1->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua1->action); + + $this->assertEquals($user1->id, $mua2->userid); + $this->assertEquals($m2id, $mua2->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua2->action); + + $this->assertEquals($user1->id, $mua3->userid); + $this->assertEquals($m3id, $mua3->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua3->action); + + $this->assertEquals($user1->id, $mua4->userid); + $this->assertEquals($m4id, $mua4->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua4->action); + } + + /** + * Tests deleting a conversation by conversation id. + */ + public function test_delete_conversation_by_id() { + global $DB; + + // Create some users. + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + + // The person doing the search. + $this->setUser($user1); + + // Send some messages back and forth. + $time = 1; + $m1id = $this->send_fake_message($user1, $user2, 'Yo!', 0, $time + 1); + $m2id = $this->send_fake_message($user2, $user1, 'Sup mang?', 0, $time + 2); + $m3id = $this->send_fake_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 3); + $m4id = $this->send_fake_message($user2, $user1, 'Word.', 0, $time + 4); + + // Delete the conversation as user 1. + $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]); + \core_message\api::delete_conversation_by_id($user1->id, $conversationid); $muas = $DB->get_records('message_user_actions', array(), 'timecreated ASC'); $this->assertCount(4, $muas); diff --git a/message/tests/events_test.php b/message/tests/events_test.php index 973eddf2a43..714461b9fce 100644 --- a/message/tests/events_test.php +++ b/message/tests/events_test.php @@ -364,6 +364,7 @@ class core_message_events_testcase extends core_message_messagelib_testcase { // Trigger and capture the event. $sink = $this->redirectEvents(); \core_message\api::delete_conversation($user1->id, $user2->id); + $this->assertDebuggingCalled(); $events = $sink->get_events(); // Get the user actions for the messages deleted by that user. diff --git a/message/upgrade.txt b/message/upgrade.txt index 2ada6522aee..aad4d002a39 100644 --- a/message/upgrade.txt +++ b/message/upgrade.txt @@ -28,6 +28,7 @@ information provided here is intended especially for developers. Please see their declaration in lib/deprecatedlib.php to view their alternatives (if applicable). * The following methods have been deprecated and should not be used any more: - \core_message\api::is_user_blocked() + - \core_message\api::delete_conversation() * The following web services have been deprecated. Please do not call these any more. - core_message_external::block_contacts, please use core_message_external::block_user instead. - core_message_external::unblock_contacts, please use core_message_external::unblock_user instead. From 15663b0bec15b0bbd538a16bc6c9ba325960a394 Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Sun, 14 Oct 2018 21:00:01 +0800 Subject: [PATCH 4/7] MDL-63547 core_message: can_delete_conversation expects a conversationid --- message/classes/api.php | 23 ++++++++++++++++++----- message/externallib.php | 10 ++++++---- message/tests/api_test.php | 15 ++++++++++++--- message/tests/externallib_test.php | 7 +++++++ message/upgrade.txt | 2 ++ 5 files changed, 45 insertions(+), 12 deletions(-) diff --git a/message/classes/api.php b/message/classes/api.php index f4e260e231a..092f47c2990 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -628,17 +628,30 @@ class api { * * @param int $userid The user id of who we want to delete the messages for (this may be done by the admin * but will still seem as if it was by the user) + * @param int $conversationid The id of the conversation * @return bool Returns true if a user can delete the conversation, false otherwise. */ - public static function can_delete_conversation($userid) { + public static function can_delete_conversation(int $userid, int $conversationid = null) : bool { global $USER; + if (is_null($conversationid)) { + debugging('\core_message\api::can_delete_conversation() now expects a \'conversationid\' to be passed.', + DEBUG_DEVELOPER); + return false; + } + $systemcontext = \context_system::instance(); - // Let's check if the user is allowed to delete this conversation. - if (has_capability('moodle/site:deleteanymessage', $systemcontext) || - ((has_capability('moodle/site:deleteownmessage', $systemcontext) && - $USER->id == $userid))) { + if (has_capability('moodle/site:deleteanymessage', $systemcontext)) { + return true; + } + + if (!self::is_user_in_conversation($userid, $conversationid)) { + return false; + } + + if (has_capability('moodle/site:deleteownmessage', $systemcontext) && + $USER->id == $userid) { return true; } diff --git a/message/externallib.php b/message/externallib.php index 30f056beb89..32185bab3c1 100644 --- a/message/externallib.php +++ b/message/externallib.php @@ -2587,10 +2587,12 @@ class core_message_external extends external_api { $user = core_user::get_user($params['userid'], '*', MUST_EXIST); core_user::require_active_user($user); - if (\core_message\api::can_delete_conversation($user->id)) { - if ($conversationid = \core_message\api::get_conversation_between_users([$userid, $otheruserid])) { - \core_message\api::delete_conversation_by_id($user->id, $conversationid); - } + if (!$conversationid = \core_message\api::get_conversation_between_users([$userid, $otheruserid])) { + return []; + } + + if (\core_message\api::can_delete_conversation($user->id, $conversationid)) { + \core_message\api::delete_conversation_by_id($user->id, $conversationid); $status = true; } else { throw new moodle_exception('You do not have permission to delete messages'); diff --git a/message/tests/api_test.php b/message/tests/api_test.php index ed4a27266ae..f3253e6eb83 100644 --- a/message/tests/api_test.php +++ b/message/tests/api_test.php @@ -1146,17 +1146,26 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $user1 = self::getDataGenerator()->create_user(); $user2 = self::getDataGenerator()->create_user(); + // Send some messages back and forth. + $time = 1; + $this->send_fake_message($user1, $user2, 'Yo!', 0, $time + 1); + $this->send_fake_message($user2, $user1, 'Sup mang?', 0, $time + 2); + $this->send_fake_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 3); + $this->send_fake_message($user2, $user1, 'Word.', 0, $time + 4); + + $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]); + // The admin can do anything. - $this->assertTrue(\core_message\api::can_delete_conversation($user1->id)); + $this->assertTrue(\core_message\api::can_delete_conversation($user1->id, $conversationid)); // Set as the user 1. $this->setUser($user1); // They can delete their own messages. - $this->assertTrue(\core_message\api::can_delete_conversation($user1->id)); + $this->assertTrue(\core_message\api::can_delete_conversation($user1->id, $conversationid)); // They can't delete someone elses. - $this->assertFalse(\core_message\api::can_delete_conversation($user2->id)); + $this->assertFalse(\core_message\api::can_delete_conversation($user2->id, $conversationid)); } /** diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index 6b503e5c7de..584f15685ca 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -3445,6 +3445,13 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $user2 = self::getDataGenerator()->create_user(); $user3 = self::getDataGenerator()->create_user(); + // Send some messages back and forth. + $time = time(); + $this->send_message($user1, $user2, 'Yo!', 0, $time); + $this->send_message($user2, $user1, 'Sup mang?', 0, $time + 1); + $this->send_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 2); + $this->send_message($user2, $user1, 'Word.', 0, $time + 3); + // The person wanting to delete the conversation. $this->setUser($user3); diff --git a/message/upgrade.txt b/message/upgrade.txt index aad4d002a39..c95b7348bfc 100644 --- a/message/upgrade.txt +++ b/message/upgrade.txt @@ -29,6 +29,8 @@ information provided here is intended especially for developers. * The following methods have been deprecated and should not be used any more: - \core_message\api::is_user_blocked() - \core_message\api::delete_conversation() +* The method \core_message\api::can_delete_conversation() now expects a 'conversationid' to be passed + as the second parameter. * The following web services have been deprecated. Please do not call these any more. - core_message_external::block_contacts, please use core_message_external::block_user instead. - core_message_external::unblock_contacts, please use core_message_external::unblock_user instead. From fa024820f28ff38f3c0fbcaf1f173eec5e845a9e Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Mon, 15 Oct 2018 17:42:15 +0800 Subject: [PATCH 5/7] MDL-63547 core_message: deprecate delete_conversation web service --- lib/db/services.php | 3 ++- message/externallib.php | 13 ++++++++++++- message/upgrade.txt | 7 ++++--- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/db/services.php b/lib/db/services.php index 10bd788f3bd..36bdc0794f4 100644 --- a/lib/db/services.php +++ b/lib/db/services.php @@ -917,7 +917,8 @@ $functions = array( 'classname' => 'core_message_external', 'methodname' => 'delete_conversation', 'classpath' => 'message/externallib.php', - 'description' => 'Deletes a conversation.', + 'description' => '** DEPRECATED ** Please do not call this function any more. + Deletes a conversation.', 'type' => 'write', 'capabilities' => 'moodle/site:deleteownmessage', 'ajax' => true, diff --git a/message/externallib.php b/message/externallib.php index 32185bab3c1..e5073b5296e 100644 --- a/message/externallib.php +++ b/message/externallib.php @@ -2541,6 +2541,7 @@ class core_message_external extends external_api { /** * Returns description of method parameters. * + * @deprecated since 3.6 * @return external_function_parameters * @since 3.2 */ @@ -2556,6 +2557,7 @@ class core_message_external extends external_api { /** * Deletes a conversation. * + * @deprecated since 3.6 * @param int $userid The user id of who we want to delete the conversation for * @param int $otheruserid The user id of the other user in the conversation * @return array @@ -2609,6 +2611,7 @@ class core_message_external extends external_api { /** * Returns description of method result value. * + * @deprecated since 3.6 * @return external_description * @since 3.2 */ @@ -2621,6 +2624,15 @@ class core_message_external extends external_api { ); } + /** + * Marking the method as deprecated. + * + * @return bool + */ + public static function delete_conversation_is_deprecated() { + return true; + } + /** * Returns description of method parameters * @@ -3017,7 +3029,6 @@ class core_message_external extends external_api { ); } - /** * Returns description of method parameters * diff --git a/message/upgrade.txt b/message/upgrade.txt index c95b7348bfc..969e3b7bfda 100644 --- a/message/upgrade.txt +++ b/message/upgrade.txt @@ -32,9 +32,10 @@ information provided here is intended especially for developers. * The method \core_message\api::can_delete_conversation() now expects a 'conversationid' to be passed as the second parameter. * The following web services have been deprecated. Please do not call these any more. - - core_message_external::block_contacts, please use core_message_external::block_user instead. - - core_message_external::unblock_contacts, please use core_message_external::unblock_user instead. - - core_message_external::create_contacts, please use core_message_external::create_contact_request instead. + - core_message_external::block_contacts(), please use core_message_external::block_user() instead. + - core_message_external::unblock_contacts(), please use core_message_external::unblock_user() instead. + - core_message_external::create_contacts(), please use core_message_external::create_contact_request() instead. + - core_message_external::delete_conversation(), please use core_message_external::delete_conversation_by_id() instead. === 3.5 === From 26f39c88618cecca0107235b27bb74d38bc3fe10 Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Mon, 15 Oct 2018 17:43:21 +0800 Subject: [PATCH 6/7] MDL-63547 core_message: added delete_conversation_by_id web service --- lib/db/services.php | 10 ++ message/externallib.php | 65 +++++++++++ message/tests/externallib_test.php | 167 +++++++++++++++++++++++++++++ version.php | 2 +- 4 files changed, 243 insertions(+), 1 deletion(-) diff --git a/lib/db/services.php b/lib/db/services.php index 36bdc0794f4..033de846610 100644 --- a/lib/db/services.php +++ b/lib/db/services.php @@ -924,6 +924,16 @@ $functions = array( 'ajax' => true, 'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE), ), + 'core_message_delete_conversation_by_id' => array( + 'classname' => 'core_message_external', + 'methodname' => 'delete_conversation_by_id', + 'classpath' => 'message/externallib.php', + 'description' => 'Deletes a conversation.', + 'type' => 'write', + 'capabilities' => 'moodle/site:deleteownmessage', + 'ajax' => true, + 'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE), + ), 'core_message_delete_message' => array( 'classname' => 'core_message_external', 'methodname' => 'delete_message', diff --git a/message/externallib.php b/message/externallib.php index e5073b5296e..a8d0f0c167a 100644 --- a/message/externallib.php +++ b/message/externallib.php @@ -2633,6 +2633,71 @@ class core_message_external extends external_api { return true; } + /** + * Returns description of method parameters. + * + * @return external_function_parameters + * @since 3.6 + */ + public static function delete_conversation_by_id_parameters() { + return new external_function_parameters( + array( + 'userid' => new external_value(PARAM_INT, 'The user id of who we want to delete the conversation for'), + 'conversationid' => new external_value(PARAM_INT, 'The id of the conversation'), + ) + ); + } + + /** + * Deletes a conversation. + * + * @param int $userid The user id of who we want to delete the conversation for + * @param int $conversationid The id of the conversation + * @return array + * @throws moodle_exception + * @since 3.6 + */ + public static function delete_conversation_by_id($userid, $conversationid) { + global $CFG; + + // Check if private messaging between users is allowed. + if (empty($CFG->messaging)) { + throw new moodle_exception('disabled', 'message'); + } + + // Validate params. + $params = [ + 'userid' => $userid, + 'conversationid' => $conversationid, + ]; + $params = self::validate_parameters(self::delete_conversation_by_id_parameters(), $params); + + // Validate context. + $context = context_system::instance(); + self::validate_context($context); + + $user = core_user::get_user($params['userid'], '*', MUST_EXIST); + core_user::require_active_user($user); + + if (\core_message\api::can_delete_conversation($user->id, $conversationid)) { + \core_message\api::delete_conversation_by_id($user->id, $conversationid); + } else { + throw new moodle_exception("You do not have permission to delete the conversation '$conversationid'"); + } + + return []; + } + + /** + * Returns description of method result value. + * + * @return external_description + * @since 3.6 + */ + public static function delete_conversation_by_id_returns() { + return new external_warnings(); + } + /** * Returns description of method parameters * diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index 584f15685ca..92dae3f1948 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -3483,6 +3483,173 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { core_message_external::delete_conversation($user1->id, $user2->id); } + /** + * Test deleting conversation. + */ + public function test_delete_conversation_by_id() { + global $DB; + + $this->resetAfterTest(true); + + // Create some users. + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + + // The person wanting to delete the conversation. + $this->setUser($user1); + + // Send some messages back and forth. + $time = time(); + $m1id = $this->send_message($user1, $user2, 'Yo!', 0, $time); + $m2id = $this->send_message($user2, $user1, 'Sup mang?', 0, $time + 1); + $m3id = $this->send_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 2); + $m4id = $this->send_message($user2, $user1, 'Word.', 0, $time + 3); + + $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]); + + // Delete the conversation. + core_message_external::delete_conversation_by_id($user1->id, $conversationid); + + $muas = $DB->get_records('message_user_actions', array(), 'timecreated ASC'); + $this->assertCount(4, $muas); + // Sort by id. + ksort($muas); + + $mua1 = array_shift($muas); + $mua2 = array_shift($muas); + $mua3 = array_shift($muas); + $mua4 = array_shift($muas); + + $this->assertEquals($user1->id, $mua1->userid); + $this->assertEquals($m1id, $mua1->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua1->action); + + $this->assertEquals($user1->id, $mua2->userid); + $this->assertEquals($m2id, $mua2->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua2->action); + + $this->assertEquals($user1->id, $mua3->userid); + $this->assertEquals($m3id, $mua3->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua3->action); + + $this->assertEquals($user1->id, $mua4->userid); + $this->assertEquals($m4id, $mua4->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua4->action); + } + + /** + * Test deleting conversation as other user. + */ + public function test_delete_conversation_by_id_as_other_user() { + global $DB; + + $this->resetAfterTest(true); + + $this->setAdminUser(); + + // Create some users. + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + + // Send some messages back and forth. + $time = time(); + $m1id = $this->send_message($user1, $user2, 'Yo!', 0, $time); + $m2id = $this->send_message($user2, $user1, 'Sup mang?', 0, $time + 1); + $m3id = $this->send_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 2); + $m4id = $this->send_message($user2, $user1, 'Word.', 0, $time + 3); + + $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]); + + // Delete the conversation. + core_message_external::delete_conversation_by_id($user1->id, $conversationid); + + $muas = $DB->get_records('message_user_actions', array(), 'timecreated ASC'); + $this->assertCount(4, $muas); + // Sort by id. + ksort($muas); + + $mua1 = array_shift($muas); + $mua2 = array_shift($muas); + $mua3 = array_shift($muas); + $mua4 = array_shift($muas); + + $this->assertEquals($user1->id, $mua1->userid); + $this->assertEquals($m1id, $mua1->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua1->action); + + $this->assertEquals($user1->id, $mua2->userid); + $this->assertEquals($m2id, $mua2->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua2->action); + + $this->assertEquals($user1->id, $mua3->userid); + $this->assertEquals($m3id, $mua3->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua3->action); + + $this->assertEquals($user1->id, $mua4->userid); + $this->assertEquals($m4id, $mua4->messageid); + $this->assertEquals(\core_message\api::MESSAGE_ACTION_DELETED, $mua4->action); + } + + /** + * Test deleting conversation as other user without proper capability. + */ + public function test_delete_conversation_by_id_as_other_user_without_cap() { + $this->resetAfterTest(true); + + // Create some users. + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + $user3 = self::getDataGenerator()->create_user(); + + // Send some messages back and forth. + $time = time(); + $this->send_message($user1, $user2, 'Yo!', 0, $time); + $this->send_message($user2, $user1, 'Sup mang?', 0, $time + 1); + $this->send_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 2); + $this->send_message($user2, $user1, 'Word.', 0, $time + 3); + + $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]); + + // The person wanting to delete the conversation. + $this->setUser($user3); + + // Ensure an exception is thrown. + $this->expectException('moodle_exception'); + core_message_external::delete_conversation_by_id($user1->id, $conversationid); + } + + /** + * Test deleting conversation with messaging disabled. + */ + public function test_delete_conversation_by_id_messaging_disabled() { + global $CFG; + + $this->resetAfterTest(true); + + // Create some users. + $user1 = self::getDataGenerator()->create_user(); + $user2 = self::getDataGenerator()->create_user(); + + // Send some messages back and forth. + $time = time(); + $this->send_message($user1, $user2, 'Yo!', 0, $time); + $this->send_message($user2, $user1, 'Sup mang?', 0, $time + 1); + $this->send_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 2); + $this->send_message($user2, $user1, 'Word.', 0, $time + 3); + + $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]); + + // The person wanting to delete the conversation. + $this->setUser($user1); + + // Disable messaging. + $CFG->messaging = 0; + + // Ensure an exception is thrown. + $this->expectException('moodle_exception'); + core_message_external::delete_conversation_by_id($user1->id, $conversationid); + } + /** * Test get message processor. */ diff --git a/version.php b/version.php index 7f3abd4d453..4627b7a75af 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2018101100.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2018101100.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. From 60b67bbcf7c3d4a889693f94fe46168065de6966 Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Mon, 15 Oct 2018 12:20:23 +0800 Subject: [PATCH 7/7] MDL-63547 core_message: web service can delete multiple conversations --- lib/db/services.php | 6 +++--- message/externallib.php | 27 ++++++++++++++++----------- message/tests/externallib_test.php | 24 ++++++++++++------------ message/upgrade.txt | 2 +- 4 files changed, 32 insertions(+), 27 deletions(-) diff --git a/lib/db/services.php b/lib/db/services.php index 033de846610..f187293dfdd 100644 --- a/lib/db/services.php +++ b/lib/db/services.php @@ -924,11 +924,11 @@ $functions = array( 'ajax' => true, 'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE), ), - 'core_message_delete_conversation_by_id' => array( + 'core_message_delete_conversations_by_id' => array( 'classname' => 'core_message_external', - 'methodname' => 'delete_conversation_by_id', + 'methodname' => 'delete_conversations_by_id', 'classpath' => 'message/externallib.php', - 'description' => 'Deletes a conversation.', + 'description' => 'Deletes a list of conversations.', 'type' => 'write', 'capabilities' => 'moodle/site:deleteownmessage', 'ajax' => true, diff --git a/message/externallib.php b/message/externallib.php index a8d0f0c167a..dec1cd573a2 100644 --- a/message/externallib.php +++ b/message/externallib.php @@ -2639,11 +2639,14 @@ class core_message_external extends external_api { * @return external_function_parameters * @since 3.6 */ - public static function delete_conversation_by_id_parameters() { + public static function delete_conversations_by_id_parameters() { return new external_function_parameters( array( 'userid' => new external_value(PARAM_INT, 'The user id of who we want to delete the conversation for'), - 'conversationid' => new external_value(PARAM_INT, 'The id of the conversation'), + 'conversationids' => new external_multiple_structure( + new external_value(PARAM_INT, 'The id of the conversation'), + 'List of conversation IDs' + ), ) ); } @@ -2652,12 +2655,12 @@ class core_message_external extends external_api { * Deletes a conversation. * * @param int $userid The user id of who we want to delete the conversation for - * @param int $conversationid The id of the conversation + * @param int[] $conversationids The ids of the conversations * @return array * @throws moodle_exception * @since 3.6 */ - public static function delete_conversation_by_id($userid, $conversationid) { + public static function delete_conversations_by_id($userid, array $conversationids) { global $CFG; // Check if private messaging between users is allowed. @@ -2668,9 +2671,9 @@ class core_message_external extends external_api { // Validate params. $params = [ 'userid' => $userid, - 'conversationid' => $conversationid, + 'conversationids' => $conversationids, ]; - $params = self::validate_parameters(self::delete_conversation_by_id_parameters(), $params); + $params = self::validate_parameters(self::delete_conversations_by_id_parameters(), $params); // Validate context. $context = context_system::instance(); @@ -2679,10 +2682,12 @@ class core_message_external extends external_api { $user = core_user::get_user($params['userid'], '*', MUST_EXIST); core_user::require_active_user($user); - if (\core_message\api::can_delete_conversation($user->id, $conversationid)) { - \core_message\api::delete_conversation_by_id($user->id, $conversationid); - } else { - throw new moodle_exception("You do not have permission to delete the conversation '$conversationid'"); + foreach ($conversationids as $conversationid) { + if (\core_message\api::can_delete_conversation($user->id, $conversationid)) { + \core_message\api::delete_conversation_by_id($user->id, $conversationid); + } else { + throw new moodle_exception("You do not have permission to delete the conversation '$conversationid'"); + } } return []; @@ -2694,7 +2699,7 @@ class core_message_external extends external_api { * @return external_description * @since 3.6 */ - public static function delete_conversation_by_id_returns() { + public static function delete_conversations_by_id_returns() { return new external_warnings(); } diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index 92dae3f1948..16d38817f27 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -3484,9 +3484,9 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { } /** - * Test deleting conversation. + * Test deleting conversations. */ - public function test_delete_conversation_by_id() { + public function test_delete_conversations_by_id() { global $DB; $this->resetAfterTest(true); @@ -3508,7 +3508,7 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]); // Delete the conversation. - core_message_external::delete_conversation_by_id($user1->id, $conversationid); + core_message_external::delete_conversations_by_id($user1->id, [$conversationid]); $muas = $DB->get_records('message_user_actions', array(), 'timecreated ASC'); $this->assertCount(4, $muas); @@ -3538,9 +3538,9 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { } /** - * Test deleting conversation as other user. + * Test deleting conversations as other user. */ - public function test_delete_conversation_by_id_as_other_user() { + public function test_delete_conversations_by_id_as_other_user() { global $DB; $this->resetAfterTest(true); @@ -3561,7 +3561,7 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]); // Delete the conversation. - core_message_external::delete_conversation_by_id($user1->id, $conversationid); + core_message_external::delete_conversations_by_id($user1->id, [$conversationid]); $muas = $DB->get_records('message_user_actions', array(), 'timecreated ASC'); $this->assertCount(4, $muas); @@ -3591,9 +3591,9 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { } /** - * Test deleting conversation as other user without proper capability. + * Test deleting conversations as other user without proper capability. */ - public function test_delete_conversation_by_id_as_other_user_without_cap() { + public function test_delete_conversations_by_id_as_other_user_without_cap() { $this->resetAfterTest(true); // Create some users. @@ -3615,13 +3615,13 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { // Ensure an exception is thrown. $this->expectException('moodle_exception'); - core_message_external::delete_conversation_by_id($user1->id, $conversationid); + core_message_external::delete_conversations_by_id($user1->id, [$conversationid]); } /** - * Test deleting conversation with messaging disabled. + * Test deleting conversations with messaging disabled. */ - public function test_delete_conversation_by_id_messaging_disabled() { + public function test_delete_conversations_by_id_messaging_disabled() { global $CFG; $this->resetAfterTest(true); @@ -3647,7 +3647,7 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { // Ensure an exception is thrown. $this->expectException('moodle_exception'); - core_message_external::delete_conversation_by_id($user1->id, $conversationid); + core_message_external::delete_conversations_by_id($user1->id, [$conversationid]); } /** diff --git a/message/upgrade.txt b/message/upgrade.txt index 969e3b7bfda..c543dc8f2b4 100644 --- a/message/upgrade.txt +++ b/message/upgrade.txt @@ -35,7 +35,7 @@ information provided here is intended especially for developers. - core_message_external::block_contacts(), please use core_message_external::block_user() instead. - core_message_external::unblock_contacts(), please use core_message_external::unblock_user() instead. - core_message_external::create_contacts(), please use core_message_external::create_contact_request() instead. - - core_message_external::delete_conversation(), please use core_message_external::delete_conversation_by_id() instead. + - core_message_external::delete_conversation(), please use core_message_external::delete_conversations_by_id() instead. === 3.5 ===