84714 - Can't change permissions from bug list

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Stephen Rasku]

Currently, you can't change the group permissions under "Change several bugs at
once".  We would like to make all open bugs public when we do a release of
software since they are all in the release notes anyway.

Comment 1

[Dave Miller [:justdave]]

This just became a 2.14.  Bug 80289 was checked in this last week, and changes 
the way groups are dealt with on the standard bug_form.  Unfortunately it had the 
side-effect of clearing all group bits any time you do a mass change.  Mainly 
because the feature requested on this bug is not present, and thus the new code 
is assuming the absense of any group controls means to clear the groups (because 
un-set checkboxes aren't transmitted).  See bug 88797.

Comment 2

[Matthew Tuck [:CodeMachine]]

I think we need three options "leave", "turn on" and "turn off".  Leave would be
default.

It's ok to turn a bug on that is already on or off that is already off - it
should do nothing.  Therefore each groupset can safely always appear.

If the bug list is a homogenous set we can drop off the first option and set the
default to be the current option, but that's an optional improvement.

Comment 3

[Joe Robins]

Just to clarify, Matt, what you're thinking of would be something like the 
following:

Groupset: [Select box: options are "Do Not Change", "Turn on the selected 
groups", "Turn off the selected groups"]
  [Checkbox for Group A] Group A
  [Checkbox for Group B] Group B
  ...


Does that sound right?  Seems reasonable to me.  I assume that in this case, we 
leave all checkboxes unchecked by default.  Further, we only show a product 
checkbox if at least one bug in the list is in that product.

I think that dropping the first option in the case where all bugs have the same 
groupset is not a good idea.  Yes, it could provide better consistency with 
current behavior, but at the cost of consistency within buglist.cgi for 
different sets.  I think that if we label the select options clearly, it should 
be self-explanatory what's going on, and we can use the same behavior for all 
bug lists.

I might be able to do some work on this today, but maybe not 'til Monday.  (I'm 
taking a long weekend for the holiday.)  Let me know if this is a good approach.

Comment 4

[Stephen Rasku]

Joe's proposal sounds good to me.

Comment 5

[Joe Robins]

As I'm working on this, I've realized that there's a slight problem with this 
approach:  If we list out the groups this way, then we can't prevent people from 
trying to turn on group bits for inactive groups.

The way I would deal with this is to put the active/inactive check in 
process_bug.cgi, and if they try to turn on an inactive group, give an error 
there.  Maybe we could supplement this by italicizing inactive groups in the 
list, and stating that bugs may not be added to italicized groups?  However, I'm 
not sure if this is the best way to handle it.  Anyone have a better idea?

For now, I'll continue to work on it this way.  If someone makes a better 
suggestion, I should be able to change direction later.

Comment 6

[Joe Robins]

Attachment: Patch to allow editing of groupset on multiple bugs

Comment 7

[Joe Robins]

OK.  I've attached a patch that follows the process I outlined earlier.  I've 
tested this on my dev instance, but I would definitely want to see someone else 
test drive this for a bit before it gets checked in.  I'm pretty sure that I've 
got it right, but not sure enough that I wouldn't want a second opinion.  :-)

Comment 8

[Dave Miller [:justdave]]

This should probably go on landfill...  I might do it later but if someone else 
beats me to it, all the better. :)

My only comment after visual inspection is that in buglist.cgi and in 
process_bug.cgi, I believe there is a variable containing the value to be used 
for the "do not change" instances.  We should continue to use this variable just 
in case someone decides they want to change the wording of "do not change"...

Comment 9

[Matthew Tuck [:CodeMachine]]

No, that is not what I was suggesting.  I was suggesting having three options
for each group (groupset is probably the wrong terminology, sorry), which should
be easier to implement and more flexible.

Dropping the first option makes more sense in this scenario.

Comment 10

[Jacob Steenhagen]

Attachment 41053 [details] [diff] is now posted at http://landfill.tequilarista.org/bz84714/ for
testing.  I've added two groups (Test1 and Test2) with the regexp of .* (all new
accounts will be automatically added to those groups) for testing.  If you
already have an account on landfill's test install (this is using the bugs_tip
database) and your id needs to be added to the Test groups, please let Tara,
Dave, or myslef know.

Comment 11

[Dave Miller [:justdave]]

I agree with Matty, I'd rather see the Three-way switch for each group bit.

 Do Not Change
 |  Turn off
 |  |  Turn on
(*)( )( ) Group #1
(*)( )( ) Group #2
(*)( )( ) Group #3

Comment 12

[Dave Miller [:justdave]]

Attachment: Alternate patch using radio buttons on each group in multi-change form

Comment 13

[Dave Miller [:justdave]]

The attachment I just uploaded uses radio buttons on each group in the
multi-change form.  The process_bug code has not been tested very well, and
should be pounded on.  I added a third group "Test Group #3" which is inactive
to demonstrate what happens when an inactive group is present.  The group name
is italicized, and the "add" button is also not shown, in that case.

This version of the patch is now live on http://landfill.tequilarista.org/bz84714/

Comment 14

[Matthew Tuck [:CodeMachine]]

Radio buttons sounds OK.  It's certainly better than the current "People not in
group can this bug" syntax which is misleading in multiple ways.  However we
should remain consistent with the existing bug page.  By consistent, I don't
mean we have to use radio buttons on the bug page, but we should use checkboxes.

Is it possible to do this with checkboxes where certain checkboxes don't
appear?  There are two reasons for this.

Firstly, as I stated, that one option should drop off if the bug list is
homogenous on that group.  I believe it is better to drop off "No change" rather
than "Turn X", because the "Turn X" option allows the user to see the current
state.

Secondly, we have to take into account inactive groups and shouldn't let them be
turned on.

So a group would entirely disappear if it's inactive and no bugs have that
group.

So it we might look something like the following example, where U, V & W are
inactive groups and X, Y & Z are active groups.  U & X are all on, V & Y are all
off, and W & Z are mixed.


Bug List
--------

Bug Page

[X] Bug is restricted to only being seen by people in the "U" group which is
closed to new bugs
[X] Bug is restricted to only being seen by people in the "X" group
[ ] Bug is restricted to only being seen by people in the "Y" group
[ ] Bug is restricted to only being seen by people in the "Z" group

Change Page
-----------

Group  Leave as some  Bug isn't restricted  Bug is restricted
       on, some off   to only being seen    to only being seen 
                      by people in this     by people in this
                      group                 group
U                          [ ]                      [X]
W         [X]              [ ]
X                          [ ]                      [X]
Y                          [X]                      [ ]
Z         [X]              [ ]                      [ ]

(Maybe use strikethrough on U and W to indicate inactivity.)

There is one small issue with this though.  If we leave it as "on" when all were
previously on, and someone turns one off in the mean time, should we then turn
it on again?  And the same for "off"?

We could store a hidden form element that basically says "Turn On" here means no
change, so no change would be made, but I'm not sure that that's what we'd
want.  How does midair collisions work for bulk change?

Comment 15

[Matthew Tuck [:CodeMachine]]

Here's a two line header alternative:

Group  Leave as some  Bug restricted to only being seen by people in this group?
       on, some off   No     Yes

Comment 16

[Matthew Tuck [:CodeMachine]]

Since I'm guessing midairs won't collide, if that's the case it'd probably be
best to do something like this instead:

Group  Currently            No change  Bug restricted to ... ?
                                       No      Yes
-U-    All Restricted          [X]       [ ]
-W-    6 Restricted, 5 Not     [X]       [ ]
 X     All Restricted          [X]       [ ]
 Y     None Restricted         [X]               [ ]
 Z     6 Restricted, 5 Not     [X]       [ ]     [ ]

Comment 17

[Matthew Tuck [:CodeMachine]]

Umm, in the last comment, I meant to make the last three columns have all
checkboxes present, so you could disintinguish the two cases.

Comment 18

[Matthew Tuck [:CodeMachine]]

D'oh!  I meant the last three rows!  And I meant distinguishing the two cases
when a midair occurs.

Comment 19

[Stephen Rasku]

Mathew's been busy...

His comment at 23:17 looks reasonable.  The only change that I would make is 
instead of:

    6 Restricted, 5 Not

I would put:

    6/11 Restricted

Comment 20

[Tara Hernandez]

Uggg.  Trying to parse Matty's comments made my eyes cross.

:)

Matty -- will try and track you down on irc.  I'm not sure I follow you, and
Dave's current implementation looks good to me.

Comment 21

[Matthew Tuck [:CodeMachine]]

Dave's implementation is OK for 2.14, but I have a few comments, mainly
nitpicks:

(a) I think group name should be the first column.
(b) I think inactive groups should be strikethrough instead of italicised as
that would be more consistent with what we do elsewhere.
(c) It would be nice if the three checkbox columns were of the same size, if
that is possible.
(d) I would prefer my wording of 2001-07-07 23:17 (with the ellipsis filled in
appropriately from the previous comment), as it better conveys the essential AND
nature of groupsets.  We just fixed this on show_bug, it would be a shame if we
didn't do the same here.

The one bug I did find with the patch on Mattfill and Landfill is that invalid
logins cause software errors.

I still need to check that changes to existing bugs emit no notifications, the
groupset section disappears when there's no groups, the inactive message
disappears when there's no inactive groups, and inactive groups don't appear
when there's no bugs on the list with that have that group.

As for the bug count, some of the above and hiding one of the options when the
list is homogenous, I'll file that on 2.16 when these get checked in.

Comment 22

[Dave Miller [:justdave]]

r= zach a= me  checked in.

Comment 23

[Matthew Tuck [:CodeMachine]]

VERIFY NOTE:  I need to file my nitpicks as separate bugs before verification.

Comment 24

[Dave Miller [:justdave]]

Moving to Bugzilla product