MDL-56509 tool_usertours: Remove guest from list and add admin

It is not possible to display a tour to users with the guest role so remove
all users with this archetype from the list.

This also changes behaviour such that it is possible to target a tour just
to administrators and administrators are not shown the tours for roles
which they do not have in the current context.

diff --git a/admin/tool/usertours/classes/local/filter/role.php b/admin/tool/usertours/classes/local/filter/role.php
index 96bc6ea..fec6b60 100644 (file)
--- a/admin/tool/usertours/classes/local/filter/role.php
+++ b/admin/tool/usertours/classes/local/filter/role.php
@@ -36,6 +36,13 @@ use context;
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class role extends base {
+    /**
+     * The Site Admin pseudo-role.
+     *
+     * @var ROLE_SITEADMIN int
+     */
+    const ROLE_SITEADMIN = -1;
+
     /**
      * The name of the filter.
      *
@@ -52,7 +59,24 @@ class role extends base {
      *                                  And whose values are the values to display
      */
     public static function get_filter_options() {
-        return role_get_names(null, ROLENAME_ALIAS, true);
+        $allroles = role_get_names(null, ROLENAME_ALIAS);
+
+        $roles = [];
+        foreach ($allroles as $role) {
+            if ($role->archetype === 'guest') {
+                // No point in including the 'guest' role as it isn't possible to show tours to a guest.
+                continue;
+            }
+            $roles[$role->shortname] = $role->localname;
+        }
+
+        // Add the Site Administrator pseudo-role.
+        $roles[self::ROLE_SITEADMIN] = get_string('administrator', 'core');
+
+        // Sort alphabetically too.
+        \core_collator::asort($roles);
+
+        return $roles;
     }
 
     /**
@@ -73,13 +97,14 @@ class role extends base {
             return true;
         }
 
-        if (is_siteadmin()) {
-            return true;
-        }
-
         // Presence within the array is sufficient. Ignore any value.
         $values = array_flip($values);
 
+        if (isset($values[self::ROLE_SITEADMIN]) && is_siteadmin()) {
+            // This tour has been restricted to a role including site admin, and this user is a site admin.
+            return true;
+        }
+
         $cache = \cache::make_from_params(\cache_store::MODE_REQUEST, 'tool_usertours', 'filter_role');
         $cachekey = "{$USER->id}_{$context->id}";
         $userroles = $cache->get($cachekey);

diff --git a/admin/tool/usertours/tests/role_filter_test.php b/admin/tool/usertours/tests/role_filter_test.php
index ade49dd..3f6fed4 100644 (file)
--- a/admin/tool/usertours/tests/role_filter_test.php
+++ b/admin/tool/usertours/tests/role_filter_test.php
@@ -119,9 +119,9 @@ class tool_usertours_role_filter_testcase extends advanced_testcase {
             }
         }
 
-        // The admin should always be able to view too.
+        // The admin can't view this one either.
         $this->setAdminUser();
-        $this->assertTrue(\tool_usertours\local\filter\role::filter_matches($tour, $context));
+        $this->assertFalse(\tool_usertours\local\filter\role::filter_matches($tour, $context));
     }
 
     /**
@@ -149,9 +149,9 @@ class tool_usertours_role_filter_testcase extends advanced_testcase {
             }
         }
 
-        // The admin should always be able to view too.
+        // The admin can't view this one either.
         $this->setAdminUser();
-        $this->assertTrue(\tool_usertours\local\filter\role::filter_matches($tour, $context));
+        $this->assertFalse(\tool_usertours\local\filter\role::filter_matches($tour, $context));
     }
 
     /**
@@ -181,8 +181,109 @@ class tool_usertours_role_filter_testcase extends advanced_testcase {
             }
         }
 
-        // The admin should always be able to view too.
+        // The admin can't view this one either.
+        $this->setAdminUser();
+        $this->assertFalse(\tool_usertours\local\filter\role::filter_matches($tour, $context));
+    }
+
+    /**
+     * Test the filter_matches function when it is targetted at an admin.
+     */
+    public function test_filter_matches_multiple_role_only_admin() {
+        $context = \context_course::instance($this->course->id);
+
+        $roles = [
+            \tool_usertours\local\filter\role::ROLE_SITEADMIN,
+        ];
+
+        $this->getDataGenerator()->enrol_user($this->student->id, $this->course->id, $this->roles['teacher']);
+
+        // Note: No need to persist this tour.
+        $tour = new \tool_usertours\tour();
+        $tour->set_filter_values('role', $roles);
+
+
+        // Note: The role filter does not use the context.
+        foreach ($this->testroles as $role) {
+            $this->setUser($this->$role);
+            $this->assertFalse(\tool_usertours\local\filter\role::filter_matches($tour, $context));
+        }
+
+        // The admin can view this one because it's only aimed at them.
+        $this->setAdminUser();
+        $this->assertTrue(\tool_usertours\local\filter\role::filter_matches($tour, $context));
+    }
+
+    /**
+     * Test the filter_matches function when multiple roles are set, including an admin user.
+     */
+    public function test_filter_matches_multiple_role_including_admin() {
+        $context = \context_course::instance($this->course->id);
+
+        $roles = [
+            \tool_usertours\local\filter\role::ROLE_SITEADMIN,
+            $this->roles['teacher'],
+            $this->roles['editingteacher'],
+        ];
+
+        // Note: No need to persist this tour.
+        $tour = new \tool_usertours\tour();
+        $tour->set_filter_values('role', $roles);
+
+        // Note: The role filter does not use the context.
+        foreach ($this->testroles as $role) {
+            $this->setUser($this->$role);
+            if ($role === 'student') {
+                $this->assertFalse(\tool_usertours\local\filter\role::filter_matches($tour, $context));
+            } else {
+                $this->assertTrue(\tool_usertours\local\filter\role::filter_matches($tour, $context));
+            }
+        }
+
+        // The admin can view this one because it's only aimed at them.
+        $this->setAdminUser();
+        $this->assertTrue(\tool_usertours\local\filter\role::filter_matches($tour, $context));
+    }
+
+    /**
+     * Test the filter_matches function when an admin user has multiple roles.
+     */
+    public function test_filter_matches_multiple_role_admin_user() {
+        global $USER;
+
+        $context = \context_course::instance($this->course->id);
+
+        $roles = [
+            \tool_usertours\local\filter\role::ROLE_SITEADMIN,
+        ];
+
         $this->setAdminUser();
+        $this->getDataGenerator()->enrol_user($USER->id, $this->course->id, $this->roles['student']);
+
+        // Note: No need to persist this tour.
+        $tour = new \tool_usertours\tour();
+        $tour->set_filter_values('role', $roles);
+
+        // The admin can view this one because it's only aimed at them.
         $this->assertTrue(\tool_usertours\local\filter\role::filter_matches($tour, $context));
     }
+
+    /**
+     * Test that the get_filter_options function does not include the guest roles.
+     */
+    public function test_get_filter_options_no_guest_roles() {
+        create_role('Test Role', 'testrole', 'This is a test role', 'guest');
+
+        $allroles = role_get_names(null, ROLENAME_ALIAS);
+        $options = \tool_usertours\local\filter\role::get_filter_options();
+
+        foreach ($allroles as $role) {
+            $hasrole = isset($options[$role->shortname]);
+            if ($role->archetype === 'guest') {
+                $this->assertFalse($hasrole);
+            } else {
+                $this->assertTrue($hasrole);
+            }
+        }
+    }
 }