Skip to content
GitLab
Commit d2aed789 authored by Andrew Nicols's avatar Andrew Nicols
Browse files

MDL-63619 tool_dataprivacy: Fix inheritance from parent contexts 

Inheritance should behave such that all contexts inherit from their
parent context.

Prior to this fix, if the value was not set on a context, then it was
getting a default of 'Inherit', but instead of inheritting from the
parent context, it was inheritting from its parent context _level_ which
is just wrong.
parent daf0b4f0
Expand all Hide whitespace changes
Inline Side-by-side
admin/tool/dataprivacy/classes/api.php
View file @ d2aed789
......@@ -914,7 +914,7 @@ class api {
* @param int $forcedvalue Use this categoryid value as if this was this context instance category.
* @return category|false
*/
public static function get_effective_context_category(\context $context, $forcedvalue=false) {
public static function get_effective_context_category(\context $context, $forcedvalue = false) {
if (!data_registry::defaults_set()) {
return false;
}
......@@ -941,15 +941,14 @@ class api {
* Returns the effective category given a context level.
*
* @param int $contextlevel
* @param int $forcedvalue Use this categoryid value as if this was this context level category.
* @return category|false
*/
public static function get_effective_contextlevel_category($contextlevel, $forcedvalue=false) {
public static function get_effective_contextlevel_category($contextlevel) {
if (!data_registry::defaults_set()) {
return false;
}
return data_registry::get_effective_contextlevel_value($contextlevel, 'category', $forcedvalue);
return data_registry::get_effective_contextlevel_value($contextlevel, 'category');
}
/**
......
admin/tool/dataprivacy/classes/data_registry.php
View file @ d2aed789
......@@ -39,18 +39,6 @@ defined('MOODLE_INTERNAL') || die();
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class data_registry {
/**
* @var array Inheritance between context levels.
*/
private static $contextlevelinheritance = [
CONTEXT_USER => [CONTEXT_SYSTEM],
CONTEXT_COURSECAT => [CONTEXT_SYSTEM],
CONTEXT_COURSE => [CONTEXT_COURSECAT, CONTEXT_SYSTEM],
CONTEXT_MODULE => [CONTEXT_COURSE, CONTEXT_COURSECAT, CONTEXT_SYSTEM],
CONTEXT_BLOCK => [CONTEXT_COURSE, CONTEXT_COURSECAT, CONTEXT_SYSTEM],
];
/**
* Returns purpose and category var names from a context class name
*
......@@ -83,7 +71,6 @@ class data_registry {
* @return int[]|false[]
*/
public static function get_defaults($contextlevel, $pluginname = '') {
$classname = \context_helper::get_class_for_level($contextlevel);
list($purposevar, $categoryvar) = self::var_names_from_context($classname, $pluginname);
......@@ -104,10 +91,10 @@ class data_registry {
}
if (empty($purposeid)) {
$purposeid = false;
$purposeid = context_instance::NOTSET;
}
if (empty($categoryid)) {
$categoryid = false;
$categoryid = context_instance::NOTSET;
}
return [$purposeid, $categoryid];
......@@ -190,19 +177,24 @@ class data_registry {
* @return persistent|false It return a 'purpose' instance or a 'category' instance, depending on $element
*/
public static function get_effective_context_value(\context $context, $element, $forcedvalue = false) {
if ($element !== 'purpose' && $element !== 'category') {
throw new coding_exception('Only \'purpose\' and \'category\' are supported.');
}
$fieldname = $element . 'id';
if (empty($forcedvalue)) {
$instance = context_instance::get_record_by_contextid($context->id, false);
// Check whether this context is a user context, or a child of a user context.
// User contexts share the same context and cannot be set individually.
$parents = $context->get_parent_contexts(true);
foreach ($parents as $parent) {
if ($parent->contextlevel == CONTEXT_USER) {
// Use the context level value for the user.
return self::get_effective_contextlevel_value(CONTEXT_USER, $element);
}
}
if (!$instance) {
// If the instance does not have a value defaults to not set, so we grab the context level default as its value.
$instancevalue = context_instance::NOTSET;
} else {
$instancevalue = context_instance::NOTSET;
if (empty($forcedvalue)) {
if ($instance = context_instance::get_record_by_contextid($context->id, false)) {
$instancevalue = $instance->get($fieldname);
}
} else {
......@@ -211,48 +203,34 @@ class data_registry {
// Not set.
if ($instancevalue == context_instance::NOTSET) {
// The effective value varies depending on the context level.
if ($context->contextlevel == CONTEXT_USER) {
// Use the context level value as we don't allow people to set specific instances values.
return self::get_effective_contextlevel_value(CONTEXT_USER, $element);
}
$parents = $context->get_parent_contexts(true);
foreach ($parents as $parent) {
if ($parent->contextlevel == CONTEXT_USER) {
// Use the context level value as we don't allow people to set specific instances values.
return self::get_effective_contextlevel_value(CONTEXT_USER, $element);
}
}
// Check if we need to pass the plugin name of an activity.
$forplugin = '';
if ($context->contextlevel == CONTEXT_MODULE) {
list($course, $cm) = get_course_and_cm_from_cmid($context->instanceid);
$forplugin = $cm->modname;
}
// Use the default context level value.
list($purposeid, $categoryid) = self::get_effective_default_contextlevel_purpose_and_category(
$context->contextlevel, false, false, $forplugin
);
return self::get_element_instance($element, $$fieldname);
$instancevalue = $$fieldname;
}
// Specific value for this context instance.
if ($instancevalue != context_instance::INHERIT) {
if (context_instance::NOTSET !== $instancevalue && context_instance::INHERIT !== $instancevalue) {
// There is an actual value. Return it.
return self::get_element_instance($element, $instancevalue);
}
// This context is using inherited so let's return the parent effective value.
$parentcontext = $context->get_parent_context();
if (!$parentcontext) {
return false;
}
// There is no value set (or it is set to inherit).
// Fetch from the parent context.
$parent = $context->get_parent_context();
// The forced value should not be transmitted to parent contexts.
return self::get_effective_context_value($parentcontext, $element);
if (CONTEXT_SYSTEM == $parent->contextlevel) {
return self::get_effective_contextlevel_value(CONTEXT_SYSTEM, $element);
} else {
return self::get_effective_context_value($context->get_parent_context(), $element);
}
}
/**
......@@ -264,11 +242,9 @@ class data_registry {
*
* @param int $contextlevel
* @param string $element 'category' or 'purpose'
* @param int $forcedvalue Use this value as if this was this context level purpose.
* @return \tool_dataprivacy\purpose|false
*/
public static function get_effective_contextlevel_value($contextlevel, $element, $forcedvalue = false) {
public static function get_effective_contextlevel_value($contextlevel, $element) {
if ($element !== 'purpose' && $element !== 'category') {
throw new coding_exception('Only \'purpose\' and \'category\' are supported.');
}
......@@ -279,39 +255,15 @@ class data_registry {
'have a purpose or a category.');
}
if ($forcedvalue === false) {
$instance = contextlevel::get_record_by_contextlevel($contextlevel, false);
if (!$instance) {
// If the context level does not have a value defaults to not set, so we grab the context level default as
// its value.
$instancevalue = context_instance::NOTSET;
} else {
$instancevalue = $instance->get($fieldname);
}
} else {
$instancevalue = $forcedvalue;
}
list($purposeid, $categoryid) = self::get_effective_default_contextlevel_purpose_and_category($contextlevel);
// Not set -> Use the default context level value.
if ($instancevalue == context_instance::NOTSET) {
list($purposeid, $categoryid) = self::get_effective_default_contextlevel_purpose_and_category($contextlevel);
// Note: The $$fieldname points to either $purposeid, or $categoryid.
if (context_instance::NOTSET !== $$fieldname && context_instance::INHERIT !== $$fieldname) {
// There is a specific value set.
return self::get_element_instance($element, $$fieldname);
}
// Specific value for this context instance.
if ($instancevalue != context_instance::INHERIT) {
return self::get_element_instance($element, $instancevalue);
}
if ($contextlevel == CONTEXT_SYSTEM) {
throw new coding_exception('Something went wrong, system defaults should be set and we should already have a value.');
}
// If we reach this point is that we are inheriting so get the parent context level and repeat.
$parentcontextlevel = reset(self::$contextlevelinheritance[$contextlevel]);
// Forced value are intentionally not passed as the force value should only affect the immediate context level.
return self::get_effective_contextlevel_value($parentcontextlevel, $element);
throw new coding_exception('Something went wrong, system defaults should be set and we should already have a value.');
}
/**
......@@ -320,13 +272,13 @@ class data_registry {
* @param int $contextlevel
* @param int|bool $forcedpurposevalue Use this value as if this was this context level purpose.
* @param int|bool $forcedcategoryvalue Use this value as if this was this context level category.
* @param string $activity The plugin name of the activity.
* @param string $component The name of the component to check.
* @return int[]
*/
public static function get_effective_default_contextlevel_purpose_and_category($contextlevel, $forcedpurposevalue = false,
$forcedcategoryvalue = false, $activity = '') {
list($purposeid, $categoryid) = self::get_defaults($contextlevel, $activity);
$forcedcategoryvalue = false, $component = '') {
// Get the defaults for this context level.
list($purposeid, $categoryid) = self::get_defaults($contextlevel, $component);
// Honour forced values.
if ($forcedpurposevalue) {
......@@ -336,37 +288,19 @@ class data_registry {
$categoryid = $forcedcategoryvalue;
}
// Not set == INHERIT for defaults.
if ($purposeid == context_instance::INHERIT || $purposeid == context_instance::NOTSET) {
$purposeid = false;
}
if ($categoryid == context_instance::INHERIT || $categoryid == context_instance::NOTSET) {
$categoryid = false;
}
if ($contextlevel == CONTEXT_USER) {
// Only user context levels inherit from a parent context level.
list($parentpurposeid, $parentcategoryid) = self::get_defaults(CONTEXT_SYSTEM);
if ($contextlevel != CONTEXT_SYSTEM && ($purposeid === false || $categoryid === false)) {
foreach (self::$contextlevelinheritance[$contextlevel] as $parent) {
list($parentpurposeid, $parentcategoryid) = self::get_defaults($parent);
// Not set == INHERIT for defaults.
if ($parentpurposeid == context_instance::INHERIT || $parentpurposeid == context_instance::NOTSET) {
$parentpurposeid = false;
}
if ($parentcategoryid == context_instance::INHERIT || $parentcategoryid == context_instance::NOTSET) {
$parentcategoryid = false;
}
if ($purposeid === false && $parentpurposeid) {
$purposeid = $parentpurposeid;
}
if ($categoryid === false && $parentcategoryid) {
$categoryid = $parentcategoryid;
}
if (context_instance::INHERIT == $purposeid || context_instance::NOTSET == $purposeid) {
$purposeid = $parentpurposeid;
}
if (context_instance::INHERIT == $categoryid || context_instance::NOTSET == $categoryid) {
$categoryid = $parentcategoryid;
}
}
// They may still be false, but we return anyway.
return [$purposeid, $categoryid];
}
......@@ -379,7 +313,6 @@ class data_registry {
* @return \core\persistent
*/
private static function get_element_instance($element, $id) {
if ($element !== 'purpose' && $element !== 'category') {
throw new coding_exception('No other elements than purpose and category are allowed');
}
......
admin/tool/dataprivacy/tests/api_test.php
View file @ d2aed789
This diff is collapsed.
admin/tool/dataprivacy/tests/behat/manage_defaults.feature
View file @ d2aed789
......@@ -32,12 +32,11 @@ Feature: Manage data registry defaults
| Purpose 2 | P5Y |
And I set the site category and purpose to "Site category" and "Site purpose"
# Setting a default for course categories should apply to everything beneath that category.
Scenario: Set course category data registry defaults
Given I set the category and purpose for the course category "scitech" to "Category 2" and "Purpose 2"
And I navigate to "Users > Privacy and policies > Data registry" in site administration
Given I navigate to "Users > Privacy and policies > Data registry" in site administration
And I click on "Set defaults" "link"
And I should see "Inherit"
And I should not see "Add a new module default"
And I press "Edit"
And I set the field "Category" to "Category 1"
And I set the field "Purpose" to "Purpose 1"
......@@ -47,27 +46,91 @@ Feature: Manage data registry defaults
And I navigate to "Users > Privacy and policies > Data registry" in site administration
And I click on "Science and technology" "link"
And I wait until the page is ready
And the field "categoryid" matches value "Category 2"
And the field "categoryid" matches value "Not set (use the default value)"
And the field "purposeid" matches value "Not set (use the default value)"
And I should see "3 years"
And I click on "Courses" "link"
And I wait until the page is ready
And I click on "Physics 101" "link"
And I wait until the page is ready
And I should see "3 years"
And I click on "Activities and resources" "link"
And I wait until the page is ready
And I should see "3 years"
And I click on "Assignment 1 (Assignment)" "link"
And I wait until the page is ready
And I should see "3 years"
# When Setting a default for course categories, and overriding a specific category, only that category and its
# children will be overridden.
# If any child is a course category, it will get the default.
Scenario: Set course category data registry defaults with override
Given I navigate to "Users > Privacy and policies > Data registry" in site administration
And I click on "Set defaults" "link"
And I press "Edit"
And I set the field "Category" to "Category 1"
And I set the field "Purpose" to "Purpose 1"
And I press "Save changes"
And I should see "Category 1"
And I should see "Purpose 1"
And I set the category and purpose for the course category "scitech" to "Category 2" and "Purpose 2"
When I navigate to "Users > Privacy and policies > Data registry" in site administration
And I click on "Science and technology" "link"
And I wait until the page is ready
Then the field "categoryid" matches value "Category 2"
And the field "purposeid" matches value "Purpose 2"
And I should see "5 years"
And I click on "Courses" "link"
And I wait until the page is ready
# Physics 101 is also a category, so it will get the category default.
And I click on "Physics 101" "link"
And I wait until the page is ready
And I should see "3 years"
And I click on "Activities and resources" "link"
And I wait until the page is ready
And I should see "3 years"
And I click on "Assignment 1 (Assignment)" "link"
And I wait until the page is ready
And I should see "3 years"
# When overriding a specific category, only that category and its children will be overridden.
Scenario: Set course category data registry defaults with override
Given I set the category and purpose for the course category "scitech" to "Category 2" and "Purpose 2"
When I navigate to "Users > Privacy and policies > Data registry" in site administration
And I click on "Science and technology" "link"
And I wait until the page is ready
Then the field "categoryid" matches value "Category 2"
And the field "purposeid" matches value "Purpose 2"
And I should see "5 years"
And I click on "Courses" "link"
And I wait until the page is ready
# Physics 101 is also a category, so it will get the category default.
And I click on "Physics 101" "link"
And I wait until the page is ready
And I should see "5 years"
And I click on "Activities and resources" "link"
And I wait until the page is ready
And I should see "5 years"
And I click on "Assignment 1 (Assignment)" "link"
And I wait until the page is ready
And I should see "5 years"
# Resetting instances removes custom values.
Scenario: Set course category data registry defaults with override
Given I set the category and purpose for the course category "scitech" to "Category 2" and "Purpose 2"
And I navigate to "Users > Privacy and policies > Data registry" in site administration
And I click on "Set defaults" "link"
And I should see "Inherit"
And I should not see "Add a new module default"
And I press "Edit"
And I set the field "Category" to "Category 1"
And I set the field "Purpose" to "Purpose 1"
And I click on "Reset instances with custom values" "checkbox"
When I press "Save changes"
Then I should see "Category 1"
When I click on "Reset instances with custom values" "checkbox"
And I press "Save changes"
And I should see "Category 1"
And I should see "Purpose 1"
And I navigate to "Users > Privacy and policies > Data registry" in site administration
And I click on "Science and technology" "link"
And I wait until the page is ready
And the field "categoryid" matches value "Not set (use the default value)"
Then the field "categoryid" matches value "Not set (use the default value)"
And the field "purposeid" matches value "Not set (use the default value)"
And I should see "3 years"
......@@ -94,6 +157,12 @@ Feature: Manage data registry defaults
And the field "categoryid" matches value "Category 2"
And the field "purposeid" matches value "Purpose 2"
And I should see "5 years (after the course end date)"
And I click on "Activities and resources" "link"
And I wait until the page is ready
And I should see "5 years"
And I click on "Assignment 1 (Assignment)" "link"
And I wait until the page is ready
And I should see "5 years"
Scenario: Set course data registry defaults with override
Given I set the category and purpose for the course "Physics 101" to "Category 2" and "Purpose 2"
......@@ -119,6 +188,12 @@ Feature: Manage data registry defaults
And the field "categoryid" matches value "Not set (use the default value)"
And the field "purposeid" matches value "Not set (use the default value)"
And I should see "3 years (after the course end date)"
And I click on "Activities and resources" "link"
And I wait until the page is ready
And I should see "3 years"
And I click on "Assignment 1 (Assignment)" "link"
And I wait until the page is ready
And I should see "3 years"
Scenario: Set module level data registry defaults
Given I set the category and purpose for the "assign1" "assign" in course "Physics 101" to "Category 2" and "Purpose 2"
......
admin/tool/dataprivacy/tests/data_registry_test.php 0 → 100644
View file @ d2aed789
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
/**
* Unit tests for the data_registry class.
*
* @package tool_dataprivacy
* @copyright 2018 Andrew Nicols <andrew@nicols.co.uk>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
defined('MOODLE_INTERNAL') || die();
use \tool_dataprivacy\data_registry;
/**
* Unit tests for the data_registry class.
*
* @package tool_dataprivacy
* @copyright 2018 Andrew Nicols <andrew@nicols.co.uk>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class tool_dataprivacy_dataregistry_testcase extends advanced_testcase {
/**
* Ensure that the get_effective_context_value only errors if provided an inappropriate element.
*
* This test is not great because we only test a limited set of values. This is a fault of the underlying API.
*/
public function test_get_effective_context_value_invalid_element() {
$this->expectException(coding_exception::class);
data_registry::get_effective_context_value(\context_system::instance(), 'invalid');
}
/**
* Ensure that the get_effective_contextlevel_value only errors if provided an inappropriate element.
*
* This test is not great because we only test a limited set of values. This is a fault of the underlying API.
*/
public function test_get_effective_contextlevel_value_invalid_element() {
$this->expectException(coding_exception::class);
data_registry::get_effective_contextlevel_value(\context_system::instance(), 'invalid');
}
}
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment