Merge branch 'MDL-22309_28' of git://github.com/dmonllao/moodle into MOODLE_28_STABLE

grade/report/singleview/classes/local/screen/grade.php

@@ -123,9 +123,13 @@ class grade extends tablelike implements selectable_items, filterable_items {
     public function init($selfitemisempty = false) {
         $roleids = explode(',', get_config('moodle', 'gradebookroles'));
 
-        $this->items = get_role_users(
-            $roleids, $this->context, false, '',
-            'u.lastname, u.firstname', null, $this->groupid);
+        $this->items = array();
+        foreach ($roleids as $roleid) {
+            // Keeping the first user appearance.
+            $this->items = $this->items + get_role_users(
+                $roleid, $this->context, false, '',
+                'u.lastname, u.firstname', null, $this->groupid);
+        }
 
         $this->totalitemcount = count_role_users($roleids, $this->context);
 

grade/report/singleview/classes/local/screen/select.php

@@ -48,11 +48,15 @@ class select extends screen {
 
         $roleids = explode(',', get_config('moodle', 'gradebookroles'));
 
-        $this->items = get_role_users(
-            $roleids, $this->context, false, '',
-            'u.id, u.lastname, u.firstname', null, $this->groupid,
-            $this->perpage * $this->page, $this->perpage
-        );
+        $this->items = array();
+        foreach ($roleids as $roleid) {
+            // Keeping the first user appearance.
+            $this->items = $this->items + get_role_users(
+                $roleid, $this->context, false, '',
+                'u.id, u.lastname, u.firstname', null, $this->groupid,
+                $this->perpage * $this->page, $this->perpage
+            );
+        }
         $this->item = $DB->get_record('course', array('id' => $this->courseid));
     }
 

lib/accesslib.php

@@ -4045,6 +4045,18 @@ function sort_by_roleassignment_authority($users, context $context, $roles = arr
 /**
  * Gets all the users assigned this role in this context or higher
  *
+ * Note that moodle is based on capabilities and it is usually better
+ * to check permissions than to check role ids as the capabilities
+ * system is more flexible. If you really need, you can to use this
+ * function but consider has_capability() as a possible substitute.
+ *
+ * The caller function is responsible for including all the
+ * $sort fields in $fields param.
+ *
+ * If $roleid is an array or is empty (all roles) you need to set $fields
+ * (and $sort by extension) params according to it, as the first field
+ * returned by the database should be unique (ra.id is the best candidate).
+ *
  * @param int $roleid (can also be an array of ints!)
  * @param context $context
  * @param bool $parent if true, get list of users assigned in higher context too
@@ -4073,6 +4085,23 @@ function get_role_users($roleid, context $context, $parent = false, $fields = ''
                   'r.shortname AS roleshortname, rn.name AS rolecoursealias';
     }
 
+    // Prevent wrong function uses.
+    if ((empty($roleid) || is_array($roleid)) && strpos($fields, 'ra.id') !== 0) {
+        debugging('get_role_users() without specifying one single roleid needs to be called prefixing ' .
+            'role assignments id (ra.id) as unique field, you can use $fields param for it.');
+
+        if (!empty($roleid)) {
+            // Solving partially the issue when specifying multiple roles.
+            $users = array();
+            foreach ($roleid as $id) {
+                // Ignoring duplicated keys keeping the first user appearance.
+                $users = $users + get_role_users($id, $context, $parent, $fields, $sort, $all, $group,
+                    $limitfrom, $limitnum, $extrawheretest, $whereorsortparams);
+            }
+            return $users;
+        }
+    }
+
     $parentcontexts = '';
     if ($parent) {
         $parentcontexts = substr($context->path, 1); // kill leading slash

lib/tests/accesslib_test.php

@@ -1381,6 +1381,7 @@ class core_accesslib_testcase extends advanced_testcase {
         $systemcontext = context_system::instance();
         $studentrole = $DB->get_record('role', array('shortname'=>'student'), '*', MUST_EXIST);
         $teacherrole = $DB->get_record('role', array('shortname'=>'editingteacher'), '*', MUST_EXIST);
+        $noeditteacherrole = $DB->get_record('role', array('shortname' => 'teacher'), '*', MUST_EXIST);
         $course = $this->getDataGenerator()->create_course();
         $coursecontext = context_course::instance($course->id);
         $otherid = create_role('Other role', 'other', 'Some other role', '');
@@ -1397,6 +1398,7 @@ class core_accesslib_testcase extends advanced_testcase {
         $this->getDataGenerator()->enrol_user($user3->id, $course->id, $teacherrole->id);
         $user4 = $this->getDataGenerator()->create_user();
         $this->getDataGenerator()->enrol_user($user4->id, $course->id, $studentrole->id);
+        $this->getDataGenerator()->enrol_user($user4->id, $course->id, $noeditteacherrole->id);
 
         $group = $this->getDataGenerator()->create_group(array('courseid'=>$course->id));
         groups_add_member($group, $user3);
@@ -1434,6 +1436,19 @@ class core_accesslib_testcase extends advanced_testcase {
         $users = get_role_users($teacherrole->id, $coursecontext, true, 'u.id, u.email, u.idnumber, u.firstname', 'u.idnumber', null, '', '', '', 'u.firstname = :xfirstname', array('xfirstname'=>'John'));
         $this->assertCount(1, $users);
         $this->assertArrayHasKey($user1->id, $users);
+
+        $users = get_role_users(array($noeditteacherrole->id, $studentrole->id), $coursecontext, false, 'ra.id', 'ra.id');
+        $this->assertDebuggingNotCalled();
+        $users = get_role_users(array($noeditteacherrole->id, $studentrole->id), $coursecontext, false, 'ra.userid', 'ra.userid');
+        $this->assertDebuggingCalled('get_role_users() without specifying one single roleid needs to be called prefixing ' .
+            'role assignments id (ra.id) as unique field, you can use $fields param for it.');
+        $users = get_role_users(array($noeditteacherrole->id, $studentrole->id), $coursecontext, false);
+        $this->assertDebuggingCalled('get_role_users() without specifying one single roleid needs to be called prefixing ' .
+            'role assignments id (ra.id) as unique field, you can use $fields param for it.');
+        $users = get_role_users(array($noeditteacherrole->id, $studentrole->id), $coursecontext,
+            false, 'u.id, u.firstname', 'u.id, u.firstname');
+        $this->assertDebuggingCalled('get_role_users() without specifying one single roleid needs to be called prefixing ' .
+            'role assignments id (ra.id) as unique field, you can use $fields param for it.');
     }
 
     /**