Last Comment Bug 354627 - Improve the UI for adding/removing inheritance in editgroups.cgi
: Improve the UI for adding/removing inheritance in editgroups.cgi
Product: Bugzilla
Classification: Server Software
Component: Administration (show other bugs)
: 2.23
: All All
-- enhancement (vote)
: Bugzilla 3.2
Assigned To: Max Kanat-Alexander
: default-qa
Depends on:
Blocks: 250877 373865 505796
  Show dependency treegraph
Reported: 2006-09-27 21:22 PDT by Max Kanat-Alexander
Modified: 2009-07-22 10:27 PDT (History)
3 users (show)
LpSolit: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

Screenshot of New UI (45.78 KB, image/png)
2006-09-28 01:02 PDT, Max Kanat-Alexander
no flags Details
v1 (54.13 KB, patch)
2006-09-28 03:40 PDT, Max Kanat-Alexander
LpSolit: review-
Details | Diff | Splinter Review
v2 (48.55 KB, patch)
2006-11-14 11:58 PST, Max Kanat-Alexander
justdave: review-
Details | Diff | Splinter Review
v3 (48.14 KB, patch)
2007-03-05 07:49 PST, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Splinter Review
v3.5 (52.32 KB, patch)
2007-03-05 20:48 PST, Max Kanat-Alexander
mkanat: review+
Details | Diff | Splinter Review

Description User image Max Kanat-Alexander 2006-09-27 21:22:09 PDT
So, instead of the series of checkboxes that we have now when you go to edit a group in editgroups.cgi, I was thinking that it would be much clearer to have a pair of <select> boxes, where you can see "groups that are a member of this group" and "groups this group is a member of".

Actually, it would be four select boxes, because you'd have to have one box that contained the groups that hadn't been added yet (kind of like the inclusion/exclusion stuff in editflagtypes.cgi).
Comment 1 User image Max Kanat-Alexander 2006-09-28 01:02:30 PDT
Created attachment 240434 [details]
Screenshot of New UI

Here's a screenshot of the UI that I have right now. What you see here is working, it's just not complete yet (I have to add Bless and Visibility, but that should be pretty easy).
Comment 2 User image Max Kanat-Alexander 2006-09-28 03:40:59 PDT
Created attachment 240446 [details] [diff] [review]

Okay, here's the patch. It's pretty big, and it has a lot of UI changes, so the best thing to do is to actually look at it live:

That's an installation where the patch is applied, and everybody has "creategroups" so you can see the new UI.

I have tested it out, and it seems to work for all my tests.
Comment 3 User image Frédéric Buclin 2006-09-28 08:04:12 PDT
Too invasive to go into 2.23.3. I'm doing QA tests these days, so do not expect a review from me for the next two weeks (I will only review bug fixes).
Comment 4 User image Marc Schumann [:Wurblzap] 2006-09-28 08:08:46 PDT
This patch is one of the few which give a tangible benefit to Bugzilla admins. We should do what we can to get it into 3.0, and getting it into 2.23.2 would be helpful to this end.
Comment 5 User image Frédéric Buclin 2006-09-28 08:14:42 PDT
I'm not saying I don't want it for 3.0. I'm just saying that we already started QA tests and that we don't want to run them all again everything an enhancement bug is checked in. Btw, enhancement bugs are not allowed for 2.23.3 anymore.

Marc, feel free to review this patch. I'm not sure how much free time I will have next month. Else this patch will miss the boat for 3.0.
Comment 6 User image Marc Schumann [:Wurblzap] 2006-09-28 08:20:26 PDT
(In reply to comment #5)
> have next month. Else this patch will miss the boat for 3.0.

<rant>If this patch is not in 3.0, then it's not the patch missing 3.0. It's 3.0 missing the patch. And every real benefit 3.0 misses makes it less worthy to be called 3.0.</rant>
Comment 7 User image Frédéric Buclin 2006-09-28 08:28:42 PDT
The trunk has been open for 9 months, and the freezing date is known for several weeks now. mkanat himself suggested to freeze two weeks after we release 2.23.3. If this enhancement was so much a "must have" feature, then why has it been opened only now? Are we going to delay 3.0 everytime someone has a great new idea?

I'm going to do by best to review this patch on time, but as you, I also have a real work and a real life. Sorry, but I won't cry if this enhancement won't go into 3.0. As I said, feel free to review it.
Comment 8 User image Marc Schumann [:Wurblzap] 2006-09-28 08:38:52 PDT
(In reply to comment #7)
> release 2.23.3. If this enhancement was so much a "must have" feature, then why
> has it been opened only now? Are we going to delay 3.0 everytime someone has a
> great new idea?

This patch is not a must have feature, but the reverse (or rather two-way) maintenance of group-group-memberships is indeed one of the few enhancements which give an immediate and day-to-day benefit.

Delaying 3.0 so that it fits development is exactly what I'm saying, yes. I'm aware that this has been shot down several times already, so I marked by previous comment a rant. (And I'm not trying to push you to review it -- I'm sorry if I sounded like that, it's not what I meant.)
Comment 9 User image Max Kanat-Alexander 2006-09-28 11:01:12 PDT
I don't think it should be in 2.23.3, because it is a major change.

It do think it should be in 3.0.

Since it already has a patch, we may let it go in after the freeze date, but I wouldn't rely on that.
Comment 10 User image Frédéric Buclin 2006-10-30 16:22:32 PST
Comment on attachment 240446 [details] [diff] [review]

highly bitrotten
Comment 11 User image Max Kanat-Alexander 2006-11-14 11:58:03 PST
Created attachment 245595 [details] [diff] [review]

Okay, fixed the bitrot.
Comment 12 User image Dave Miller [:justdave] ( 2006-11-14 16:29:52 PST
Comment on attachment 245595 [details] [diff] [review]

Hmm, this is a vast improvement over all the checkboxes, however, the UI isn't immediately intuitive... we might need some instructions on the page to explain it.

This page a good candidate for some ajax love to make it more intuitive.
For example: have the two select boxes...  left one might be labelled "available groups" right one labelled "current groups", and Add and Remove buttons (which may only have arrows on the buttons) between the two select boxes that's a little more ituitive, but a bit on the hard-to-use side if you don't have javascript.  What's there now would be a good fallback if there's no javascript, but it needs instructions :)  (and we can always add the ajax stuff later if we get instructions for this)

The six different selectbox groups are a bit overwhelming, too.  Maybe we need the three grant types more clearly distinguished (which a group heading over each of the three), then the direction of the grant broken down within each of those. (grant types in this case == inheritance, grant, and visibility)
Comment 13 User image Dave Miller [:justdave] ( 2006-11-14 16:55:38 PST
ok, idea for the current UI...

Right now the labels for the individual select boxes are "Add" and "Current (Remove)"

I think it would be more intuitive if we labeled them like this:

    Available               Current
 (select to add)       (select to remove)
Comment 14 User image Dave Miller [:justdave] ( 2006-11-14 16:57:32 PST
It would probably seem less overwhelming if we had a single select box for each of the six categories, also, which only had the "current" list in it.  Select a group from that list to remove it, and hit a button next to it to add...  the Add button would pop up a new select box with the list of available groups.
Comment 15 User image Dave Miller [:justdave] ( 2006-11-14 16:58:30 PST
You can tell I'm just throwing out random ideas, too, right? :)  More discussion is welcome.
Comment 16 User image Dave Miller [:justdave] ( 2006-11-14 17:05:17 PST
Comment on attachment 245595 [details] [diff] [review]

fixing the attribution... (one of these days I'll remember to log in with the right account)
Comment 17 User image Frédéric Buclin 2006-11-15 09:34:01 PST
Retargetting this bug to 3.2, per our discussion on IRC:

<LpSolit> justdave: about this UI rewrite for editgroups.cgi, should we postpone this patch to 3.2 now that you r- it?
<LpSolit> justdave: else it will probably require days before an updated patch is submitted and reviewed
<LpSolit> and we will be far after the freezing date
<justdave> ah, yes, that probably ought to wait now I guess.
<justdave> it'd be a shame to leave it out, but we did kinda set those rules in advance. :)
<justdave> and based on the comments I left on it, it probably needs more work yet than would be appropriate for a rush job
Comment 18 User image Max Kanat-Alexander 2007-03-05 07:49:27 PST
Created attachment 257366 [details] [diff] [review]

Okay, I updated the patch to make the headers clearer.

I think any further UI improvements we should actually just do in a separate bug. This bug is both a huge code cleanup and a major UI improvement already.
Comment 19 User image Frédéric Buclin 2007-03-05 15:29:45 PST
Comment on attachment 257366 [details] [diff] [review]

>Index: editgroups.cgi

>+sub _do_add {

>+    foreach my $add (@$add_items) {
>+        if (!grep($_->id == $add->id, @$current)) {

Nit: next if grep($_->id == $add->id, @$current) would avoid this whole IF block and the additional indentation.

>Index: Bugzilla/

>+# Parameters that are lists of groups.
>+use constant GROUP_PARAMS => qw(chartgroup insidergroup timetrackinggroup);

Add querysharegroup to the list.

>Index: template/en/default/admin/groups/edit.html.tmpl

>+  <table class="grant_table">
>+    <tr>
>+      <th class="one">Groups That Are a Member of This Group<br>
>+        (&quot;Users in <var>X</var> are automatically in  
>+         [%+ FILTER html %]&quot;)</th>
>+      <th>Groups That This Group Is a Member Of<br>
>+        (&quot;If you are in [% FILTER html %], you are 
>+         automatically also in...&quot;)</th>
>+    <tr>

The last <tr> should be </tr>.

>+<form method="post" action="editgroups.cgi">

>+    <legend>Remove all explict memberships from users whose login names
>+      match the following regular expression:</legend>

>+  <input type="hidden" name="token" value="[% token FILTER html %]">

From what I can see, you don't validate the token from this form.

>+[% BLOCK select_pair %]

>+      <th><label for="[% "${name}_remove" FILTER html %]">Current<br>
>+        (select to remove)</label></th>

Nit: </th> should be on a newline, for consistency with other cells.

>+        <select multiple="multiple" size="[% size FILTER html %]"
>+                name="[% "${name}_remove" FILTER html %]"
>+                id="[% "${name}_add" FILTER html %]">

Err... id must contain "_remove", not "_add".

Moreover, do not forget to remove admin/groups/change.html.tmpl which is no longer in use.

Your patch seems to work fine. r=LpSolit with my main comments (non-nits) above fixed.
Comment 20 User image Max Kanat-Alexander 2007-03-05 20:48:54 PST
Created attachment 257471 [details] [diff] [review]

Okay, here it is with all of LpSolit's comments fixed. I tested the changes. Carrying forward r+.
Comment 21 User image Max Kanat-Alexander 2007-03-05 20:54:33 PST
Hooray! :-)

Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.83; previous revision: 1.82
Checking in Bugzilla/;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/,v  <--
new revision: 1.20; previous revision: 1.19
Removing template/en/default/admin/groups/change.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/change.html.tmpl,v  <--  change.html.tmpl
new revision: delete; previous revision: 1.2
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/confirm-remove.html.tmpl,v
Checking in template/en/default/admin/groups/confirm-remove.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/confirm-remove.html.tmpl,v  <--  confirm-remove.html.tmpl
initial revision: 1.1
Checking in template/en/default/admin/groups/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.12; previous revision: 1.11
Checking in template/en/default/admin/groups/remove.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/remove.html.tmpl,v  <--  remove.html.tmpl
new revision: 1.3; previous revision: 1.2
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.51; previous revision: 1.50
Comment 22 User image Max Kanat-Alexander 2008-07-01 00:07:37 PDT
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.

Note You need to log in before you can comment on or make changes to this bug.