From 14de10c4637a22516fedbc7fdb262ca6cf9734b5 Mon Sep 17 00:00:00 2001
From: Mark Nelson <markn@moodle.com>
Date: Sun, 14 Oct 2018 19:15:30 +0800
Subject: [PATCH] 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++;
         }