From e926b4d0467f0f36f17c9ffae9e7312eec27f678 Mon Sep 17 00:00:00 2001
From: Andrew Nicols <andrew@nicols.co.uk>
Date: Mon, 12 Nov 2018 08:36:23 +0800
Subject: [PATCH] MDL-63968 message: Correct use of get_in_or_equal

---
 message/classes/api.php    | 10 +++----
 message/tests/api_test.php | 53 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/message/classes/api.php b/message/classes/api.php
index 43fa325726f..86a174b65ca 100644
--- a/message/classes/api.php
+++ b/message/classes/api.php
@@ -516,12 +516,10 @@ class api {
         // If we need to return ONLY favourites, or NO favourites, generate the SQL snippet.
         $favouritesql = "";
         $favouriteparams = [];
-        if (is_bool($favourites)) {
-            if (!empty($favouriteconversationids)) {
-                list ($insql, $inparams) = $DB->get_in_or_equal($favouriteconversationids, SQL_PARAMS_NAMED, 'favouriteids');
-                $favouritesql = $favourites ? " AND mc.id {$insql} " : " AND mc.id NOT {$insql} ";
-                $favouriteparams = $inparams;
-            }
+        if (null !== $favourites && !empty($favouriteconversationids)) {
+            list ($insql, $favouriteparams) =
+                    $DB->get_in_or_equal($favouriteconversationids, SQL_PARAMS_NAMED, 'favouriteids', $favourites);
+            $favouritesql = " AND mc.id {$insql} ";
         }
 
         // If we need to restrict type, generate the SQL snippet.
diff --git a/message/tests/api_test.php b/message/tests/api_test.php
index dceda6f79cb..163a4981c88 100644
--- a/message/tests/api_test.php
+++ b/message/tests/api_test.php
@@ -1271,6 +1271,51 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
         $this->assertNotContains($ic1->id, array_column($conversations, 'id'));
     }
 
+    /**
+     * Test verifying the behaviour of get_conversations() when fetching favourite conversations with only a single
+     * favourite.
+     */
+    public function test_get_conversations_favourite_conversations_single() {
+        // Get a bunch of conversations, some group, some individual and in different states.
+        list($user1, $user2, $user3, $user4, $ic1, $ic2, $ic3,
+            $gc1, $gc2, $gc3, $gc4, $gc5, $gc6) = $this->create_conversation_test_data();
+
+        // Mark a single conversation as favourites.
+        \core_message\api::set_favourite_conversation($ic2->id, $user1->id);
+
+        // Get the conversation, first with no restrictions, confirming the favourite status of the conversations.
+        $conversations = \core_message\api::get_conversations($user1->id);
+        $this->assertCount(6, $conversations);
+        foreach ($conversations as $conv) {
+            if (in_array($conv->id, [$ic2->id])) {
+                $this->assertTrue($conv->isfavourite);
+            } else {
+                $this->assertFalse($conv->isfavourite);
+            }
+        }
+
+        // Now, get ONLY favourite conversations.
+        $conversations = \core_message\api::get_conversations($user1->id, 0, 20, null, true);
+        $this->assertCount(1, $conversations);
+        foreach ($conversations as $conv) {
+            $this->assertTrue($conv->isfavourite);
+            $this->assertEquals($ic2->id, $conv->id);
+        }
+
+        // Now, try ONLY favourites of type 'group'.
+        $conversations = \core_message\api::get_conversations($user1->id, 0, 20,
+            \core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, true);
+        $this->assertEmpty($conversations);
+
+        // And NO favourite conversations.
+        $conversations = \core_message\api::get_conversations($user1->id, 0, 20, null, false);
+        $this->assertCount(5, $conversations);
+        foreach ($conversations as $conv) {
+            $this->assertFalse($conv->isfavourite);
+            $this->assertNotEquals($ic2, $conv->id);
+        }
+    }
+
     /**
      * Test verifying the behaviour of get_conversations() when fetching favourite conversations.
      */
@@ -1289,13 +1334,16 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
         \core_message\api::set_favourite_conversation($ic1->id, $user1->id);
         \core_message\api::set_favourite_conversation($gc2->id, $user1->id);
         \core_message\api::set_favourite_conversation($gc5->id, $user1->id);
+        $favouriteids = [$ic1->id, $gc2->id, $gc5->id];
 
         // Get the conversations, first with no restrictions, confirming the favourite status of the conversations.
         $conversations = \core_message\api::get_conversations($user1->id);
         $this->assertCount(6, $conversations);
         foreach ($conversations as $conv) {
-            if (in_array($conv->id, [$ic1->id, $gc2->id, $gc5->id])) {
+            if (in_array($conv->id, $favouriteids)) {
                 $this->assertTrue($conv->isfavourite);
+            } else {
+                $this->assertFalse($conv->isfavourite);
             }
         }
 
@@ -1304,6 +1352,7 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
         $this->assertCount(3, $conversations);
         foreach ($conversations as $conv) {
             $this->assertTrue($conv->isfavourite);
+            $this->assertNotFalse(array_search($conv->id, $favouriteids));
         }
 
         // Now, try ONLY favourites of type 'group'.
@@ -1312,6 +1361,7 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
         $this->assertCount(2, $conversations);
         foreach ($conversations as $conv) {
             $this->assertTrue($conv->isfavourite);
+            $this->assertNotFalse(array_search($conv->id, [$gc2->id, $gc5->id]));
         }
 
         // And NO favourite conversations.
@@ -1319,6 +1369,7 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
         $this->assertCount(3, $conversations);
         foreach ($conversations as $conv) {
             $this->assertFalse($conv->isfavourite);
+            $this->assertFalse(array_search($conv->id, $favouriteids));
         }
     }