From 96f1a7a2657fad31483421678253f00b699dd6e3 Mon Sep 17 00:00:00 2001 From: tjhunt Date: Thu, 30 Oct 2008 06:26:18 +0000 Subject: [PATCH] roles admin: Improve the usability of the assign roles interface - this is the first half of the work. Part of MDL-16993. To do this, I found a way to clean up the method signatures of a couple of accesslib methods, so I did, hence the fact that this seems to touch some unrelated files. Also, there is a nice new method in accesslib get_context_url, which gives you the natural URL for a context, so the course view page, or the user profile, etc. --- admin/roles/assign.html | 1 - admin/roles/assign.php | 67 +++++++------- admin/roles/override.php | 4 +- backup/restore_form.html | 13 +-- blocks/admin/block_admin.php | 2 +- lang/en_utf8/role.php | 4 + lib/accesslib.php | 152 +++++++++++++++++++++++++------ theme/standard/styles_layout.css | 8 ++ 8 files changed, 173 insertions(+), 78 deletions(-) diff --git a/admin/roles/assign.html b/admin/roles/assign.html index eb953b8bfb5..099bb2dad4d 100755 --- a/admin/roles/assign.html +++ b/admin/roles/assign.html @@ -3,7 +3,6 @@ - diff --git a/admin/roles/assign.php b/admin/roles/assign.php index afc98322cef..8c78c30a9c9 100755 --- a/admin/roles/assign.php +++ b/admin/roles/assign.php @@ -14,7 +14,6 @@ $remove = optional_param('remove', 0, PARAM_BOOL); $showall = optional_param('showall', 0, PARAM_BOOL); $searchtext = optional_param('searchtext', '', PARAM_RAW); // search string - $previoussearch = optional_param('previoussearch', 0, PARAM_BOOL); $hidden = optional_param('hidden', 0, PARAM_BOOL); // whether this assignment is hidden $extendperiod = optional_param('extendperiod', 0, PARAM_INT); $extendbase = optional_param('extendbase', 0, PARAM_INT); @@ -23,9 +22,7 @@ $errors = array(); - $previoussearch = ($searchtext != '') or ($previoussearch) ? 1:0; - - $baseurl = 'assign.php?contextid='.$contextid; + $baseurl = $CFG->wwwroot . '/' . $CFG->admin . '/role/assign.php?contextid=' . $contextid; if (!empty($userid)) { $baseurl .= '&userid='.$userid; } @@ -35,8 +32,9 @@ if (! $context = get_context_instance_by_id($contextid)) { print_error('wrongcontextid', 'error'); - } + $isfrontpage = $context->contextlevel == CONTEXT_COURSE && $context->instanceid == SITEID; + $contextname = print_context_name($context); $inmeta = 0; if ($context->contextlevel == CONTEXT_COURSE) { @@ -63,15 +61,14 @@ /// needed for tabs.php - $overridableroles = get_overridable_roles($context, 'name', ROLENAME_BOTH); - $assignableroles = get_assignable_roles($context, 'name', ROLENAME_BOTH); + $overridableroles = get_overridable_roles($context, ROLENAME_BOTH); + list($assignableroles, $assigncounts, $nameswithcounts) = get_assignable_roles($context, ROLENAME_BOTH, true); /// Get some language strings $strpotentialusers = get_string('potentialusers', 'role'); $strexistingusers = get_string('existingusers', 'role'); $straction = get_string('assignroles', 'role'); - $strroletoassign = get_string('roletoassign', 'role'); $strsearch = get_string('search'); $strshowall = get_string('showall'); $strparticipants = get_string('participants'); @@ -104,7 +101,7 @@ } } -/// Make sure this user can assign that role +/// Make sure this user can assign this role if ($roleid) { if (!isset($assignableroles[$roleid])) { @@ -158,7 +155,6 @@ } - /// Process incoming role assignment if ($frm = data_submitted()) { @@ -249,24 +245,20 @@ add_to_log($course->id, 'role', 'unassign', 'admin/roles/assign.php?contextid='.$context->id.'&roleid='.$roleid, $rolename, '', $USER->id); } else if ($showall) { $searchtext = ''; - $previoussearch = 0; } - - - } - if ($context->contextlevel==CONTEXT_COURSE and $context->instanceid == SITEID) { + if ($isfrontpage) { print_heading_with_help(get_string('frontpageroles', 'admin'), 'assignroles'); } else { - print_heading_with_help(get_string('assignrolesin', 'role', print_context_name($context)), 'assignroles'); + print_heading_with_help(get_string('assignrolesin', 'role', $contextname), 'assignroles'); } if ($context->contextlevel==CONTEXT_SYSTEM) { print_box(get_string('globalroleswarning', 'role')); } - if ($roleid) { /// prints a form to swap roles + if ($roleid) { /// Get all existing participants in this context. // Why is this not done with get_users??? @@ -383,12 +375,6 @@ $usercount = $DB->count_records_sql("$countfields $sql", $params); } - echo '
'; - $assignableroles = array('0'=>get_string('listallroles', 'role').'...') + $assignableroles; - popup_form("$CFG->wwwroot/$CFG->admin/roles/assign.php?userid=$userid&courseid=$courseid&contextid=$contextid&roleid=", - $assignableroles, 'switchrole', $roleid, '', '', '', false, 'self', $strroletoassign); - echo '
'; - print_simple_box_start('center'); include('assign.html'); print_simple_box_end(); @@ -404,8 +390,18 @@ print_simple_box_end(); } + /// Print a form to swap roles, and a link back to the all roles list. + echo '
'; + popup_form($baseurl . '&roleid=', $nameswithcounts, 'switchrole', + $roleid, '', '', '', false, 'self', get_string('assignanotherrole', 'role')); + echo '

' . get_string('backtoallroles', 'role') . '

'; + echo '
'; + } else { // Print overview table + // Print instruction + print_heading(get_string('chooseroletoassign', 'role'), 'center', 3); + // sync metacourse enrolments if needed if ($inmeta) { sync_metacourse($course); @@ -413,15 +409,12 @@ // Get the names of role holders for roles with between 1 and MAX_USERS_TO_LIST_PER_ROLE users, // and so determine whether to show the extra column. - $rolehodlercount = array(); $rolehodlernames = array(); - $strmorethanten = get_string('morethan', 'role', MAX_USERS_TO_LIST_PER_ROLE); + $strmorethanmax = get_string('morethan', 'role', MAX_USERS_TO_LIST_PER_ROLE); $showroleholders = false; foreach ($assignableroles as $roleid => $rolename) { - $countusers = count_role_users($roleid, $context); - $rolehodlercount[$roleid] = $countusers; $roleusers = ''; - if (0 < $countusers && $countusers <= MAX_USERS_TO_LIST_PER_ROLE) { + if (0 < $assigncounts[$roleid] && $assigncounts[$roleid] <= MAX_USERS_TO_LIST_PER_ROLE) { $roleusers = get_role_users($roleid, $context, false, 'u.id, u.lastname, u.firstname'); if (!empty($roleusers)) { $strroleusers = array(); @@ -431,8 +424,8 @@ $rolehodlernames[$roleid] = implode('
', $strroleusers); $showroleholders = true; } - } else if ($countusers > MAX_USERS_TO_LIST_PER_ROLE) { - $rolehodlernames[$roleid] = ''.$strmorethanten.''; + } else if ($assigncounts[$roleid] > MAX_USERS_TO_LIST_PER_ROLE) { + $rolehodlernames[$roleid] = ''.$strmorethanmax.''; } else { $rolehodlernames[$roleid] = ''; } @@ -443,18 +436,18 @@ $table->cellpadding = 5; $table->cellspacing = 0; $table->width = '60%'; - $table->head = array(get_string('roles', 'role'), get_string('description'), get_string('users')); + $table->head = array(get_string('role'), get_string('description'), get_string('userswiththisrole', 'role')); $table->wrap = array('nowrap', '', 'nowrap'); - $table->align = array('right', 'left', 'center'); + $table->align = array('left', 'left', 'center'); if ($showroleholders) { - $table->head[] = ''; + $table->headspan = array(1, 1, 2); $table->wrap[] = 'nowrap'; $table->align[] = 'left'; } foreach ($assignableroles as $roleid => $rolename) { $description = format_string($DB->get_field('role', 'description', array('id'=>$roleid))); - $row = array(''.$rolename.'',$description, $rolehodlercount[$roleid]); + $row = array(''.$rolename.'',$description, $assigncounts[$roleid]); if ($showroleholders) { $row[] = $rolehodlernames[$roleid]; } @@ -462,8 +455,12 @@ } print_table($table); + + if (!$isfrontpage && ($url = get_context_url($context))) { + echo '
' . + get_string('backto', '', $contextname) . '
'; + } } print_footer($course); - ?> diff --git a/admin/roles/override.php b/admin/roles/override.php index 34c99232f46..b5c5631be7a 100755 --- a/admin/roles/override.php +++ b/admin/roles/override.php @@ -50,8 +50,8 @@ } /// needed for tabs.php - $overridableroles = get_overridable_roles($context, 'name', ROLENAME_BOTH); - $assignableroles = get_assignable_roles($context, 'name', ROLENAME_BOTH); + $overridableroles = get_overridable_roles($context, ROLENAME_BOTH); + $assignableroles = get_assignable_roles($context, ROLENAME_BOTH); /// Get some language strings diff --git a/backup/restore_form.html b/backup/restore_form.html index efe6cc33f8c..11b998123d2 100644 --- a/backup/restore_form.html +++ b/backup/restore_form.html @@ -601,8 +601,7 @@ $xml_file = $CFG->dataroot."/temp/backup/".$backup_unique_code."/moodle.xml"; $info = restore_read_xml_info($xml_file); // fix for MDL-9068, front page course is just a normal course -$siterolesarray = get_assignable_roles (get_context_instance(CONTEXT_COURSE, $course->id), "shortname", ROLENAME_ORIGINAL); -$siterolesnamearray = get_assignable_roles (get_context_instance(CONTEXT_COURSE, $course->id), "name", ROLENAME_ORIGINAL); +$siterolesarray = get_assignable_roles(get_context_instance(CONTEXT_COURSE, $course->id), ROLENAME_ORIGINALANDSHORT); $allroles = $DB->get_records('role'); echo (''); @@ -654,13 +653,9 @@ if ($info->backup_moodle_version < 2006092801) { $roles = restore_read_xml_roles($xml_file); if (!empty($roles->roles)) { // possible to have course with no roles - foreach ($siterolesarray as $siteroleid=>$siteroleshortname) { - $siteroleschoicearray[$siteroleid] = $siterolesnamearray[$siteroleid]." (". $siterolesarray[$siteroleid].")"; - } - foreach ($roles->roles as $roleid=>$role) { - $mappableroles = $siteroleschoicearray; + $mappableroles = $siterolesarray; echo ('
'); echo ''; @@ -678,8 +673,8 @@ if ($info->backup_moodle_version < 2006092801) { // no exact role found, let's try to match shortname // this is useful in situations where basic roles differ slightly in definition $matchrole = 0; - foreach ($siterolesarray as $siteroleid=>$siteroleshortname) { - if ($siteroleshortname == $role->shortname) { + foreach ($siterolesarray as $siteroleid => $notused) { + if ($siteroleshortname == $allroles[$siteroleid]->shortname) { $matchrole = $siteroleid; break; } diff --git a/blocks/admin/block_admin.php b/blocks/admin/block_admin.php index cdce0214402..c3aa4189b58 100644 --- a/blocks/admin/block_admin.php +++ b/blocks/admin/block_admin.php @@ -67,7 +67,7 @@ class block_admin extends block_list { if (has_capability('moodle/role:assign', $context)) { $this->content->items[]=''.get_string('assignroles', 'role').''; $this->content->icons[]=''; - } else if (get_overridable_roles($context, 'name', ROLENAME_ORIGINAL)) { + } else if (get_overridable_roles($context, ROLENAME_ORIGINAL)) { $this->content->items[]=''.get_string('overridepermissions', 'role').''; $this->content->icons[]=''; } diff --git a/lang/en_utf8/role.php b/lang/en_utf8/role.php index a63609ca5f6..15747e54284 100644 --- a/lang/en_utf8/role.php +++ b/lang/en_utf8/role.php @@ -7,10 +7,12 @@ $string['allow'] = 'Allow'; $string['allowassign'] = 'Allow role assignments'; $string['allowoverride'] = 'Allow role overrides'; $string['allsiteusers'] = 'All site users'; +$string['assignanotherrole'] = 'Assign another role'; $string['assignroles'] = 'Assign roles'; $string['assignrolesin'] = 'Assign roles in $a'; $string['assignglobalroles'] = 'Assign system roles'; $string['assignmentcontext'] = 'Assignment context'; +$string['backtoallroles'] = 'Back to the list of all roles'; $string['blog:create'] = 'Create new blog entries'; $string['blog:manageentries'] = 'Edit and manage entries'; $string['blog:manageofficialtags'] = 'Manage official tags'; @@ -26,6 +28,7 @@ $string['category:create'] = 'Create categories'; $string['category:delete'] = 'Delete categories'; $string['category:update'] = 'Update categories'; $string['category:visibility'] = 'See hidden categories'; +$string['chooseroletoassign'] = 'Please choose a role to assign'; $string['context'] = 'Context'; $string['course:activityvisibility'] = 'Hide/show activities'; $string['course:bulkmessaging'] = 'Send a message to many people'; @@ -182,6 +185,7 @@ $string['user:viewdetails'] = 'View user profiles'; $string['user:viewhiddendetails'] = 'View hidden details of users'; $string['user:viewuseractivitiesreport'] = 'See user activity reports'; $string['userhashiddenassignments'] = 'This user has one or more hidden role assignments in this course'; +$string['userswiththisrole'] = 'Users with role'; $string['userswithrole'] = 'All users with a role'; $string['viewrole'] = 'View role details'; $string['xuserswiththerole'] = 'Users with the role \"$a->role\"'; diff --git a/lib/accesslib.php b/lib/accesslib.php index 021dc55271f..0122461bb41 100755 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -155,6 +155,7 @@ define('RISK_DATALOSS', 0x0020); define('ROLENAME_ORIGINAL', 0);// the name as defined in the role definition define('ROLENAME_ALIAS', 1); // the name as defined by a role alias define('ROLENAME_BOTH', 2); // Both, like this: Role alias (Original) +define('ROLENAME_ORIGINALANDSHORT', 0); // the name as defined in the role definition and the shortname in brackets $context_cache = array(); // Cache of all used context objects for performance (by level and instance) $context_cache_id = array(); // Index to above cache by id @@ -3243,7 +3244,14 @@ function capabilities_cleanup($component, $newcapdef=NULL) { /** - * prints human readable context identifier. + * Prints human readable context identifier. + * + * @param object $context the context. + * @param boolean $withprefix whether to prefix the name of the context with the + * type of context, e.g. User, Course, Forum, etc. + * @param boolean $short whether to user the short name of the thing. Only applies + * to course contexts + * @return string the human readable context name. */ function print_context_name($context, $withprefix = true, $short = false) { global $DB; @@ -3299,14 +3307,13 @@ function print_context_name($context, $withprefix = true, $short = false) { break; case CONTEXT_MODULE: // 1 to 1 to course - if ($cm = $DB->get_record('course_modules', array('id'=>$context->instanceid))) { - if ($module = $DB->get_record('modules', array('id'=>$cm->module))) { - if ($mod = $DB->get_record($module->name, array('id'=>$cm->instance))) { - if ($withprefix){ - $name = get_string('activitymodule').': '; - } - $name .= $mod->name; + if ($cm = $DB->get_record_sql('SELECT cm.*, md.name AS modname FROM {course_modules} cm ' . + 'JOIN {modules} md ON md.id = cm.module WHERE cm.id = ?', array($context->instanceid))) { + if ($mod = $DB->get_record($cm->modname, array('id' => $cm->instance))) { + if ($withprefix){ + $name = get_string('modulename', $cm->modname).': '; } + $name .= $mod->name; } } break; @@ -3331,11 +3338,60 @@ function print_context_name($context, $withprefix = true, $short = false) { default: error ('This is an unknown context (' . $context->contextlevel . ') in print_context_name!'); return false; - } + return $name; } +/** + * Get a URL for a context, if there is a natural one. For example, for + * CONTEXT_COURSE, this is the course page. For CONTEXT_USER it is the + * user profile page. + * + * First three parameters as for + * @param object $context the context. + * @return string a suitable URL, or blank. + */ +function get_context_url($context) { + global $CFG, $COURSE, $DB; + + $url = ''; + switch ($context->contextlevel) { + case CONTEXT_USER: + $url = $CFG->wwwroot . '/user/view.php?id=' . $context->instanceid; + if ($COURSE->id != SITEID) { + $url .= '&courseid=' . $COURSE->id; + } + break; + + case CONTEXT_COURSECAT: // Coursecat -> coursecat or site + $url = $CFG->wwwroot . '/course/category.php?id=' . $context->instanceid; + break; + + case CONTEXT_COURSE: // 1 to 1 to course cat + if ($context->instanceid == SITEID) { + $url = $CFG->wwwroot . '/'; + } else { + $url = $CFG->wwwroot . '/course/view.php?id=' . $context->instanceid; + } + break; + + case CONTEXT_MODULE: // 1 to 1 to course + if ($modname = $DB->get_field_sql('SELECT md.name AS modname FROM {course_modules} cm ' . + 'JOIN {modules} md ON md.id = cm.module WHERE cm.id = ?', array($context->instanceid))) { + $url = $CFG->wwwroot . '/mod/' . $modname . '/view.php?id=' . $context->instanceid; + } + break; + + case CONTEXT_SYSTEM: + case CONTEXT_GROUP: + case CONTEXT_BLOCK: + default: + $url = ''; + } + + return $url; +} /** * Extracts the relevant capabilities given a contextid. @@ -4001,12 +4057,16 @@ function allow_assign($sroleid, $troleid) { /** * Gets a list of roles that this user can assign in this context - * @param object $context - * @param string $field - * @param int $rolenamedisplay - * @return array + * @param object $context the context. + * @param string $field the field to return for each role. + * @param int $rolenamedisplay the type of role name to display. One of the + * ROLENAME_X constants. Default ROLENAME_ALIAS. + * @param $withusercounts if true, count the number of users with each role. + * @return array if $withusercounts is false, then an array $roleid => $rolename. + * if $withusercounts is true, returns a list of three arrays, + * $rolenames, $rolecounts, and $nameswithcounts. */ -function get_assignable_roles($context, $field='name', $rolenamedisplay=ROLENAME_ALIAS) { +function get_assignable_roles($context, $rolenamedisplay = ROLENAME_ALIAS, $withusercounts = false) { global $USER, $DB; if (!has_capability('moodle/role:assign', $context)) { @@ -4017,37 +4077,69 @@ function get_assignable_roles($context, $field='name', $rolenamedisplay=ROLENAME $parents[] = $context->id; $contexts = implode(',' , $parents); - if (!$roles = $DB->get_records_sql("SELECT ro.* - FROM {role} ro, - ( + $params = array(); + $extrafields = ''; + if ($rolenamedisplay == ROLENAME_ORIGINALANDSHORT) { + $extrafields .= ', ro.shortname'; + } + + if ($withusercounts) { + $countjoinandgroupby = + "JOIN + GROUP BY ro.id, ro.name$extrafields"; + $extrafields = ', (SELECT count(u.id) + FROM {role_assignments} cra JOIN {user} u ON cra.userid = u.id + WHERE cra.roleid = ro.id AND cra.contextid = :conid AND u.deleted = 0 + ) AS usercount'; + $params['conid'] = $context->id; + } + + $params['userid'] = $USER->id; + if (!$roles = $DB->get_records_sql("SELECT ro.id, ro.name$extrafields + FROM {role} ro + JOIN ( SELECT DISTINCT r.id FROM {role} r, {role_assignments} ra, {role_allow_assign} raa WHERE ra.userid = :userid AND ra.contextid IN ($contexts) AND raa.roleid = ra.roleid AND r.id = raa.allowassign - ) inline_view - WHERE ro.id = inline_view.id - ORDER BY ro.sortorder ASC", array('userid'=>$USER->id))) { + ) inline_view ON ro.id = inline_view.id + ORDER BY ro.sortorder ASC", $params)) { return array(); } + $rolenames = array(); foreach ($roles as $role) { - $roles[$role->id] = $role->$field; + $rolenames[$role->id] = $role->name; + if ($rolenamedisplay == ROLENAME_ORIGINALANDSHORT) { + $rolenames[$role->id] .= ' (' . $role->shortname . ')'; + } + } + if ($rolenamedisplay != ROLENAME_ORIGINALANDSHORT) { + $rolenames = role_fix_names($rolenames, $context, $rolenamedisplay); } - return role_fix_names($roles, $context, $rolenamedisplay); + if (!$withusercounts) { + return $rolenames; + } + + $rolecounts = array(); + $nameswithcounts = array(); + foreach ($roles as $role) { + $nameswithcounts[$role->id] = $rolenames[$role->id] . ' (' . $roles[$role->id]->usercount . ')'; + $rolecounts[$role->id] = $roles[$role->id]->usercount; + } + return array($rolenames, $rolecounts, $nameswithcounts); } /** * Gets a list of roles that this user can assign in this context, for the switchrole menu * * @param object $context - * @param string $field - * @param int $rolenamedisplay * @return array */ -function get_assignable_roles_for_switchrole($context, $field='name', $rolenamedisplay=ROLENAME_ALIAS) { +function get_assignable_roles_for_switchrole($context) { global $USER, $DB; if (!has_capability('moodle/role:assign', $context)) { @@ -4075,11 +4167,11 @@ function get_assignable_roles_for_switchrole($context, $field='name', $rolenamed return array(); } + $rolenames = array(); foreach ($roles as $role) { - $roles[$role->id] = $role->$field; + $rolenames[$role->id] = $role->name; } - - return role_fix_names($roles, $context, $rolenamedisplay); + return role_fix_names($rolenames, $context, ROLENAME_ALIAS); } /** @@ -4089,7 +4181,7 @@ function get_assignable_roles_for_switchrole($context, $field='name', $rolenamed * @param int $rolenamedisplay * @return array */ -function get_overridable_roles($context, $field='name', $rolenamedisplay=ROLENAME_ALIAS) { +function get_overridable_roles($context, $rolenamedisplay=ROLENAME_ALIAS) { global $USER, $DB; if (!has_capability('moodle/role:override', $context) and !has_capability('moodle/role:safeoverride', $context)) { @@ -4116,7 +4208,7 @@ function get_overridable_roles($context, $field='name', $rolenamedisplay=ROLENAM } foreach ($roles as $role) { - $roles[$role->id] = $role->$field; + $roles[$role->id] = $role->name; } return role_fix_names($roles, $context, $rolenamedisplay); diff --git a/theme/standard/styles_layout.css b/theme/standard/styles_layout.css index 16d7304ea28..1229060051e 100644 --- a/theme/standard/styles_layout.css +++ b/theme/standard/styles_layout.css @@ -1040,6 +1040,14 @@ body#admin-modules table.generaltable td.c0 margin-right:auto; } +#admin-roles-manage .backlink, +#admin-roles-assign .backlink, +#admin-roles-override .backlink { + text-align: right; + width: 90%; + margin: 2em auto 1em; +} + #admin-roles-manage table.rolecap, #admin-roles-override table.rolecap { margin-left:auto;