From 138816b1b126a5b5b1fed53be8490484a3ad3775 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Mon, 19 Feb 2018 16:37:41 +0800 Subject: [PATCH 1/3] MDL-61475 mod_choice: Add implementation of Privacy API --- mod/choice/classes/privacy/provider.php | 190 ++++++++++++++++++ mod/choice/lang/en/choice.php | 5 + mod/choice/tests/privacy_provider_test.php | 212 +++++++++++++++++++++ 3 files changed, 407 insertions(+) create mode 100644 mod/choice/classes/privacy/provider.php create mode 100644 mod/choice/tests/privacy_provider_test.php diff --git a/mod/choice/classes/privacy/provider.php b/mod/choice/classes/privacy/provider.php new file mode 100644 index 00000000000..4dce12679e8 --- /dev/null +++ b/mod/choice/classes/privacy/provider.php @@ -0,0 +1,190 @@ +. + +/** + * Privacy Subsystem implementation for mod_choice. + * + * @package mod_choice + * @copyright 2018 Jun Pataleta + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace mod_choice\privacy; + +use context_module; +use core_privacy\metadata\item_collection; +use core_privacy\metadata\provider as core_provider; +use core_privacy\request\approved_contextlist; +use core_privacy\request\contextlist; +use core_privacy\request\deletion_criteria; +use core_privacy\request\plugin\provider as plugin_provider; +use core_privacy\request\writer; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Implementation of the privacy subsystem plugin provider for the choice activity module. + * + * @copyright 2018 Jun Pataleta + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class provider implements core_provider, plugin_provider { + /** + * @inheritdoc + */ + public static function get_metadata(item_collection $items) : item_collection { + $items->add_database_table( + 'choice_answers', + [ + 'choiceid' => 'privacy:metadata:choice_answers:choiceid', + 'optionid' => 'privacy:metadata:choice_answers:optionid', + 'userid' => 'privacy:metadata:choice_answers:userid', + 'timemodified' => 'privacy:metadata:choice_answers:timemodified', + ], + 'privacy:metadata:choice_answers' + ); + + return $items; + } + + /** + * @inheritdoc + */ + public static function get_contexts_for_userid(int $userid) : contextlist { + // Fetch all choice answers. + $sql = "SELECT c.id + FROM {context} c + INNER JOIN {course_modules} cm ON cm.id = c.instanceid AND c.contextlevel = :contextlevel + INNER JOIN {modules} m ON m.id = cm.module AND m.name = :modname + INNER JOIN {choice} ch ON ch.id = cm.instance + LEFT JOIN {choice_options} co ON co.choiceid = ch.id + LEFT JOIN {choice_answers} ca ON ca.optionid = co.id AND ca.choiceid = ch.id + WHERE ca.userid = :userid"; + + $params = [ + 'modname' => 'choice', + 'contextlevel' => CONTEXT_MODULE, + 'userid' => $userid, + ]; + $contextlist = new contextlist(); + $contextlist->add_from_sql($sql, $params); + + return $contextlist; + } + + /** + * @inheritdoc + */ + public static function export_user_data(approved_contextlist $contextlist) { + global $DB; + + if (empty($contextlist->count())) { + return; + } + + $userid = $contextlist->get_user()->id; + + list($contextsql, $contextparams) = $DB->get_in_or_equal($contextlist->get_contextids(), SQL_PARAMS_NAMED); + + $sql = "SELECT ca.id, + cm.id AS cmid, + ch.id as choiceid, + ch.name as choicename, + ch.intro, + ch.introformat, + co.text as answer, + ca.timemodified + FROM {context} c + INNER JOIN {course_modules} cm ON cm.id = c.instanceid + INNER JOIN {choice} ch ON ch.id = cm.instance + LEFT JOIN {choice_options} co ON co.choiceid = ch.id + LEFT JOIN {choice_answers} ca ON ca.optionid = co.id AND ca.choiceid = ch.id + WHERE c.id {$contextsql} + AND ca.userid = :userid"; + + $params = [ + 'userid' => $userid + ]; + $params += $contextparams; + + $choiceanswers = $DB->get_recordset_sql($sql, $params); + // Group choice answers per activity. (We might fetch multiple choice answers that belong to a single choice activity). + $answergroups = []; + foreach ($choiceanswers as $choiceanswer) { + $cmid = $choiceanswer->cmid; + if (empty($answergroups[$cmid])) { + $context = context_module::instance($cmid); + $data = (object)[ + 'id' => $choiceanswer->id, + 'choiceid' => $choiceanswer->choiceid, + 'choicename' => $choiceanswer->choicename, + 'answer' => $choiceanswer->answer, + 'timemodified' => $choiceanswer->timemodified, + ]; + + $data->intro = writer::with_context($context) + ->rewrite_pluginfile_urls([], 'mod_choice', 'intro', $choiceanswer->choiceid, $choiceanswer->intro); + $data->answer = [$choiceanswer->answer]; + $answergroups[$choiceanswer->cmid] = $data; + } else { + $answergroups[$choiceanswer->cmid]->answer[] = $choiceanswer->answer; + } + } + $choiceanswers->close(); + + // Export the data. + foreach ($answergroups as $cmid => $answergroup) { + $context = context_module::instance($cmid); + writer::with_context($context) + // Export the choice answer. + ->export_data([], $answergroup) + + // Export the associated files. + ->export_area_files([], 'mod_choice', 'intro', $answergroup->choiceid); + } + } + + /** + * @inheritdoc + */ + public static function delete_for_context(deletion_criteria $criteria) { + global $DB; + + $context = $criteria->get_context(); + if (empty($context)) { + return; + } + $instanceid = $DB->get_field('course_modules', 'instance', ['id' => $context->instanceid], MUST_EXIST); + $DB->delete_records('choice_answers', ['choiceid' => $instanceid]); + } + + /** + * @inheritdoc + */ + public static function delete_user_data(approved_contextlist $contextlist) { + global $DB; + + if (empty($contextlist->count())) { + return; + } + + $userid = $contextlist->get_user()->id; + foreach ($contextlist->get_contexts() as $context) { + $instanceid = $DB->get_field('course_modules', 'instance', ['id' => $context->instanceid], MUST_EXIST); + $DB->delete_records('choice_answers', ['choiceid' => $instanceid, 'userid' => $userid]); + } + } +} diff --git a/mod/choice/lang/en/choice.php b/mod/choice/lang/en/choice.php index 3a732cb4053..ee568e29e76 100644 --- a/mod/choice/lang/en/choice.php +++ b/mod/choice/lang/en/choice.php @@ -109,6 +109,11 @@ $string['pluginadministration'] = 'Choice administration'; $string['pluginname'] = 'Choice'; $string['previewonly'] = 'This is just a preview of the available options for this activity. You will not be able to submit your choice until {$a}.'; $string['privacy'] = 'Privacy of results'; +$string['privacy:metadata:choice_answers'] = 'Information about the user\'s chosen answer(s) for a given choice activity'; +$string['privacy:metadata:choice_answers:choiceid'] = 'The ID of the choice activity the user is providing answer for'; +$string['privacy:metadata:choice_answers:optionid'] = 'The ID of option that the user selected'; +$string['privacy:metadata:choice_answers:userid'] = 'The ID of the user answering this choice activity'; +$string['privacy:metadata:choice_answers:timemodified'] = 'The timestamp indicating when the choice was modified by the user'; $string['publish'] = 'Publish results'; $string['publishafteranswer'] = 'Show results to students after they answer'; $string['publishafterclose'] = 'Show results to students only after the choice is closed'; diff --git a/mod/choice/tests/privacy_provider_test.php b/mod/choice/tests/privacy_provider_test.php new file mode 100644 index 00000000000..112a80d3299 --- /dev/null +++ b/mod/choice/tests/privacy_provider_test.php @@ -0,0 +1,212 @@ +. + +/** + * Privacy provider tests. + * + * @package mod_choice + * @copyright 2018 Jun Pataleta + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +use core_privacy\metadata\item_collection; +use core_privacy\request\deletion_criteria; +use mod_choice\privacy\provider; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Privacy provider tests class. + * + * @package mod_choice + * @copyright 2018 Jun Pataleta + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class mod_choice_privacy_provider_testcase extends advanced_testcase { + /** @var stdClass The student object. */ + protected $student; + + /** @var stdClass The choice object. */ + protected $choice; + + /** @var stdClass The course object. */ + protected $course; + + /** + * @inheritdoc + */ + protected function setUp() { + $this->resetAfterTest(); + + global $DB; + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + $options = ['fried rice', 'spring rolls', 'sweet and sour pork', 'satay beef', 'gyouza']; + $params = [ + 'course' => $course->id, + 'option' => $options, + 'name' => 'First Choice Activity', + 'showpreview' => 0 + ]; + + $plugingenerator = $generator->get_plugin_generator('mod_choice'); + // The choice activity the user will answer. + $choice = $plugingenerator->create_instance($params); + // Create another choice activity. + $plugingenerator->create_instance($params); + $cm = get_coursemodule_from_instance('choice', $choice->id); + + // Create a student which will make a choice. + $student = $generator->create_user(); + $studentrole = $DB->get_record('role', ['shortname' => 'student']); + $generator->enrol_user($student->id, $course->id, $studentrole->id); + + $choicewithoptions = choice_get_choice($choice->id); + $optionids = array_keys($choicewithoptions->option); + + choice_user_submit_response($optionids[2], $choice, $student->id, $course, $cm); + $this->student = $student; + $this->choice = $choice; + $this->course = $course; + } + + /** + * Test for provider::get_metadata(). + */ + public function test_get_metadata() { + $collection = new item_collection('mod_choice'); + $newcollection = provider::get_metadata($collection); + $itemcollection = $newcollection->get_item_collection(); + $this->assertCount(1, $itemcollection); + + $table = reset($itemcollection); + $this->assertEquals('choice_answers', $table->get_name()); + + $privacyfields = $table->get_privacy_fields(); + $this->assertArrayHasKey('choiceid', $privacyfields); + $this->assertArrayHasKey('optionid', $privacyfields); + $this->assertArrayHasKey('userid', $privacyfields); + $this->assertArrayHasKey('timemodified', $privacyfields); + + $this->assertEquals('privacy:metadata:choice_answers', $table->get_summary()); + } + + /** + * Test for provider::get_contexts_for_userid(). + */ + public function test_get_contexts_for_userid() { + $cm = get_coursemodule_from_instance('choice', $this->choice->id); + + $contextlist = provider::get_contexts_for_userid($this->student->id); + $this->assertCount(1, $contextlist); + $contextforuser = $contextlist->current(); + $cmcontext = context_module::instance($cm->id); + $this->assertEquals($cmcontext->id, $contextforuser->id); + } + + /** + * Test for provider::delete_for_context(). + */ + public function test_delete_for_context() { + global $DB; + + $choice = $this->choice; + $generator = $this->getDataGenerator(); + $cm = get_coursemodule_from_instance('choice', $this->choice->id); + + // Create another student who will answer the choice activity. + $student = $generator->create_user(); + $studentrole = $DB->get_record('role', ['shortname' => 'student']); + $generator->enrol_user($student->id, $this->course->id, $studentrole->id); + + $choicewithoptions = choice_get_choice($choice->id); + $optionids = array_keys($choicewithoptions->option); + + choice_user_submit_response($optionids[1], $choice, $student->id, $this->course, $cm); + + // Before deletion, we should have 2 responses. + $count = $DB->count_records('choice_answers', ['choiceid' => $choice->id]); + $this->assertEquals(2, $count); + + // Delete data based on context. + $cmcontext = context_module::instance($cm->id); + $criteria = new deletion_criteria($cmcontext); + provider::delete_for_context($criteria); + + // After deletion, the choice answers for that choice activity should have been deleted. + $count = $DB->count_records('choice_answers', ['choiceid' => $choice->id]); + $this->assertEquals(0, $count); + } + + /** + * Test for provider::delete_user_data(). + */ + public function test_delete_user_data() { + global $DB; + + $choice = $this->choice; + $generator = $this->getDataGenerator(); + $cm1 = get_coursemodule_from_instance('choice', $this->choice->id); + + // Create a second choice activity. + $options = ['Boracay', 'Camiguin', 'Bohol', 'Cebu', 'Coron']; + $params = [ + 'course' => $this->course->id, + 'option' => $options, + 'name' => 'Which do you think is the best island in the Philippines?', + 'showpreview' => 0 + ]; + $plugingenerator = $generator->get_plugin_generator('mod_choice'); + $choice2 = $plugingenerator->create_instance($params); + $plugingenerator->create_instance($params); + $cm2 = get_coursemodule_from_instance('choice', $choice2->id); + + // Make a selection for the first student for the 2nd choice activity. + $choicewithoptions = choice_get_choice($choice2->id); + $optionids = array_keys($choicewithoptions->option); + choice_user_submit_response($optionids[2], $choice2, $this->student->id, $this->course, $cm2); + + // Create another student who will answer the first choice activity. + $otherstudent = $generator->create_user(); + $studentrole = $DB->get_record('role', ['shortname' => 'student']); + $generator->enrol_user($otherstudent->id, $this->course->id, $studentrole->id); + + $choicewithoptions = choice_get_choice($choice->id); + $optionids = array_keys($choicewithoptions->option); + + choice_user_submit_response($optionids[1], $choice, $otherstudent->id, $this->course, $cm1); + + // Before deletion, we should have 2 responses. + $count = $DB->count_records('choice_answers', ['choiceid' => $choice->id]); + $this->assertEquals(2, $count); + + $context1 = context_module::instance($cm1->id); + $context2 = context_module::instance($cm2->id); + $contextlist = new \core_privacy\request\approved_contextlist($this->student, [$context1->id, $context2->id]); + provider::delete_user_data($contextlist); + + // After deletion, the choice answers for the first student should have been deleted. + $count = $DB->count_records('choice_answers', ['choiceid' => $choice->id, 'userid' => $this->student->id]); + $this->assertEquals(0, $count); + + // Confirm that we only have one choice answer available. + $choiceanswers = $DB->get_records('choice_answers'); + $this->assertCount(1, $choiceanswers); + $lastresponse = reset($choiceanswers); + // And that it's the other student's response. + $this->assertEquals($otherstudent->id, $lastresponse->userid); + } +} From f1c1db434a48b9126d85cd3182dc00e3239878d9 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 28 Feb 2018 15:42:53 +0800 Subject: [PATCH 2/3] MDL-61475 mod_choice: Update core_privacy implementation --- mod/choice/classes/privacy/provider.php | 123 +++++++++++---------- mod/choice/tests/privacy_provider_test.php | 41 ++++--- 2 files changed, 90 insertions(+), 74 deletions(-) diff --git a/mod/choice/classes/privacy/provider.php b/mod/choice/classes/privacy/provider.php index 4dce12679e8..697d547fadc 100644 --- a/mod/choice/classes/privacy/provider.php +++ b/mod/choice/classes/privacy/provider.php @@ -24,14 +24,12 @@ namespace mod_choice\privacy; -use context_module; -use core_privacy\metadata\item_collection; -use core_privacy\metadata\provider as core_provider; -use core_privacy\request\approved_contextlist; -use core_privacy\request\contextlist; -use core_privacy\request\deletion_criteria; -use core_privacy\request\plugin\provider as plugin_provider; -use core_privacy\request\writer; +use core_privacy\local\metadata\collection; +use core_privacy\local\request\approved_contextlist; +use core_privacy\local\request\contextlist; +use core_privacy\local\request\deletion_criteria; +use core_privacy\local\request\helper; +use core_privacy\local\request\writer; defined('MOODLE_INTERNAL') || die(); @@ -41,11 +39,19 @@ defined('MOODLE_INTERNAL') || die(); * @copyright 2018 Jun Pataleta * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class provider implements core_provider, plugin_provider { +class provider implements + // This plugin stores personal data. + \core_privacy\local\metadata\provider, + + // This plugin is a core_user_data_provider. + \core_privacy\local\request\plugin\provider { /** - * @inheritdoc + * Return the fields which contain personal data. + * + * @param collection $items a reference to the collection to use to store the metadata. + * @return collection the updated collection of metadata items. */ - public static function get_metadata(item_collection $items) : item_collection { + public static function get_metadata(collection $items) : collection { $items->add_database_table( 'choice_answers', [ @@ -61,7 +67,10 @@ class provider implements core_provider, plugin_provider { } /** - * @inheritdoc + * Get the list of contexts that contain user information for the specified user. + * + * @param int $userid the userid. + * @return contextlist the list of contexts containing user info for the user. */ public static function get_contexts_for_userid(int $userid) : contextlist { // Fetch all choice answers. @@ -70,8 +79,8 @@ class provider implements core_provider, plugin_provider { INNER JOIN {course_modules} cm ON cm.id = c.instanceid AND c.contextlevel = :contextlevel INNER JOIN {modules} m ON m.id = cm.module AND m.name = :modname INNER JOIN {choice} ch ON ch.id = cm.instance - LEFT JOIN {choice_options} co ON co.choiceid = ch.id - LEFT JOIN {choice_answers} ca ON ca.optionid = co.id AND ca.choiceid = ch.id + INNER JOIN {choice_options} co ON co.choiceid = ch.id + INNER JOIN {choice_answers} ca ON ca.optionid = co.id AND ca.choiceid = ch.id WHERE ca.userid = :userid"; $params = [ @@ -86,7 +95,9 @@ class provider implements core_provider, plugin_provider { } /** - * @inheritdoc + * Export personal data for the given approved_contextlist. User and context information is contained within the contextlist. + * + * @param approved_contextlist $contextlist a list of contexts approved for export. */ public static function export_user_data(approved_contextlist $contextlist) { global $DB; @@ -95,75 +106,65 @@ class provider implements core_provider, plugin_provider { return; } - $userid = $contextlist->get_user()->id; + $user = $contextlist->get_user(); list($contextsql, $contextparams) = $DB->get_in_or_equal($contextlist->get_contextids(), SQL_PARAMS_NAMED); - $sql = "SELECT ca.id, - cm.id AS cmid, - ch.id as choiceid, - ch.name as choicename, - ch.intro, - ch.introformat, - co.text as answer, + $sql = "SELECT cm.id AS cmid, + co.text as answer, ca.timemodified FROM {context} c INNER JOIN {course_modules} cm ON cm.id = c.instanceid INNER JOIN {choice} ch ON ch.id = cm.instance - LEFT JOIN {choice_options} co ON co.choiceid = ch.id - LEFT JOIN {choice_answers} ca ON ca.optionid = co.id AND ca.choiceid = ch.id + INNER JOIN {choice_options} co ON co.choiceid = ch.id + INNER JOIN {choice_answers} ca ON ca.optionid = co.id AND ca.choiceid = ch.id WHERE c.id {$contextsql} AND ca.userid = :userid"; - $params = [ - 'userid' => $userid - ]; - $params += $contextparams; + $params = ['userid' => $user->id] + $contextparams; + // Create an array of the user's choice instances, supporting multiple answers per choice instance. + $choiceinstances = []; $choiceanswers = $DB->get_recordset_sql($sql, $params); - // Group choice answers per activity. (We might fetch multiple choice answers that belong to a single choice activity). - $answergroups = []; foreach ($choiceanswers as $choiceanswer) { - $cmid = $choiceanswer->cmid; - if (empty($answergroups[$cmid])) { - $context = context_module::instance($cmid); - $data = (object)[ - 'id' => $choiceanswer->id, - 'choiceid' => $choiceanswer->choiceid, - 'choicename' => $choiceanswer->choicename, - 'answer' => $choiceanswer->answer, - 'timemodified' => $choiceanswer->timemodified, + if (empty($choiceinstances[$choiceanswer->cmid])) { + $data = (object) [ + 'answer' => [], + 'timemodified' => \core_privacy\local\request\transform::datetime($choiceanswer->timemodified), ]; - - $data->intro = writer::with_context($context) - ->rewrite_pluginfile_urls([], 'mod_choice', 'intro', $choiceanswer->choiceid, $choiceanswer->intro); - $data->answer = [$choiceanswer->answer]; - $answergroups[$choiceanswer->cmid] = $data; - } else { - $answergroups[$choiceanswer->cmid]->answer[] = $choiceanswer->answer; + $choiceinstances[$choiceanswer->cmid] = $data; } + + // Instance exists, just add the additional answer. + $choiceinstances[$choiceanswer->cmid]->answer[] = $choiceanswer->answer; } $choiceanswers->close(); - // Export the data. - foreach ($answergroups as $cmid => $answergroup) { - $context = context_module::instance($cmid); - writer::with_context($context) - // Export the choice answer. - ->export_data([], $answergroup) + // Now export the data. + foreach ($choiceinstances as $cmid => $choicedata) { + $context = \context_module::instance($cmid); - // Export the associated files. - ->export_area_files([], 'mod_choice', 'intro', $answergroup->choiceid); + // Fetch the generic module data for the choice. + $contextdata = helper::get_context_data($context, $user); + + // Merge with choice data and write it. + $contextdata = (object) array_merge((array) $contextdata, (array) $choicedata); + writer::with_context($context) + ->export_data([], $contextdata); + + // Write generic module intro files. + helper::export_context_files($context, $user); } } /** - * @inheritdoc + * Delete all data for all users in the specified context. + * + * @param \context $context the context to delete in. */ - public static function delete_for_context(deletion_criteria $criteria) { + public static function delete_data_for_all_users_in_context(\context $context) { global $DB; - $context = $criteria->get_context(); if (empty($context)) { return; } @@ -172,9 +173,11 @@ class provider implements core_provider, plugin_provider { } /** - * @inheritdoc + * Delete all user data for the specified user, in the specified contexts. + * + * @param approved_contextlist $contextlist a list of contexts approved for deletion. */ - public static function delete_user_data(approved_contextlist $contextlist) { + public static function delete_data_for_user(approved_contextlist $contextlist) { global $DB; if (empty($contextlist->count())) { diff --git a/mod/choice/tests/privacy_provider_test.php b/mod/choice/tests/privacy_provider_test.php index 112a80d3299..44a7e62ab1a 100644 --- a/mod/choice/tests/privacy_provider_test.php +++ b/mod/choice/tests/privacy_provider_test.php @@ -22,8 +22,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -use core_privacy\metadata\item_collection; -use core_privacy\request\deletion_criteria; +use core_privacy\local\metadata\collection; +use core_privacy\local\request\deletion_criteria; use mod_choice\privacy\provider; defined('MOODLE_INTERNAL') || die(); @@ -35,7 +35,7 @@ defined('MOODLE_INTERNAL') || die(); * @copyright 2018 Jun Pataleta * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class mod_choice_privacy_provider_testcase extends advanced_testcase { +class mod_choice_privacy_provider_testcase extends \core_privacy\tests\provider_testcase { /** @var stdClass The student object. */ protected $student; @@ -46,7 +46,7 @@ class mod_choice_privacy_provider_testcase extends advanced_testcase { protected $course; /** - * @inheritdoc + * {@inheritdoc} */ protected function setUp() { $this->resetAfterTest(); @@ -87,9 +87,9 @@ class mod_choice_privacy_provider_testcase extends advanced_testcase { * Test for provider::get_metadata(). */ public function test_get_metadata() { - $collection = new item_collection('mod_choice'); + $collection = new collection('mod_choice'); $newcollection = provider::get_metadata($collection); - $itemcollection = $newcollection->get_item_collection(); + $itemcollection = $newcollection->get_collection(); $this->assertCount(1, $itemcollection); $table = reset($itemcollection); @@ -118,9 +118,22 @@ class mod_choice_privacy_provider_testcase extends advanced_testcase { } /** - * Test for provider::delete_for_context(). + * Test for provider::export_user_data(). */ - public function test_delete_for_context() { + public function test_export_for_context() { + $cm = get_coursemodule_from_instance('choice', $this->choice->id); + $cmcontext = context_module::instance($cm->id); + + // Export all of the data for the context. + $this->export_context_data_for_user($this->student->id, $cmcontext, 'mod_choice'); + $writer = \core_privacy\local\request\writer::with_context($cmcontext); + $this->assertTrue($writer->has_any_data()); + } + + /** + * Test for provider::delete_data_for_all_users_in_context(). + */ + public function test_delete_data_for_all_users_in_context() { global $DB; $choice = $this->choice; @@ -143,8 +156,7 @@ class mod_choice_privacy_provider_testcase extends advanced_testcase { // Delete data based on context. $cmcontext = context_module::instance($cm->id); - $criteria = new deletion_criteria($cmcontext); - provider::delete_for_context($criteria); + provider::delete_data_for_all_users_in_context($cmcontext); // After deletion, the choice answers for that choice activity should have been deleted. $count = $DB->count_records('choice_answers', ['choiceid' => $choice->id]); @@ -152,9 +164,9 @@ class mod_choice_privacy_provider_testcase extends advanced_testcase { } /** - * Test for provider::delete_user_data(). + * Test for provider::delete_data_for_user(). */ - public function test_delete_user_data() { + public function test_delete_data_for_user_() { global $DB; $choice = $this->choice; @@ -195,8 +207,9 @@ class mod_choice_privacy_provider_testcase extends advanced_testcase { $context1 = context_module::instance($cm1->id); $context2 = context_module::instance($cm2->id); - $contextlist = new \core_privacy\request\approved_contextlist($this->student, [$context1->id, $context2->id]); - provider::delete_user_data($contextlist); + $contextlist = new \core_privacy\local\request\approved_contextlist($this->student, 'choice', + [$context1->id, $context2->id]); + provider::delete_data_for_user($contextlist); // After deletion, the choice answers for the first student should have been deleted. $count = $DB->count_records('choice_answers', ['choiceid' => $choice->id, 'userid' => $this->student->id]); From a57960ee11ba6842741b1a4c15fef93c1329ee7e Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 13 Mar 2018 09:52:19 +0800 Subject: [PATCH 3/3] MDL-61475 mod_choice: perf improvement when exporting personal data Instead of creating an array containing all record information and then writing, we know write periodically during the recordset iteration, thus alleviating any memory concerns associated with the array approach. --- mod/choice/classes/privacy/provider.php | 62 ++++++++++++++++--------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/mod/choice/classes/privacy/provider.php b/mod/choice/classes/privacy/provider.php index 697d547fadc..229efc9b102 100644 --- a/mod/choice/classes/privacy/provider.php +++ b/mod/choice/classes/privacy/provider.php @@ -119,44 +119,60 @@ class provider implements INNER JOIN {choice_options} co ON co.choiceid = ch.id INNER JOIN {choice_answers} ca ON ca.optionid = co.id AND ca.choiceid = ch.id WHERE c.id {$contextsql} - AND ca.userid = :userid"; + AND ca.userid = :userid + ORDER BY cm.id"; $params = ['userid' => $user->id] + $contextparams; - // Create an array of the user's choice instances, supporting multiple answers per choice instance. - $choiceinstances = []; + // Reference to the choice activity seen in the last iteration of the loop. By comparing this with the current record, and + // because we know the results are ordered, we know when we've moved to the answers for a new choice activity and therefore + // when we can export the complete data for the last activity. + $lastcmid = null; + $choiceanswers = $DB->get_recordset_sql($sql, $params); foreach ($choiceanswers as $choiceanswer) { - if (empty($choiceinstances[$choiceanswer->cmid])) { - $data = (object) [ + // If we've moved to a new choice, then write the last choice data and reinit the choice data array. + if ($lastcmid != $choiceanswer->cmid) { + if (!empty($choicedata)) { + $context = \context_module::instance($lastcmid); + self::export_choice_data_for_user($choicedata, $context, $user); + } + $choicedata = [ 'answer' => [], 'timemodified' => \core_privacy\local\request\transform::datetime($choiceanswer->timemodified), ]; - $choiceinstances[$choiceanswer->cmid] = $data; } - - // Instance exists, just add the additional answer. - $choiceinstances[$choiceanswer->cmid]->answer[] = $choiceanswer->answer; + $choicedata['answer'][] = $choiceanswer->answer; + $lastcmid = $choiceanswer->cmid; } $choiceanswers->close(); - // Now export the data. - foreach ($choiceinstances as $cmid => $choicedata) { - $context = \context_module::instance($cmid); - - // Fetch the generic module data for the choice. - $contextdata = helper::get_context_data($context, $user); - - // Merge with choice data and write it. - $contextdata = (object) array_merge((array) $contextdata, (array) $choicedata); - writer::with_context($context) - ->export_data([], $contextdata); - - // Write generic module intro files. - helper::export_context_files($context, $user); + // The data for the last activity won't have been written yet, so make sure to write it now! + if (!empty($choicedata)) { + $context = \context_module::instance($lastcmid); + self::export_choice_data_for_user($choicedata, $context, $user); } } + /** + * Export the supplied personal data for a single choice activity, along with any generic data or area files. + * + * @param array $choicedata the personal data to export for the choice. + * @param \context_module $context the context of the choice. + * @param \stdClass $user the user record + */ + protected static function export_choice_data_for_user(array $choicedata, \context_module $context, \stdClass $user) { + // Fetch the generic module data for the choice. + $contextdata = helper::get_context_data($context, $user); + + // Merge with choice data and write it. + $contextdata = (object)array_merge((array)$contextdata, $choicedata); + writer::with_context($context)->export_data([], $contextdata); + + // Write generic module intro files. + helper::export_context_files($context, $user); + } + /** * Delete all data for all users in the specified context. *