MDL-56509 tool_usertours: Fetch all role shortnames

diff --git a/admin/tool/usertours/classes/local/filter/role.php b/admin/tool/usertours/classes/local/filter/role.php
index fec6b60..a64ee83 100644 (file)
--- a/admin/tool/usertours/classes/local/filter/role.php
+++ b/admin/tool/usertours/classes/local/filter/role.php
@@ -105,7 +105,11 @@ class role extends base {
             return true;
         }
 
+        // Use a request cache to save on DB queries.
+        // We may be checking multiple tours and they'll all be for the same userid, and contextid
         $cache = \cache::make_from_params(\cache_store::MODE_REQUEST, 'tool_usertours', 'filter_role');
+
+        // Get all of the roles used in this context, including special roles such as user, and frontpageuser.
         $cachekey = "{$USER->id}_{$context->id}";
         $userroles = $cache->get($cachekey);
         if ($userroles === false) {
@@ -113,8 +117,20 @@ class role extends base {
             $cache->set($cachekey, $userroles);
         }
 
+        // Some special roles do not include the shortname.
+        // Therefore we must fetch all roles too. Thankfully these don't actually change based on context.
+        // They do require a DB call, so let's cache it.
+        $cachekey = "allroles";
+        $allroles = $cache->get($cachekey);
+        if ($allroles === false) {
+            $allroles = get_all_roles();
+            $cache->set($cachekey, $allroles);
+        }
+
+        // Now we can check whether any of the user roles are in the list of allowed roles for this filter.
         foreach ($userroles as $role) {
-            if (isset($values[$role->roleid])) {
+            $shortname = $allroles[$role->roleid]->shortname;
+            if (isset($values[$shortname])) {
                 return true;
             }
         }

diff --git a/admin/tool/usertours/tests/role_filter_test.php b/admin/tool/usertours/tests/role_filter_test.php
index 3f6fed4..940e517 100644 (file)
--- a/admin/tool/usertours/tests/role_filter_test.php
+++ b/admin/tool/usertours/tests/role_filter_test.php
@@ -102,7 +102,7 @@ class tool_usertours_role_filter_testcase extends advanced_testcase {
         $context = \context_course::instance($this->course->id);
 
         $roles = [
-            $this->roles['student'],
+            'student',
         ];
 
         // Note: No need to persist this tour.
@@ -131,8 +131,8 @@ class tool_usertours_role_filter_testcase extends advanced_testcase {
         $context = \context_course::instance($this->course->id);
 
         $roles = [
-            $this->roles['teacher'],
-            $this->roles['editingteacher'],
+            'teacher',
+            'editingteacher',
         ];
 
         // Note: No need to persist this tour.
@@ -161,7 +161,7 @@ class tool_usertours_role_filter_testcase extends advanced_testcase {
         $context = \context_course::instance($this->course->id);
 
         $roles = [
-            $this->roles['student'],
+            'student',
         ];
 
         $this->getDataGenerator()->enrol_user($this->student->id, $this->course->id, $this->roles['teacher']);
@@ -222,8 +222,8 @@ class tool_usertours_role_filter_testcase extends advanced_testcase {
 
         $roles = [
             \tool_usertours\local\filter\role::ROLE_SITEADMIN,
-            $this->roles['teacher'],
-            $this->roles['editingteacher'],
+            'teacher',
+            'editingteacher',
         ];
 
         // Note: No need to persist this tour.