From: tjhunt <tjhunt>
Date: Fri, 14 Nov 2008 05:46:27 +0000 (+0000)
Subject: role override ui: MDL-8313 basic mode + toggle + usability improvements
X-Git-Url: http://git.mjollnir.org/gw?a=commitdiff_plain;h=8aee9bcc27b9ce998ef04d89774d968bab1f7065;p=moodle.git

role override ui: MDL-8313 basic mode + toggle + usability improvements

* Basic mode just hides the prohobit column. Sadly that is all we can do.
* Button to toggle in and out of advanced mode, with user_perference.
* The permission you are selected is now on the page as a label, rather than just being in a tool-tip. (Not pretty, but helpful, I think. I may change my mind on this.)
* You can click anywhere in the table cell, rather than having to hit the radio button exactly.
* Brief instructions at the top of the page.
* Submit buttons both top and bottom of the page.
---

diff --git a/admin/roles/lib.php b/admin/roles/lib.php
index 33a641a294..330e822e58 100644
--- a/admin/roles/lib.php
+++ b/admin/roles/lib.php
@@ -108,7 +108,7 @@ abstract class capability_table_base {
                     $this->get_row_classes($capability)))) . '">';
 
         /// Table cell for the capability name.
-            echo '<td class="name"><span class="cap-desc">' . get_capability_docs_link($capability) .
+            echo '<th scope="row" class="name"><span class="cap-desc">' . get_capability_docs_link($capability) .
                     '<span class="cap-name">' . $capability->name . '</span></span></td>';
 
         /// Add the cells specific to this table.
@@ -284,7 +284,7 @@ abstract class capability_table_with_risks extends capability_table_base {
     }
 
     protected function add_header_cells() {
-        echo '<th class="risk" colspan="' . $this->num_extra_columns() . '" scope="col">' . get_string('risks','role') . '</th>';
+        echo '<th class="risk" colspan="' . count($this->allrisks) . '" scope="col">' . get_string('risks','role') . '</th>';
     }
 
     protected function num_extra_columns() {
@@ -330,9 +330,11 @@ abstract class capability_table_with_risks extends capability_table_base {
     }
 }
 
-class override_permissions_capability_table extends capability_table_with_risks {
+class override_permissions_table_advanced extends capability_table_with_risks {
     protected $roleid;
     protected $inheritedcapabilities;
+    protected $displaypermissions;
+    protected $strnotset;
     protected $localoverrides;
     protected $changed = array(); // $localoverrides that were changed by the submitted data, and so need to be saved.
     protected $haslockedcapabiltites = false;
@@ -353,6 +355,8 @@ class override_permissions_capability_table extends capability_table_with_risks
         global $DB;
         parent::__construct($context, 'overriderolestable');
         $this->roleid = $roleid;
+        $this->displaypermissions = $this->allpermissions;
+        $this->strnotset = get_string('notset', 'role');
 
     /// Get the capabiltites from the parent context, so that can be shown in the interface.
         $parentcontext = get_context_instance_by_id(get_parent_contextid($context));
@@ -418,14 +422,12 @@ class override_permissions_capability_table extends capability_table_with_risks
     }
 
     protected function add_header_cells() {
-        foreach ($this->strperms as $permname => $strpermname) {
-            echo '<th class="' . $permname . '" scope="col">' . $strpermname . '</th>';
-        }
+        echo '<th colspan="' . count($this->displaypermissions) . '" scope="col">' . get_string('permission', 'role') . '</th>';
         parent::add_header_cells();
     }
 
     protected function num_extra_columns() {
-        return count($this->strperms) + parent::num_extra_columns();
+        return count($this->displaypermissions) + parent::num_extra_columns();
     }
 
     protected function skip_row($capability) {
@@ -433,13 +435,19 @@ class override_permissions_capability_table extends capability_table_with_risks
     }
 
     protected function add_row_cells($capability) {
+        $this->add_permission_cells($capability);
+        parent::add_row_cells($capability);
+    }
+
+    protected function add_permission_cells($capability) {
         $disabled = '';
         if ($capability->locked || $this->inheritedcapabilities[$capability->name] == CAP_PROHIBIT) {
             $disabled = ' disabled="disabled"';
         }
 
     /// One cell for each possible permission.
-        foreach ($this->allpermissions as $perm => $permname) {
+        foreach ($this->displaypermissions as $perm => $permname) {
+            $strperm = $this->strperms[$permname];
             $extraclass = '';
             if ($perm != CAP_INHERIT && $perm == $this->inheritedcapabilities[$capability->name]) {
                 $extraclass = ' capcurrent';
@@ -449,11 +457,43 @@ class override_permissions_capability_table extends capability_table_with_risks
                 $checked = ' checked="checked"';
             }
             echo '<td class="' . $permname . $extraclass . '">';
-            echo '<input type="radio" title="' . $this->strperms[$permname] . '" name="' . $capability->name .
-                    '" value="' . $perm . '"' . $checked . $disabled . ' />';
+            echo '<label><input type="radio" name="' . $capability->name .
+                    '" value="' . $perm . '"' . $checked . $disabled . ' /> ';
+            if ($perm == CAP_INHERIT) {
+                $inherited = $this->inheritedcapabilities[$capability->name];
+                if ($inherited == CAP_INHERIT) {
+                    $inherited = $this->strnotset;
+                } else {
+                    $inherited = $this->strperms[$this->allpermissions[$inherited]];
+                }
+                $strperm .= ' (' . $inherited . ')';
+            }
+            echo '<span class="note">' . $strperm . '</span>';
+            echo '</label></td>';
+        }
+    }
+}
+
+class override_permissions_table_basic extends override_permissions_table_advanced {
+    protected $stradvmessage;
+
+    public function __construct($context, $roleid, $safeoverridesonly) {
+        global $DB;
+        parent::__construct($context, $roleid, $safeoverridesonly);
+        unset($this->displaypermissions[CAP_PROHIBIT]);
+        $this->stradvmessage = get_string('useshowadvancedtochange', 'role');
+    }
+
+    protected function add_permission_cells($capability) {
+        if ($this->localoverrides[$capability->name] == CAP_PROHIBIT) {
+            $permname = $this->allpermissions[CAP_PROHIBIT];
+            echo '<td class="' . $permname . '" colspan="' . count($this->displaypermissions) . '">';
+            echo '<input type="hidden" name="' . $capability->name . '" value="' . CAP_PROHIBIT . '" />';
+            echo $this->strperms[$permname] . '<span class="note">' . $this->stradvmessage . '</span>';
             echo '</td>';
+        } else {
+            parent::add_permission_cells($capability);
         }
-        parent::add_row_cells($capability);
     }
 }
 
@@ -485,7 +525,9 @@ abstract class role_assign_user_selector_base extends user_selector_base {
     }
 
     protected function get_options() {
+        global $CFG;
         $options = parent::get_options();
+        $options['file'] = $CFG->admin . '/roles/lib.php';
         $options['roleid'] = $this->roleid;
         $options['contextid'] = $this->context->id;
         return $options;
diff --git a/admin/roles/override.php b/admin/roles/override.php
index d2ced57a6e..682d906bef 100755
--- a/admin/roles/override.php
+++ b/admin/roles/override.php
@@ -117,9 +117,13 @@
         }
     }
 
-/// get all cababilities
+/// If we are actually overriding a role, create the table object, and save changes if appropriate.
     if ($roleid) {
-        $overridestable = new override_permissions_capability_table($context, $roleid, $safeoverridesonly);
+        if ($showadvanced) {
+            $overridestable = new override_permissions_table_advanced($context, $roleid, $safeoverridesonly);
+        } else {
+            $overridestable = new override_permissions_table_basic($context, $roleid, $safeoverridesonly);
+        }
 
         if (optional_param('savechanges', false, PARAM_BOOL) && confirm_sesskey()) {
             $overridestable->save_changes();
@@ -184,9 +188,8 @@
                 $showadvancedlabel = get_string('showadvanced', 'form');
             }
             ?>
-<form id="overrideform" action="<?php echo $baseurl; ?>" method="post"><div>
+<form id="overrideform" action="<?php echo $baseurl . '&amp;roleid=' . $roleid; ?>" method="post"><div>
     <input type="hidden" name="sesskey" value="<?php p(sesskey()); ?>" />
-    <input type="hidden" name="roleid" value="<?php p($roleid); ?>" />
 
     <div class="advancedbutton">
         <input type="submit" name="toggleadvanced" value="<?php echo $showadvancedlabel ?>" />
@@ -197,10 +200,11 @@
     </div>
             <?php
 
+            echo '<p class="overridenotice">' . get_string('highlightedcellshowsinherit', 'role') . ' </p>';
             $overridestable->display();
 
             if ($overridestable->has_locked_capabiltites()) {
-                echo '<div class="sefeoverridenotice">' . get_string('safeoverridenotice', 'role') . "</div>\n";
+                echo '<p class="overridenotice">' . get_string('safeoverridenotice', 'role') . "</p>\n";
             }
 
             ?>
diff --git a/admin/roles/roles.js b/admin/roles/roles.js
index c1588116c9..799d62afe0 100644
--- a/admin/roles/roles.js
+++ b/admin/roles/roles.js
@@ -96,7 +96,7 @@ cap_table_filter = {
                 capssincelastheading = 0;
             }
             if (YAHOO.util.Dom.hasClass(row, 'rolecap')) {
-                var capcell = YAHOO.util.Dom.getElementsByClassName('name', 'td', row)[0];
+                var capcell = YAHOO.util.Dom.getElementsByClassName('name', 'th', row)[0];
                 var capname = capcell.innerText || capcell.textContent;
                 if (capname.indexOf(filtertext) >= 0) {
                     cap_table_filter.set_visible(row, true);
diff --git a/lang/en_utf8/role.php b/lang/en_utf8/role.php
index cb64fb68f7..00030661fd 100644
--- a/lang/en_utf8/role.php
+++ b/lang/en_utf8/role.php
@@ -100,6 +100,8 @@ $string['grade:view'] = 'View own grades';
 $string['grade:viewall'] = 'View grades of other users';
 $string['grade:viewhidden'] = 'View hidden grades for owner';
 $string['hidden'] = 'Hidden';
+$string['highlightedcellshowsinherit'] = 'The highlighted cells in the table below show the permission (if any) that will be inherited. Apart from the capabilties whose permission you actually want to alter, you should leave everything set to Inherit.';
+$string['highlightedcellshowsdefault'] = 'The highlighted cells in the table below show the default permission for this type of role.';
 $string['inactiveformorethan'] = 'inactive for more than $a->timeperiod';
 $string['ingroup'] = 'in the group \"$a->group\"';
 $string['inherit'] = 'Inherit';
@@ -133,6 +135,7 @@ $string['overrideroles'] = 'Override roles';
 $string['overriderolesin'] = 'Override roles in $a';
 $string['overrides'] = 'Overrides';
 $string['overridesbycontext'] = 'Overrides (by context)';
+$string['permission'] = 'Permission';
 $string['permissions'] = 'Permissions';
 $string['permissionsforuser'] = 'Permissions for user $a';
 $string['potentialusers'] = '$a potential users';
@@ -214,6 +217,7 @@ $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['useshowadvancedtochange'] = 'Use \'Show advanced\' to change';
 $string['viewrole'] = 'View role details';
 $string['whydoesuserhavecap'] = 'Why does $a->fullname have capability $a->capability in context $a->context?';
 $string['whydoesusernothavecap'] = 'Why does $a->fullname not have capability $a->capability in context $a->context?';
diff --git a/theme/standard/styles_color.css b/theme/standard/styles_color.css
index f0723e84e9..4b8cd81048 100644
--- a/theme/standard/styles_color.css
+++ b/theme/standard/styles_color.css
@@ -1198,7 +1198,8 @@ table.explainpermissions .overridden {
 }
 
 #admin-roles-manage .rolecap .cap-desc .cap-name,
-#admin-roles-override .rolecap .cap-desc .cap-name {
+#admin-roles-override .rolecap .cap-desc .cap-name,
+.rolecap .note {
   color: #888;
 }
 
diff --git a/theme/standard/styles_fonts.css b/theme/standard/styles_fonts.css
index bf47905090..99a762568a 100644
--- a/theme/standard/styles_fonts.css
+++ b/theme/standard/styles_fonts.css
@@ -250,7 +250,8 @@ body#admin-index .copyright {
 }
 
 #admin-roles-manage .rolecap .cap-desc .cap-name,
-#admin-roles-override .rolecap .cap-desc .cap-name {
+#admin-roles-override .rolecap .cap-desc .cap-name,
+.rolecap .note {
   font-size: 0.75em;
 }
 
diff --git a/theme/standard/styles_layout.css b/theme/standard/styles_layout.css
index a001252b9d..846fd41808 100644
--- a/theme/standard/styles_layout.css
+++ b/theme/standard/styles_layout.css
@@ -1134,7 +1134,8 @@ table.roledesc {
 }
 
 table.rolecap {
-  width: 100%;
+  margin-left: auto;
+  margin-right: auto;
 }
 .capabilitysearchui {
   text-align: left;
@@ -1145,14 +1146,23 @@ table.rolecap .hiddenrow {
   display: none;
 }
 
+tr.rolecap th {
+  text-align: left;
+  font-weight: normal;
+}
 table.rolecap .inherit,
 table.rolecap .allow,
 table.rolecap .prevent,
 table.rolecap .prohibit {
   text-align:center;
 }
-
-table.rolecap .cap-desc .cap-name {
+table.rolecap label {
+  display: block;
+  width: 100%;
+  height: 2.5em;
+}
+table.rolecap .cap-desc .cap-name,
+.rolecap .note {
   display: block;
 }
 
@@ -1162,8 +1172,9 @@ table.rolecap .cap-desc .cap-name {
   padding-top: 0.75em;
 }
 
-#admin-roles-override .sefeoverridenotice {
-  text-align:center;
+#admin-roles-override .overridenotice {
+    margin: 1em 10% 2em;
+    text-align: left;
 }
 
 table.explainpermissions {