From 523342964242afc6ca87e2deb74493c3c6486810 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Wed, 24 Oct 2018 04:53:44 +1100 Subject: [PATCH] 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 | 80 +++++++---- message/tests/privacy_provider_test.php | 177 +++++++++++++++++++----- 2 files changed, 189 insertions(+), 68 deletions(-) diff --git a/message/classes/privacy/provider.php b/message/classes/privacy/provider.php index b9049e595bd..5dc24c4832e 100644 --- a/message/classes/privacy/provider.php +++ b/message/classes/privacy/provider.php @@ -175,9 +175,30 @@ 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 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). + // There is no need to check the message_user_actions table, as there needs to be a message in order to be a message action. + // So, checking messages table would suffice. + + $hasdata = false; + $hasdata = $hasdata || $DB->record_exists_select('notifications', 'useridfrom = ? OR useridto = ?', [$userid, $userid]); + $hasdata = $hasdata || $DB->record_exists('message_conversation_members', ['userid' => $userid]); + $hasdata = $hasdata || $DB->record_exists('messages', ['useridfrom' => $userid]); + $hasdata = $hasdata || $DB->record_exists_select('message_contacts', 'userid = ? OR contactid = ?', [$userid, $userid]); + $hasdata = $hasdata || $DB->record_exists_select('message_users_blocked', 'userid = ? OR blockeduserid = ?', + [$userid, $userid]); + $hasdata = $hasdata || $DB->record_exists_select('message_contact_requests', 'userid = ? OR requesteduserid = ?', + [$userid, $userid]); + + if ($hasdata) { + $contextlist->add_user_context($userid); + } return $contextlist; } @@ -192,17 +213,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); @@ -225,19 +246,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('messages'); - $DB->delete_records('message_user_actions'); - $DB->delete_records('message_conversation_members'); - $DB->delete_records('message_contacts'); - $DB->delete_records('message_contact_requests'); - $DB->delete_records('message_users_blocked'); - $DB->delete_records('notifications'); } /** @@ -246,22 +257,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('messages', ['useridfrom' => $userid]); $DB->delete_records('message_user_actions', ['userid' => $userid]); @@ -280,7 +300,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_select('message_contacts', 'userid = ? OR contactid = ?', [$userid, $userid], 'id ASC')) { @@ -302,7 +322,7 @@ class provider implements protected static function export_user_data_contact_requests(int $userid) { global $DB; - $context = \context_system::instance(); + $context = \context_user::instance($userid); if ($contactrequests = $DB->get_records_select('message_contact_requests', 'userid = ? OR requesteduserid = ?', [$userid, $userid], 'id ASC')) { @@ -334,7 +354,7 @@ class provider implements protected static function export_user_data_blocked_users(int $userid) { global $DB; - $context = \context_system::instance(); + $context = \context_user::instance($userid); if ($blockedusers = $DB->get_records('message_users_blocked', ['userid' => $userid], 'id ASC')) { $blockedusersdata = []; @@ -355,7 +375,7 @@ class provider implements protected static function export_user_data_messages(int $userid) { global $DB; - $context = \context_system::instance(); + $context = \context_user::instance($userid); $sql = "SELECT DISTINCT mcm.conversationid as id FROM {message_conversation_members} mcm @@ -427,7 +447,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 = []; $select = "useridfrom = ? OR useridto = ?"; diff --git a/message/tests/privacy_provider_test.php b/message/tests/privacy_provider_test.php index 2e15a7bb57a..2a3c197e91a 100644 --- a/message/tests/privacy_provider_test.php +++ b/message/tests/privacy_provider_test.php @@ -192,16 +192,70 @@ 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($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( + 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); + } + + /** + * 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_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( + 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(SYSCONTEXTID, $contextforuser->id); + $this->assertEquals( + context_user::instance($user2->id)->id, + $contextforuser->id); } /** @@ -220,9 +274,11 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide \core_message\api::add_contact($user1->id, $user3->id); \core_message\api::add_contact($user1->id, $user4->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']); @@ -255,9 +311,11 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide \core_message\api::create_contact_request($user3->id, $user1->id); \core_message\api::create_contact_request($user1->id, $user4->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); $contactrequests = (array) $writer->get_data([get_string('contactrequests', 'core_message')]); @@ -292,9 +350,11 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide \core_message\api::block_user($user1->id, $user3->id); \core_message\api::block_user($user1->id, $user4->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); $blockedusers = (array) $writer->get_data([get_string('blockedusers', 'core_message')]); @@ -344,9 +404,11 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide \core_message\api::delete_message($user1->id, $m2); \core_message\api::delete_message($user1->id, $m5); - $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()); @@ -440,9 +502,11 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $this->create_notification($user2->id, $user3->id, $now + (2 * DAYSECS)); $this->create_notification($user3->id, $user2->id, $now + (1 * DAYSECS)); - $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()); @@ -465,63 +529,100 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide $user2 = $this->getDataGenerator()->create_user(); $user3 = $this->getDataGenerator()->create_user(); $user4 = $this->getDataGenerator()->create_user(); + $user5 = $this->getDataGenerator()->create_user(); $now = time(); $timeread = $now - DAYSECS; - $systemcontext = \context_system::instance(); + $user1context = context_user::instance($user1->id); // Create contacts. \core_message\api::add_contact($user1->id, $user2->id); + \core_message\api::add_contact($user2->id, $user3->id); // Create contact requests. \core_message\api::create_contact_request($user1->id, $user3->id); + \core_message\api::create_contact_request($user2->id, $user4->id); // Block a user. \core_message\api::block_user($user1->id, $user3->id); + \core_message\api::block_user($user3->id, $user4->id); // Create messages. $m1 = $this->create_message($user1->id, $user2->id, $now + (9 * DAYSECS), true); $m2 = $this->create_message($user2->id, $user1->id, $now + (8 * DAYSECS)); + $m3 = $this->create_message($user2->id, $user3->id, $now + (7 * DAYSECS)); // Create notifications. $n1 = $this->create_notification($user1->id, $user2->id, $now + (9 * DAYSECS), $timeread); $n2 = $this->create_notification($user2->id, $user1->id, $now + (8 * DAYSECS)); + $n3 = $this->create_notification($user2->id, $user3->id, $now + (7 * DAYSECS)); // Delete one of the messages. \core_message\api::delete_message($user1->id, $m2); - // There should be 1 contact. - $this->assertEquals(1, $DB->count_records('message_contacts')); + // There should be 2 contacts. + $this->assertEquals(2, $DB->count_records('message_contacts')); - // There should be 1 contact request. - $this->assertEquals(1, $DB->count_records('message_contact_requests')); + // There should be 2 contact requests. + $this->assertEquals(2, $DB->count_records('message_contact_requests')); - // There should be 1 blocked user. - $this->assertEquals(1, $DB->count_records('message_users_blocked')); + // There should be 2 blocked users. + $this->assertEquals(2, $DB->count_records('message_users_blocked')); - // There should be two messages. - $this->assertEquals(2, $DB->count_records('messages')); + // There should be 3 messages. + $this->assertEquals(3, $DB->count_records('messages')); - // There should be two user actions - one for reading the message, one for deleting. + // There should be 2 user actions - one for reading the message, one for deleting. $this->assertEquals(2, $DB->count_records('message_user_actions')); - // There should be two conversation members. - $this->assertEquals(2, $DB->count_records('message_conversation_members')); + // There should be 4 conversation members. + $this->assertEquals(4, $DB->count_records('message_conversation_members')); - // There should be two notifications + one for the contact request. - $this->assertEquals(3, $DB->count_records('notifications')); + // There should be 3 notifications + 2 for the contact request. + $this->assertEquals(5, $DB->count_records('notifications')); - 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_contact_requests')); - $this->assertEquals(0, $DB->count_records('message_users_blocked')); - $this->assertEquals(0, $DB->count_records('messages')); - $this->assertEquals(0, $DB->count_records('message_user_actions')); - $this->assertEquals(0, $DB->count_records('message_conversation_members')); - $this->assertEquals(0, $DB->count_records('notifications')); + // Confirm there is only 1 contact left. + $this->assertEquals(1, $DB->count_records('message_contacts')); + // And it is not related to user1. + $this->assertEquals(0, + $DB->count_records_select('message_contacts', 'userid = ? OR contactid = ?', [$user1->id, $user1->id])); + + // Confirm there is only 1 contact request left. + $this->assertEquals(1, $DB->count_records('message_contact_requests')); + // And it is not related to user1. + $this->assertEquals(0, + $DB->count_records_select('message_contact_requests', 'userid = ? OR requesteduserid = ?', + [$user1->id, $user1->id])); + + // Confirm there is only 1 blocked user left. + $this->assertEquals(1, $DB->count_records('message_users_blocked')); + // And it is not related to user1. + $this->assertEquals(0, + $DB->count_records_select('message_users_blocked', 'userid = ? OR blockeduserid = ?', [$user1->id, $user1->id])); + + // Confirm there are only 2 messages left. + $this->assertEquals(2, $DB->count_records('messages')); + // And none of them are from user1. + $this->assertEquals(0, $DB->count_records('messages', ['useridfrom' => $user1->id])); + + // Confirm there is only 1 user action left - the one that is for user2 reading the message. + $this->assertEquals(1, $DB->count_records('message_user_actions')); + // And it is not for user1. + $this->assertEquals(0, $DB->count_records('message_user_actions', ['userid' => $user1->id])); + + // Confirm there are only 3 conversation members left. + $this->assertEquals(3, $DB->count_records('message_conversation_members')); + // And user1 is not in any conversation. + $this->assertEquals(0, $DB->count_records('message_conversation_members', ['userid' => $user1->id])); + + // Confirm there is only 1 notification + 1 for the contact request. + $this->assertEquals(2, $DB->count_records('notifications')); + // And it is not related to user1. + $this->assertEquals(0, + $DB->count_records_select('notifications', 'useridfrom = ? OR useridto = ? ', [$user1->id, $user1->id])); } /** @@ -588,9 +689,9 @@ class core_message_privacy_provider_testcase extends \core_privacy\tests\provide // There should be three notifications + two for the contact requests. $this->assertEquals(5, $DB->count_records('notifications')); - $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. -- 2.17.1