diff --git a/message/classes/api.php b/message/classes/api.php index fb3b8e76c59..2b3bcee12d5 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -352,9 +352,14 @@ class api { } // 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) { - $sql = "SELECT u.id + // 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 WHERE u.deleted = 0 AND u.confirmed = 1 @@ -365,73 +370,53 @@ 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(); + while ($records = $DB->get_records_sql($sql, $params + $excludeparams, $limitfrom, $limitnum)) { + yield $records; + $limitfrom += $limitnum; + } + }; - $foundusers = $DB->get_records_sql($sql, $params + $excludeparams, $limitfrom, $limitnum); - } else { - 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). + // 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; - // 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 - 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(); - while ($records = $DB->get_records_sql($sql, $params + $excludeparams, $limitfrom, $limitnum)) { - yield $records; - $limitfrom += $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) { + // User visibility checks: only return users who are visible to the user performing the search. + // Which visibility check to use depends on the 'messagingallusers' (site wide messaging) setting: + // - If enabled, return matched users whose profiles are visible to the current user anywhere (site or course). + // - If disabled, only return matched users whose course profiles are visible to the current user. + $userdetails = \core_message\helper::search_get_user_details($user); - // 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. + // 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; - } 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++; } + if (count($returnedusers) == $limitnum) { + break 2; + } + $noofvalidseenrecords++; } } - $foundusers = $returnedusers; } + $foundusers = $returnedusers; $noncontacts = []; if (!empty($foundusers)) { diff --git a/message/classes/helper.php b/message/classes/helper.php index f4353570dbd..aa061c23450 100644 --- a/message/classes/helper.php +++ b/message/classes/helper.php @@ -772,4 +772,37 @@ class helper { return $renderer->render_from_template($template, $templatecontext); } + + /** + * Returns user details for a user, if they are visible to the current user in the message search. + * + * This method checks the visibility of a user specifically for the purpose of inclusion in the message search results. + * Visibility depends on the site-wide messaging setting 'messagingallusers': + * If enabled, visibility depends only on the core notion of visibility; a visible site or course profile. + * If disabled, visibility requires that the user be sharing a course with the searching user, and have a visible profile there. + * The current user is always returned. + * + * @param \stdClass $user + * @return array the array of userdetails, if visible, or an empty array otherwise. + */ + public static function search_get_user_details(\stdClass $user) : array { + global $CFG, $USER; + require_once($CFG->dirroot . '/user/lib.php'); + + if ($CFG->messagingallusers || $user->id == $USER->id) { + return \user_get_user_details_courses($user) ?? []; // This checks visibility of site and course profiles. + } else { + // Messaging specific: user must share a course with the searching user AND have a visible profile there. + $sharedcourses = enrol_get_shared_courses($USER, $user); + foreach ($sharedcourses as $course) { + if (user_can_view_profile($user, $course)) { + $userdetails = user_get_user_details($user, $course); + if (!is_null($userdetails)) { + return $userdetails; + } + } + } + } + return []; + } } diff --git a/message/tests/api_test.php b/message/tests/api_test.php index 4e9f4e09aa0..a94c6ca71cb 100644 --- a/message/tests/api_test.php +++ b/message/tests/api_test.php @@ -314,11 +314,12 @@ class core_message_api_testcase extends core_message_messagelib_testcase { * profile. */ public function test_message_search_users_messagingallusers_disabled() { + global $DB; $this->resetAfterTest(); // Create some users. $users = []; - foreach (range(1, 7) as $i) { + 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; @@ -328,6 +329,8 @@ class core_message_api_testcase extends core_message_messagelib_testcase { // Enrol a few users in the same course, but leave them as non-contacts. $course1 = $this->getDataGenerator()->create_course(); + $course2 = $this->getDataGenerator()->create_course(); + $this->setAdminUser(); $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id); $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id); @@ -338,6 +341,11 @@ class core_message_api_testcase extends core_message_messagelib_testcase { \core_message\api::add_contact($users[3]->id, $users[1]->id); \core_message\api::add_contact($users[1]->id, $users[4]->id); + // Enrol a user as a teacher in the course, and make the teacher role a course contact role. + $this->getDataGenerator()->enrol_user($users[8]->id, $course2->id, 'editingteacher'); + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + set_config('coursecontact', $teacherrole->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]); @@ -374,7 +382,8 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $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. + // When site wide messaging is disabled, we expect to see only those users who we share a course with and whose profiles + // are visible in that course. This excludes users like course contacts. $this->assertCount(2, $noncontacts); $this->assertEquals($users[6]->id, $noncontacts[0]->id); $this->assertEquals($users[7]->id, $noncontacts[1]->id); @@ -391,15 +400,16 @@ class core_message_api_testcase extends core_message_messagelib_testcase { /** * 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. + * This test verifies that any contacts are returned, as well as any non-contacts, + * provided the searching user can view their profile. */ public function test_message_search_users_messagingallusers_enabled() { + global $DB; $this->resetAfterTest(); // Create some users. $users = []; - foreach (range(1, 8) as $i) { + foreach (range(1, 9) 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; @@ -407,18 +417,25 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $users[$i] = $user; } - // Enrol a few users in the same course, but leave them as non-contacts. $course1 = $this->getDataGenerator()->create_course(); + $coursecontext = \context_course::instance($course1->id); + + // Enrol a few users in the same course, but leave them as non-contacts. $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); + $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id, 'student'); + $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id, 'student'); + $this->getDataGenerator()->enrol_user($users[7]->id, $course1->id, 'student'); // 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); + // Enrol a user as a teacher in the course, and make the teacher role a course contact role. + $this->getDataGenerator()->enrol_user($users[9]->id, $course1->id, 'editingteacher'); + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + set_config('coursecontact', $teacherrole->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]); @@ -455,25 +472,15 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $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); - + // If site wide messaging is enabled, we expect to only be able to search for users whose profiles we can view. + // In this case, as a student, that's the course contact for course2 and those noncontacts sharing a course with user1. + $this->assertCount(3, $noncontacts); + $this->assertEquals($users[6]->id, $noncontacts[0]->id); + $this->assertEquals($users[7]->id, $noncontacts[1]->id); + $this->assertEquals($users[9]->id, $noncontacts[2]->id); + $this->assertCount(1, $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); + $this->assertCount(0, $noncontacts[2]->conversations); } /** @@ -653,12 +660,10 @@ class core_message_api_testcase extends core_message_messagelib_testcase { $this->assertEquals($users[3]->id, $contacts[1]->id); // Check that we retrieved the correct non-contacts. - // 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); + // Site-wide messaging is disabled, so we expect to be able to search for any 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); } /** diff --git a/message/tests/behat/message_delete_conversation.feature b/message/tests/behat/message_delete_conversation.feature index 630a5dbc381..8d7abe4f5b4 100644 --- a/message/tests/behat/message_delete_conversation.feature +++ b/message/tests/behat/message_delete_conversation.feature @@ -9,6 +9,13 @@ Feature: Message delete conversations | username | firstname | lastname | email | | student1 | Student | 1 | student1@example.com | | student2 | Student | 2 | student2@example.com | + And the following "courses" exist: + | name | shortname | + | course1 | C1 | + And the following "course enrolments" exist: + | user | course | role | + | student1 | C1 | student | + | student2 | C1 | student | And the following config values are set as admin: | messaging | 1 | | messagingallusers | 1 | @@ -52,7 +59,7 @@ Feature: Message delete conversations And I should see "Hi!" in the "Student 2" "group_message_conversation" And I should see "##today##j F##" in the "Student 2" "group_message_conversation" - Scenario: Delete a stared conversation + Scenario: Delete a starred conversation Given the following "favourite conversations" exist: | user | contact | | student1 | student2 | @@ -77,7 +84,7 @@ Feature: Message delete conversations And I should see "What do you need?" in the "Student 1" "group_message_conversation" And I should see "##today##j F##" in the "Student 1" "group_message_conversation" - Scenario: Cancel deleting a stared conversation + Scenario: Cancel deleting a starred conversation Given the following "favourite conversations" exist: | user | contact | | student1 | student2 | @@ -95,7 +102,7 @@ Feature: Message delete conversations And I should see "Hi!" in the "Student 2" "group_message_conversation" And I should see "##today##j F##" in the "Student 2" "group_message_conversation" - Scenario: Check a deleted stared conversation is still stared + Scenario: Check a deleted starred conversation is still starred Given the following "favourite conversations" exist: | user | contact | | student1 | student2 | @@ -113,4 +120,4 @@ Feature: Message delete conversations And I send "Hi!" message to "Student 2" user And I go back in "view-conversation" message drawer And I go back in "view-search" message drawer - And I should see "Student 2" in the "//*[@data-region='message-drawer']//div[@data-region='view-overview-favourites']" "xpath_element" \ No newline at end of file + And I should see "Student 2" in the "//*[@data-region='message-drawer']//div[@data-region='view-overview-favourites']" "xpath_element" diff --git a/message/tests/behat/message_drawer_manage_contacts.feature b/message/tests/behat/message_drawer_manage_contacts.feature index b129e24dbf3..4c57223ac82 100644 --- a/message/tests/behat/message_drawer_manage_contacts.feature +++ b/message/tests/behat/message_drawer_manage_contacts.feature @@ -11,6 +11,15 @@ Feature: Manage contacts | student2 | Student | 2 | student2@example.com | | student3 | Student | 3 | student3@example.com | | student4 | Student | 4 | student4@example.com | + And the following "courses" exist: + | fullname | shortname | + | course1 | C1 | + And the following "course enrolments" exist: + | user | course | role | + | student1 | C1 | student | + | student2 | C1 | student | + | student3 | C1 | student | + | student4 | C1 | student | And the following "message contacts" exist: | user | contact | | student1 | student2 | @@ -80,4 +89,4 @@ Feature: Manage contacts And I click on "Remove from contacts" "link" And I click on "Remove" "button" And I go back in "view-conversation" message drawer - And I should see "No contacts" in the "//*[@data-region='empty-message-container']" "xpath_element" \ No newline at end of file + And I should see "No contacts" in the "//*[@data-region='empty-message-container']" "xpath_element" diff --git a/message/tests/behat/message_manage_preferences.feature b/message/tests/behat/message_manage_preferences.feature index 964e3d24d31..f8c304abe40 100644 --- a/message/tests/behat/message_manage_preferences.feature +++ b/message/tests/behat/message_manage_preferences.feature @@ -5,6 +5,7 @@ Feature: Manage preferences I need to be able to update my messaging preferences Background: + # Note: This course is using separate groups mode. Given the following "courses" exist: | fullname | shortname | category | groupmode | | Course 1 | C1 | 0 | 1 | @@ -14,14 +15,20 @@ Feature: Manage preferences | student2 | Student | 2 | student2@example.com | | student3 | Student | 3 | student3@example.com | | student4 | Student | 4 | student4@example.com | + | student5 | Student | 5 | student5@example.com | And the following "course enrolments" exist: | user | course | role | | student1 | C1 | student | | student2 | C1 | student | | student3 | C1 | student | + | student4 | C1 | student | And the following "groups" exist: | name | course | idnumber | enablemessaging | | Group 1 | C1 | G1 | 1 | + And the following "group members" exist: + | user | group | + | student1 | G1 | + | student4 | G1 | And the following "message contacts" exist: | user | contact | | student1 | student2 | @@ -29,7 +36,8 @@ Feature: Manage preferences | messaging | 1 | | messagingallusers | 1 | - Scenario: Allow send me a message whe you are a contact and the prefrence is my contacts only + # Recipient has 'My contacts only' set. + Scenario: Allow sending a message when you are a contact Given I log in as "student1" And I open messaging And I open messaging settings preferences @@ -40,7 +48,20 @@ Feature: Manage preferences And I send "Hi!" message to "Student 1" user And I should see "Hi!" in the "//*[@data-region='message-drawer']//div[@data-region='content-message-container']" "xpath_element" - Scenario: Not allowed to send a message if you are not contact to the sender or you are not in the same course + # Recipient has 'My contacts and anyone in my courses' set. + Scenario: Disallow sending a message if you are neither contacts with the recipient nor do you share a course + Given I log in as "student1" + And I open messaging + And I open messaging settings preferences + When I click on "//label[text()[contains(.,'My contacts and anyone in my courses')]]" "xpath_element" + And I log out + Then I log in as "student5" + And I open messaging + And I search for "Student 1" in messaging + And I should see "No results" + + # Recipient has 'My contacts and anyone in my courses' set. + Scenario: Allow sending a message if you share a group in a shared course Given I log in as "student1" And I open messaging And I open messaging settings preferences @@ -48,10 +69,11 @@ Feature: Manage preferences And I log out Then I log in as "student4" And I open messaging - And I select "Student 1" user in messaging - And I should see "You are unable to message this user" in the "//*[@data-region='content-messages-footer-unable-to-message']" "xpath_element" + And I send "Hi!" message to "Student 1" user + And I should see "Hi!" in the "//*[@data-region='message-drawer']//div[@data-region='content-message-container']" "xpath_element" - Scenario: Allow send me a message whe you are a contact and the prefrence is my contacts only + # Recipient has 'My contacts and anyone in my courses' set. + Scenario: Disallow sending a message if you are neither a contact, nor are in the same group in a shared course Given I log in as "student1" And I open messaging And I open messaging settings preferences @@ -59,21 +81,22 @@ Feature: Manage preferences And I log out Then I log in as "student3" And I open messaging - And I send "Hi!" message to "Student 1" user - And I should see "Hi!" in the "//*[@data-region='message-drawer']//div[@data-region='content-message-container']" "xpath_element" + And I search for "Student 1" in messaging + And I should see "No results" - Scenario: Allowed to send a message if you are not contact to the sender and you are not in the same course + # Recipient has 'Anyone on the site' set. Only users whose profiles are visible can be found via the search. + Scenario: Disallow sending a message if you are neither a contact nor do you share a course with the user. Given I log in as "student1" And I open messaging And I open messaging settings preferences When I click on "//label[text()[contains(.,'Anyone on the site')]]" "xpath_element" And I log out - Then I log in as "student4" + Then I log in as "student5" And I open messaging - And I send "Hi!" message to "Student 1" user - And I should see "Hi!" in the "//*[@data-region='message-drawer']//div[@data-region='content-message-container']" "xpath_element" + And I search for "Student 1" in messaging + And I should see "No results" - Scenario: Allow send a message using Enter button + Scenario: Sending a message when 'User enter to send' is enabled Given I log in as "student1" And I open messaging And I select "Student 2" user in messaging @@ -81,7 +104,7 @@ Feature: Manage preferences And I press key "13" in "//textarea[@data-region='send-message-txt']" "xpath_element" Then I should see "Hi!" in the "//*[@data-region='message-drawer']//div[@data-region='content-message-container']" "xpath_element" - Scenario: No allow to send a messade using Enter button + Scenario: Sending a message after 'Use enter to send' is disabled Given I log in as "student1" And I open messaging And I open messaging settings preferences diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index f8a209bd894..5099995bafb 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -2554,11 +2554,12 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { * profile. */ public function test_message_search_users_messagingallusers_disabled() { + global $DB; $this->resetAfterTest(); // Create some users. $users = []; - foreach (range(1, 7) as $i) { + 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; @@ -2568,6 +2569,8 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { // Enrol a few users in the same course, but leave them as non-contacts. $course1 = $this->getDataGenerator()->create_course(); + $course2 = $this->getDataGenerator()->create_course(); + $this->setAdminUser(); $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id); $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id); @@ -2578,6 +2581,11 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { \core_message\api::add_contact($users[3]->id, $users[1]->id); \core_message\api::add_contact($users[1]->id, $users[4]->id); + // Enrol a user as a teacher in the course, and make the teacher role a course contact role. + $this->getDataGenerator()->enrol_user($users[8]->id, $course2->id, 'editingteacher'); + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + set_config('coursecontact', $teacherrole->id); + // Create individual conversations between some users, one contact and one non-contact. \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, [$users[1]->id, $users[2]->id]); @@ -2619,7 +2627,8 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $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. + // When site wide messaging is disabled, we expect to see only those users who we share a course with and whose profiles + // are visible in that course. This excludes users like course contacts. $this->assertCount(2, $noncontacts); $this->assertEquals($users[6]->id, $noncontacts[0]['id']); $this->assertEquals($users[7]->id, $noncontacts[1]['id']); @@ -2639,11 +2648,12 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { * can view their respective profile. */ public function test_message_search_users_messagingallusers_enabled() { + global $DB; $this->resetAfterTest(); // Create some users. $users = []; - foreach (range(1, 8) as $i) { + foreach (range(1, 9) 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; @@ -2653,6 +2663,8 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { // Enrol a few users in the same course, but leave them as non-contacts. $course1 = $this->getDataGenerator()->create_course(); + $course2 = $this->getDataGenerator()->create_course(); + $this->setAdminUser(); $this->getDataGenerator()->enrol_user($users[1]->id, $course1->id); $this->getDataGenerator()->enrol_user($users[6]->id, $course1->id); @@ -2663,6 +2675,11 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { \core_message\api::add_contact($users[3]->id, $users[1]->id); \core_message\api::add_contact($users[1]->id, $users[4]->id); + // Enrol a user as a teacher in the course, and make the teacher role a course contact role. + $this->getDataGenerator()->enrol_user($users[9]->id, $course2->id, 'editingteacher'); + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + set_config('coursecontact', $teacherrole->id); + // Create individual conversations between some users, one contact and one non-contact. \core_message\api::create_conversation(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, [$users[1]->id, $users[2]->id]); @@ -2704,24 +2721,19 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $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']); + // If site wide messaging is enabled, we expect to be able to search for any users whose profiles we can view. + // In this case, as a student, that's the course contact for course2 and those noncontacts sharing a course with user1. + $this->assertCount(3, $noncontacts); + $this->assertEquals($users[6]->id, $noncontacts[0]['id']); + $this->assertEquals($users[7]->id, $noncontacts[1]['id']); + $this->assertEquals($users[9]->id, $noncontacts[2]['id']); // Verify the correct conversations were returned for the non-contacts. - $this->assertCount(0, $noncontacts[0]['conversations']); - + $this->assertCount(1, $noncontacts[0]['conversations']); $this->assertCount(1, $noncontacts[1]['conversations']); - $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, $noncontacts[1]['conversations'][0]['type']); - - $this->assertCount(1, $noncontacts[2]['conversations']); - $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[2]['conversations'][0]['type']); - - $this->assertCount(1, $noncontacts[3]['conversations']); - $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[3]['conversations'][0]['type']); + $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_INDIVIDUAL, $noncontacts[0]['conversations'][0]['type']); + $this->assertEquals(\core_message\api::MESSAGE_CONVERSATION_TYPE_GROUP, $noncontacts[1]['conversations'][0]['type']); + $this->assertCount(0, $noncontacts[2]['conversations']); } /** @@ -2907,12 +2919,10 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $this->assertEquals($users[3]->id, $contacts[1]['id']); // Check that we retrieved the correct non-contacts. - // 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']); + // Site-wide messaging is disabled, so we expect to be able to search for any users whose profile we can view. + $this->assertCount(2, $noncontacts); + $this->assertEquals($users[6]->id, $noncontacts[0]['id']); + $this->assertEquals($users[7]->id, $noncontacts[1]['id']); } /** diff --git a/message/tests/helper_test.php b/message/tests/helper_test.php index 8de40f6493f..7b27d56ba1f 100644 --- a/message/tests/helper_test.php +++ b/message/tests/helper_test.php @@ -67,4 +67,114 @@ class core_message_helper_testcase extends advanced_testcase { $this->assertEquals($user4->id, array_shift($memberinfo)->id); $this->assertEquals($user2->id, array_shift($memberinfo)->id); } + + /** + * Test search_get_user_details returns the correct profile data when $CFG->messagingallusers is disabled. + */ + public function test_search_get_user_details_sitewide_disabled() { + global $DB; + set_config('messagingallusers', false); + + // Two students sharing course 1, visible profile within course (no groups). + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + $course1 = $this->getDataGenerator()->create_course((object) ['groupmode' => 0]); + $this->getDataGenerator()->enrol_user($user1->id, $course1->id); + $this->getDataGenerator()->enrol_user($user2->id, $course1->id); + + // A teacher in course 1. + $user3 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user3->id, $course1->id, 'editingteacher'); + + // Two students sharing course 2, separate groups (profiles not visible to one another). + // Note: no groups are created here, but separate groups mode alone is enough to restrict profile access. + $user4 = $this->getDataGenerator()->create_user(); + $user5 = $this->getDataGenerator()->create_user(); + $course2 = $this->getDataGenerator()->create_course((object) ['groupmode' => 1]); + $this->getDataGenerator()->enrol_user($user4->id, $course2->id); + $this->getDataGenerator()->enrol_user($user5->id, $course2->id); + + // A teacher in course 2. + $user6 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user6->id, $course2->id, 'editingteacher'); + + // Teacher and course contact in course 3. + $user7 = $this->getDataGenerator()->create_user(); + $course3 = $this->getDataGenerator()->create_course(); + $this->getDataGenerator()->enrol_user($user7->id, $course3->id, 'editingteacher'); + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + + // Make teachers course contacts. + set_config('coursecontact', $teacherrole->id); + + // User 1 should be able to see users within their course, but not course contacts or students in other courses. + $this->setUser($user1); + $this->assertNotEmpty(\core_message\helper::search_get_user_details($user2)); // Student in same course. + $this->assertEmpty(\core_message\helper::search_get_user_details($user4)); // Student in another course. + $this->assertNotEmpty(\core_message\helper::search_get_user_details($user3)); // Teacher in same course. + $this->assertEmpty(\core_message\helper::search_get_user_details($user7)); // Teacher (course contact) in another course. + + // User 3 should be able to see the teacher in their own course, but not other students in that course nor course contacts + // or students in other courses. + $this->setUser($user4); + $this->assertEmpty(\core_message\helper::search_get_user_details($user5)); // Student in same course. + $this->assertEmpty(\core_message\helper::search_get_user_details($user1)); // Student in another course. + $this->assertNotEmpty(\core_message\helper::search_get_user_details($user6)); // Teacher in same course. + $this->assertEmpty(\core_message\helper::search_get_user_details($user7)); // Teacher (course contact) in another course. + } + + /** + * Test search_get_user_details returns the correct profile data when $CFG->messagingallusers is enabled. + */ + public function test_search_get_user_details_sitewide_enabled() { + global $DB; + set_config('messagingallusers', true); + + // Two students sharing course 1, visible profile within course (no groups). + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + $course1 = $this->getDataGenerator()->create_course((object) ['groupmode' => 0]); + $this->getDataGenerator()->enrol_user($user1->id, $course1->id); + $this->getDataGenerator()->enrol_user($user2->id, $course1->id); + + // A teacher in course 1. + $user3 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user3->id, $course1->id, 'editingteacher'); + + // Two students sharing course 2, separate groups (profiles not visible to one another). + // Note: no groups are created here, but separate groups mode alone is enough to restrict profile access. + $user4 = $this->getDataGenerator()->create_user(); + $user5 = $this->getDataGenerator()->create_user(); + $course2 = $this->getDataGenerator()->create_course((object) ['groupmode' => 1]); + $this->getDataGenerator()->enrol_user($user4->id, $course2->id); + $this->getDataGenerator()->enrol_user($user5->id, $course2->id); + + // A teacher in course 2. + $user6 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user6->id, $course2->id, 'editingteacher'); + + // Teacher and course contact in course 3. + $user7 = $this->getDataGenerator()->create_user(); + $course3 = $this->getDataGenerator()->create_course(); + $this->getDataGenerator()->enrol_user($user7->id, $course3->id, 'editingteacher'); + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + + // Make teachers course contacts. + set_config('coursecontact', $teacherrole->id); + + // User 1 should be able to see users within their course and course contacts, but not students in other courses. + $this->setUser($user1); + $this->assertNotEmpty(\core_message\helper::search_get_user_details($user2)); // Student in same course. + $this->assertEmpty(\core_message\helper::search_get_user_details($user4)); // Student in another course. + $this->assertNotEmpty(\core_message\helper::search_get_user_details($user3)); // Teacher in same course. + $this->assertNotEmpty(\core_message\helper::search_get_user_details($user7)); // Teacher (course contact) in another course. + + // User 3 should be able to see the teacher in their own course, but not other students in that course nor course contacts + // or students in other courses. + $this->setUser($user4); + $this->assertEmpty(\core_message\helper::search_get_user_details($user5)); // Student in same course. + $this->assertEmpty(\core_message\helper::search_get_user_details($user1)); // Student in another course. + $this->assertNotEmpty(\core_message\helper::search_get_user_details($user6)); // Teacher in same course. + $this->assertNotEmpty(\core_message\helper::search_get_user_details($user7)); // Teacher (course contact) in another course. + } } diff --git a/user/tests/behat/view_full_profile.feature b/user/tests/behat/view_full_profile.feature index 485beb6ee8f..c235b7b2c57 100644 --- a/user/tests/behat/view_full_profile.feature +++ b/user/tests/behat/view_full_profile.feature @@ -83,9 +83,12 @@ Feature: Access to full profiles of users And I click on "//div[@class='userselector']/descendant::option[contains(., 'Student 3')]" "xpath_element" And I press "Add" And I log out + # Message search will not return a course contact unless the searcher shares a course with them, + # or site-wide messaging is enabled ($CFG->messagingallusers). When I log in as "student1" - And I view the "Student 3" contact in the message area - Then I should see "First access to site" + And I open messaging + And I search for "Student 3" in messaging + Then I should see "No results" @javascript Scenario: View full profiles of someone in the same group in a course with separate groups.