From 734d9f403b10605fe0935d4a6f5fae6ca2af6d48 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Fri, 26 Oct 2018 17:07:52 +1100 Subject: [PATCH 1/4] MDL-63712 core_message: Data should be in user context, not system This issue is a part of the MDL-62560 Epic. --- message/classes/privacy/provider.php | 67 +++++++----- message/tests/privacy_provider_test.php | 130 +++++++++++++++++++----- 2 files changed, 148 insertions(+), 49 deletions(-) diff --git a/message/classes/privacy/provider.php b/message/classes/privacy/provider.php index 5c3f84ee5cc..3d5c4f8a187 100644 --- a/message/classes/privacy/provider.php +++ b/message/classes/privacy/provider.php @@ -140,9 +140,25 @@ class provider implements * @return contextlist the list of contexts containing user info for the user. */ public static function get_contexts_for_userid(int $userid) : contextlist { - // Messages are in the system context. + global $DB; + $contextlist = new contextlist(); - $contextlist->add_system_context(); + + // Messages are in the user context. + // For the sake of performance, there is no need to call add_from_sql for each of the bellow cases. + // It is enough to add the user's context as soon as we come to the conclusion that the user has some data. + // Also, the order of checking is sorted by the probability of occurrence (just by guess). + + $hasdata = false; + $hasdata = $hasdata || $DB->record_exists_select('message', 'useridfrom = ? OR ( notification = 1 AND useridto = ?)', + [$userid, $userid]); + $hasdata = $hasdata || $DB->record_exists_select('message_read', 'useridfrom = ? OR ( notification = 1 AND useridto = ?)', + [$userid, $userid]); + $hasdata = $hasdata || $DB->record_exists_select('message_contacts', 'userid = ? OR contactid = ?', [$userid, $userid]); + + if ($hasdata) { + $contextlist->add_user_context($userid); + } return $contextlist; } @@ -157,17 +173,17 @@ class provider implements return; } - // Remove non-system contexts. If it ends up empty then early return. - $contexts = array_filter($contextlist->get_contexts(), function($context) { - return $context->contextlevel == CONTEXT_SYSTEM; + $userid = $contextlist->get_user()->id; + + // Remove non-user and invalid contexts. If it ends up empty then early return. + $contexts = array_filter($contextlist->get_contexts(), function($context) use($userid) { + return $context->contextlevel == CONTEXT_USER && $context->instanceid == $userid; }); if (empty($contexts)) { return; } - $userid = $contextlist->get_user()->id; - // Export the contacts. self::export_user_data_contacts($userid); @@ -184,15 +200,9 @@ class provider implements * @param \context $context the context to delete in. */ public static function delete_data_for_all_users_in_context(\context $context) { - global $DB; - - if (!$context instanceof \context_system) { - return; + if ($context instanceof \context_user) { + static::delete_user_data($context->instanceid); } - - $DB->delete_records('message'); - $DB->delete_records('message_read'); - $DB->delete_records('message_contacts'); } /** @@ -201,22 +211,31 @@ class provider implements * @param approved_contextlist $contextlist a list of contexts approved for deletion. */ public static function delete_data_for_user(approved_contextlist $contextlist) { - global $DB; - if (empty($contextlist->count())) { return; } - // Remove non-system contexts. If it ends up empty then early return. - $contexts = array_filter($contextlist->get_contexts(), function($context) { - return $context->contextlevel == CONTEXT_SYSTEM; + $userid = $contextlist->get_user()->id; + + // Remove non-user and invalid contexts. If it ends up empty then early return. + $contexts = array_filter($contextlist->get_contexts(), function($context) use ($userid) { + return $context->contextlevel == CONTEXT_USER && $context->instanceid == $userid; }); if (empty($contexts)) { return; } - $userid = $contextlist->get_user()->id; + static::delete_user_data($userid); + } + + /** + * Delete all user data for the specified user. + * + * @param int $userid The user id + */ + protected static function delete_user_data(int $userid) { + global $DB; $DB->delete_records_select('message', 'useridfrom = ? AND notification = 0', [$userid]); $DB->delete_records_select('message_read', 'useridfrom = ? AND notification = 0', [$userid]); @@ -233,7 +252,7 @@ class provider implements protected static function export_user_data_contacts(int $userid) { global $DB; - $context = \context_system::instance(); + $context = \context_user::instance($userid); // Get the user's contacts. if ($contacts = $DB->get_records('message_contacts', ['userid' => $userid], 'id ASC')) { @@ -256,7 +275,7 @@ class provider implements protected static function export_user_data_messages(int $userid) { global $DB; - $context = \context_system::instance(); + $context = \context_user::instance($userid); $users = self::get_userids_in_conversation_with($userid); if (!empty($users)) { @@ -334,7 +353,7 @@ class provider implements protected static function export_user_data_notifications(int $userid) { global $DB; - $context = \context_system::instance(); + $context = \context_user::instance($userid); $notificationdata = []; $sql = "SELECT id, useridfrom, useridto, subject, fullmessage, fullmessageformat, diff --git a/message/tests/privacy_provider_test.php b/message/tests/privacy_provider_test.php index 9c2560c4efb..d726c710fba 100644 --- a/message/tests/privacy_provider_test.php +++ b/message/tests/privacy_provider_test.php @@ -159,16 +159,66 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide } /** - * Test for provider::get_contexts_for_userid(). + * Test for provider::get_contexts_for_userid() when there is no message or notification. */ - public function test_get_contexts_for_userid() { + public function test_get_contexts_for_userid_no_data() { $this->resetAfterTest(); $user = $this->getDataGenerator()->create_user(); $contextlist = provider::get_contexts_for_userid($user->id); + $this->assertEmpty($contextlist); + } + + /** + * Test for provider::get_contexts_for_userid() when there is a message between users. + */ + public function test_get_contexts_for_userid_with_message() { + $this->resetAfterTest(); + + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + + $this->create_message_or_notification($user1->id, $user2->id, time() - (9 * DAYSECS)); + + // Test for the sender. + $contextlist = provider::get_contexts_for_userid($user1->id); $this->assertCount(1, $contextlist); $contextforuser = $contextlist->current(); - $this->assertEquals(SYSCONTEXTID, $contextforuser->id); + $this->assertEquals( + context_user::instance($user1->id)->id, + $contextforuser->id); + + // Test for the receiver. + $contextlist = provider::get_contexts_for_userid($user2->id); + $this->assertCount(0, $contextlist); + } + + /** + * Test for provider::get_contexts_for_userid() when there is a notification between users. + */ + public function test_get_contexts_for_userid_with_notification() { + $this->resetAfterTest(); + + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + + $this->create_message_or_notification($user1->id, $user2->id, time() - (9 * DAYSECS), true); + + // Test for the sender. + $contextlist = provider::get_contexts_for_userid($user1->id); + $this->assertCount(1, $contextlist); + $contextforuser = $contextlist->current(); + $this->assertEquals( + context_user::instance($user1->id)->id, + $contextforuser->id); + + // Test for the receiver. + $contextlist = provider::get_contexts_for_userid($user2->id); + $this->assertCount(1, $contextlist); + $contextforuser = $contextlist->current(); + $this->assertEquals( + context_user::instance($user2->id)->id, + $contextforuser->id); } /** @@ -190,9 +240,11 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide message_add_contact($user3->id, 0, $user1->id); message_add_contact($user4->id, 1, $user1->id); - $this->export_context_data_for_user($user1->id, \context_system::instance(), 'core_message'); + $user1context = context_user::instance($user1->id); - $writer = writer::with_context(\context_system::instance()); + $this->export_context_data_for_user($user1->id, $user1context, 'core_message'); + + $writer = writer::with_context($user1context); $contacts = (array) $writer->get_data([get_string('contacts', 'core_message')]); usort($contacts, ['static', 'sort_contacts']); @@ -248,9 +300,11 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide message_delete_message($dbm2, $user1->id); message_delete_message($dbm5, $user1->id); - $this->export_context_data_for_user($user1->id, \context_system::instance(), 'core_message'); + $user1context = context_user::instance($user1->id); - $writer = writer::with_context(\context_system::instance()); + $this->export_context_data_for_user($user1->id, $user1context, 'core_message'); + + $writer = writer::with_context($user1context); $this->assertTrue($writer->has_any_data()); @@ -344,9 +398,11 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $this->create_message_or_notification($user2->id, $user3->id, $now + (2 * DAYSECS), true); $this->create_message_or_notification($user3->id, $user2->id, $now + (1 * DAYSECS), true); - $this->export_context_data_for_user($user1->id, \context_system::instance(), 'core_message'); + $user1context = context_user::instance($user1->id); - $writer = writer::with_context(\context_system::instance()); + $this->export_context_data_for_user($user1->id, $user1context, 'core_message'); + + $writer = writer::with_context($user1context); $this->assertTrue($writer->has_any_data()); @@ -367,53 +423,77 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide // Create users to test with. $user1 = $this->getDataGenerator()->create_user(); $user2 = $this->getDataGenerator()->create_user(); + $user3 = $this->getDataGenerator()->create_user(); $now = time(); $timeread = $now - DAYSECS; - $systemcontext = \context_system::instance(); + $user1context = context_user::instance($user1->id); // Create contacts. message_add_contact($user1->id, 0, $user2->id); message_add_contact($user2->id, 0, $user1->id); + message_add_contact($user2->id, 0, $user3->id); + message_add_contact($user3->id, 0, $user2->id); // Create messages. $m1 = $this->create_message_or_notification($user1->id, $user2->id, $now + (9 * DAYSECS), false, $timeread); $m2 = $this->create_message_or_notification($user2->id, $user1->id, $now + (8 * DAYSECS)); $m3 = $this->create_message_or_notification($user2->id, $user1->id, $now + (7 * DAYSECS), false, $timeread); $m4 = $this->create_message_or_notification($user1->id, $user2->id, $now + (6 * DAYSECS)); + $m5 = $this->create_message_or_notification($user2->id, $user3->id, $now + (7 * DAYSECS)); // Create notifications. $n1 = $this->create_message_or_notification($user1->id, $user2->id, $now + (9 * DAYSECS), true, $timeread); $n2 = $this->create_message_or_notification($user2->id, $user1->id, $now + (8 * DAYSECS), true); - $m3 = $this->create_message_or_notification($user2->id, $user1->id, $now + (7 * DAYSECS), true, $timeread); - $m4 = $this->create_message_or_notification($user1->id, $user2->id, $now + (6 * DAYSECS), true); + $n3 = $this->create_message_or_notification($user2->id, $user1->id, $now + (7 * DAYSECS), true, $timeread); + $n4 = $this->create_message_or_notification($user1->id, $user2->id, $now + (6 * DAYSECS), true); + $n5 = $this->create_message_or_notification($user2->id, $user3->id, $now + (7 * DAYSECS), true); // Delete one of the messages. $dbm2 = $DB->get_record('message', ['id' => $m2]); message_delete_message($dbm2, $user1->id); - // There should be two contacts. - $this->assertEquals(2, $DB->count_records('message_contacts')); + // There should be 4 contacts. + $this->assertEquals(4, $DB->count_records('message_contacts')); - // There should be two unread messages. - $this->assertEquals(2, $DB->count_records('message', ['notification' => 0])); + // There should be 3 unread messages. + $this->assertEquals(3, $DB->count_records('message', ['notification' => 0])); // There should be two read messages. $this->assertEquals(2, $DB->count_records('message_read', ['notification' => 0])); - // There should be two unread notifications. - $this->assertEquals(2, $DB->count_records('message', ['notification' => 1])); + // There should be 3 unread notifications. + $this->assertEquals(3, $DB->count_records('message', ['notification' => 1])); // There should be two read notifications. $this->assertEquals(2, $DB->count_records('message_read', ['notification' => 1])); - provider::delete_data_for_all_users_in_context($systemcontext); + provider::delete_data_for_all_users_in_context($user1context); - // Confirm all has been deleted. - $this->assertEquals(0, $DB->count_records('message_contacts')); - $this->assertEquals(0, $DB->count_records('message')); - $this->assertEquals(0, $DB->count_records('message_read')); + // Confirm there are only 2 contacts left. + $this->assertEquals(2, $DB->count_records('message_contacts')); + // And none of them are not related to user1. + $this->assertEquals(0, + $DB->count_records_select('message_contacts', 'userid = ? OR contactid = ?', [$user1->id, $user1->id])); + + // Confirm there are only 2 unread messages left. + $this->assertEquals(2, $DB->count_records('message', ['notification' => 0])); + // And none of them are from user1. + $this->assertEquals(0, $DB->count_records('message', ['notification' => 0, 'useridfrom' => $user1->id])); + + // Confirm there is only 1 read message left. + $this->assertEquals(1, $DB->count_records('message_read', ['notification' => 0])); + // And it is not from user1. + $this->assertEquals(0, $DB->count_records('message_read', ['notification' => 0, 'useridfrom' => $user1->id])); + + // Confirm there is only 1 unread notification left. + $this->assertEquals(1, $DB->count_records('message', ['notification' => 1])); + // And it is not from user1. + $this->assertEquals(0, $DB->count_records('message', ['notification' => 1, 'useridfrom' => $user1->id])); + + // Confirm there is no read notifications left. + $this->assertEquals(0, $DB->count_records('message_read', ['notification' => 1])); } /** @@ -468,9 +548,9 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide // There should be two read notifications. $this->assertEquals(2, $DB->count_records('message_read', ['notification' => 1])); - $systemcontext = \context_system::instance(); + $user1context = context_user::instance($user1->id); $contextlist = new \core_privacy\local\request\approved_contextlist($user1, 'core_message', - [$systemcontext->id]); + [$user1context->id]); provider::delete_data_for_user($contextlist); // Confirm the user 2 data still exists. From 23d4767f8c12fc33ecf9db21b00a152bcc30d19e Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Fri, 26 Oct 2018 17:25:16 +1100 Subject: [PATCH 2/4] MDL-63712 core_message: support removal of multiple users in a context This issue is a part of the MDL-62560 Epic. --- message/classes/privacy/provider.php | 63 +++++++++- message/tests/privacy_provider_test.php | 159 ++++++++++++++++++++++++ 2 files changed, 221 insertions(+), 1 deletion(-) diff --git a/message/classes/privacy/provider.php b/message/classes/privacy/provider.php index 3d5c4f8a187..d553bff3446 100644 --- a/message/classes/privacy/provider.php +++ b/message/classes/privacy/provider.php @@ -18,6 +18,7 @@ * Privacy Subsystem implementation for core_message. * * @package core_message + * @category privacy * @copyright 2018 Mark Nelson * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ @@ -25,8 +26,10 @@ namespace core_message\privacy; use core_privacy\local\metadata\collection; use core_privacy\local\request\approved_contextlist; +use core_privacy\local\request\approved_userlist; use core_privacy\local\request\contextlist; use core_privacy\local\request\transform; +use core_privacy\local\request\userlist; use core_privacy\local\request\writer; defined('MOODLE_INTERNAL') || die(); @@ -40,7 +43,8 @@ defined('MOODLE_INTERNAL') || die(); class provider implements \core_privacy\local\metadata\provider, \core_privacy\local\request\subsystem\provider, - \core_privacy\local\request\user_preference_provider { + \core_privacy\local\request\user_preference_provider, + \core_privacy\local\request\core_userlist_provider { /** * Return the fields which contain personal data. @@ -163,6 +167,39 @@ class provider implements return $contextlist; } + /** + * Get the list of users who have data within a context. + * + * @param userlist $userlist The userlist containing the list of users who have data in this context/plugin combination. + */ + public static function get_users_in_context(userlist $userlist) { + global $DB; + + $context = $userlist->get_context(); + + if (!$context instanceof \context_user) { + return; + } + + $userid = $context->instanceid; + + // Messages are in the user context. + // For the sake of performance, there is no need to call add_from_sql for each of the bellow cases. + // It is enough to add the user's context as soon as we come to the conclusion that the user has some data. + // Also, the order of checking is sorted by the probability of occurrence (just by guess). + + $hasdata = false; + $hasdata = $hasdata || $DB->record_exists_select('message', 'useridfrom = ? OR ( notification = 1 AND useridto = ?)', + [$userid, $userid]); + $hasdata = $hasdata || $DB->record_exists_select('message_read', 'useridfrom = ? OR ( notification = 1 AND useridto = ?)', + [$userid, $userid]); + $hasdata = $hasdata || $DB->record_exists_select('message_contacts', 'userid = ? OR contactid = ?', [$userid, $userid]); + + if ($hasdata) { + $userlist->add_user($userid); + } + } + /** * Export personal data for the given approved_contextlist. User and context information is contained within the contextlist. * @@ -229,6 +266,30 @@ class provider implements static::delete_user_data($userid); } + /** + * Delete multiple users within a single context. + * + * @param approved_userlist $userlist The approved context and user information to delete information for. + */ + public static function delete_data_for_users(approved_userlist $userlist) { + $context = $userlist->get_context(); + + if (!$context instanceof \context_user) { + return; + } + + // Remove invalid users. If it ends up empty then early return. + $userids = array_filter($userlist->get_userids(), function($userid) use ($context) { + return $context->instanceid == $userid; + }); + + if (empty($userids)) { + return; + } + + static::delete_user_data($context->instanceid); + } + /** * Delete all user data for the specified user. * diff --git a/message/tests/privacy_provider_test.php b/message/tests/privacy_provider_test.php index d726c710fba..b12e27aaf87 100644 --- a/message/tests/privacy_provider_test.php +++ b/message/tests/privacy_provider_test.php @@ -582,6 +582,165 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $this->assertEquals($n4, $notificationsread->id); } + /** + * Test for provider::get_users_in_context() when there is no message or notification. + */ + public function test_get_users_in_context_no_data() { + $this->resetAfterTest(); + + $user = $this->getDataGenerator()->create_user(); + $usercontext = context_user::instance($user->id); + + $userlist = new \core_privacy\local\request\userlist($usercontext, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + + $this->assertEmpty($userlist->get_userids()); + } + + /** + * Test for provider::get_users_in_context() when there is a message between users. + */ + public function test_get_users_in_context_with_message() { + $this->resetAfterTest(); + + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + + $user1context = context_user::instance($user1->id); + $user2context = context_user::instance($user2->id); + + $this->create_message_or_notification($user1->id, $user2->id, time() - (9 * DAYSECS)); + + // Test for the sender. + $userlist = new \core_privacy\local\request\userlist($user1context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(1, $userlist); + $userincontext = $userlist->current(); + $this->assertEquals($user1->id, $userincontext->id); + + // Test for the receiver. + $userlist = new \core_privacy\local\request\userlist($user2context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(0, $userlist); + } + + /** + * Test for provider::get_users_in_context() when there is a notification between users. + */ + public function test_get_users_in_context_with_notification() { + $this->resetAfterTest(); + + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + + $user1context = context_user::instance($user1->id); + $user2context = context_user::instance($user2->id); + + $this->create_message_or_notification($user1->id, $user2->id, time() - (9 * DAYSECS), true); + + // Test for the sender. + $userlist = new \core_privacy\local\request\userlist($user1context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(1, $userlist); + $userincontext = $userlist->current(); + $this->assertEquals($user1->id, $userincontext->id); + + // Test for the receiver. + $userlist = new \core_privacy\local\request\userlist($user2context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(1, $userlist); + $userincontext = $userlist->current(); + $this->assertEquals($user2->id, $userincontext->id); + } + + /** + * Test for provider::delete_data_for_users(). + */ + public function test_delete_data_for_users() { + global $DB; + + $this->resetAfterTest(); + + // Create users to test with. + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + $user3 = $this->getDataGenerator()->create_user(); + + $now = time(); + $timeread = $now - DAYSECS; + + // Create contacts. + message_add_contact($user1->id, 0, $user2->id); + message_add_contact($user2->id, 0, $user1->id); + message_add_contact($user2->id, 0, $user3->id); + + // Create messages. + $m1 = $this->create_message_or_notification($user1->id, $user2->id, $now + (9 * DAYSECS), false, $timeread); + $m2 = $this->create_message_or_notification($user2->id, $user1->id, $now + (8 * DAYSECS)); + $m3 = $this->create_message_or_notification($user2->id, $user1->id, $now + (7 * DAYSECS), false, $timeread); + $m4 = $this->create_message_or_notification($user1->id, $user2->id, $now + (6 * DAYSECS)); + + // Create notifications. + $n1 = $this->create_message_or_notification($user1->id, $user2->id, $now + (9 * DAYSECS), true, $timeread); + $n2 = $this->create_message_or_notification($user2->id, $user1->id, $now + (8 * DAYSECS), true); + $n3 = $this->create_message_or_notification($user2->id, $user3->id, $now + (8 * DAYSECS), true); + $n4 = $this->create_message_or_notification($user3->id, $user2->id, $now + (8 * DAYSECS), true, $timeread); + + // Delete one of the messages. + $dbm2 = $DB->get_record('message', ['id' => $m2]); + message_delete_message($dbm2, $user1->id); + + // There should be three contacts. + $this->assertEquals(3, $DB->count_records('message_contacts')); + + // There should be two unread messages. + $this->assertEquals(2, $DB->count_records('message', ['notification' => 0])); + + // There should be two read messages. + $this->assertEquals(2, $DB->count_records('message_read', ['notification' => 0])); + + // There should be two unread notifications. + $this->assertEquals(2, $DB->count_records('message', ['notification' => 1])); + + // There should be two read notifications. + $this->assertEquals(2, $DB->count_records('message_read', ['notification' => 1])); + + $user1context = context_user::instance($user1->id); + $approveduserlist = new \core_privacy\local\request\approved_userlist($user1context, 'core_message', + [$user1->id, $user2->id]); + provider::delete_data_for_users($approveduserlist); + + // Only user1's data should be deleted. User2 should be skipped as user2 is an invalid user for user1context. + + // Confirm the user 2 data still exists. + $contacts = $DB->get_records('message_contacts'); + $messages = $DB->get_records('message', ['notification' => 0]); + $messagesread = $DB->get_records('message_read', ['notification' => 0]); + $notifications = $DB->get_records('message', ['notification' => 1]); + $notificationsread = $DB->get_records('message_read', ['notification' => 1]); + + $this->assertCount(1, $contacts); + $contact = reset($contacts); + $this->assertEquals($user3->id, $contact->userid); + $this->assertEquals($user2->id, $contact->contactid); + + $this->assertCount(1, $messages); + $message = reset($messages); + $this->assertEquals($m2, $message->id); + + $this->assertCount(1, $messagesread); + $messagesread = reset($messagesread); + $this->assertEquals($m3, $messagesread->id); + + $this->assertCount(1, $notifications); + $notifications = reset($notifications); + $this->assertEquals($n3, $notifications->id); + + $this->assertCount(1, $notificationsread); + $notificationsread = reset($notificationsread); + $this->assertEquals($n4, $notificationsread->id); + } + /** * Creates a message or notification to be used for testing. * From a05d4df146a38afd74555b9c373441546346ef45 Mon Sep 17 00:00:00 2001 From: Michael Hawkins Date: Thu, 1 Nov 2018 16:13:49 +0800 Subject: [PATCH 3/4] MDL-63712 core_message: Minor provider improvements This issue is part of the MDL-62560 Epic. --- message/classes/privacy/provider.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/message/classes/privacy/provider.php b/message/classes/privacy/provider.php index d553bff3446..07070eb7d7c 100644 --- a/message/classes/privacy/provider.php +++ b/message/classes/privacy/provider.php @@ -184,7 +184,7 @@ class provider implements $userid = $context->instanceid; // Messages are in the user context. - // For the sake of performance, there is no need to call add_from_sql for each of the bellow cases. + // For the sake of performance, there is no need to call add_from_sql for each of the below cases. // It is enough to add the user's context as soon as we come to the conclusion that the user has some data. // Also, the order of checking is sorted by the probability of occurrence (just by guess). @@ -298,10 +298,8 @@ class provider implements protected static function delete_user_data(int $userid) { global $DB; - $DB->delete_records_select('message', 'useridfrom = ? AND notification = 0', [$userid]); - $DB->delete_records_select('message_read', 'useridfrom = ? AND notification = 0', [$userid]); - $DB->delete_records_select('message', '(useridfrom = ? OR useridto = ?) AND notification = 1', [$userid, $userid]); - $DB->delete_records_select('message_read', '(useridfrom = ? OR useridto = ?) AND notification = 1', [$userid, $userid]); + $DB->delete_records_select('message', 'useridfrom = ? OR (useridto = ? AND notification = 1)', [$userid, $userid]); + $DB->delete_records_select('message_read', 'useridfrom = ? OR (useridto = ? AND notification = 1)', [$userid, $userid]); $DB->delete_records_select('message_contacts', 'userid = ? OR contactid = ?', [$userid, $userid]); } From 9898c108a321e56674c76dbd86d8d5dc646354cf Mon Sep 17 00:00:00 2001 From: Michael Hawkins Date: Thu, 1 Nov 2018 16:14:01 +0800 Subject: [PATCH 4/4] MDL-63712 core_message: Unit test improvements for fetching context/user This issue is part of the MDL-62560 Epic. --- message/tests/privacy_provider_test.php | 99 +++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/message/tests/privacy_provider_test.php b/message/tests/privacy_provider_test.php index b12e27aaf87..0b06e1c80d0 100644 --- a/message/tests/privacy_provider_test.php +++ b/message/tests/privacy_provider_test.php @@ -178,6 +178,12 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $user1 = $this->getDataGenerator()->create_user(); $user2 = $this->getDataGenerator()->create_user(); + // Test nothing is found before message is sent. + $contextlist = provider::get_contexts_for_userid($user1->id); + $this->assertCount(0, $contextlist); + $contextlist = provider::get_contexts_for_userid($user2->id); + $this->assertCount(0, $contextlist); + $this->create_message_or_notification($user1->id, $user2->id, time() - (9 * DAYSECS)); // Test for the sender. @@ -202,6 +208,12 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $user1 = $this->getDataGenerator()->create_user(); $user2 = $this->getDataGenerator()->create_user(); + // Test nothing is found before notification is created. + $contextlist = provider::get_contexts_for_userid($user1->id); + $this->assertCount(0, $contextlist); + $contextlist = provider::get_contexts_for_userid($user2->id); + $this->assertCount(0, $contextlist); + $this->create_message_or_notification($user1->id, $user2->id, time() - (9 * DAYSECS), true); // Test for the sender. @@ -221,6 +233,40 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $contextforuser->id); } + /** + * Test for provider::get_contexts_for_userid() when a users has a contact. + */ + public function test_get_contexts_for_userid_with_contact() { + $this->resetAfterTest(); + + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + + // Test nothing is found before contact is created. + $contextlist = provider::get_contexts_for_userid($user1->id); + $this->assertCount(0, $contextlist); + $contextlist = provider::get_contexts_for_userid($user2->id); + $this->assertCount(0, $contextlist); + + message_add_contact($user2->id, 0, $user1->id); + + // Test for the user adding the contact. + $contextlist = provider::get_contexts_for_userid($user1->id); + $this->assertCount(1, $contextlist); + $contextforuser = $contextlist->current(); + $this->assertEquals( + context_user::instance($user1->id)->id, + $contextforuser->id); + + // Test for the user who is the contact. + $contextlist = provider::get_contexts_for_userid($user2->id); + $this->assertCount(1, $contextlist); + $contextforuser = $contextlist->current(); + $this->assertEquals( + context_user::instance($user2->id)->id, + $contextforuser->id); + } + /** * Test for provider::export_user_data(). */ @@ -609,6 +655,14 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $user1context = context_user::instance($user1->id); $user2context = context_user::instance($user2->id); + // Test nothing is found before message is sent. + $userlist = new \core_privacy\local\request\userlist($user1context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(0, $userlist); + $userlist = new \core_privacy\local\request\userlist($user2context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(0, $userlist); + $this->create_message_or_notification($user1->id, $user2->id, time() - (9 * DAYSECS)); // Test for the sender. @@ -636,6 +690,14 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $user1context = context_user::instance($user1->id); $user2context = context_user::instance($user2->id); + // Test nothing is found before notification is created. + $userlist = new \core_privacy\local\request\userlist($user1context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(0, $userlist); + $userlist = new \core_privacy\local\request\userlist($user2context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(0, $userlist); + $this->create_message_or_notification($user1->id, $user2->id, time() - (9 * DAYSECS), true); // Test for the sender. @@ -653,6 +715,43 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $this->assertEquals($user2->id, $userincontext->id); } + /** + * Test for provider::get_users_in_context() when a users has a contact. + */ + public function test_get_users_in_context_with_contact() { + $this->resetAfterTest(); + + $user1 = $this->getDataGenerator()->create_user(); + $user2 = $this->getDataGenerator()->create_user(); + + $user1context = context_user::instance($user1->id); + $user2context = context_user::instance($user2->id); + + // Test nothing is found before contact is created. + $userlist = new \core_privacy\local\request\userlist($user1context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(0, $userlist); + $userlist = new \core_privacy\local\request\userlist($user2context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(0, $userlist); + + message_add_contact($user2->id, 0, $user1->id); + + // Test for the user adding the contact. + $userlist = new \core_privacy\local\request\userlist($user1context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(1, $userlist); + $userincontext = $userlist->current(); + $this->assertEquals($user1->id, $userincontext->id); + + // Test for the user who is the contact. + $userlist = new \core_privacy\local\request\userlist($user2context, 'core_message'); + \core_message\privacy\provider::get_users_in_context($userlist); + $this->assertCount(1, $userlist); + $userincontext = $userlist->current(); + $this->assertEquals($user2->id, $userincontext->id); + } + /** * Test for provider::delete_data_for_users(). */