Merge branch 'MDL-63974-master' of https://github.com/snake/moodle
authorJun Pataleta <jun@moodle.com>
Thu, 15 Nov 2018 03:22:16 +0000 (11:22 +0800)
committerJun Pataleta <jun@moodle.com>
Thu, 15 Nov 2018 03:22:16 +0000 (11:22 +0800)
message/classes/api.php
message/externallib.php
message/tests/api_test.php
message/tests/externallib_test.php

index 291e012..abc917c 100644 (file)
@@ -218,23 +218,22 @@ class api {
         $excludeusers = array($userid, $CFG->siteguest);
         list($exclude, $excludeparams) = $DB->get_in_or_equal($excludeusers, SQL_PARAMS_NAMED, 'param', false);
 
-        $params = array('search' => '%' . $search . '%', 'userid1' => $userid, 'userid2' => $userid, 'userid3' => $userid);
-
         // Ok, let's search for contacts first.
         $contacts = array();
         $sql = "SELECT $ufields, mub.id as isuserblocked
-                FROM {user} u
-                JOIN {message_contacts} mc
-                  ON (u.id = mc.contactid AND mc.userid = :userid1) OR (u.id = mc.userid AND mc.contactid = :userid2)
-           LEFT JOIN {message_users_blocked} mub
-                  ON (mub.userid = :userid3 AND mub.blockeduserid = u.id)
-               WHERE u.deleted = 0
-                 AND u.confirmed = 1
-                 AND " . $DB->sql_like($fullname, ':search', false) . "
-                 AND u.id $exclude
-            ORDER BY " . $DB->sql_fullname();
-
-        if ($users = $DB->get_records_sql($sql, $params + $excludeparams, 0, $limitnum)) {
+                  FROM {user} u
+                  JOIN {message_contacts} mc
+                    ON u.id = mc.contactid
+             LEFT JOIN {message_users_blocked} mub
+                    ON (mub.userid = :userid2 AND mub.blockeduserid = u.id)
+                 WHERE mc.userid = :userid
+                   AND u.deleted = 0
+                   AND u.confirmed = 1
+                   AND " . $DB->sql_like($fullname, ':search', false) . "
+                   AND u.id $exclude
+              ORDER BY " . $DB->sql_fullname();
+        if ($users = $DB->get_records_sql($sql, array('userid' => $userid, 'userid2' => $userid,
+                'search' => '%' . $search . '%') + $excludeparams, 0, $limitnum)) {
             foreach ($users as $user) {
                 $user->blocked = $user->isuserblocked ? 1 : 0;
                 $contacts[] = helper::create_contact($user);
@@ -262,46 +261,22 @@ class api {
             }
         }
 
-        // Let's get those non-contacts.
+        // Let's get those non-contacts. Toast them gears boi.
+        // Note - you can only block contacts, so these users will not be blocked, so no need to get that
+        // extra detail from the database.
         $noncontacts = array();
-        if ($CFG->messagingallusers) {
-            // In case $CFG->messagingallusers is enabled, search for all users site-wide but are not user's contact.
-            $sql = "SELECT $ufields
-                      FROM {user} u
-                 LEFT JOIN {message_users_blocked} mub
-                        ON (mub.userid = :userid1 AND mub.blockeduserid = u.id)
-                     WHERE u.deleted = 0
-                       AND u.confirmed = 1
-                       AND " . $DB->sql_like($fullname, ':search', false) . "
-                       AND u.id $exclude
-                       AND NOT EXISTS (SELECT mc.id
-                                         FROM {message_contacts} mc
-                                        WHERE (mc.userid = u.id AND mc.contactid = :userid2)
-                                           OR (mc.userid = :userid3 AND mc.contactid = u.id))
-                  ORDER BY " . $DB->sql_fullname();
-        } else {
-            // In case $CFG->messagingallusers is disabled, search for users you have a conversation with.
-            // Messaging setting could change, so could exist an old conversation with users you cannot message anymore.
-            $sql = "SELECT $ufields, mub.id as isuserblocked
-                      FROM {user} u
-                 LEFT JOIN {message_users_blocked} mub
-                        ON (mub.userid = :userid1 AND mub.blockeduserid = u.id)
-                INNER JOIN {message_conversation_members} cm
-                        ON u.id = cm.userid
-                INNER JOIN {message_conversation_members} cm2
-                        ON cm.conversationid = cm2.conversationid AND cm2.userid = :userid
-                     WHERE u.deleted = 0
-                       AND u.confirmed = 1
-                       AND " . $DB->sql_like($fullname, ':search', false) . "
-                       AND u.id $exclude
-                       AND NOT EXISTS (SELECT mc.id
-                                         FROM {message_contacts} mc
-                                        WHERE (mc.userid = u.id AND mc.contactid = :userid2)
-                                           OR (mc.userid = :userid3 AND mc.contactid = u.id))
-                  ORDER BY " . $DB->sql_fullname();
-            $params['userid'] = $userid;
-        }
-        if ($users = $DB->get_records_sql($sql,  $params + $excludeparams, 0, $limitnum)) {
+        $sql = "SELECT $ufields
+                  FROM {user} u
+                 WHERE u.deleted = 0
+                   AND u.confirmed = 1
+                   AND " . $DB->sql_like($fullname, ':search', false) . "
+                   AND u.id $exclude
+                   AND u.id NOT IN (SELECT contactid
+                                      FROM {message_contacts}
+                                     WHERE userid = :userid)
+              ORDER BY " . $DB->sql_fullname();
+        if ($users = $DB->get_records_sql($sql,  array('userid' => $userid, 'search' => '%' . $search . '%') + $excludeparams,
+                0, $limitnum)) {
             foreach ($users as $user) {
                 $noncontacts[] = helper::create_contact($user);
             }
@@ -319,9 +294,14 @@ class api {
      * @param int $limitnum
      * @return array
      */
-    public static function message_search_users(int $userid, string $search, int $limitfrom = 0, int $limitnum = 1000) : array {
+    public static function message_search_users(int $userid, string $search, int $limitfrom = 0, int $limitnum = 20) : array {
         global $CFG, $DB;
 
+        // Check if messaging is enabled.
+        if (empty($CFG->messaging)) {
+            throw new \moodle_exception('disabled', 'message');
+        }
+
         // Used to search for contacts.
         $fullname = $DB->sql_fullname();
 
@@ -343,7 +323,7 @@ class api {
               ORDER BY " . $DB->sql_fullname();
         $foundusers = $DB->get_records_sql_menu($sql, $params + $excludeparams, $limitfrom, $limitnum);
 
-        $orderedcontacs = array();
+        $orderedcontacts = array();
         if (!empty($foundusers)) {
             $contacts = helper::get_member_info($userid, array_keys($foundusers));
             // The get_member_info returns an associative array, so is not ordered in the same way.
@@ -351,33 +331,38 @@ class api {
             foreach ($foundusers as $key => $value) {
                 $contact = $contacts[$key];
                 $contact->conversations = self::get_conversations_between_users($userid, $key, 0, 1000);
-                $orderedcontacs[] = $contact;
+                $orderedcontacts[] = $contact;
             }
         }
 
         // Let's get those non-contacts.
+        // If site wide messaging is enabled, we just fetch any matched users which are non-contacts.
         if ($CFG->messagingallusers) {
-            // In case $CFG->messagingallusers is enabled, search for all users site-wide but are not user's contact.
             $sql = "SELECT u.id
-                      FROM {user} u
-                     WHERE u.deleted = 0
-                       AND u.confirmed = 1
-                       AND " . $DB->sql_like($fullname, ':search', false) . "
-                       AND u.id $exclude
-                       AND NOT EXISTS (SELECT mc.id
-                                         FROM {message_contacts} mc
-                                        WHERE (mc.userid = u.id AND mc.contactid = :userid1)
-                                           OR (mc.userid = :userid2 AND mc.contactid = u.id))
-                  ORDER BY " . $DB->sql_fullname();
+                  FROM {user} u
+                 WHERE u.deleted = 0
+                   AND u.confirmed = 1
+                   AND " . $DB->sql_like($fullname, ':search', false) . "
+                   AND u.id $exclude
+                   AND NOT EXISTS (SELECT mc.id
+                                     FROM {message_contacts} mc
+                                    WHERE (mc.userid = u.id AND mc.contactid = :userid1)
+                                       OR (mc.userid = :userid2 AND mc.contactid = u.id))
+              ORDER BY " . $DB->sql_fullname();
+
+            $foundusers = $DB->get_records_sql($sql, $params + $excludeparams, $limitfrom, $limitnum);
         } else {
-            // In case $CFG->messagingallusers is disabled, search for users you have a conversation with.
-            // Messaging setting could change, so could exist an old conversation with users you cannot message anymore.
-            $sql = "SELECT u.id
+            require_once($CFG->dirroot . '/user/lib.php');
+            // If site-wide messaging is disabled, then we should only be able to search for users who we are allowed to see.
+            // Because we can't achieve all the required visibility checks in SQL, we'll iterate through the non-contact records
+            // and stop once we have enough matching the 'visible' criteria.
+            // TODO: MDL-63983 - Improve the performance of non-contact searches when site-wide messaging is disabled (default).
+
+            // Use a local generator to achieve this iteration.
+            $getnoncontactusers = function ($limitfrom = 0, $limitnum = 0) use($fullname, $exclude, $params, $excludeparams) {
+                global $DB;
+                $sql = "SELECT u.*
                       FROM {user} u
-                INNER JOIN {message_conversation_members} cm
-                        ON u.id = cm.userid
-                INNER JOIN {message_conversation_members} cm2
-                        ON cm.conversationid = cm2.conversationid AND cm2.userid = :userid
                      WHERE u.deleted = 0
                        AND u.confirmed = 1
                        AND " . $DB->sql_like($fullname, ':search', false) . "
@@ -387,11 +372,52 @@ class api {
                                         WHERE (mc.userid = u.id AND mc.contactid = :userid1)
                                            OR (mc.userid = :userid2 AND mc.contactid = u.id))
                   ORDER BY " . $DB->sql_fullname();
-            $params['userid'] = $userid;
+                while ($records = $DB->get_records_sql($sql, $params + $excludeparams, $limitfrom, $limitnum)) {
+                    yield $records;
+                    $limitfrom += $limitnum;
+                }
+            };
+
+            // Fetch in batches of $limitnum * 2 to improve the chances of matching a user without going back to the DB.
+            // The generator cannot function without a sensible limiter, so set one if this is not set.
+            $batchlimit = ($limitnum == 0) ? 20 : $limitnum;
+
+            // We need to make the offset param work with the generator.
+            // Basically, if we want to get say 10 records starting at the 40th record, we need to see 50 records and return only
+            // those after the 40th record. We can never pass the method's offset param to the generator as we need to manage the
+            // position within those valid records ourselves.
+            // See MDL-63983 dealing with performance improvements to this area of code.
+            $noofvalidseenrecords = 0;
+            $returnedusers = [];
+            foreach ($getnoncontactusers(0, $batchlimit) as $users) {
+                foreach ($users as $id => $user) {
+                    $userdetails = \user_get_user_details_courses($user);
+
+                    // Return the user only if the searched field is returned.
+                    // Otherwise it means that the $USER was not allowed to search the returned user.
+                    if (!empty($userdetails) and !empty($userdetails['fullname'])) {
+                        // We know we've matched, but only save the record if it's within the offset area we need.
+                        if ($limitfrom == 0) {
+                            // No offset specified, so just save.
+                            $returnedusers[$id] = $user;
+                        } else {
+                            // There is an offset in play.
+                            // If we've passed enough records already (> offset value), then we can save this one.
+                            if ($noofvalidseenrecords >= $limitfrom) {
+                                $returnedusers[$id] = $user;
+                            }
+                        }
+                        if (count($returnedusers) == $limitnum) {
+                            break 2;
+                        }
+                        $noofvalidseenrecords++;
+                    }
+                }
+            }
+            $foundusers = $returnedusers;
         }
-        $foundusers = $DB->get_records_sql_menu($sql, $params + $excludeparams, $limitfrom, $limitnum);
 
-        $orderednoncontacs = array();
+        $orderednoncontacts = array();
         if (!empty($foundusers)) {
             $noncontacts = helper::get_member_info($userid, array_keys($foundusers));
             // The get_member_info returns an associative array, so is not ordered in the same way.
@@ -399,11 +425,11 @@ class api {
             foreach ($foundusers as $key => $value) {
                 $contact = $noncontacts[$key];
                 $contact->conversations = self::get_conversations_between_users($userid, $key, 0, 1000);
-                $orderednoncontacs[] = $contact;
+                $orderednoncontacts[] = $contact;
             }
         }
 
-        return array($orderedcontacs, $orderednoncontacs);
+        return array($orderedcontacts, $orderednoncontacts);
     }
 
     /**
index d200778..0c9c055 100644 (file)
@@ -1366,12 +1366,7 @@ class core_message_external extends external_api {
      * @since 3.6
      */
     public static function message_search_users($userid, $search, $limitfrom = 0, $limitnum = 0) {
-        global $CFG, $USER;
-
-        // Check if messaging is enabled.
-        if (empty($CFG->messaging)) {
-            throw new moodle_exception('disabled', 'message');
-        }
+        global $USER;
 
         $systemcontext = context_system::instance();
 
index 145ec68..78dfbfd 100644 (file)
@@ -307,87 +307,346 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
     }
 
     /**
-     * Tests searching users.
+     * Tests searching for users when site-wide messaging is disabled.
+     *
+     * This test verifies that any contacts are returned, as well as any non-contacts whose profile we can view.
+     * If checks this by placing some users in the same course, where default caps would permit a user to view another user's
+     * profile.
      */
-    public function test_message_search_users() {
+    public function test_message_search_users_messagingallusers_disabled() {
+        $this->resetAfterTest();
+
         // Create some users.
-        $user1 = new stdClass();
-        $user1->firstname = 'User search';
-        $user1->lastname = 'One';
-        $user1 = self::getDataGenerator()->create_user($user1);
+        $users = [];
+        foreach (range(1, 7) as $i) {
+            $user = new stdClass();
+            $user->firstname = ($i == 4) ? 'User' : 'User search'; // Ensure the fourth user won't match the search term.
+            $user->lastname = $i;
+            $user = $this->getDataGenerator()->create_user($user);
+            $users[$i] = $user;
+        }
+
+        // Enrol a few users in the same course, but leave them as non-contacts.
+        $course1 = $this->getDataGenerator()->create_course();
+        $this->setAdminUser();
+        $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[7]->id, $course1->id);
+
+        // Add some other users as contacts.
+        \core_message\api::add_contact($users[1]->id, $users[2]->id);
+        \core_message\api::add_contact($users[3]->id, $users[1]->id);
+        \core_message\api::add_contact($users[1]->id, $users[4]->id);
+
+        // Create individual conversations between some users, one contact and one non-contact.
+        $ic1 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
+            [$users[1]->id, $users[2]->id]);
+        $ic2 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
+            [$users[6]->id, $users[1]->id]);
+
+        // Create a group conversation between 4 users, including a contact and a non-contact.
+        $gc1 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP,
+            [$users[1]->id, $users[2]->id, $users[4]->id, $users[7]->id], 'Project chat');
 
         // Set as the user performing the search.
-        $this->setUser($user1);
+        $this->setUser($users[1]);
 
-        $user2 = new stdClass();
-        $user2->firstname = 'User search';
-        $user2->lastname = 'Two';
-        $user2 = self::getDataGenerator()->create_user($user2);
+        // Perform a search with $CFG->messagingallusers disabled.
+        set_config('messagingallusers', 0);
+        $result = \core_message\api::message_search_users($users[1]->id, 'search');
 
-        $user3 = new stdClass();
-        $user3->firstname = 'User search';
-        $user3->lastname = 'Three';
-        $user3 = self::getDataGenerator()->create_user($user3);
+        // Confirm that we returns contacts and non-contacts.
+        $this->assertArrayHasKey(0, $result);
+        $this->assertArrayHasKey(1, $result);
+        $contacts = $result[0];
+        $noncontacts = $result[1];
 
-        $user4 = new stdClass();
-        $user4->firstname = 'User';
-        $user4->lastname = 'Four';
-        $user4 = self::getDataGenerator()->create_user($user4);
+        // Check that we retrieved the correct contacts.
+        $this->assertCount(2, $contacts);
+        $this->assertEquals($users[2]->id, $contacts[0]->id);
+        $this->assertEquals($users[3]->id, $contacts[1]->id);
 
-        $user5 = new stdClass();
-        $user5->firstname = 'User search';
-        $user5->lastname = 'Five';
-        $user5 = self::getDataGenerator()->create_user($user5);
+        // Verify the correct conversations were returned for the contacts.
+        $this->assertCount(2, $contacts[0]->conversations);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $contacts[0]->conversations[$gc1->id]->type);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, $contacts[0]->conversations[$ic1->id]->type);
 
-        $user6 = new stdClass();
-        $user6->firstname = 'User search';
-        $user6->lastname = 'Six';
-        $user6 = self::getDataGenerator()->create_user($user6);
+        $this->assertCount(0, $contacts[1]->conversations);
 
-        $user7 = new stdClass();
-        $user7->firstname = 'User search';
-        $user7->lastname = 'Seven';
-        $user7 = self::getDataGenerator()->create_user($user7);
+        // Check that we retrieved the correct non-contacts.
+        // When site wide messaging is disabled, we expect to see only those users whose profiles we can view.
+        $this->assertCount(2, $noncontacts);
+        $this->assertEquals($users[6]->id, $noncontacts[0]->id);
+        $this->assertEquals($users[7]->id, $noncontacts[1]->id);
 
-        // Add some users as contacts.
-        \core_message\api::add_contact($user1->id, $user2->id);
-        \core_message\api::add_contact($user3->id, $user1->id);
-        \core_message\api::add_contact($user1->id, $user4->id);
+        // Verify the correct conversations were returned for the non-contacts.
+        $this->assertCount(1, $noncontacts[0]->conversations);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
+            $noncontacts[0]->conversations[$ic2->id]->type);
 
-        // Create private conversations with some users.
-        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user1->id, $user6->id));
-        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user7->id, $user1->id));
+        $this->assertCount(1, $noncontacts[1]->conversations);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[1]->conversations[$gc1->id]->type);
+    }
 
-        // Perform a search $CFG->messagingallusers setting enabled.
+    /**
+     * Tests searching for users when site-wide messaging is enabled.
+     *
+     * This test verifies that any contacts are returned, as well as any non-contacts, regardless of whether the searching user
+     * can view their respective profile.
+     */
+    public function test_message_search_users_messagingallusers_enabled() {
+        $this->resetAfterTest();
+
+        // Create some users.
+        $users = [];
+        foreach (range(1, 8) as $i) {
+            $user = new stdClass();
+            $user->firstname = ($i == 4) ? 'User' : 'User search'; // Ensure the fourth user won't match the search term.
+            $user->lastname = $i;
+            $user = $this->getDataGenerator()->create_user($user);
+            $users[$i] = $user;
+        }
+
+        // Enrol a few users in the same course, but leave them as non-contacts.
+        $course1 = $this->getDataGenerator()->create_course();
+        $this->setAdminUser();
+        $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[7]->id, $course1->id);
+
+        // Add some other users as contacts.
+        \core_message\api::add_contact($users[1]->id, $users[2]->id);
+        \core_message\api::add_contact($users[3]->id, $users[1]->id);
+        \core_message\api::add_contact($users[1]->id, $users[4]->id);
+
+        // Create individual conversations between some users, one contact and one non-contact.
+        $ic1 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
+            [$users[1]->id, $users[2]->id]);
+        $ic2 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
+            [$users[6]->id, $users[1]->id]);
+
+        // Create a group conversation between 5 users, including a contact and a non-contact, and a user NOT in a shared course.
+        $gc1 = \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP,
+            [$users[1]->id, $users[2]->id, $users[4]->id, $users[7]->id, $users[8]->id], 'Project chat');
+
+        // Set as the user performing the search.
+        $this->setUser($users[1]);
+
+        // Perform a search with $CFG->messagingallusers enabled.
         set_config('messagingallusers', 1);
-        list($contacts, $noncontacts) = \core_message\api::message_search_users($user1->id, 'search');
+        $result = \core_message\api::message_search_users($users[1]->id, 'search');
+
+        // Confirm that we returns contacts and non-contacts.
+        $this->assertArrayHasKey(0, $result);
+        $this->assertArrayHasKey(1, $result);
+        $contacts = $result[0];
+        $noncontacts = $result[1];
 
         // Check that we retrieved the correct contacts.
         $this->assertCount(2, $contacts);
-        $this->assertEquals($user3->id, $contacts[0]->id);
-        $this->assertEquals($user2->id, $contacts[1]->id);
+        $this->assertEquals($users[2]->id, $contacts[0]->id);
+        $this->assertEquals($users[3]->id, $contacts[1]->id);
+
+        // Verify the correct conversations were returned for the contacts.
+        $this->assertCount(2, $contacts[0]->conversations);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $contacts[0]->conversations[$gc1->id]->type);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, $contacts[0]->conversations[$ic1->id]->type);
+
+        $this->assertCount(0, $contacts[1]->conversations);
+
+        // Check that we retrieved the correct non-contacts.
+        // If site wide messaging is enabled, we expect to be able to search for any users.
+        $this->assertCount(4, $noncontacts);
+        $this->assertEquals($users[5]->id, $noncontacts[0]->id);
+        $this->assertEquals($users[6]->id, $noncontacts[1]->id);
+        $this->assertEquals($users[7]->id, $noncontacts[2]->id);
+        $this->assertEquals($users[8]->id, $noncontacts[3]->id);
+
+        // Verify the correct conversations were returned for the non-contacts.
+        $this->assertCount(0, $noncontacts[0]->conversations);
+
+        $this->assertCount(1, $noncontacts[1]->conversations);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
+            $noncontacts[1]->conversations[$ic2->id]->type);
+
+        $this->assertCount(1, $noncontacts[2]->conversations);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[2]->conversations[$gc1->id]->type);
+
+        $this->assertCount(1, $noncontacts[3]->conversations);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[3]->conversations[$gc1->id]->type);
+    }
+
+    /**
+     * Verify searching for users works even if no matching users from either contacts, or non-contacts can be found.
+     */
+    public function test_message_search_users_with_empty_result() {
+        $this->resetAfterTest();
+
+        // Create some users, but make sure neither will match the search term.
+        $user1 = new stdClass();
+        $user1->firstname = 'User';
+        $user1->lastname = 'One';
+        $user1 = $this->getDataGenerator()->create_user($user1);
+        $user2 = new stdClass();
+        $user2->firstname = 'User';
+        $user2->lastname = 'Two';
+        $user2 = $this->getDataGenerator()->create_user($user2);
+
+        // Perform a search as user1.
+        $this->setUser($user1);
+        $result = \core_message\api::message_search_users($user1->id, 'search');
+
+        // Check results are empty.
+        $this->assertCount(0, $result[0]);
+        $this->assertCount(0, $result[1]);
+    }
+
+    /**
+     * Test verifying that limits and offsets work for both the contacts and non-contacts return data.
+     */
+    public function test_message_search_users_limit_offset() {
+        $this->resetAfterTest();
+
+        // Create 20 users.
+        $users = [];
+        foreach (range(1, 20) as $i) {
+            $user = new stdClass();
+            $user->firstname = "User search";
+            $user->lastname = $i;
+            $user = $this->getDataGenerator()->create_user($user);
+            $users[$i] = $user;
+        }
+
+        // Enrol the first 9 users in the same course, but leave them as non-contacts.
+        $this->setAdminUser();
+        $course1 = $this->getDataGenerator()->create_course();
+        foreach (range(1, 9) as $i) {
+            $this->getDataGenerator()->enrol_user($users[$i]->id, $course1->id);
+        }
+
+        // Add 5 users, starting at the 11th user, as contacts for user1.
+        foreach (range(11, 15) as $i) {
+            \core_message\api::add_contact($users[1]->id, $users[$i]->id);
+        }
+
+        // Set as the user performing the search.
+        $this->setUser($users[1]);
+
+        // Search using a limit of 3.
+        // This tests the case where we have more results than the limit for both contacts and non-contacts.
+        $result = \core_message\api::message_search_users($users[1]->id, 'search', 0, 3);
+        $contacts = $result[0];
+        $noncontacts = $result[1];
+
+        // Check that we retrieved the correct contacts.
+        $this->assertCount(3, $contacts);
+        $this->assertEquals($users[11]->id, $contacts[0]->id);
+        $this->assertEquals($users[12]->id, $contacts[1]->id);
+        $this->assertEquals($users[13]->id, $contacts[2]->id);
 
         // Check that we retrieved the correct non-contacts.
         $this->assertCount(3, $noncontacts);
-        $this->assertEquals($user5->id, $noncontacts[0]->id);
-        $this->assertEquals($user7->id, $noncontacts[1]->id);
-        $this->assertEquals($user6->id, $noncontacts[2]->id);
+        $this->assertEquals($users[2]->id, $noncontacts[0]->id);
+        $this->assertEquals($users[3]->id, $noncontacts[1]->id);
+        $this->assertEquals($users[4]->id, $noncontacts[2]->id);
+
+        // Now, offset to get the next batch of results.
+        // We expect to see 2 contacts, and 3 non-contacts.
+        $result = \core_message\api::message_search_users($users[1]->id, 'search', 3, 3);
+        $contacts = $result[0];
+        $noncontacts = $result[1];
+        $this->assertCount(2, $contacts);
+        $this->assertEquals($users[14]->id, $contacts[0]->id);
+        $this->assertEquals($users[15]->id, $contacts[1]->id);
+
+        $this->assertCount(3, $noncontacts);
+        $this->assertEquals($users[5]->id, $noncontacts[0]->id);
+        $this->assertEquals($users[6]->id, $noncontacts[1]->id);
+        $this->assertEquals($users[7]->id, $noncontacts[2]->id);
+
+        // Now, offset to get the next batch of results.
+        // We expect to see 0 contacts, and 2 non-contacts.
+        $result = \core_message\api::message_search_users($users[1]->id, 'search', 6, 3);
+        $contacts = $result[0];
+        $noncontacts = $result[1];
+        $this->assertCount(0, $contacts);
+
+        $this->assertCount(2, $noncontacts);
+        $this->assertEquals($users[8]->id, $noncontacts[0]->id);
+        $this->assertEquals($users[9]->id, $noncontacts[1]->id);
+    }
+
+    /**
+     * Tests searching users as a user having the 'moodle/user:viewdetails' capability.
+     */
+    public function test_message_search_users_with_cap() {
+        $this->resetAfterTest();
+        global $DB;
+
+        // Create some users.
+        $users = [];
+        foreach (range(1, 8) as $i) {
+            $user = new stdClass();
+            $user->firstname = ($i == 4) ? 'User' : 'User search'; // Ensure the fourth user won't match the search term.
+            $user->lastname = $i;
+            $user = $this->getDataGenerator()->create_user($user);
+            $users[$i] = $user;
+        }
 
-        // Perform a search $CFG->messagingallusers setting disabled.
+        // Enrol a few users in the same course, but leave them as non-contacts.
+        $course1 = $this->getDataGenerator()->create_course();
+        $this->setAdminUser();
+        $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[7]->id, $course1->id);
+
+        // Add some other users as contacts.
+        \core_message\api::add_contact($users[1]->id, $users[2]->id);
+        \core_message\api::add_contact($users[3]->id, $users[1]->id);
+        \core_message\api::add_contact($users[1]->id, $users[4]->id);
+
+        // Set as the user performing the search.
+        $this->setUser($users[1]);
+
+        // Grant the authenticated user role the capability 'user:viewdetails' at site context.
+        $authenticatedrole = $DB->get_record('role', ['shortname' => 'user'], '*', MUST_EXIST);
+        assign_capability('moodle/user:viewdetails', CAP_ALLOW, $authenticatedrole->id, context_system::instance());
+
+        // Perform a search with $CFG->messagingallusers disabled.
         set_config('messagingallusers', 0);
-        list($contacts, $noncontacts) = \core_message\api::message_search_users($user1->id, 'search');
+        $result = \core_message\api::message_search_users($users[1]->id, 'search');
+        $contacts = $result[0];
+        $noncontacts = $result[1];
 
         // Check that we retrieved the correct contacts.
         $this->assertCount(2, $contacts);
-        $this->assertEquals($user3->id, $contacts[0]->id);
-        $this->assertEquals($user2->id, $contacts[1]->id);
+        $this->assertEquals($users[2]->id, $contacts[0]->id);
+        $this->assertEquals($users[3]->id, $contacts[1]->id);
 
         // Check that we retrieved the correct non-contacts.
-        $this->assertCount(2, $noncontacts);
-        $this->assertEquals($user7->id, $noncontacts[0]->id);
-        $this->assertEquals($user6->id, $noncontacts[1]->id);
+        // Site-wide messaging is disabled, but since we can see all users, we expect to be able to search for any users.
+        $this->assertCount(4, $noncontacts);
+        $this->assertEquals($users[5]->id, $noncontacts[0]->id);
+        $this->assertEquals($users[6]->id, $noncontacts[1]->id);
+        $this->assertEquals($users[7]->id, $noncontacts[2]->id);
+        $this->assertEquals($users[8]->id, $noncontacts[3]->id);
+    }
+
+    /**
+     * Tests searching users with messaging disabled.
+     */
+    public function test_message_search_users_messaging_disabled() {
+        $this->resetAfterTest();
+
+        // Create a user.
+        $user = $this->getDataGenerator()->create_user();
+
+        // Disable messaging.
+        set_config('messaging', 0);
+
+        // Ensure an exception is thrown.
+        $this->expectException('moodle_exception');
+        \core_message\api::message_search_users($user->id, 'User');
     }
 
     /**
@@ -453,101 +712,6 @@ class core_message_api_testcase extends core_message_messagelib_testcase {
         $this->assertCount(0, \core_message\api::get_conversations_between_users($user6->id, $user1->id));
     }
 
-    /**
-     * Tests searching users with and without conversations.
-     */
-    public function test_message_search_users_with_and_without_conversations() {
-        // Create some users.
-        $user1 = new stdClass();
-        $user1->firstname = 'User search';
-        $user1->lastname = 'One';
-        $user1 = self::getDataGenerator()->create_user($user1);
-
-        // Set as the user performing the search.
-        $this->setUser($user1);
-
-        $user2 = new stdClass();
-        $user2->firstname = 'User search';
-        $user2->lastname = 'Two';
-        $user2 = self::getDataGenerator()->create_user($user2);
-
-        $user3 = new stdClass();
-        $user3->firstname = 'User search';
-        $user3->lastname = 'Three';
-        $user3 = self::getDataGenerator()->create_user($user3);
-
-        $user4 = new stdClass();
-        $user4->firstname = 'User';
-        $user4->lastname = 'Four';
-        $user4 = self::getDataGenerator()->create_user($user4);
-
-        $user5 = new stdClass();
-        $user5->firstname = 'User search';
-        $user5->lastname = 'Five';
-        $user5 = self::getDataGenerator()->create_user($user5);
-
-        // Add a user as contact.
-        \core_message\api::add_contact($user1->id, $user2->id);
-
-        // Create private conversations with some users.
-        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user1->id, $user2->id));
-        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user3->id, $user1->id));
-
-        // Create a group conversation with users.
-        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP,
-            array($user1->id, $user2->id, $user4->id),
-            'Project chat');
-
-        // Perform a search $CFG->messagingallusers setting enabled.
-        set_config('messagingallusers', 1);
-        list($contacts, $noncontacts) = \core_message\api::message_search_users($user1->id, 'search');
-
-        // Check that we retrieved the correct contacts.
-        $this->assertCount(1, $contacts);
-
-        // Check that we retrieved the correct conversations for contacts.
-        $this->assertCount(2, $contacts[0]->conversations);
-
-        // Check that we retrieved the correct non-contacts.
-        $this->assertCount(2, $noncontacts);
-        $this->assertEquals($user5->id, $noncontacts[0]->id);
-        $this->assertEquals($user3->id, $noncontacts[1]->id);
-
-        // Check that we retrieved the correct conversations for non-contacts.
-        $this->assertCount(0, $noncontacts[0]->conversations);
-        $this->assertCount(1, $noncontacts[1]->conversations);
-    }
-
-    /**
-     * Tests searching users with empty result.
-     */
-    public function test_message_search_users_with_empty_result() {
-
-        // Create some users.
-        $user1 = new stdClass();
-        $user1->firstname = 'User';
-        $user1->lastname = 'One';
-        $user1 = self::getDataGenerator()->create_user($user1);
-
-        // Set as the user performing the search.
-        $this->setUser($user1);
-
-        $user2 = new stdClass();
-        $user2->firstname = 'User';
-        $user2->lastname = 'Two';
-        $user2 = self::getDataGenerator()->create_user($user2);
-
-        // Perform a search $CFG->messagingallusers setting enabled.
-        set_config('messagingallusers', 1);
-        list($contacts, $noncontacts) = \core_message\api::message_search_users($user1->id, 'search');
-
-        // Check results are empty.
-        $this->assertEquals(0, count($contacts));
-        $this->assertEquals(0, count($noncontacts));
-    }
-
     /**
      * Tests searching messages.
      */
index 54fe35d..9d2f0ad 100644 (file)
@@ -2251,312 +2251,371 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
     }
 
     /**
-     * Tests searching users.
+     * Tests searching for users when site-wide messaging is disabled.
+     *
+     * This test verifies that any contacts are returned, as well as any non-contacts whose profile we can view.
+     * If checks this by placing some users in the same course, where default caps would permit a user to view another user's
+     * profile.
      */
-    public function test_message_search_users() {
-        $this->resetAfterTest(true);
+    public function test_message_search_users_messagingallusers_disabled() {
+        $this->resetAfterTest();
 
         // Create some users.
-        $user1 = new stdClass();
-        $user1->firstname = 'User search';
-        $user1->lastname = 'One';
-        $user1 = self::getDataGenerator()->create_user($user1);
-        // Set as the user performing the search.
-        $this->setUser($user1);
-
-        $user2 = new stdClass();
-        $user2->firstname = 'User search';
-        $user2->lastname = 'Two';
-        $user2 = self::getDataGenerator()->create_user($user2);
-
-        $user3 = new stdClass();
-        $user3->firstname = 'User search';
-        $user3->lastname = 'Three';
-        $user3 = self::getDataGenerator()->create_user($user3);
-
-        $user4 = new stdClass();
-        $user4->firstname = 'User';
-        $user4->lastname = 'Four';
-        $user4 = self::getDataGenerator()->create_user($user4);
-
-        $user5 = new stdClass();
-        $user5->firstname = 'User search';
-        $user5->lastname = 'Five';
-        $user5 = self::getDataGenerator()->create_user($user5);
-
-        $user6 = new stdClass();
-        $user6->firstname = 'User search';
-        $user6->lastname = 'Six';
-        $user6 = self::getDataGenerator()->create_user($user6);
+        $users = [];
+        foreach (range(1, 7) as $i) {
+            $user = new stdClass();
+            $user->firstname = ($i == 4) ? 'User' : 'User search'; // Ensure the fourth user won't match the search term.
+            $user->lastname = $i;
+            $user = $this->getDataGenerator()->create_user($user);
+            $users[$i] = $user;
+        }
 
-        $user7 = new stdClass();
-        $user7->firstname = 'User search';
-        $user7->lastname = 'Seven';
-        $user7 = self::getDataGenerator()->create_user($user7);
+        // Enrol a few users in the same course, but leave them as non-contacts.
+        $course1 = $this->getDataGenerator()->create_course();
+        $this->setAdminUser();
+        $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[7]->id, $course1->id);
 
-        // Add some users as contacts.
-        \core_message\api::add_contact($user1->id, $user2->id);
-        \core_message\api::add_contact($user3->id, $user1->id);
-        \core_message\api::add_contact($user1->id, $user4->id);
+        // Add some other users as contacts.
+        \core_message\api::add_contact($users[1]->id, $users[2]->id);
+        \core_message\api::add_contact($users[3]->id, $users[1]->id);
+        \core_message\api::add_contact($users[1]->id, $users[4]->id);
 
-        // Create private conversations with some users.
+        // Create individual conversations between some users, one contact and one non-contact.
         \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user1->id, $user6->id));
+            [$users[1]->id, $users[2]->id]);
         \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user7->id, $user1->id));
-
-        // Perform a search $CFG->messagingallusers setting enabled.
-        set_config('messagingallusers', 1);
-        $result = core_message_external::message_search_users($user1->id, 'search');
-
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(),
-            $result);
+            [$users[6]->id, $users[1]->id]);
 
-        // Confirm that we returns contacts and non-contacts.
-        $contacts = $result['contacts'];
-        $noncontacts = $result['noncontacts'];
-
-        // Check that we retrieved the correct contacts.
-        $this->assertCount(2, $contacts);
-        $this->assertEquals($user3->id, $contacts[0]['id']);
-        $this->assertEquals($user2->id, $contacts[1]['id']);
+        // Create a group conversation between 4 users, including a contact and a non-contact.
+        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP,
+            [$users[1]->id, $users[2]->id, $users[4]->id, $users[7]->id], 'Project chat');
 
-        // Check that we retrieved the correct non-contacts.
-        $this->assertCount(3, $noncontacts);
-        $this->assertEquals($user5->id, $noncontacts[0]['id']);
-        $this->assertEquals($user7->id, $noncontacts[1]['id']);
-        $this->assertEquals($user6->id, $noncontacts[2]['id']);
+        // Set as the user performing the search.
+        $this->setUser($users[1]);
 
-        // Perform a search $CFG->messagingallusers setting disabled.
+        // Perform a search with $CFG->messagingallusers disabled.
         set_config('messagingallusers', 0);
-        $result = core_message_external::message_search_users($user1->id, 'search');
-
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(),
-            $result);
+        $result = core_message_external::message_search_users($users[1]->id, 'search');
+        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(), $result);
 
         // Confirm that we returns contacts and non-contacts.
+        $this->assertArrayHasKey('contacts', $result);
+        $this->assertArrayHasKey('noncontacts', $result);
         $contacts = $result['contacts'];
         $noncontacts = $result['noncontacts'];
 
         // Check that we retrieved the correct contacts.
         $this->assertCount(2, $contacts);
-        $this->assertEquals($user3->id, $contacts[0]['id']);
-        $this->assertEquals($user2->id, $contacts[1]['id']);
+        $this->assertEquals($users[2]->id, $contacts[0]['id']);
+        $this->assertEquals($users[3]->id, $contacts[1]['id']);
+
+        // Verify the correct conversations were returned for the contacts.
+        $this->assertCount(2, $contacts[0]['conversations']);
+        // We can't rely on the ordering of conversations within the results, so sort by id first.
+        usort($contacts[0]['conversations'], function($a, $b) {
+            return $a['id'] < $b['id'];
+        });
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $contacts[0]['conversations'][0]['type']);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, $contacts[0]['conversations'][1]['type']);
+
+        $this->assertCount(0, $contacts[1]['conversations']);
 
         // Check that we retrieved the correct non-contacts.
+        // When site wide messaging is disabled, we expect to see only those users whose profiles we can view.
         $this->assertCount(2, $noncontacts);
-        $this->assertEquals($user7->id, $noncontacts[0]['id']);
-        $this->assertEquals($user6->id, $noncontacts[1]['id']);
+        $this->assertEquals($users[6]->id, $noncontacts[0]['id']);
+        $this->assertEquals($users[7]->id, $noncontacts[1]['id']);
+
+        // Verify the correct conversations were returned for the non-contacts.
+        $this->assertCount(1, $noncontacts[0]['conversations']);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, $noncontacts[0]['conversations'][0]['type']);
+
+        $this->assertCount(1, $noncontacts[1]['conversations']);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[1]['conversations'][0]['type']);
     }
 
     /**
-     * Tests searching users as another user.
+     * Tests searching for users when site-wide messaging is enabled.
+     *
+     * This test verifies that any contacts are returned, as well as any non-contacts, regardless of whether the searching user
+     * can view their respective profile.
      */
-    public function test_message_search_users_as_other_user() {
-        $this->resetAfterTest(true);
-
-        // The person doing the search.
-        $this->setAdminUser();
+    public function test_message_search_users_messagingallusers_enabled() {
+        $this->resetAfterTest();
 
         // Create some users.
-        $user1 = new stdClass();
-        $user1->firstname = 'User search';
-        $user1->lastname = 'One';
-        $user1 = self::getDataGenerator()->create_user($user1);
-
-        $user2 = new stdClass();
-        $user2->firstname = 'User search';
-        $user2->lastname = 'Two';
-        $user2 = self::getDataGenerator()->create_user($user2);
-
-        $user3 = new stdClass();
-        $user3->firstname = 'User search';
-        $user3->lastname = 'Three';
-        $user3 = self::getDataGenerator()->create_user($user3);
-
-        $user4 = new stdClass();
-        $user4->firstname = 'User';
-        $user4->lastname = 'Four';
-        $user4 = self::getDataGenerator()->create_user($user4);
-
-        $user5 = new stdClass();
-        $user5->firstname = 'User search';
-        $user5->lastname = 'Five';
-        $user5 = self::getDataGenerator()->create_user($user5);
-
-        $user6 = new stdClass();
-        $user6->firstname = 'User search';
-        $user6->lastname = 'Six';
-        $user6 = self::getDataGenerator()->create_user($user6);
+        $users = [];
+        foreach (range(1, 8) as $i) {
+            $user = new stdClass();
+            $user->firstname = ($i == 4) ? 'User' : 'User search'; // Ensure the fourth user won't match the search term.
+            $user->lastname = $i;
+            $user = $this->getDataGenerator()->create_user($user);
+            $users[$i] = $user;
+        }
 
-        $user7 = new stdClass();
-        $user7->firstname = 'User search';
-        $user7->lastname = 'Seven';
-        $user7 = self::getDataGenerator()->create_user($user7);
+        // Enrol a few users in the same course, but leave them as non-contacts.
+        $course1 = $this->getDataGenerator()->create_course();
+        $this->setAdminUser();
+        $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[7]->id, $course1->id);
 
-        // Add some users as contacts.
-        \core_message\api::add_contact($user1->id, $user2->id);
-        \core_message\api::add_contact($user3->id, $user1->id);
-        \core_message\api::add_contact($user1->id, $user4->id);
+        // Add some other users as contacts.
+        \core_message\api::add_contact($users[1]->id, $users[2]->id);
+        \core_message\api::add_contact($users[3]->id, $users[1]->id);
+        \core_message\api::add_contact($users[1]->id, $users[4]->id);
 
-        // Create private conversations with some users.
+        // Create individual conversations between some users, one contact and one non-contact.
         \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user1->id, $user6->id));
+            [$users[1]->id, $users[2]->id]);
         \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user7->id, $user1->id));
+            [$users[6]->id, $users[1]->id]);
 
-        // Perform a search $CFG->messagingallusers setting enabled.
-        set_config('messagingallusers', 1);
-        $result = core_message_external::message_search_users($user1->id, 'search');
+        // Create a group conversation between 5 users, including a contact and a non-contact, and a user NOT in a shared course.
+        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP,
+            [$users[1]->id, $users[2]->id, $users[4]->id, $users[7]->id, $users[8]->id], 'Project chat');
 
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(),
-            $result);
+        // Set as the user performing the search.
+        $this->setUser($users[1]);
+
+        // Perform a search with $CFG->messagingallusers enabled.
+        set_config('messagingallusers', 1);
+        $result = core_message_external::message_search_users($users[1]->id, 'search');
+        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(), $result);
 
         // Confirm that we returns contacts and non-contacts.
+        $this->assertArrayHasKey('contacts', $result);
+        $this->assertArrayHasKey('noncontacts', $result);
         $contacts = $result['contacts'];
         $noncontacts = $result['noncontacts'];
 
         // Check that we retrieved the correct contacts.
         $this->assertCount(2, $contacts);
-        $this->assertEquals($user3->id, $contacts[0]['id']);
-        $this->assertEquals($user2->id, $contacts[1]['id']);
+        $this->assertEquals($users[2]->id, $contacts[0]['id']);
+        $this->assertEquals($users[3]->id, $contacts[1]['id']);
 
-        // Check that we retrieved the correct non-contacts.
-        $this->assertCount(3, $noncontacts);
-        $this->assertEquals($user5->id, $noncontacts[0]['id']);
-        $this->assertEquals($user7->id, $noncontacts[1]['id']);
-        $this->assertEquals($user6->id, $noncontacts[2]['id']);
+        // Verify the correct conversations were returned for the contacts.
+        $this->assertCount(2, $contacts[0]['conversations']);
+        // We can't rely on the ordering of conversations within the results, so sort by id first.
+        usort($contacts[0]['conversations'], function($a, $b) {
+            return $a['id'] < $b['id'];
+        });
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $contacts[0]['conversations'][0]['type']);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, $contacts[0]['conversations'][1]['type']);
 
-        // Perform a search $CFG->messagingallusers setting disabled.
-        set_config('messagingallusers', 0);
-        $result = core_message_external::message_search_users($user1->id, 'search');
+        $this->assertCount(0, $contacts[1]['conversations']);
 
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(),
-            $result);
+        // Check that we retrieved the correct non-contacts.
+        // If site wide messaging is enabled, we expect to be able to search for any users.
+        $this->assertCount(4, $noncontacts);
+        $this->assertEquals($users[5]->id, $noncontacts[0]['id']);
+        $this->assertEquals($users[6]->id, $noncontacts[1]['id']);
+        $this->assertEquals($users[7]->id, $noncontacts[2]['id']);
+        $this->assertEquals($users[8]->id, $noncontacts[3]['id']);
+
+        // Verify the correct conversations were returned for the non-contacts.
+        $this->assertCount(0, $noncontacts[0]['conversations']);
 
-        // Confirm that we returns contacts and non-contacts.
-        $contacts = $result['contacts'];
-        $noncontacts = $result['noncontacts'];
+        $this->assertCount(1, $noncontacts[1]['conversations']);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, $noncontacts[1]['conversations'][0]['type']);
 
-        // Check that we retrieved the correct contacts.
-        $this->assertCount(2, $contacts);
-        $this->assertEquals($user3->id, $contacts[0]['id']);
-        $this->assertEquals($user2->id, $contacts[1]['id']);
+        $this->assertCount(1, $noncontacts[2]['conversations']);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[2]['conversations'][0]['type']);
 
-        // Check that we retrieved the correct non-contacts.
-        $this->assertCount(2, $noncontacts);
-        $this->assertEquals($user7->id, $noncontacts[0]['id']);
-        $this->assertEquals($user6->id, $noncontacts[1]['id']);
+        $this->assertCount(1, $noncontacts[3]['conversations']);
+        $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[3]['conversations'][0]['type']);
     }
 
     /**
-     * Tests searching users as another user without the proper capabilities.
+     * Verify searching for users works even if no matching users from either contacts, or non-contacts can be found.
      */
-    public function test_message_search_users_as_other_user_without_cap() {
-        $this->resetAfterTest(true);
+    public function test_message_search_users_with_empty_result() {
+        $this->resetAfterTest();
 
-        // Create some users.
-        $user1 = self::getDataGenerator()->create_user();
-        $user2 = self::getDataGenerator()->create_user();
+        // Create some users, but make sure neither will match the search term.
+        $user1 = new stdClass();
+        $user1->firstname = 'User';
+        $user1->lastname = 'One';
+        $user1 = $this->getDataGenerator()->create_user($user1);
+        $user2 = new stdClass();
+        $user2->firstname = 'User';
+        $user2->lastname = 'Two';
+        $user2 = $this->getDataGenerator()->create_user($user2);
 
-        // The person doing the search for another user.
+        // Perform a search as user1.
         $this->setUser($user1);
+        $result = core_message_external::message_search_users($user1->id, 'search');
+        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(), $result);
 
-        // Ensure an exception is thrown.
-        $this->expectException('moodle_exception');
-        core_message_external::message_search_users($user2->id, 'User');
-        $this->assertDebuggingCalled();
+        // Check results are empty.
+        $this->assertCount(0, $result['contacts']);
+        $this->assertCount(0, $result['noncontacts']);
     }
 
     /**
-     * Tests searching users with and without conversations.
+     * Test verifying that limits and offsets work for both the contacts and non-contacts return data.
      */
-    public function test_message_search_users_with_and_without_conversations() {
-        $this->resetAfterTest(true);
+    public function test_message_search_users_limit_offset() {
+        $this->resetAfterTest();
 
-        // Create some users.
-        $user1 = new stdClass();
-        $user1->firstname = 'User search';
-        $user1->lastname = 'One';
-        $user1 = self::getDataGenerator()->create_user($user1);
+        // Create 20 users.
+        $users = [];
+        foreach (range(1, 20) as $i) {
+            $user = new stdClass();
+            $user->firstname = "User search";
+            $user->lastname = $i;
+            $user = $this->getDataGenerator()->create_user($user);
+            $users[$i] = $user;
+        }
+
+        // Enrol the first 9 users in the same course, but leave them as non-contacts.
+        $this->setAdminUser();
+        $course1 = $this->getDataGenerator()->create_course();
+        foreach (range(1, 9) as $i) {
+            $this->getDataGenerator()->enrol_user($users[$i]->id, $course1->id);
+        }
+
+        // Add 5 users, starting at the 11th user, as contacts for user1.
+        foreach (range(11, 15) as $i) {
+            \core_message\api::add_contact($users[1]->id, $users[$i]->id);
+        }
 
         // Set as the user performing the search.
-        $this->setUser($user1);
+        $this->setUser($users[1]);
 
-        $user2 = new stdClass();
-        $user2->firstname = 'User search';
-        $user2->lastname = 'Two';
-        $user2 = self::getDataGenerator()->create_user($user2);
+        // Search using a limit of 3.
+        // This tests the case where we have more results than the limit for both contacts and non-contacts.
+        $result = core_message_external::message_search_users($users[1]->id, 'search', 0, 3);
+        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(), $result);
+        $contacts = $result['contacts'];
+        $noncontacts = $result['noncontacts'];
 
-        $user3 = new stdClass();
-        $user3->firstname = 'User search';
-        $user3->lastname = 'Three';
-        $user3 = self::getDataGenerator()->create_user($user3);
+        // Check that we retrieved the correct contacts.
+        $this->assertCount(3, $contacts);
+        $this->assertEquals($users[11]->id, $contacts[0]['id']);
+        $this->assertEquals($users[12]->id, $contacts[1]['id']);
+        $this->assertEquals($users[13]->id, $contacts[2]['id']);
 
-        $user4 = new stdClass();
-        $user4->firstname = 'User';
-        $user4->lastname = 'Four';
-        $user4 = self::getDataGenerator()->create_user($user4);
+        // Check that we retrieved the correct non-contacts.
+        $this->assertCount(3, $noncontacts);
+        $this->assertEquals($users[2]->id, $noncontacts[0]['id']);
+        $this->assertEquals($users[3]->id, $noncontacts[1]['id']);
+        $this->assertEquals($users[4]->id, $noncontacts[2]['id']);
+
+        // Now, offset to get the next batch of results.
+        // We expect to see 2 contacts, and 3 non-contacts.
+        $result = core_message_external::message_search_users($users[1]->id, 'search', 3, 3);
+        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(), $result);
+        $contacts = $result['contacts'];
+        $noncontacts = $result['noncontacts'];
+        $this->assertCount(2, $contacts);
+        $this->assertEquals($users[14]->id, $contacts[0]['id']);
+        $this->assertEquals($users[15]->id, $contacts[1]['id']);
 
-        $user5 = new stdClass();
-        $user5->firstname = 'User search';
-        $user5->lastname = 'Five';
-        $user5 = self::getDataGenerator()->create_user($user5);
+        $this->assertCount(3, $noncontacts);
+        $this->assertEquals($users[5]->id, $noncontacts[0]['id']);
+        $this->assertEquals($users[6]->id, $noncontacts[1]['id']);
+        $this->assertEquals($users[7]->id, $noncontacts[2]['id']);
+
+        // Now, offset to get the next batch of results.
+        // We expect to see 0 contacts, and 2 non-contacts.
+        $result = core_message_external::message_search_users($users[1]->id, 'search', 6, 3);
+        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(), $result);
+        $contacts = $result['contacts'];
+        $noncontacts = $result['noncontacts'];
+        $this->assertCount(0, $contacts);
 
-        // Add a user as contact.
-        \core_message\api::add_contact($user1->id, $user2->id);
+        $this->assertCount(2, $noncontacts);
+        $this->assertEquals($users[8]->id, $noncontacts[0]['id']);
+        $this->assertEquals($users[9]->id, $noncontacts[1]['id']);
+    }
 
-        // Create private conversations with some users.
-        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user1->id, $user2->id));
-        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL,
-            array($user3->id, $user1->id));
+    /**
+     * Tests searching users as another user having the 'moodle/user:viewdetails' capability.
+     */
+    public function test_message_search_users_with_cap() {
+        $this->resetAfterTest();
+        global $DB;
 
-        // Create a group conversation with users.
-        \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP,
-            array($user1->id, $user2->id, $user4->id),
-            'Project chat');
+        // Create some users.
+        $users = [];
+        foreach (range(1, 8) as $i) {
+            $user = new stdClass();
+            $user->firstname = ($i == 4) ? 'User' : 'User search'; // Ensure the fourth user won't match the search term.
+            $user->lastname = $i;
+            $user = $this->getDataGenerator()->create_user($user);
+            $users[$i] = $user;
+        }
 
-        // Perform a search $CFG->messagingallusers setting enabled.
-        set_config('messagingallusers', 1);
-        $result = core_message_external::message_search_users($user1->id, 'search');
+        // Enrol a few users in the same course, but leave them as non-contacts.
+        $course1 = $this->getDataGenerator()->create_course();
+        $this->setAdminUser();
+        $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($users[7]->id, $course1->id);
 
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(),
-            $result);
+        // Add some other users as contacts.
+        \core_message\api::add_contact($users[1]->id, $users[2]->id);
+        \core_message\api::add_contact($users[3]->id, $users[1]->id);
+        \core_message\api::add_contact($users[1]->id, $users[4]->id);
 
-        // Confirm that we returns contacts and non-contacts.
+        // Set as the user performing the search.
+        $this->setUser($users[1]);
+
+        // Grant the authenticated user role the capability 'user:viewdetails' at site context.
+        $authenticatedrole = $DB->get_record('role', ['shortname' => 'user'], '*', MUST_EXIST);
+        assign_capability('moodle/user:viewdetails', CAP_ALLOW, $authenticatedrole->id, context_system::instance());
+
+        // Perform a search with $CFG->messagingallusers disabled.
+        set_config('messagingallusers', 0);
+        $result = core_message_external::message_search_users($users[1]->id, 'search');
+        $result = external_api::clean_returnvalue(core_message_external::message_search_users_returns(), $result);
         $contacts = $result['contacts'];
         $noncontacts = $result['noncontacts'];
 
         // Check that we retrieved the correct contacts.
-        $this->assertCount(1, $contacts);
-
-        // Check that we retrieved the correct conversations for contacts.
-        $this->assertCount(2, $contacts[0]['conversations']);
+        $this->assertCount(2, $contacts);
+        $this->assertEquals($users[2]->id, $contacts[0]['id']);
+        $this->assertEquals($users[3]->id, $contacts[1]['id']);
 
         // Check that we retrieved the correct non-contacts.
-        $this->assertCount(2, $noncontacts);
-        $this->assertEquals($user5->id, $noncontacts[0]['id']);
-        $this->assertEquals($user3->id, $noncontacts[1]['id']);
+        // Site-wide messaging is disabled, but since we can see all users, we expect to be able to search for any users.
+        $this->assertCount(4, $noncontacts);
+        $this->assertEquals($users[5]->id, $noncontacts[0]['id']);
+        $this->assertEquals($users[6]->id, $noncontacts[1]['id']);
+        $this->assertEquals($users[7]->id, $noncontacts[2]['id']);
+        $this->assertEquals($users[8]->id, $noncontacts[3]['id']);
+    }
 
-        // Check that we retrieved the correct conversations for non-contacts.
-        $this->assertCount(0, $noncontacts[0]['conversations']);
-        $this->assertCount(1, $noncontacts[1]['conversations']);
+    /**
+     * Tests searching users as another user without the 'moodle/user:viewdetails' capability.
+     */
+    public function test_message_search_users_without_cap() {
+        $this->resetAfterTest();
+
+        // Create some users.
+        $user1 = $this->getDataGenerator()->create_user();
+        $user2 = $this->getDataGenerator()->create_user();
+
+        // The person doing the search for another user.
+        $this->setUser($user1);
+
+        // Ensure an exception is thrown.
+        $this->expectException('moodle_exception');
+        core_message_external::message_search_users($user2->id, 'User');
+        $this->assertDebuggingCalled();
     }
 
     /**
      * Tests searching users with messaging disabled.
      */
     public function test_message_search_users_messaging_disabled() {
-        $this->resetAfterTest(true);
+        $this->resetAfterTest();
 
         // Create some skeleton data just so we can call the WS.
-        $user = self::getDataGenerator()->create_user();
+        $user = $this->getDataGenerator()->create_user();
 
         // The person doing the search.
         $this->setUser($user);
@@ -2567,7 +2626,6 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
         // Ensure an exception is thrown.
         $this->expectException('moodle_exception');
         core_message_external::message_search_users($user->id, 'User');
-        $this->assertDebuggingCalled();
     }
 
     /**