MDL-62029 core_course: Fixes to context-aware provider implementation.
authorJake Dallimore <jake@moodle.com>
Mon, 7 May 2018 01:56:34 +0000 (09:56 +0800)
committerJake Dallimore <jake@moodle.com>
Wed, 9 May 2018 07:34:41 +0000 (15:34 +0800)
course/classes/privacy/provider.php
course/tests/privacy_test.php
privacy/classes/local/request/context_aware_provider.php
privacy/classes/manager.php

index a4c209d..41a8136 100644 (file)
@@ -35,7 +35,6 @@ use \core_privacy\local\request\transform;
 /**
  * Privacy class for requesting user data.
  *
- * @package    core_course
  * @copyright  2018 Adrian Greeve <adrian@moodle.com>
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
@@ -103,27 +102,47 @@ class provider implements
     }
 
     /**
-     * Exports course information based on the whole approved context list collection.
+     * Give the component a chance to include any contextual information deemed relevant to any child contexts which are
+     * exporting personal data.
+     *
+     * By giving the component access to the full list of contexts being exported across all components, it can determine whether a
+     * descendant context is being exported, and decide whether to add relevant contextual information about itself. Having access
+     * to the full list of contexts being exported is what makes this component a context aware provider.
      *
-     * @param  \core_privacy\local\request\contextlist_collection $contextcollection The collection of approved context lists.
+     * E.g.
+     * If, during the core export process, a course module is included in the contextlist_collection but the course containing the
+     * module is not (perhaps there's no longer a user enrolment), then the course should include general contextual information in
+     * the export so we know basic details about which course the module belongs to. This method allows the course to make that
+     * decision, based on the existence of any decendant module contexts in the collection.
+     *
+     * @param \core_privacy\local\request\contextlist_collection $contextlistcollection
      */
-    public static function export_complete_context_data(\core_privacy\local\request\contextlist_collection $completelist) {
+    public static function export_context_data(\core_privacy\local\request\contextlist_collection $contextlistcollection) {
         global $DB;
 
-        $coursecontextids = $DB->get_records('context', ['contextlevel' => CONTEXT_COURSE], '', 'id, instanceid');
-
+        $coursecontextids = $DB->get_records_menu('context', ['contextlevel' => CONTEXT_COURSE], '', 'id, instanceid');
         $courseids = [];
-        foreach ($completelist as $component) {
+        foreach ($contextlistcollection as $component) {
             foreach ($component->get_contexts() as $context) {
-                if ($context->contextlevel == CONTEXT_USER
-                        || $context->contextlevel == CONTEXT_SYSTEM
-                        || $context->contextlevel == CONTEXT_COURSECAT) {
-                    // Move onto the next context as these will not contain course contexts.
+                // All course contexts have been accounted for, so skip all checks.
+                if (empty($coursecontextids)) {
+                    break;
+                }
+                // Only course, module, and block contexts are checked.
+                if (in_array($context->contextlevel, [CONTEXT_USER, CONTEXT_SYSTEM, CONTEXT_COURSECAT])) {
+                    continue;
+                }
+                // If the context is a course, then we just add it without the need to check context path.
+                if ($context->contextlevel == CONTEXT_COURSE) {
+                    $courseids[$context->id] = $context->instanceid;
+                    unset($coursecontextids[$context->id]);
                     continue;
                 }
-                foreach ($coursecontextids as $contextid => $record) {
+                // Otherwise, we need to check all the course context paths, to see if this context is a descendant.
+                foreach ($coursecontextids as $contextid => $instanceid) {
                     if (stripos($context->path, '/' . $contextid . '/') !== false) {
-                        $courseids[$contextid] = $record->instanceid;
+                        $courseids[$contextid] = $instanceid;
+                        unset($coursecontextids[$contextid]);
                     }
                 }
             }
@@ -143,7 +162,8 @@ class provider implements
                 'fullname' => $course->fullname,
                 'shortname' => $course->shortname,
                 'idnumber' => $course->idnumber,
-                'summary' => writer::with_context($context)->rewrite_pluginfile_urls([], 'course', 'summary', 0, $course->summary),
+                'summary' => writer::with_context($context)->rewrite_pluginfile_urls([], 'course', 'summary', 0,
+                                                                                     format_string($course->summary)),
                 'format' => get_string('pluginname', 'format_' . $course->format),
                 'startdate' => transform::datetime($course->startdate),
                 'enddate' => transform::datetime($course->enddate)
index c8b55b9..b2d9033 100644 (file)
@@ -67,33 +67,67 @@ class core_course_privacy_testcase extends \core_privacy\tests\provider_testcase
         $this->assertCount(2, $completiondata->criteria);
     }
 
-    public function test_export_complete_context_data() {
+    /**
+     * Verify that if a module context is included in the contextlist_collection and its parent course is not, the
+     * export_context_data() call picks this up, and that the contextual course information is included.
+     */
+    public function test_export_context_data_module_context_only() {
         $this->resetAfterTest();
-        $user = $this->getDataGenerator()->create_user();
+
+        // Create a course and a single module.
         $course1 = $this->getDataGenerator()->create_course(['fullname' => 'Course 1', 'shortname' => 'C1']);
         $context1 = context_course::instance($course1->id);
-        $course2 = $this->getDataGenerator()->create_course(['fullname' => 'Course 2', 'shortname' => 'C2']);
-        $context2 = context_course::instance($course2->id);
-        $course3 = $this->getDataGenerator()->create_course(['fullname' => 'Course 3', 'shortname' => 'C3']);
-
-        $this->setUser($user);
-        $modforum = $this->getDataGenerator()->create_module('forum', ['course' => $course1->id]);
-        $modresource = $this->getDataGenerator()->create_module('resource', ['course' => $course2->id]);
-        $modpage = $this->getDataGenerator()->create_module('page', ['course' => $course3->id]);
-        $forumcontext = context_module::instance($modforum->cmid);
-        $resourcecontext = context_module::instance($modresource->cmid);
+        $modassign = $this->getDataGenerator()->create_module('assign', ['course' => $course1->id, 'name' => 'assign test 1']);
+        $assigncontext = context_module::instance($modassign->cmid);
 
+        // Now, let's assume during user info export, only the coursemodule context is returned in the contextlist_collection.
+        $user = $this->getDataGenerator()->create_user();
         $collection = new \core_privacy\local\request\contextlist_collection($user->id);
-        $approvedlist = new \core_privacy\local\request\approved_contextlist($user, 'mod_forum', [$forumcontext->id]);
+        $approvedlist = new \core_privacy\local\request\approved_contextlist($user, 'mod_assign', [$assigncontext->id]);
         $collection->add_contextlist($approvedlist);
-        $approvedlist = new \core_privacy\local\request\approved_contextlist($user, 'mod_resource', [$resourcecontext->id]);
+
+        // Now, verify that core_course will detect this, and add relevant contextual information.
+        \core_course\privacy\provider::export_context_data($collection);
+        $writer = \core_privacy\local\request\writer::with_context($context1);
+        $this->assertTrue($writer->has_any_data());
+        $writerdata = $writer->get_data();
+        $this->assertObjectHasAttribute('fullname', $writerdata);
+        $this->assertObjectHasAttribute('shortname', $writerdata);
+        $this->assertObjectHasAttribute('idnumber', $writerdata);
+        $this->assertObjectHasAttribute('summary', $writerdata);
+    }
+
+    /**
+     * Verify that if a module context and its parent course context are both included in the contextlist_collection, that course
+     * contextual information is present in the export.
+     */
+    public function test_export_context_data_course_and_module_contexts() {
+        $this->resetAfterTest();
+
+        // Create a course and a single module.
+        $course1 = $this->getDataGenerator()->create_course(['fullname' => 'Course 1', 'shortname' => 'C1']);
+        $context1 = context_course::instance($course1->id);
+        $modassign = $this->getDataGenerator()->create_module('assign', ['course' => $course1->id, 'name' => 'assign test 1']);
+        $assigncontext = context_module::instance($modassign->cmid);
+
+        // Now, assume during user info export, that both module and course contexts are returned in the contextlist_collection.
+        $user = $this->getDataGenerator()->create_user();
+        $collection = new \core_privacy\local\request\contextlist_collection($user->id);
+        $approvedlist = new \core_privacy\local\request\approved_contextlist($user, 'mod_assign', [$assigncontext->id]);
+        $approvedlist2 = new \core_privacy\local\request\approved_contextlist($user, 'core_course', [$context1->id]);
         $collection->add_contextlist($approvedlist);
+        $collection->add_contextlist($approvedlist2);
 
-        $writer = \core_privacy\local\request\writer::with_context(context_system::instance());
-        \core_course\privacy\provider::export_complete_context_data($collection);
-        $courses = $writer->get_data();
-        print_object($courses);
-        // print_object($writer);
+        // Now, verify that core_course still adds relevant contextual information, even for courses which are explicitly listed in
+        // the contextlist_collection.
+        \core_course\privacy\provider::export_context_data($collection);
+        $writer = \core_privacy\local\request\writer::with_context($context1);
+        $this->assertTrue($writer->has_any_data());
+        $writerdata = $writer->get_data();
+        $this->assertObjectHasAttribute('fullname', $writerdata);
+        $this->assertObjectHasAttribute('shortname', $writerdata);
+        $this->assertObjectHasAttribute('idnumber', $writerdata);
+        $this->assertObjectHasAttribute('summary', $writerdata);
     }
 
     /**
index bc5a4b3..01baad9 100644 (file)
@@ -15,8 +15,7 @@
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
 /**
- * This file contains the \core_privacy\local\request\plugin\provider interface to describe
- * a class which provides data in some form for a plugin.
+ * File containing the provider interface for plugins needing access to all approved contexts to fill in relevant contextual data.
  *
  * Plugins should implement this if they need access to all approved contexts.
  *
@@ -29,7 +28,7 @@ namespace core_privacy\local\request;
 defined('MOODLE_INTERNAL') || die();
 
 /**
- * The provider interface for plugins which need access to all approved contexts to fill in user data.
+ * The provider interface for plugins which need access to all approved contexts to fill in relevant contextual data.
  *
  * @copyright  2018 Adrian Greeve <adriangreeve.com>
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
@@ -37,9 +36,14 @@ defined('MOODLE_INTERNAL') || die();
 interface context_aware_provider extends \core_privacy\local\request\core_data_provider {
 
     /**
-     * Export context information based on the whole approved context list collection.
+     * Give the component a chance to include any contextual information deemed relevant to any child contexts which are
+     * exporting personal data.
+     *
+     * By giving the component access to the full list of contexts being exported across all components, it can determine whether a
+     * descendant context is being exported, and decide whether to add relevant contextual information about itself. Having access
+     * to the full list of contexts being exported is what makes this component a context aware provider.
      *
      * @param  \core_privacy\local\request\contextlist_collection $contextcollection The collection of approved context lists.
      */
-    public static function export_complete_context_data(\core_privacy\local\request\contextlist_collection $contextcollection);
+    public static function export_context_data(\core_privacy\local\request\contextlist_collection $contextcollection);
 }
index 14f20c7..0cd66f2 100644 (file)
@@ -228,13 +228,11 @@ class manager {
             if ($this->component_implements($component, \core_privacy\local\request\user_preference_provider::class)) {
                 $this->get_provider_classname($component)::export_user_preferences($contextlistcollection->get_userid());
             }
-        }
 
-        // Check each component for context aware providers.
-        foreach ($this->get_component_list() as $component) {
-            // Core user preference providers.
+            // Contextual information providers. Give each component a chance to include context information based on the
+            // existence of a child context in the contextlist_collection.
             if ($this->component_implements($component, \core_privacy\local\request\context_aware_provider::class)) {
-                $this->get_provider_classname($component)::export_complete_context_data($contextlistcollection);
+                $this->get_provider_classname($component)::export_context_data($contextlistcollection);
             }
         }