Merge branch 'MDL-72325-master-tourspolicy' of git://github.com/mudrd8mz/moodle

admin/tool/usertours/classes/manager.php

@@ -624,6 +624,12 @@ class manager {
     public static function get_matching_tours(\moodle_url $pageurl): array {
         global $PAGE;
 
+        if (\core_user::awaiting_action()) {
+            // User not fully ready to use the site. Don't show any tours, we need the user to get properly set up so
+            // that all require_login() and other bits work as expected.
+            return [];
+        }
+
         $tours = cache::get_matching_tourdata($pageurl);
 
         $matches = [];

admin/tool/usertours/tests/manager_test.php

@@ -325,6 +325,8 @@ class tool_usertours_manager_testcase extends advanced_testcase {
     public function test_get_matching_tours(array $alltours, string $url, array $expected) {
         $this->resetAfterTest();
 
+        $this->setGuestUser();
+
         foreach ($alltours as $tourconfig) {
             $tour = $this->helper_create_tour((object) $tourconfig);
             $this->helper_create_step((object) ['tourid' => $tour->get_id()]);
@@ -336,4 +338,34 @@ class tool_usertours_manager_testcase extends advanced_testcase {
             $this->assertEquals($expected[$i], $matches[$i]->get_name());
         }
     }
+
+    /**
+     * Test that no matching tours are returned if there is pending site policy agreement.
+     */
+    public function test_get_matching_tours_for_user_without_site_policy_agreed() {
+        global $CFG;
+
+        $this->resetAfterTest();
+        $this->setGuestUser();
+
+        $tour = $this->helper_create_tour((object) [
+            'pathmatch' => '/%',
+            'enabled' => true,
+            'name' => 'Test tour',
+            'description' => '',
+            'configdata' => '',
+        ]);
+
+        $this->helper_create_step((object) [
+            'tourid' => $tour->get_id(),
+        ]);
+
+        $matches = \tool_usertours\manager::get_matching_tours(new moodle_url('/'));
+        $this->assertEquals(1, count($matches));
+
+        $CFG->sitepolicyguest = 'https://example.com';
+
+        $matches = \tool_usertours\manager::get_matching_tours(new moodle_url('/'));
+        $this->assertEmpty($matches);
+    }
 }

lib/classes/user.php

@@ -1134,4 +1134,39 @@ class core_user {
         }
     }
 
+    /**
+     * Is the user expected to perform an action to start using Moodle properly?
+     *
+     * This covers cases such as filling the profile, changing password or agreeing to the site policy.
+     *
+     * @param stdClass $user User object, defaults to the current user.
+     * @return bool
+     */
+    public static function awaiting_action(stdClass $user = null): bool {
+        global $USER;
+
+        if ($user === null) {
+            $user = $USER;
+        }
+
+        if (user_not_fully_set_up($user)) {
+            // Awaiting the user to fill all fields in the profile.
+            return true;
+        }
+
+        if (get_user_preferences('auth_forcepasswordchange', false, $user)) {
+            // Awaiting the user to change their password.
+            return true;
+        }
+
+        if (empty($user->policyagreed) && !is_siteadmin($user)) {
+            $manager = new \core_privacy\local\sitepolicy\manager();
+
+            if ($manager->is_defined(isguestuser($user))) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }

lib/tests/user_test.php

@@ -770,4 +770,53 @@ class core_user_testcase extends advanced_testcase {
         $this->assertFalse(\core_user::is_real_user(core_user::get_support_user()->id, true));
     }
 
+    /**
+     * Tests for the {@see \core_user::awaiting_action()} method.
+     */
+    public function test_awaiting_action() {
+        global $CFG, $DB, $USER;
+
+        $guest = \core_user::get_user($CFG->siteguest);
+        $student = $this->getDataGenerator()->create_user();
+        $teacher = $this->getDataGenerator()->create_user();
+        $manager = $this->getDataGenerator()->create_user();
+        $admin = get_admin();
+
+        $this->getDataGenerator()->role_assign($DB->get_field('role', 'id', ['shortname' => 'manager']),
+            $manager->id, \context_system::instance()->id);
+
+        // Scenario: Guests required to agree to site policy.
+        $this->assertFalse(\core_user::awaiting_action($guest));
+
+        $CFG->sitepolicyguest = 'https://example.com';
+        $this->assertTrue(\core_user::awaiting_action($guest));
+
+        $guest->policyagreed = 1;
+        $this->assertFalse(\core_user::awaiting_action($guest));
+
+        // Scenario: Student required to fill their profile.
+        $this->assertFalse(\core_user::awaiting_action($student));
+
+        $student->firstname = '';
+        $this->assertTrue(\core_user::awaiting_action($student));
+
+        $student->firstname = 'Alice';
+        $this->assertFalse(\core_user::awaiting_action($student));
+
+        // Scenario: Teacher force to change their password.
+        $this->assertFalse(\core_user::awaiting_action($teacher));
+
+        set_user_preference('auth_forcepasswordchange', 1, $teacher);
+        $this->assertTrue(\core_user::awaiting_action($teacher));
+
+        unset_user_preference('auth_forcepasswordchange', $teacher);
+        $this->assertFalse(\core_user::awaiting_action($teacher));
+
+        // Scenario: Admins do not need to agree to the policy but others do.
+        $this->assertFalse(\core_user::awaiting_action($admin));
+        $this->assertFalse(\core_user::awaiting_action($manager));
+        $CFG->sitepolicy = 'https://example.com';
+        $this->assertFalse(\core_user::awaiting_action($admin));
+        $this->assertTrue(\core_user::awaiting_action($manager));
+    }
 }

lib/upgrade.txt

@@ -60,6 +60,9 @@ information provided here is intended especially for developers.
   - generate_uuid
 * The YUI moodle-core-formchangechecker module has been deprecated and replaced with a new AMD module
   core_form/changechecker.
+* New method \core_user::awaiting_action() has been introduced to check if the user is fully ready to use the site or
+  whether there is an action (such as filling the missing profile field, changing password or agreeing to the site
+  policy) needed.
 
 === 3.11.2 ===
 * For security reasons, filelib has been updated so all requests now use emulated redirects.

message/classes/helper.php

@@ -551,10 +551,7 @@ class helper {
         global $USER, $CFG, $PAGE;
 
         // Early bail out conditions.
-        if (empty($CFG->messaging) || !isloggedin() || isguestuser() || user_not_fully_set_up($USER) ||
-            get_user_preferences('auth_forcepasswordchange') ||
-            (!$USER->policyagreed && !is_siteadmin() &&
-                ($manager = new \core_privacy\local\sitepolicy\manager()) && $manager->is_defined())) {
+        if (empty($CFG->messaging) || !isloggedin() || isguestuser() || \core_user::awaiting_action()) {
             return '';
         }
 

message/output/popup/lib.php

@@ -34,10 +34,7 @@ function message_popup_render_navbar_output(\renderer_base $renderer) {
     global $USER, $CFG;
 
     // Early bail out conditions.
-    if (!isloggedin() || isguestuser() || user_not_fully_set_up($USER) ||
-        get_user_preferences('auth_forcepasswordchange') ||
-        (!$USER->policyagreed && !is_siteadmin() &&
-            ($manager = new \core_privacy\local\sitepolicy\manager()) && $manager->is_defined())) {
+    if (!isloggedin() || isguestuser() || \core_user::awaiting_action()) {
         return '';
     }
 

user/classes/analytics/indicator/user_profile_set.php

@@ -72,8 +72,7 @@ class user_profile_set extends \core_analytics\local\indicator\linear {
         // Nothing set results in -1.
         $calculatedvalue = self::MIN_VALUE;
 
-        $sitepolicymanager = new \core_privacy\local\sitepolicy\manager();
-        if ($sitepolicymanager->is_defined() && !$user->policyagreed) {
+        if (\core_user::awaiting_action($user)) {
             return self::MIN_VALUE;
         }