354627 - Improve the UI for adding/removing inheritance in editgroups.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Max Kanat-Alexander]

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

[Max Kanat-Alexander]

Attachment: 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

[Max Kanat-Alexander]

Attachment: v1

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:

http://landfill.bugzilla.org/bz354627/

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

[Frédéric Buclin]

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

[Marc Schumann [:Wurblzap]]

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

[Frédéric Buclin]

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

[Marc Schumann [:Wurblzap]]

(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

[Frédéric Buclin]

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

[Marc Schumann [:Wurblzap]]

(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

[Max Kanat-Alexander]

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

[Frédéric Buclin]

Comment on attachment 240446 [details] [diff] [review]
v1

highly bitrotten

Comment 11

[Max Kanat-Alexander]

Attachment: v2

Okay, fixed the bitrot.

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 245595 [details] [diff] [review]
v2

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

[Dave Miller [:justdave]]

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

[Dave Miller [:justdave]]

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

[Dave Miller [:justdave]]

You can tell I'm just throwing out random ideas, too, right? :)  More discussion is welcome.

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 245595 [details] [diff] [review]
v2

fixing the attribution... (one of these days I'll remember to log in with the right account)

Comment 17

[Frédéric Buclin]

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

[Max Kanat-Alexander]

Attachment: v3

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

[Frédéric Buclin]

Comment on attachment 257366 [details] [diff] [review]
v3

>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/Group.pm

>+# 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  
>+         [%+ group.name FILTER html %]&quot;)</th>
>+      <th>Groups That This Group Is a Member Of<br>
>+        (&quot;If you are in [% group.name 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

[Max Kanat-Alexander]

Attachment: v3.5

Okay, here it is with all of LpSolit's comments fixed. I tested the changes. Carrying forward r+.

Comment 21

[Max Kanat-Alexander]

Hooray! :-)

Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.83; previous revision: 1.82
done
Checking in Bugzilla/Group.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Group.pm,v  <--  Group.pm
new revision: 1.20; previous revision: 1.19
done
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
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/confirm-remove.html.tmpl,v
done
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
done
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
done
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
done
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
done

Comment 22

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.