MDL-56509 tool_usertours: Fetch all role shortnames

mvaraujo · Nov 1, 2016 · 41c1314 · 41c1314
1 parent a1f6a27
commit 41c1314
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 7 deletions.
diff --git a/admin/tool/usertours/classes/local/filter/role.php b/admin/tool/usertours/classes/local/filter/role.php
@@ -105,16 +105,32 @@ public static function filter_matches(tour $tour, context $context) {
             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) {
             $userroles = get_user_roles_with_special($context);
             $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
@@ -102,7 +102,7 @@ public function test_filter_matches_single_role() {
         $context = \context_course::instance($this->course->id);
 
         $roles = [
-            $this->roles['student'],
+            'student',
         ];
 
         // Note: No need to persist this tour.
@@ -131,8 +131,8 @@ public function test_filter_matches_multiple_role() {
         $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 @@ public function test_filter_matches_multiple_role_one_user() {
         $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 @@ public function test_filter_matches_multiple_role_including_admin() {
 
         $roles = [
             \tool_usertours\local\filter\role::ROLE_SITEADMIN,
-            $this->roles['teacher'],
-            $this->roles['editingteacher'],
+            'teacher',
+            'editingteacher',
         ];
 
         // Note: No need to persist this tour.