From 23b663ed55f9fcb65a905655654c00143d19f8e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 29 Aug 2018 18:00:00 +0200 Subject: [PATCH 1/3] MDL-63013 tool_policy: Policy agreement style can be defined now The patch introduces a new property of a policy document called "Agreement style". It allows the admin to define if the policy should be accepted together with all others on the consent page (legacy and default behaviour) or on its page before the consent page is reached (the new optional behaviour). --- admin/tool/policy/classes/form/policydoc.php | 2 + admin/tool/policy/classes/policy_version.php | 14 ++++++ admin/tool/policy/db/install.xml | 3 +- admin/tool/policy/db/upgrade.php | 53 ++++++++++++++++++++ admin/tool/policy/lang/en/tool_policy.php | 1 + admin/tool/policy/lib.php | 10 ++-- admin/tool/policy/version.php | 2 +- 7 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 admin/tool/policy/db/upgrade.php diff --git a/admin/tool/policy/classes/form/policydoc.php b/admin/tool/policy/classes/form/policydoc.php index 91e4ce015dd50..9c161800052e8 100644 --- a/admin/tool/policy/classes/form/policydoc.php +++ b/admin/tool/policy/classes/form/policydoc.php @@ -89,6 +89,8 @@ public function definition() { api::policy_content_field_options()); $mform->addRule('content_editor', null, 'required', null, 'client'); + $mform->addElement('selectyesno', 'agreementstyle', get_string('policypriorityagreement', 'tool_policy')); + if (!$formdata->id || $formdata->status == policy_version::STATUS_DRAFT) { // Creating a new version or editing a draft/archived version. $mform->addElement('hidden', 'minorchange'); diff --git a/admin/tool/policy/classes/policy_version.php b/admin/tool/policy/classes/policy_version.php index b3bbd29161906..f675ac91a4418 100644 --- a/admin/tool/policy/classes/policy_version.php +++ b/admin/tool/policy/classes/policy_version.php @@ -69,6 +69,12 @@ class policy_version extends persistent { /** @var int Policy version has been archived. */ const STATUS_ARCHIVED = 2; + /** @var int Policy to be accepted together with others on the consent page. */ + const AGREEMENTSTYLE_CONSENTPAGE = 0; + + /** @var int Policy to be accepted on its own page before reaching the consent page. */ + const AGREEMENTSTYLE_OWNPAGE = 1; + /** * Return the definition of the properties of this model. * @@ -106,6 +112,14 @@ protected static function define_properties() { 'policyid' => [ 'type' => PARAM_INT, ], + 'agreementstyle' => [ + 'type' => PARAM_INT, + 'choices' => [ + self::AGREEMENTSTYLE_CONSENTPAGE, + self::AGREEMENTSTYLE_OWNPAGE, + ], + 'default' => self::AGREEMENTSTYLE_CONSENTPAGE, + ], 'revision' => [ 'type' => PARAM_TEXT, 'default' => '', diff --git a/admin/tool/policy/db/install.xml b/admin/tool/policy/db/install.xml index c1601f644195e..0ab0d63746a13 100644 --- a/admin/tool/policy/db/install.xml +++ b/admin/tool/policy/db/install.xml @@ -1,5 +1,5 @@ - @@ -26,6 +26,7 @@ + diff --git a/admin/tool/policy/db/upgrade.php b/admin/tool/policy/db/upgrade.php new file mode 100644 index 0000000000000..7aabeef030c39 --- /dev/null +++ b/admin/tool/policy/db/upgrade.php @@ -0,0 +1,53 @@ +. + +/** + * Plugin upgrade steps are defined here. + * + * @package tool_policy + * @category upgrade + * @copyright 2018 David Mudrák + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * Execute the plugin upgrade steps from the given old version. + * + * @param int $oldversion + * @return bool + */ +function xmldb_tool_policy_upgrade($oldversion) { + global $DB; + + $dbman = $DB->get_manager(); + + if ($oldversion < 2018082900) { + // Add field agreementstyle to the table tool_policy_versions. + $table = new xmldb_table('tool_policy_versions'); + $field = new xmldb_field('agreementstyle', XMLDB_TYPE_INTEGER, '3', null, XMLDB_NOTNULL, null, '0', 'policyid'); + + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + upgrade_plugin_savepoint(true, 2018082900, 'tool', 'policy'); + } + + return true; +} + diff --git a/admin/tool/policy/lang/en/tool_policy.php b/admin/tool/policy/lang/en/tool_policy.php index 3ede09b9590d7..e3250cba21c80 100644 --- a/admin/tool/policy/lang/en/tool_policy.php +++ b/admin/tool/policy/lang/en/tool_policy.php @@ -129,6 +129,7 @@ $string['policydoctype99'] = 'Other policy'; $string['policydocuments'] = 'Policy documents'; $string['policynamedversion'] = 'Policy {$a->name} (version {$a->revision} - {$a->id})'; +$string['policypriorityagreement'] = 'Show policy before showing other policies'; $string['policyversionacceptedinbehalf'] = 'Consent for this policy has been given on your behalf.'; $string['policyversionacceptedinotherlang'] = 'Consent for this policy version has been given in a different language.'; $string['previousversions'] = '{$a} previous versions'; diff --git a/admin/tool/policy/lib.php b/admin/tool/policy/lib.php index 563254d814c92..4f36d31103d05 100644 --- a/admin/tool/policy/lib.php +++ b/admin/tool/policy/lib.php @@ -85,9 +85,13 @@ function tool_policy_before_standard_html_head() { && empty($USER->policyagreed) && (isguestuser() || !isloggedin())) { $output = $PAGE->get_renderer('tool_policy'); - $page = new \tool_policy\output\guestconsent(); - - $message = $output->render($page); + try { + $page = new \tool_policy\output\guestconsent(); + $message = $output->render($page); + } catch (dml_read_exception $e) { + // During upgrades, the new plugin code with new SQL could be in place but the DB not upgraded yet. + $message = null; + } } return $message; diff --git a/admin/tool/policy/version.php b/admin/tool/policy/version.php index e87145d972267..9f09c60ac7400 100644 --- a/admin/tool/policy/version.php +++ b/admin/tool/policy/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2018051400; // The current plugin version (Date: YYYYMMDDXX). +$plugin->version = 2018082900; // The current plugin version (Date: YYYYMMDDXX). $plugin->requires = 2018050800; // Requires this Moodle version. $plugin->component = 'tool_policy'; // Full name of the plugin (used for diagnostics). From 362ae1b4434a4d6f55321edf1449a100229015f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 29 Aug 2018 23:43:57 +0200 Subject: [PATCH 2/3] MDL-63013 tool_policy: Change acceptance flow for policies with own page The idea of the patch is to check the list of policies that are shown before and on the consent page. If that list contains a policy that is supposed to be accepted on its own page, the existing flow is interrupted and the user is redirected to a view.php to display that particular policy. The view template for such a policy contains a button for getting the user agreement. When pressed, the policy is marked as accepted and we can go to start again. To make this working during the signup, we need to extend the existing logic and start tracking which particular policies the visitor accepted prior reaching the signup form. Similarly, we need to start track which particular policies were displayed and use that list when evaluating which policies were unchecked on the consent page. --- .../policy/classes/output/page_agreedocs.php | 84 ++++++++++++++----- .../policy/classes/output/page_viewdoc.php | 13 +++ admin/tool/policy/index.php | 12 ++- .../policy/templates/page_agreedocs.mustache | 1 + .../policy/templates/page_viewdoc.mustache | 6 +- 5 files changed, 91 insertions(+), 25 deletions(-) diff --git a/admin/tool/policy/classes/output/page_agreedocs.php b/admin/tool/policy/classes/output/page_agreedocs.php index af6ebd6194784..771ac0560a27b 100644 --- a/admin/tool/policy/classes/output/page_agreedocs.php +++ b/admin/tool/policy/classes/output/page_agreedocs.php @@ -51,6 +51,9 @@ class page_agreedocs implements renderable, templatable { /** @var array $policies List of public policies objects with information about the user acceptance. */ protected $policies = null; + /** @var array List of policy version ids that were displayed to the user to agree with. */ + protected $listdocs = null; + /** @var array $agreedocs List of policy identifiers which the user has agreed using the form. */ protected $agreedocs = null; @@ -75,22 +78,20 @@ class page_agreedocs implements renderable, templatable { /** * Prepare the page for rendering. * - * @param array $agreedocs Array with the policy identifiers which the user has agreed using the form. + * @param array $listdocs List of policy version ids that were displayed to the user to agree with. + * @param array $agreedocs List of policy version ids that the user actually agreed with. * @param int $behalfid The userid to accept the policy versions as (such as child's id). * @param string $action Form action to identify when user agreeds policies. */ - public function __construct($agreedocs = null, $behalfid = 0, $action = null) { + public function __construct(array $listdocs, array $agreedocs = [], $behalfid = 0, $action = null) { global $USER; $realuser = manager::get_realuser(); + $this->listdocs = $listdocs; $this->agreedocs = $agreedocs; - if (empty($this->agreedocs)) { - $this->agreedocs = []; - } - $this->action = $action; - $this->isexistinguser = isloggedin() && !isguestuser(); + $behalfid = $behalfid ?: $USER->id; if ($realuser->id != $behalfid) { $this->behalfuser = core_user::get_user($behalfid, '*', MUST_EXIST); @@ -124,12 +125,14 @@ protected function accept_and_revoke_policies() { // Accept / revoke policies. $acceptversionids = array(); foreach ($this->policies as $policy) { - if (in_array($policy->id, $this->agreedocs)) { - // Save policy version doc to accept it. - $acceptversionids[] = $policy->id; - } else { - // Revoke policy doc. - api::revoke_acceptance($policy->id, $this->behalfid); + if (in_array($policy->id, $this->listdocs)) { + if (in_array($policy->id, $this->agreedocs)) { + // Save policy version doc to accept it. + $acceptversionids[] = $policy->id; + } else { + // If the policy was displayed but not agreed, revoke the eventually given acceptance. + api::revoke_acceptance($policy->id, $this->behalfid); + } } } // Accept all policy docs saved in $acceptversionids. @@ -158,15 +161,26 @@ protected function accept_and_revoke_policies() { } else { // New user. if (!empty($this->action) && confirm_sesskey()) { - // The form has been sent. $currentpolicyversionids = []; + $presignupcache = \cache::make('core', 'presignup'); + $acceptances = $presignupcache->get('tool_policy_policyversionidsagreed'); + if (!$acceptances) { + $acceptances = []; + } foreach ($this->policies as $policy) { $currentpolicyversionids[] = $policy->id; + if (in_array($policy->id, $this->listdocs)) { + if (in_array($policy->id, $this->agreedocs)) { + $acceptances[] = $policy->id; + } else { + $acceptances = array_values(array_diff($acceptances, [$policy->id])); + } + } } // If the user has accepted all the policies, add it to the session to let continue with the signup process. - $this->signupuserpolicyagreed = empty(array_diff($currentpolicyversionids, $this->agreedocs)); - \cache::make('core', 'presignup')->set('tool_policy_userpolicyagreed', - $this->signupuserpolicyagreed); + $this->signupuserpolicyagreed = empty(array_diff($currentpolicyversionids, $acceptances)); + $presignupcache->set('tool_policy_userpolicyagreed', $this->signupuserpolicyagreed); + $presignupcache->set('tool_policy_policyversionidsagreed', $acceptances); } else if (empty($this->policies)) { // There are no policies to agree to. Update the policyagreed value to avoid show empty consent page. \cache::make('core', 'presignup')->set('tool_policy_userpolicyagreed', 1); @@ -190,18 +204,41 @@ protected function accept_and_revoke_policies() { * @param moodle_url $returnurl URL to return after shown the policy docs. */ protected function redirect_to_policies($userid, $returnurl = null) { + + // Make a list of all policies that the user has not accepted yet. $allpolicies = $this->policies; + if ($this->isexistinguser) { $acceptances = api::get_user_acceptances($userid); - foreach ($allpolicies as $policy) { + foreach ($allpolicies as $ix => $policy) { if (api::is_user_version_accepted($userid, $policy->id, $acceptances)) { - // If this version is accepted by the user, remove from the pending policies list. - unset($allpolicies[array_search($policy, $allpolicies)]); + unset($allpolicies[$ix]); + } + } + } else { + $presignupcache = \cache::make('core', 'presignup'); + $acceptances = $presignupcache->get('tool_policy_policyversionidsagreed'); + if ($acceptances) { + foreach ($allpolicies as $ix => $policy) { + if (in_array($policy->id, $acceptances)) { + unset($allpolicies[$ix]); + } } } } if (!empty($allpolicies)) { + // Check if some of the to-be-accepted policies should be agreed on their own page. + foreach ($allpolicies as $policy) { + if ($policy->agreementstyle == policy_version::AGREEMENTSTYLE_OWNPAGE) { + if (empty($returnurl)) { + $returnurl = (new moodle_url('/admin/tool/policy/index.php'))->out_as_local_url(false); + } + $urlparams = ['versionid' => $policy->id, 'returnurl' => $returnurl]; + redirect(new moodle_url('/admin/tool/policy/view.php', $urlparams)); + } + } + $currentpolicyversionids = []; foreach ($allpolicies as $policy) { $currentpolicyversionids[] = $policy->id; @@ -232,6 +269,8 @@ protected function redirect_to_policies($userid, $returnurl = null) { ]; redirect(new moodle_url('/admin/tool/policy/view.php', $urlparams)); } + } else { + $this->redirect_to_previous_url(); } } @@ -401,7 +440,10 @@ public function export_for_template(renderer_base $output) { } } - $data->policies = array_values($this->policies); + // Filter out policies already shown on their own page, keep just policies to be shown here on the consent page. + $data->policies = array_values(array_filter($this->policies, function ($policy) { + return $policy->agreementstyle == policy_version::AGREEMENTSTYLE_CONSENTPAGE; + })); // If viewing docs in behalf of other user, get his/her full name and profile link. if (!empty($this->behalfuser)) { diff --git a/admin/tool/policy/classes/output/page_viewdoc.php b/admin/tool/policy/classes/output/page_viewdoc.php index 95613a73b9af9..73c216feec78f 100644 --- a/admin/tool/policy/classes/output/page_viewdoc.php +++ b/admin/tool/policy/classes/output/page_viewdoc.php @@ -151,6 +151,7 @@ protected function prepare_global_page_access() { * @return stdClass */ public function export_for_template(renderer_base $output) { + global $USER; $data = (object) [ 'pluginbaseurl' => (new moodle_url('/admin/tool/policy'))->out(false), @@ -163,6 +164,18 @@ public function export_for_template(renderer_base $output) { $data->editurl = (new moodle_url('/admin/tool/policy/editpolicydoc.php', $paramsurl))->out(false); } + if ($this->policy->agreementstyle == policy_version::AGREEMENTSTYLE_OWNPAGE) { + if (!api::is_user_version_accepted($USER->id, $this->policy->id)) { + unset($data->returnurl); + $data->accepturl = (new moodle_url('/admin/tool/policy/index.php', [ + 'listdoc[]' => $this->policy->id, + 'agreedoc[]' => $this->policy->id, + 'submit' => 'accept', + 'sesskey' => sesskey(), + ]))->out(false); + } + } + $data->policy = clone($this->policy); return $data; diff --git a/admin/tool/policy/index.php b/admin/tool/policy/index.php index 067d8fabc342a..9f03344d594ff 100644 --- a/admin/tool/policy/index.php +++ b/admin/tool/policy/index.php @@ -18,7 +18,8 @@ * Show a user the policy documents to be agreed to. * * Script parameters: - * agreedoc= Policy version id which have been accepted by the user. + * listdoc= List of policy version ids that were displayed to the user to accept. + * agreedoc= List of policy version ids that were accepted by the user. * behalfid= The user id to view the policy version as (such as child's id). * * @package tool_policy @@ -37,7 +38,8 @@ $submit = optional_param('submit', null, PARAM_NOTAGS); $cancel = optional_param('cancel', null, PARAM_NOTAGS); -$agreedocs = optional_param_array('agreedoc', null, PARAM_INT); +$listdocs = optional_param_array('listdoc', [], PARAM_INT); +$agreedocs = optional_param_array('agreedoc', [], PARAM_INT); $behalfid = optional_param('userid', null, PARAM_INT); $PAGE->set_context(context_system::instance()); @@ -45,6 +47,10 @@ $PAGE->set_url('/admin/tool/policy/index.php'); $PAGE->set_popup_notification_allowed(false); +if (array_diff($agreedocs, $listdocs)) { + throw new moodle_exception('invalidaccessparameter'); +} + if (isloggedin() && !isguestuser()) { // Existing user. $haspermissionagreedocs = api::can_accept_policies($behalfid); @@ -61,7 +67,7 @@ if (!$behalfid && \core\session\manager::is_loggedinas()) { $behalfid = $USER->id; } - $outputpage = new \tool_policy\output\page_agreedocs($agreedocs, $behalfid, $submit); + $outputpage = new \tool_policy\output\page_agreedocs($listdocs, $agreedocs, $behalfid, $submit); } $output = $PAGE->get_renderer('tool_policy'); diff --git a/admin/tool/policy/templates/page_agreedocs.mustache b/admin/tool/policy/templates/page_agreedocs.mustache index 31674de4217dd..8773b4b7fd3a6 100644 --- a/admin/tool/policy/templates/page_agreedocs.mustache +++ b/admin/tool/policy/templates/page_agreedocs.mustache @@ -92,6 +92,7 @@