Merge branch 'MDL-37810-master' of https://github.com/snake/moodle
authorEloy Lafuente (stronk7) <stronk7@moodle.org>
Mon, 18 Sep 2017 23:45:29 +0000 (01:45 +0200)
committerEloy Lafuente (stronk7) <stronk7@moodle.org>
Mon, 18 Sep 2017 23:45:29 +0000 (01:45 +0200)
lib/accesslib.php
lib/tests/accesslib_test.php
user/renderer.php

index c60077d..0e15bf9 100644 (file)
@@ -2475,20 +2475,25 @@ function get_component_string($component, $contextlevel) {
 
 /**
  * Gets the list of roles assigned to this context and up (parents)
- * from the list of roles that are visible on user profile page
- * and participants page.
+ * from the aggregation of:
+ * a) the list of roles that are visible on user profile page and participants page (profileroles setting) and;
+ * b) if applicable, those roles the current user can assign in the context.
  *
  * @param context $context
  * @return array
  */
 function get_profile_roles(context $context) {
     global $CFG, $DB;
-
-    if (empty($CFG->profileroles)) {
-        return array();
+    // If the current user can assign roles, then they can also see those assignable roles on the profile and participants page,
+    // provided the roles are assigned to at least 1 user in the context.
+    $policyroles = empty($CFG->profileroles) ? [] : array_map('trim', explode(',', $CFG->profileroles));
+    $assignableroles = array_keys(get_assignable_roles($context));
+    $rolesinscope = array_values(array_unique(array_merge($policyroles, $assignableroles)));
+    if (empty($rolesinscope)) {
+        return [];
     }
 
-    list($rallowed, $params) = $DB->get_in_or_equal(explode(',', $CFG->profileroles), SQL_PARAMS_NAMED, 'a');
+    list($rallowed, $params) = $DB->get_in_or_equal($rolesinscope, SQL_PARAMS_NAMED, 'a');
     list($contextlist, $cparams) = $DB->get_in_or_equal($context->get_parent_context_ids(true), SQL_PARAMS_NAMED, 'p');
     $params = array_merge($params, $cparams);
 
@@ -2547,18 +2552,21 @@ function get_roles_used_in_context(context $context) {
  */
 function get_user_roles_in_course($userid, $courseid) {
     global $CFG, $DB;
-
-    if (empty($CFG->profileroles)) {
-        return '';
-    }
-
     if ($courseid == SITEID) {
         $context = context_system::instance();
     } else {
         $context = context_course::instance($courseid);
     }
+    // If the current user can assign roles, then they can also see those assignable roles on the profile and participants page,
+    // provided the roles are assigned to at least 1 user in the context.
+    $policyroles = empty($CFG->profileroles) ? [] : array_map('trim', explode(',', $CFG->profileroles));
+    $assignableroles = array_keys(get_assignable_roles($context));
+    $rolesinscope = array_values(array_unique(array_merge($policyroles, $assignableroles)));
+    if (empty($rolesinscope)) {
+        return '';
+    }
 
-    list($rallowed, $params) = $DB->get_in_or_equal(explode(',', $CFG->profileroles), SQL_PARAMS_NAMED, 'a');
+    list($rallowed, $params) = $DB->get_in_or_equal($rolesinscope, SQL_PARAMS_NAMED, 'a');
     list($contextlist, $cparams) = $DB->get_in_or_equal($context->get_parent_context_ids(true), SQL_PARAMS_NAMED, 'p');
     $params = array_merge($params, $cparams);
 
index 04f76fe..14375ee 100644 (file)
@@ -1533,6 +1533,7 @@ class core_accesslib_testcase extends advanced_testcase {
 
         $teacherrole = $DB->get_record('role', array('shortname'=>'editingteacher'), '*', MUST_EXIST);
         $studentrole = $DB->get_record('role', array('shortname'=>'student'), '*', MUST_EXIST);
+        $managerrole = $DB->get_record('role', array('shortname' => 'manager'), '*', MUST_EXIST);
         $course = $this->getDataGenerator()->create_course();
         $coursecontext = context_course::instance($course->id);
         $teacherrename = (object)array('roleid'=>$teacherrole->id, 'name'=>'Učitel', 'contextid'=>$coursecontext->id);
@@ -1541,6 +1542,7 @@ class core_accesslib_testcase extends advanced_testcase {
         $roleids = explode(',', $CFG->profileroles); // Should include teacher and student in new installs.
         $this->assertTrue(in_array($teacherrole->id, $roleids));
         $this->assertTrue(in_array($studentrole->id, $roleids));
+        $this->assertFalse(in_array($managerrole->id, $roleids));
 
         $user1 = $this->getDataGenerator()->create_user();
         role_assign($teacherrole->id, $user1->id, $coursecontext->id);
@@ -1548,6 +1550,8 @@ class core_accesslib_testcase extends advanced_testcase {
         $user2 = $this->getDataGenerator()->create_user();
         role_assign($studentrole->id, $user2->id, $coursecontext->id);
         $user3 = $this->getDataGenerator()->create_user();
+        $user4 = $this->getDataGenerator()->create_user();
+        role_assign($managerrole->id, $user4->id, $coursecontext->id);
 
         $roles = get_user_roles_in_course($user1->id, $course->id);
         $this->assertEquals(1, preg_match_all('/,/', $roles, $matches));
@@ -1559,6 +1563,25 @@ class core_accesslib_testcase extends advanced_testcase {
 
         $roles = get_user_roles_in_course($user3->id, $course->id);
         $this->assertSame('', $roles);
+
+        // Managers should be able to see a link to their own role type, given they can assign it in the context.
+        $this->setUser($user4);
+        $roles = get_user_roles_in_course($user4->id, $course->id);
+        $this->assertNotEmpty($roles);
+        $this->assertEquals(1, count(explode(',', $roles)));
+        $this->assertTrue(strpos($roles, role_get_name($managerrole, $coursecontext)) !== false);
+
+        // Managers should see 2 roles if viewing a user who has been enrolled as a student and a teacher in the course.
+        $roles = get_user_roles_in_course($user1->id, $course->id);
+        $this->assertEquals(2, count(explode(',', $roles)));
+        $this->assertTrue(strpos($roles, role_get_name($studentrole, $coursecontext)) !== false);
+        $this->assertTrue(strpos($roles, role_get_name($teacherrole, $coursecontext)) !== false);
+
+        // Students should not see the manager role if viewing a manager's profile.
+        $this->setUser($user2);
+        $roles = get_user_roles_in_course($user4->id, $course->id);
+        $this->assertEmpty($roles); // Should see 0 roles on the manager's profile.
+        $this->assertFalse(strpos($roles, role_get_name($managerrole, $coursecontext)) !== false);
     }
 
     /**
@@ -3166,6 +3189,136 @@ class core_accesslib_testcase extends advanced_testcase {
         $this->assertTrue(array_key_exists($student->id, $users));
         $this->assertFalse(array_key_exists($guest->id, $users));
     }
+
+    /**
+     * Test the get_profile_roles() function.
+     */
+    public function test_get_profile_roles() {
+        global $DB;
+        $this->resetAfterTest();
+
+        $course = $this->getDataGenerator()->create_course();
+        $coursecontext = context_course::instance($course->id);
+
+        // Assign a student role.
+        $studentrole = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST);
+        $user1 = $this->getDataGenerator()->create_user();
+        role_assign($studentrole->id, $user1->id, $coursecontext);
+
+        // Assign an editing teacher role.
+        $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher'), '*', MUST_EXIST);
+        $user2 = $this->getDataGenerator()->create_user();
+        role_assign($teacherrole->id, $user2->id, $coursecontext);
+
+        // Create a custom role that can be assigned at course level, but don't assign it yet.
+        create_role('Custom role', 'customrole', 'Custom course role');
+        $customrole = $DB->get_record('role', array('shortname' => 'customrole'), '*', MUST_EXIST);
+        set_role_contextlevels($customrole->id, [CONTEXT_COURSE]);
+        allow_assign($teacherrole->id, $customrole->id); // Allow teacher to assign the role in the course.
+
+        // Set the site policy 'profileroles' to show student, teacher and non-editing teacher roles (i.e. not the custom role).
+        $neteacherrole = $DB->get_record('role', array('shortname' => 'teacher'), '*', MUST_EXIST);
+        set_config('profileroles', "{$studentrole->id}, {$teacherrole->id}, {$neteacherrole->id}");
+
+        // A student in the course (given they can't assign roles) should see those roles which are:
+        // - listed in the 'profileroles' site policy AND
+        // - are assigned in the course context (or parent contexts).
+        // In this case, the non-editing teacher role is not assigned and should not be returned.
+        $expected = [
+            $teacherrole->id => (object) [
+                'id' => $teacherrole->id,
+                'name' => '',
+                'shortname' => $teacherrole->shortname,
+                'sortorder' => $teacherrole->sortorder,
+                'coursealias' => null
+            ],
+            $studentrole->id => (object) [
+                'id' => $studentrole->id,
+                'name' => '',
+                'shortname' => $studentrole->shortname,
+                'sortorder' => $studentrole->sortorder,
+                'coursealias' => null
+            ]
+        ];
+        $this->setUser($user1);
+        $this->assertEquals($expected, get_profile_roles($coursecontext));
+
+        // An editing teacher should also see only 2 roles at this stage as only 2 roles are assigned: 'teacher' and 'student'.
+        $this->setUser($user2);
+        $this->assertEquals($expected, get_profile_roles($coursecontext));
+
+        // Assign a custom role in the course.
+        $user3 = $this->getDataGenerator()->create_user();
+        role_assign($customrole->id, $user3->id, $coursecontext);
+
+        // Confirm that the teacher can see the custom role now that it's assigned.
+        $expectedteacher = [
+            $teacherrole->id => (object) [
+                'id' => $teacherrole->id,
+                'name' => '',
+                'shortname' => $teacherrole->shortname,
+                'sortorder' => $teacherrole->sortorder,
+                'coursealias' => null
+            ],
+            $studentrole->id => (object) [
+                'id' => $studentrole->id,
+                'name' => '',
+                'shortname' => $studentrole->shortname,
+                'sortorder' => $studentrole->sortorder,
+                'coursealias' => null
+            ],
+            $customrole->id => (object) [
+                'id' => $customrole->id,
+                'name' => 'Custom role',
+                'shortname' => $customrole->shortname,
+                'sortorder' => $customrole->sortorder,
+                'coursealias' => null
+            ]
+        ];
+        $this->setUser($user2);
+        $this->assertEquals($expectedteacher, get_profile_roles($coursecontext));
+
+        // And that the student can't, because the role isn't included in the 'profileroles' site policy.
+        $expectedstudent = [
+            $teacherrole->id => (object) [
+                'id' => $teacherrole->id,
+                'name' => '',
+                'shortname' => $teacherrole->shortname,
+                'sortorder' => $teacherrole->sortorder,
+                'coursealias' => null
+            ],
+            $studentrole->id => (object) [
+                'id' => $studentrole->id,
+                'name' => '',
+                'shortname' => $studentrole->shortname,
+                'sortorder' => $studentrole->sortorder,
+                'coursealias' => null
+            ]
+        ];
+        $this->setUser($user1);
+        $this->assertEquals($expectedstudent, get_profile_roles($coursecontext));
+
+        // If we have no roles listed in the site policy, the teacher should only see the student and custom roles.
+        $expectedteacher = [
+            $studentrole->id => (object) [
+                'id' => $studentrole->id,
+                'name' => '',
+                'shortname' => $studentrole->shortname,
+                'sortorder' => $studentrole->sortorder,
+                'coursealias' => null
+            ],
+            $customrole->id => (object) [
+                'id' => $customrole->id,
+                'name' => 'Custom role',
+                'shortname' => $customrole->shortname,
+                'sortorder' => $customrole->sortorder,
+                'coursealias' => null
+            ]
+        ];
+        set_config('profileroles', "");
+        $this->setUser($user2);
+        $this->assertEquals($expectedteacher, get_profile_roles($coursecontext));
+    }
 }
 
 /**
index d881f0c..89ceb2b 100644 (file)
@@ -281,11 +281,7 @@ class core_user_renderer extends plugin_renderer_base {
         }
 
         // Filter options for role.
-        $roleseditable = has_capability('moodle/role:assign', $context);
         $roles = role_fix_names(get_profile_roles($context), $context, ROLENAME_ALIAS, true);
-        if ($roleseditable) {
-            $roles += get_assignable_roles($context, ROLENAME_ALIAS);
-        }
         $criteria = get_string('role');
         $roleoptions = [];
         foreach ($roles as $id => $role) {