MDL-61899 tool_dataprivacy: Fixes during integration review
authorJun Pataleta <jun@moodle.com>
Wed, 18 Apr 2018 05:16:30 +0000 (13:16 +0800)
committerEloy Lafuente (stronk7) <stronk7@moodle.org>
Wed, 18 Apr 2018 16:23:13 +0000 (18:23 +0200)
* Disable tool by default.
* Add format columns for 'comments' and 'dpocomment' fields in
tool_dataprivacy_request table.
* Use data request exporter when sending email notification to DPO

admin/tool/dataprivacy/classes/api.php
admin/tool/dataprivacy/classes/data_request.php
admin/tool/dataprivacy/classes/external/data_request_exporter.php
admin/tool/dataprivacy/db/install.xml
admin/tool/dataprivacy/settings.php
admin/tool/dataprivacy/tests/api_test.php

index bba717c..5038e98 100644 (file)
@@ -36,7 +36,7 @@ use moodle_exception;
 use moodle_url;
 use required_capability_exception;
 use stdClass;
-use tool_dataprivacy\local\helper;
+use tool_dataprivacy\external\data_request_exporter;
 use tool_dataprivacy\task\initiate_data_request_task;
 use tool_dataprivacy\task\process_data_request_task;
 
@@ -393,12 +393,18 @@ class api {
     public static function notify_dpo($dpo, data_request $request) {
         global $PAGE, $SITE;
 
+        $output = $PAGE->get_renderer('tool_dataprivacy');
+
+        $usercontext = \context_user::instance($request->get('requestedby'));
+        $requestexporter = new data_request_exporter($request, ['context' => $usercontext]);
+        $requestdata = $requestexporter->export($output);
+
         // Create message to send to the Data Protection Officer(s).
         $typetext = null;
-        $typetext = helper::get_request_type_string($request->get('type'));
+        $typetext = $requestdata->typename;
         $subject = get_string('datarequestemailsubject', 'tool_dataprivacy', $typetext);
 
-        $requestedby = core_user::get_user($request->get('requestedby'));
+        $requestedby = $requestdata->requestedbyuser;
         $datarequestsurl = new moodle_url('/admin/tool/dataprivacy/datarequests.php');
         $message = new message();
         $message->courseid          = $SITE->id;
@@ -406,7 +412,7 @@ class api {
         $message->name              = 'contactdataprotectionofficer';
         $message->userfrom          = $requestedby;
         $message->replyto           = $requestedby->email;
-        $message->replytoname       = fullname($requestedby->email);
+        $message->replytoname       = $requestedby->fullname;
         $message->subject           = $subject;
         $message->fullmessageformat = FORMAT_HTML;
         $message->notification      = 1;
@@ -415,20 +421,19 @@ class api {
 
         // Prepare the context data for the email message body.
         $messagetextdata = [
-            'requestedby' => fullname($requestedby),
+            'requestedby' => $requestedby->fullname,
             'requesttype' => $typetext,
-            'requestdate' => userdate($request->get('timecreated')),
-            'requestcomments' => text_to_html($request->get('comments')),
+            'requestdate' => userdate($requestdata->timecreated),
+            'requestcomments' => $requestdata->messagehtml,
             'datarequestsurl' => $datarequestsurl
         ];
-        $requestingfor = core_user::get_user($request->get('userid'));
+        $requestingfor = $requestdata->foruser;
         if ($requestedby->id == $requestingfor->id) {
             $messagetextdata['requestfor'] = $messagetextdata['requestedby'];
         } else {
-            $messagetextdata['requestfor'] = fullname($requestingfor);
+            $messagetextdata['requestfor'] = $requestingfor->fullname;
         }
 
-        $output = $PAGE->get_renderer('tool_dataprivacy');
         // Email the data request to the Data Protection Officer(s)/Admin(s).
         $messagetextdata['dponame'] = fullname($dpo);
         // Render message email body.
index ffad4ff..d5ab218 100644 (file)
@@ -56,6 +56,16 @@ class data_request extends persistent {
                 'type' => PARAM_TEXT,
                 'default' => ''
             ],
+            'commentsformat' => [
+                'choices' => [
+                    FORMAT_HTML,
+                    FORMAT_MOODLE,
+                    FORMAT_PLAIN,
+                    FORMAT_MARKDOWN
+                ],
+                'type' => PARAM_INT,
+                'default' => FORMAT_PLAIN
+            ],
             'userid' => [
                 'default' => 0,
                 'type' => PARAM_INT
@@ -88,6 +98,16 @@ class data_request extends persistent {
                 'type' => PARAM_TEXT,
                 'null' => NULL_ALLOWED
             ],
+            'dpocommentformat' => [
+                'choices' => [
+                    FORMAT_HTML,
+                    FORMAT_MOODLE,
+                    FORMAT_PLAIN,
+                    FORMAT_MARKDOWN
+                ],
+                'type' => PARAM_INT,
+                'default' => FORMAT_PLAIN
+            ],
         ];
     }
 }
index 7ba3266..9996349 100644 (file)
@@ -127,6 +127,8 @@ class data_request_exporter extends persistent_exporter {
             $user = core_user::get_user($requestedbyid, '*', MUST_EXIST);
             $userexporter = new user_summary_exporter($user);
             $values['requestedbyuser'] = $userexporter->export($output);
+        } else {
+            $values['requestedbyuser'] = $values['foruser'];
         }
 
         if (!empty($this->persistent->get('dpo'))) {
index 7528ae6..65d8926 100644 (file)
@@ -9,11 +9,13 @@
         <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
         <FIELD NAME="type" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Data request type"/>
         <FIELD NAME="comments" TYPE="text" NOTNULL="false" SEQUENCE="false" COMMENT="More details about the request"/>
+        <FIELD NAME="commentsformat" TYPE="int" LENGTH="2" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
         <FIELD NAME="userid" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="The user ID the request is being made for"/>
         <FIELD NAME="requestedby" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="The user ID of the one making the request"/>
         <FIELD NAME="status" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="The current status of the data request"/>
         <FIELD NAME="dpo" TYPE="int" LENGTH="10" NOTNULL="false" DEFAULT="0" SEQUENCE="false" COMMENT="The user ID of the Data Protection Officer who is reviewing th request"/>
         <FIELD NAME="dpocomment" TYPE="text" NOTNULL="false" SEQUENCE="false" COMMENT="DPO's comments (e.g. reason for rejecting the request, etc.)"/>
+        <FIELD NAME="dpocommentformat" TYPE="int" LENGTH="2" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
         <FIELD NAME="usermodified" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="The user who created/modified this request object"/>
         <FIELD NAME="timecreated" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="The time this data request was created"/>
         <FIELD NAME="timemodified" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="The last time this data request was updated"/>
index 9ba6bda..3f28b00 100644 (file)
@@ -28,10 +28,10 @@ if ($hassiteconfig) {
     $privacysettings = $ADMIN->locate('privacysettings');
 
     if ($ADMIN->fulltree) {
-        // Contact data protection officer.
+        // Contact data protection officer. Disabled by default.
         $privacysettings->add(new admin_setting_configcheckbox('tool_dataprivacy/contactdataprotectionofficer',
                 new lang_string('contactdataprotectionofficer', 'tool_dataprivacy'),
-                new lang_string('contactdataprotectionofficer_desc', 'tool_dataprivacy'), 1)
+                new lang_string('contactdataprotectionofficer_desc', 'tool_dataprivacy'), 0)
         );
 
         // Fetch roles that are assignable.
index 12d436c..62d29c1 100644 (file)
@@ -217,16 +217,16 @@ class tool_dataprivacy_api_testcase extends advanced_testcase {
      * Test for api::can_contact_dpo()
      */
     public function test_can_contact_dpo() {
-        // Default ('contactdataprotectionofficer' is enabled by default).
-        $this->assertTrue(api::can_contact_dpo());
-
-        // Disable.
-        set_config('contactdataprotectionofficer', 0, 'tool_dataprivacy');
+        // Default ('contactdataprotectionofficer' is disabled by default).
         $this->assertFalse(api::can_contact_dpo());
 
-        // Enable again.
+        // Enable.
         set_config('contactdataprotectionofficer', 1, 'tool_dataprivacy');
         $this->assertTrue(api::can_contact_dpo());
+
+        // Disable again.
+        set_config('contactdataprotectionofficer', 0, 'tool_dataprivacy');
+        $this->assertFalse(api::can_contact_dpo());
     }
 
     /**
@@ -467,9 +467,10 @@ class tool_dataprivacy_api_testcase extends advanced_testcase {
      */
     public function notify_dpo_provider() {
         return [
-            [api::DATAREQUEST_TYPE_EXPORT, 'requesttypeexport'],
-            [api::DATAREQUEST_TYPE_DELETE, 'requesttypedelete'],
-            [api::DATAREQUEST_TYPE_OTHERS, 'requesttypeothers'],
+            [false, api::DATAREQUEST_TYPE_EXPORT, 'requesttypeexport', 'Export my user data'],
+            [false, api::DATAREQUEST_TYPE_DELETE, 'requesttypedelete', 'Delete my user data'],
+            [false, api::DATAREQUEST_TYPE_OTHERS, 'requesttypeothers', 'Nothing. Just wanna say hi'],
+            [true, api::DATAREQUEST_TYPE_EXPORT, 'requesttypeexport', 'Admin export data of another user'],
         ];
     }
 
@@ -477,20 +478,28 @@ class tool_dataprivacy_api_testcase extends advanced_testcase {
      * Test for api::notify_dpo()
      *
      * @dataProvider notify_dpo_provider
+     * @param bool $byadmin Whether the admin requests data on behalf of the user
      * @param int $type The request type
      * @param string $typestringid The request lang string identifier
+     * @param string $comments The requestor's message to the DPO.
      */
-    public function test_notify_dpo($type, $typestringid) {
+    public function test_notify_dpo($byadmin, $type, $typestringid, $comments) {
         $generator = new testing_data_generator();
         $user1 = $generator->create_user();
+        // Let's just use admin as DPO (It's the default if not set).
+        $dpo = get_admin();
+        if ($byadmin) {
+            $this->setAdminUser();
+            $requestedby = $dpo;
+        } else {
+            $this->setUser($user1);
+            $requestedby = $user1;
+        }
 
-        // Make a data request as user 1.
-        $this->setUser($user1);
-        $request = api::create_data_request($user1->id, $type);
+        // Make a data request for user 1.
+        $request = api::create_data_request($user1->id, $type, $comments);
 
         $sink = $this->redirectMessages();
-        // Let's just use admin as DPO (It's the default if not set).
-        $dpo = get_admin();
         $messageid = api::notify_dpo($dpo, $request);
         $this->assertNotFalse($messageid);
         $messages = $sink->get_messages();
@@ -498,7 +507,7 @@ class tool_dataprivacy_api_testcase extends advanced_testcase {
         $message = reset($messages);
 
         // Check some of the message properties.
-        $this->assertEquals($user1->id, $message->useridfrom);
+        $this->assertEquals($requestedby->id, $message->useridfrom);
         $this->assertEquals($dpo->id, $message->useridto);
         $typestring = get_string($typestringid, 'tool_dataprivacy');
         $subject = get_string('datarequestemailsubject', 'tool_dataprivacy', $typestring);