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 33a641a29485e700b6a0923bfa1719c1cd5f84ce..330e822e58546b9c06cf897727441c6d9e103c82 100644 (file)
--- 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 d2ced57a6ee08f9bfbf1282955fd20a73794ca35..682d906bef5e6a2f333f68f2d989428dae7803f0 100755 (executable)
--- 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 c1588116c977fb8e5fba9ceddad4e2b9b7b1e68d..799d62afe06f852a7047950cbbf4cbee84bf8379 100644 (file)
--- 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 cb64fb68f76818e62f1d796e7ac0f842d94e9de9..00030661fda633bd6c34aaeb80154c3c3210d676 100644 (file)
--- 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 f0723e84e986875bf41e206f23e6db32e4d1e557..4b8cd810481a995869cf9fd186a93b69592f2e79 100644 (file)
--- 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 bf479050903ff2adb60ca40e389fc222a511b5fb..99a762568a4d4d340939cc3f9f79f140c530a41b 100644 (file)
--- 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 a001252b9de7b1d28e800c20f6dc121742cc0d23..846fd41808db652cc0b5e1468e7af646111b1e60 100644 (file)
--- 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 {