diff --git a/grade/report/singleview/classes/local/screen/grade.php b/grade/report/singleview/classes/local/screen/grade.php index 2739f0f5896b1..51346d1b99a78 100644 --- a/grade/report/singleview/classes/local/screen/grade.php +++ b/grade/report/singleview/classes/local/screen/grade.php @@ -123,9 +123,13 @@ public function original_definition() { 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); diff --git a/grade/report/singleview/classes/local/screen/select.php b/grade/report/singleview/classes/local/screen/select.php index 0719fd07ad039..c90d9ba0137d2 100644 --- a/grade/report/singleview/classes/local/screen/select.php +++ b/grade/report/singleview/classes/local/screen/select.php @@ -48,11 +48,15 @@ public function init($selfitemisempty = false) { $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)); } diff --git a/lib/accesslib.php b/lib/accesslib.php index 15a1802603b51..ae8ab33af38bf 100644 --- a/lib/accesslib.php +++ b/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 diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php index c1af37e183c54..035e0f8eaef7c 100644 --- a/lib/tests/accesslib_test.php +++ b/lib/tests/accesslib_test.php @@ -1381,6 +1381,7 @@ public function test_get_role_users() { $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 @@ public function test_get_role_users() { $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 @@ public function test_get_role_users() { $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.'); } /**