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

RESOLVED FIXED in Bugzilla 3.2



13 years ago
10 years ago


(Reporter: mkanat, Assigned: mkanat)


Bugzilla 3.2
Dependency tree / graph
Bug Flags:
approval +



(2 attachments, 3 obsolete attachments)



13 years ago
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

13 years ago
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).
Assignee: administration → mkanat

Comment 2

13 years ago
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.
Attachment #240446 - Flags: review?(LpSolit)

Comment 3

13 years ago
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).
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

13 years ago
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.
(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

13 years ago
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.
(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

13 years ago
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

13 years ago
Comment on attachment 240446 [details] [diff] [review]

highly bitrotten
Attachment #240446 - Flags: review?(LpSolit) → review-

Comment 11

12 years ago
Created attachment 245595 [details] [diff] [review]

Okay, fixed the bitrot.
Attachment #240446 - Attachment is obsolete: true
Attachment #245595 - Flags: review?(LpSolit)


12 years ago
Attachment #245595 - Flags: review?(LpSolit) → review?(justdave)
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)
Attachment #245595 - Flags: review?(justdave) → review-
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)
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.
You can tell I'm just throwing out random ideas, too, right? :)  More discussion is welcome.
Comment on attachment 245595 [details] [diff] [review]

fixing the attribution... (one of these days I'll remember to log in with the right account)
Attachment #245595 - Flags: review- → review?(justdave)
Attachment #245595 - Flags: review?(justdave) → review-

Comment 17

12 years ago
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
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2

Comment 18

12 years ago
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.
Attachment #245595 - Attachment is obsolete: true
Attachment #257366 - Flags: review?(LpSolit)

Comment 19

12 years ago
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.
Attachment #257366 - Flags: review?(LpSolit) → review+


12 years ago
Flags: approval+

Comment 20

12 years ago
Created attachment 257471 [details] [diff] [review]

Okay, here it is with all of LpSolit's comments fixed. I tested the changes. Carrying forward r+.
Attachment #257366 - Attachment is obsolete: true
Attachment #257471 - Flags: review+

Comment 21

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED


12 years ago
Blocks: 250877


12 years ago
Keywords: relnote

Comment 22

11 years ago
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.
Keywords: relnote


10 years ago
Blocks: 505796
You need to log in before you can comment on or make changes to this bug.