From 98d24a48245588ce3c42dc35e1f3b8864c45ebea Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Mon, 21 May 2018 11:41:42 +0800 Subject: [PATCH 1/2] MDL-62446 mod_glossary: improve privacy unit tests --- mod/glossary/tests/privacy_provider_test.php | 23 ++++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/mod/glossary/tests/privacy_provider_test.php b/mod/glossary/tests/privacy_provider_test.php index b5a09c3e688..660c5969425 100644 --- a/mod/glossary/tests/privacy_provider_test.php +++ b/mod/glossary/tests/privacy_provider_test.php @@ -84,7 +84,7 @@ class mod_glossary_privacy_provider_testcase extends \core_privacy\tests\provide $this->teacher = $teacher; $this->setUser($student->id); - $ge1 = $this->plugingenerator->create_content($glossary, ['concept' => 'first', 'approved' => 1]); + $ge1 = $this->plugingenerator->create_content($glossary, ['concept' => 'first', 'approved' => 1], ['one']); // Student create a comment on a glossary entry. $this->setUser($student); @@ -164,10 +164,13 @@ class mod_glossary_privacy_provider_testcase extends \core_privacy\tests\provide $generator->enrol_user($student2->id, $this->course->id, 'student'); $this->setUser($student2); - $ge3 = $this->plugingenerator->create_content($this->glossary, ['concept' => 'first', 'approved' => 1]); + $ge3 = $this->plugingenerator->create_content($this->glossary, ['concept' => 'first', 'approved' => 1], ['three']); $comment = $this->get_comment_object($context, $ge3->id); $comment->add('User 2 comment'); + $this->plugingenerator->create_category($this->glossary, ['cat1'], [$ge3]); + $count = $DB->count_records('glossary_entries_categories', ['entryid' => $ge3->id]); + $this->assertEquals(1, $count); core_tag_tag::set_item_tags('mod_glossary', 'glossary_entries', $ge3->id, $context, ['Pizza', 'Noodles']); // As a teacher, rate student 2 entry. @@ -178,14 +181,17 @@ class mod_glossary_privacy_provider_testcase extends \core_privacy\tests\provide // Before deletion, we should have 2 entries. $count = $DB->count_records('glossary_entries', ['glossaryid' => $this->glossary->id]); $this->assertEquals(2, $count); - + $aliascount = $DB->count_records('glossary_alias'); + $this->assertEquals(2, $aliascount); // Delete data based on context. provider::delete_data_for_all_users_in_context($context); - // After deletion, the glossary entries for that glossary activity should have been deleted. + // After deletion, the glossary entries and aliases for that glossary activity should have been deleted. $count = $DB->count_records('glossary_entries', ['glossaryid' => $this->glossary->id]); $this->assertEquals(0, $count); - + $this->assertEquals(0, $DB->count_records('glossary_alias')); + $count = $DB->count_records('glossary_entries_categories', ['entryid' => $ge3->id]); + $this->assertEquals(0, $count); $tagcount = $DB->count_records('tag_instance', ['component' => 'mod_glossary', 'itemtype' => 'glossary_entries', 'itemid' => $ge3->id]); $this->assertEquals(0, $tagcount); @@ -222,7 +228,7 @@ class mod_glossary_privacy_provider_testcase extends \core_privacy\tests\provide $this->setUser($student2); $ge3 = $this->plugingenerator->create_content($this->glossary, ['concept' => 'second user glossary entry', - 'approved' => 1]); + 'approved' => 1], ['three']); $comment = $this->get_comment_object($context1, $ge3->id); $comment->add('User 2 comment'); @@ -235,7 +241,8 @@ class mod_glossary_privacy_provider_testcase extends \core_privacy\tests\provide $tagcount = $DB->count_records('tag_instance', ['component' => 'mod_glossary', 'itemtype' => 'glossary_entries', 'itemid' => $ge3->id]); $this->assertEquals(2, $tagcount); - + $aliascount = $DB->count_records('glossary_alias', ['entryid' => $ge3->id]); + $this->assertEquals(1, $aliascount); // Create another student who will add an entry to the first glossary. $contextlist = new \core_privacy\local\request\approved_contextlist($student2, 'glossary', [$context1->id, $context2->id]); @@ -252,6 +259,8 @@ class mod_glossary_privacy_provider_testcase extends \core_privacy\tests\provide $commentcount = $DB->count_records('comments', ['component' => 'mod_glossary', 'commentarea' => 'glossary_entry', 'itemid' => $ge3->id, 'userid' => $student2->id]); $this->assertEquals(0, $commentcount); + $aliascount = $DB->count_records('glossary_alias', ['entryid' => $ge3->id]); + $this->assertEquals(0, $aliascount); // Student's 1 entries, comments and tags should not be removed. $count = $DB->count_records('glossary_entries', ['glossaryid' => $this->glossary->id, From 424441471d709abb581a660cb0b2fd70aa295eca Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Wed, 30 May 2018 13:38:15 +0800 Subject: [PATCH 2/2] MDL-62446 mod_glossary: optimize provider code --- mod/glossary/classes/privacy/provider.php | 91 ++++++++++------------- 1 file changed, 39 insertions(+), 52 deletions(-) diff --git a/mod/glossary/classes/privacy/provider.php b/mod/glossary/classes/privacy/provider.php index e4eb689f62d..691fe687a2c 100644 --- a/mod/glossary/classes/privacy/provider.php +++ b/mod/glossary/classes/privacy/provider.php @@ -116,8 +116,9 @@ class provider implements } $user = $contextlist->get_user(); - list($contextsql, $contextparams) = $DB->get_in_or_equal($contextlist->get_contextids(), SQL_PARAMS_NAMED); + $ratingquery = \core_rating\privacy\provider::get_sql_join('r', 'mod_glossary', 'entry', 'ge.id', $user->id); + $sql = "SELECT ge.id as entryid, cm.id AS cmid, ge.userid, @@ -132,37 +133,19 @@ class provider implements JOIN {course_modules} cm ON g.id = cm.instance JOIN {modules} m ON cm.module = m.id AND m.name = :modulename JOIN {context} c ON cm.id = c.instanceid AND c.contextlevel = :contextlevel + LEFT JOIN {comments} com ON com.itemid = ge.id AND com.commentarea = :commentarea AND com.userid = :commentuserid + {$ratingquery->join} WHERE c.id {$contextsql} - AND ( - ge.userid = :userid - OR - EXISTS ( - SELECT 1 - FROM {comments} com - WHERE com.commentarea = :commentarea AND com.itemid = ge.id AND com.userid = :commentuserid - ) - OR - EXISTS ( - SELECT 1 - FROM {rating} r - WHERE r.contextid = c.id - AND r.itemid = ge.id - AND r.component = :ratingcomponent - AND r.ratingarea = :ratingarea - AND r.userid = :ratinguserid - ) - ) + AND (ge.userid = :userid OR com.id IS NOT NULL OR {$ratingquery->userwhere}) ORDER BY ge.id, cm.id"; $params = [ - 'userid' => $user->id, - 'modulename' => 'glossary', - 'contextlevel' => CONTEXT_MODULE, - 'commentarea' => 'glossary_entry', - 'commentuserid' => $user->id, - 'ratingcomponent' => 'mod_glossary', - 'ratingarea' => 'entry', - 'ratinguserid' => $user->id - ] + $contextparams; + 'userid' => $user->id, + 'modulename' => 'glossary', + 'contextlevel' => CONTEXT_MODULE, + 'commentarea' => 'glossary_entry', + 'commentuserid' => $user->id + ] + $contextparams; + $params = array_merge($params, $ratingquery->params); $glossaryentries = $DB->get_recordset_sql($sql, $params); // Reference to the glossary activity seen in the last iteration of the loop. By comparing this with the @@ -266,14 +249,13 @@ class provider implements $instanceid = $cm->instance; - $entries = $DB->get_records('glossary_entries', ['glossaryid' => $instanceid]); - foreach ($entries as $entry) { - // Delete related entry categories. - $DB->delete_records('glossary_entries_categories', ['entryid' => $entry->id]); + $entries = $DB->get_records('glossary_entries', ['glossaryid' => $instanceid], '', 'id'); - // Delete related entry aliases. - $DB->delete_records('glossary_alias', ['entryid' => $entry->id]); - } + // Delete related entry aliases. + $DB->delete_records_list('glossary_alias', 'entryid', array_keys($entries)); + + // Delete related entry categories. + $DB->delete_records_list('glossary_entries_categories', 'entryid', array_keys($entries)); // Delete entry and attachment files. get_file_storage()->delete_area_files($context->id, 'mod_glossary', 'entry'); @@ -310,25 +292,30 @@ class provider implements $instanceid = $DB->get_field('course_modules', 'instance', ['id' => $context->instanceid], MUST_EXIST); - $entries = $DB->get_records('glossary_entries', ['glossaryid' => $instanceid, 'userid' => $userid]); - foreach ($entries as $entry) { - // Delete related entry categories. - $DB->delete_records('glossary_entries_categories', ['entryid' => $entry->id]); + $entries = $DB->get_records('glossary_entries', ['glossaryid' => $instanceid, 'userid' => $userid], + '', 'id'); - // Delete related entry aliases. - $DB->delete_records('glossary_alias', ['entryid' => $entry->id]); - - // Delete tags. - \core_tag\privacy\provider::delete_item_tags($context, 'mod_glossary', 'glossary_entries', $entry->id); - - // Delete entry and attachment files. - get_file_storage()->delete_area_files($context->id, 'mod_glossary', 'entry', $entry->id); - get_file_storage()->delete_area_files($context->id, 'mod_glossary', 'attachment', $entry->id); - - // Delete related ratings. - \core_rating\privacy\provider::delete_ratings($context, 'mod_glossary', 'entry', $entry->id); + if (!$entries) { + continue; } + list($insql, $inparams) = $DB->get_in_or_equal(array_keys($entries), SQL_PARAMS_NAMED); + // Delete related entry aliases. + $DB->delete_records_list('glossary_alias', 'entryid', array_keys($entries)); + + // Delete related entry categories. + $DB->delete_records_list('glossary_entries_categories', 'entryid', array_keys($entries)); + + // Delete related entry and attachment files. + get_file_storage()->delete_area_files_select($context->id, 'mod_glossary', 'entry', $insql, $inparams); + get_file_storage()->delete_area_files_select($context->id, 'mod_glossary', 'attachment', $insql, $inparams); + + // Delete user tags related to this glossary. + \core_tag\privacy\provider::delete_item_tags_select($context, 'mod_glossary', 'glossary_entries', $insql, $inparams); + + // Delete related ratings. + \core_rating\privacy\provider::delete_ratings_select($context, 'mod_glossary', 'entry', $insql, $inparams); + // Delete comments. \core_comment\privacy\provider::delete_comments_for_user($contextlist, 'mod_glossary', 'glossary_entry');