bug_form.pl should use checkboxes for groups instead of select list

RESOLVED FIXED in Bugzilla 2.14

Status

()

Bugzilla
Bugzilla-General
--
minor
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: Joe Robins, Assigned: Joe Robins)

Tracking

unspecified
Bugzilla 2.14

Details

Attachments

(7 attachments)

(Assignee)

Description

17 years ago
Having a long list of select boxes for group assignments of bugs is messy and 
annoying to work with.  Having a set of checkboxes would make it much easier to 
use.
(Assignee)

Comment 1

17 years ago
Fixing this bug requires changes to two places:
bug_form.pl needs to be changes to display the bugs differently.
process_bug.cgi needs to change how it checks the group bits from the form.

I've made both of these changes on my dev instance, and will attach a diff 
shortly.
(Assignee)

Comment 2

17 years ago
Created attachment 34085 [details] [diff] [review]
Use checkboxes instead of select lists for groupsets
(Assignee)

Comment 3

17 years ago
OK, I lied.  It also required a modification to buglist.cgi, which I realized 
when testing making changes to several bugs at once.

The attachment contains a diff for all three of these files.  Someone want to 
check it out and let me know if everything seems hunky-dory?
(Assignee)

Comment 4

17 years ago
Created attachment 34106 [details] [diff] [review]
Use checkboxes instead of select list for groups when entering a new bug
(Assignee)

Comment 5

17 years ago
It has been pointed out to me that I missed entering new bugs with the first 
patch I put up.  I've just added a second patch with the diffs for enter_bug.cgi 
and post_bug.cgi that should take care of that.
future note to whoever reviews these...  BOTH of the prior patches are required, 
the second one doesn't replace the first.
One concern I have with this...  needs to check the groups already set on the bug 
and make sure that someone who doesn't have access to a certain group can't clear 
it if they change the bug.  This could happen if the patches to allow a reporter 
to see their bugs no matter how it's restricted are put in.  If the user gets an 
"override" on seeing the bug because they're the reporter or the QA or the 
Assignee, etc, but aren't in that group, it won't put those groups on the bug for 
them to see.  If they make changes to the bug, that group then won't be on the 
list of groups that are set when the submitted changes come through.
That won't be an issue with post_bug, btw, since there obviously won't be any 
prior groups set in that case...  :)  only with process_bug.
(Assignee)

Comment 9

17 years ago
Hmm...  I see two potential ways to fix that.

1) Check if the person is the reporter in process_bug.cgi, and, if so, then 
check if the reporter has permissions for each group that the bug is in, and 
include the bits for any group that the reporter doesn't have access to.

I don't like this idea very much, since it's limited to just the issue of the 
reporter, and if a similar issue comes up, this solution won't hold.  
Furthermore, it's pretty kludgy.

2) In bug_form.pl and bug_list.cgi, when printing out the group list, remove the 
"$bit & $::usergroupset != 0" part from the SQL query, so that we get a list of 
all bug groups, not just those that the person has access to.  Then, for each 
bit, we check if the user is in the group.  If so, we display the checkbox; if 
not, we put the bit-$bit as a hidden field in the form.  That way, we'll still 
get all the values we need to properly construct the groupset in processbug.cgi.

What do you think?  That shouldn't be too difficult to do...

And if I do this, should I create one new patch with all the latest diffs in it, 
or just redo the diff for the ones I change now?
I don't like either of those...

1) leaves you open to problems any other exceptions besides reporter are created.
2) leaves you open to hackers if people edit their HTML source before committing.

How about this...

Iterate through all of the groups (not just the one the reporter has access to).

if (the user has access to the bit) {
  set according to presence of form field
} else {
  set according to the existence of the bit in the bug prior to changes,
  completely ignoring anything that might be set in the bug form
  (to prevent hacking the HTML)
}
oh, it'll be easier if you make a new patch with everything in it.
(Assignee)

Comment 12

17 years ago
Hmm...  Unfortunately, this is a little more complicated than one would want.  
If I make another database call while looping through values, I'll lose the rest 
of the results from the first database call, so I can't have MySql do a check on 
the groupset of the user, or of the bug, for each bit as we go.

On the other hand, Bugzilla is supported on versions of Perl which (so I'm told) 
don't support 64-bit arithmetic, so I can't just do the checks directly in Perl. 
 (Correct me if I'm wrong on this, and I'll be very happy and finish this thing 
up.)

One way to do this that I see:

Start by doing what we have now, looping through every bug group that the user 
has access to and building up the groupset.  Then do a second select to get the 
sum of bits in the groups table where bug.groupset & bit != 0 and user.groupset 
& bit = 0, and add that in to the value in the query.  (That is, get any group 
bits which were previously set, which the user didn't have access to, and add 
them in.)

What do you think?
You won't lose data if you do PushSQLState and PopSQLState...

but on the other hand, I think your alternative method will be less stressful on 
the database anyway, and it should work.
(Assignee)

Comment 14

17 years ago
Created attachment 34135 [details] [diff] [review]
Use checkboxes for groups on bugs; new patch obsoletes earlier two patches
(Assignee)

Comment 15

17 years ago
Man...  And I thought this was going to be an easy bug to fix.

I've created a new patch with the additional check.  It's a little ugly, because 
we don't yet have the bug id at that point, so I had to get it, but I had to 
check both $::FORM{'id'} and $::FORM{'id_*'} to figure out which we had, whether 
we were coming from showbug.cgi or buglist.cgi.  Fortunately, if we're coming 
from buglist.cgi, we only have to deal with groups if the groupset of all bugs 
is the same, so I could just take the first id I came up with and use that one.

I've tested this on my dev instance as much as I could.  However, I don't have 
the reporter patch, so I couldn't test a case where the user was not in the 
group of the bug, to make sure that it doesn't clobber the old groupset.  If 
someone here does have that patch, and wants to test this out, it'd be 
marvelous.

Hopefully, this will be the last change needed on this one...  I'll wait 'til 
someone else passes judgement, though.

(P.S. PushSQLState?  PopSQLState?  What are these wondrous things?  I can find 
no reference to or use of them anywhere in the Bugzilla code, or anywhere 
else...)
Hmm, I was going to say for a minute that we could just look for the form fields 
in post_bug.cgi since there were no prior groups anyway.  Then again, you don't 
want them manipulating the HTML to set groups they don't have access to.  But on 
the flip side, if you did just add the form fields together, you could prevent 
that by &&ing the result with $::usergroupset when you submit it to the 
database...  one less DB query...
Yah, I remembered the routine names wrong...

sub PushGlobalSQLState()
sub PopGlobalSQLState()
sub SavedSQLStates()

They're defined in globals.pl.
(Assignee)

Comment 18

17 years ago
Dave, do you think that the one extra DB call is significant enough that I 
should revise the change to post_bug.cgi for that?  Or is it OK the way it is?
why not...  the less calls to the DB the better.  It's in the roadmap. :)
(Assignee)

Comment 20

17 years ago
OK.  In that case, we don't need any changes to post_bug.cgi at all.  It can be 
left as-is, and will work fine with the checkboxes.

Should I post another attachment without the post_bug.cgi in the diffs, or can 
whoever patches this one just excise the post_bug.cgi change themselves?
As long as the existing code in post_bug_cgi ensures that you can't set bits you 
don't have access to, yeah, doesn't need a patch for that file.  I can remove 
that part I think.
(Assignee)

Comment 22

17 years ago
Created attachment 34955 [details] [diff] [review]
Final version of checkbox patch (I hope)
(Assignee)

Comment 23

17 years ago
You were right, the code in post_bug.cgi would allow someone to set bits they 
don't have access to.  I've fixed that, and posted a new patch with all the 
changes.  This should be it, I hope.
Keywords: patch, review
Target Milestone: --- → Bugzilla 2.14
installed this on bugzilla.syndicomm.com for a test run
OK, this looks really good. :-)

Only thing I might ask is that the "Only users in the selected groups can view 
this bug:" text should probably be in boldface, like the rest of the field titles 
on the page.
hmmm....  another thought on this...

It looks really silly having the product groups for all of the products you have 
access to listed there.

All non-product groups you have access to should show up, but only the product 
group associated with the current product should.

Here comes the tricky part...  in order to remain backward compatible...  if the 
user has access to a product bit, and it's set already on the bug, but it isn't 
the current product, it should show up.  In other words, if a bit that shouldn't 
be there is already on, and the user has access to it, show it.  But don't show 
it and don't let them set it if it's not already set.  Confuse you enough? :)
(Assignee)

Comment 27

17 years ago
Oy!

I agree with you that it would be nice not to list the product groups for other 
products there.  (In fact, one of my users was kvetching at me about that just 
the other day.  I applied the checkbox patch to our live instance, and he 
complained, because while a list of 15 checkboxes is marginally nicer than a 
list of 15 select lists, it's still not as nice as just one checkbox.)

The problem is that there is currently no way to distinguish between a product 
group and any other bug group.  We could add another flag to the groups table, 
isproductgroup, but I haven't made schema changes before to Bugzilla, so I'm not 
sure what additional things have to be dealt with so that it doesn't break 
anything.  (And something would also need to be done to checksetup.pl, right?)

Alternatively, we could just use a different value for product groups in the 
isbuggroup column.  Non-product bug groups could have a 1 there, and product 
groups could have a 2?  I don't think this would require major changes to most 
things, since the check is always "isbuggroup != 0", not "isbuggroup = 1".  This 
would require a change to editgroups.cgi (maybe?) to allow someone to toggle 
whether a group was a product group or not (or do we just want this set when the 
group is created, based on how it gets created, and not allow it to change?), 
and a way to change all the product groups in existing installations to reflect 
this change.  There'd also be a tweak to editproducts.cgi, so that the 
isbuggroup value gets set to 2 instead of 1.

Thoughts?

Also, you're also right about the backward compatibility.  That wouldn't be too 
difficult to manage, I think.  This could probably even all be handled by one 
complicated SQL query.  So another question: How much do we want to simply 
reduce the number of database calls, and how much do we want to avoid tangled 
SQL queries?  Would we rather have one really complex query or two or three 
simpler queries?

Comment 28

17 years ago
if (defined $::proddesc{$name}) looks like it will tell you if it's a product.  
Of course, you'll have to add name to the query on the groups table... 
Yeah, at present, product group names match the product names.  Should this be 
checked in as-is and we make a new bug for removing the product groups from other 
products?  Something else I forgot was if you change the product the bug is 
assigned to, the product group would probably need to change too.  This might be 
better on a separate bug.
This patch is going to conflict with the patch on bug 75482.  However, they 
probably need to have some code in common....  :-)
something else I just noticed on this...  in post_bug.cgi, the checkboxes are in 
alphabetical order by group description.  in bug_form.pl, they are in numerical 
order by group id.
(Assignee)

Comment 32

17 years ago
Jake: You're right that checking for $::proddesc{$name} is sufficient to check 
if there's a product that corresponds to a group.  I suppose that it is possible 
that people would name their products and groups in such a fashion as to fake 
this check out, but unlikely enough that we shouldn't worry about it.

Dave: I think we could include this change on this bug, simply because we're 
still fiddling with the appearance of the list of group restrictions.  However, 
you're right that this is going to collide with the patch on bug 75482.  How 
should we deal with that?  What does one do to generate a patch against a 
revision that isn't yet in the repository?

Also, you're right that they're being sorted differently in the two places.  
I'll fix that so that it sorts everything by description (which makes more sense 
for display purposes) rather than id.
removing patch keyword to get this off the checkin radar for now, since Joe said 
he was still adding stuff to it.
Keywords: patch, review
OK, Joe...  since you asked. :-)  The bug with the patch that was colliding with 
this one just got checked in.  Do a cvs update and see what happens.

Comment 35

17 years ago
yay, high traffic bug (thanks for the help joe).  so this is really a ui bug
(even though it affects a security feature).  i'm going to leave it for now, but
won't hold 2.14 for it if it comes down to the wire.  also, if there's a ton of
code changes after all, i'll definitely bump it.
Status: NEW → ASSIGNED
*** Bug 85384 has been marked as a duplicate of this bug. ***
Joe was working on this...
Assignee: tara → jmrobins
Status: ASSIGNED → NEW
(Assignee)

Comment 38

17 years ago
Sorry...  Work has been really busy for the last week or two.  I probably won't 
get to this 'til at least next week.  Is that OK?  What's the time frame for 
2.14?  I don't want to hold things up, but I don't want to miss out, either.
we were intending for the end of this week, but there's no way we're going to 
make it.  I'm sure if you get it in before the end of the month it'll make it in.
(Assignee)

Comment 40

17 years ago
Created attachment 39731 [details] [diff] [review]
Really the final patch!  Please?
(Assignee)

Comment 41

17 years ago
OK...  My train of thought derailed a bit over the last couple of weeks, but 
I've gone through this this afternoon, and I'm pretty sure this is how it should 
be.  Someone want to stick this on a test instance and make sure that everything 
is hunky-dory?

Of course, if Dave changes how bug groups and product groups work, as we were 
discussing in bug 68022, then some of this isn't going to be valid any more...  
Dave, I'll leave it at your discretion to figure out what to do with any 
collisions.

I hope this is it.
If it works we can check it in without waiting.  I haven't screwed with enter_bug 
or bug_form/process_bug yet, so it can't be any worse to modify :)
Keywords: patch, review
I have applied the latest patch (attachement 39731) on my bugzilla installation,
and I have two comments and a bug report:

1. On the bug entry form, I see some checkboxes now. Maybe something like
"(leave all blank for public bugs)" should be added to the header text?

2. I had a problem with the patch. For some reason the part about bug_form.pl
was skipped: 
$ patch < groupsui.patch
patching file buglist.cgi
patching file enter_bug.cgi
patching file post_bug.cgi
patching file process_bug.cgi
$
After applying the bug_form.pl part by hand, I get the checkboxes even for
show_bug.cgi, very nice indeed. :-)

3. This seems to be a bug: It doesn't seem to be possible any more to make a
restricted bug public again. When I uncheck all checkboxes and submit the
change, it seems like everything is fine, but when I view the bug next time, it
still has the old restrictions. Changing restrictions seems to work though.
Is this intended behaviour, a bug in the patch, or a problem on my side?
(Assignee)

Comment 44

17 years ago
Responding point by point to Andreas:

1) Did you not have the select boxes previously for the bug group assignments?  
This patch shouldn't have added anything you didn't already have, only changed 
the select boxes into checkboxes...

2) Hrm.  That may be my fault.  I manually edited the diff, because in addition 
to the checkbox change, I have another change on my dev instance (using a 
dropdown for assigned-to).  The stuff I edited out was all later in the file 
than the checkbox stuff, so I didn't think that would affect it.  Apparently, I 
was wrong.  When I fix your bug below, I'll temporarily edit out the assigned-to 
stuff to make the diff more cleanly.

3) Oops.  That's a bug in the patch.  I thought I had tested that case, but I 
managed to miss it somehow...  It should only take a couple of minutes to fix 
that.  I'll take care of it and test it, and then attach yet another patch here.

If you have a chance to answer my question on point (1) before I attach the new 
patch, that'd be good.  I'd really like this to be the last one, so if there's 
more work to be done with regard to point (1), I'll wait on the patch 'til I 
finish it.
> 1) Did you not have the select boxes previously for the bug group assignments?

Yes, I had select boxes there before. Maybe I should have said: I have
checkboxes there now, and that's fine. My comment was just the suggestion that
the header text could be more clear if there was a hint that no checkbox checked
means that the bug is public. (Before, this hint was there implicitly in the
option description of the selects: "People not in this group can see the bug."
so if you selected this option in all cases, you knew that the bug was public.)

Thanks for your work on this :-)
(Assignee)

Comment 46

17 years ago
Created attachment 40078 [details] [diff] [review]
Yet another checkbox patch
(Assignee)

Comment 47

17 years ago
OK.  I've fixed the bug there was with clearing all permissions on a bug, and 
I've added a bit of clarifying text on enter_bug.cgi.  Wanna take another look 
at it now, and make sure that everything is hunky-dory?
Applies cleanly now, and seems to be functional. I have only verified part of
the functionality:
- if you are not logged in, then you can view a public bug
- if you are not logged in, then you cannot view a restricted bug
- if you are logged in with good permissions, then you can view a restricted bug
- to test this, I changed a restricted bug to "public" and back

So I'm fine with this patch as-is, and I'll keep it in my installation. If you
want to get funky, you can add some similar clarification text to the other
places where the the checkboxes are shown, not only on enter_bug.cgi, but I'm
not sure if that's necessary.
OK, latest version has been installed on syndicomm.  Initial impressions are I 
love it. :-)  A few quick tests indicate that removing groups works good, and I 
like that only the product group for that product shows up. :-)

r= justdave

checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Just FYI, tinderbox immediately flagged warnings on this after I checked it in.  
%::proddesc in post_bug.cgi and $::usergroupset in bug_form.pl, "used only once".  
I added these to the sillyness sub at the top, and the warnings went away.

Comment 51

17 years ago
This is nice, I really like it... with one exception...  If all your groups are
product groups, but you have a product with a group, you get the "Only users in
the selected groups can view this bug:" message w/out any checkboxes below it. 
This happens in enter_bug.cgi and show_bug.cgi.

Also, it might be nice if beneath the aforementioned text it said that if no
boxes are selected, a bug is public (like it does on enter_bug.cgi).

Example of what I have:
I have 4 products (Bugzilla, MIS, SPIT, Testing) but only two groups (MIS,
SPIT)... if I'm viewing (or entering) a bug in Bugzilla or Testing I get the
group header w/out any checkboxes.  If I log in as a user not in either of these
groups, I don't get this header.
Status: RESOLVED → REOPENED
Keywords: patch, review
Resolution: FIXED → ---
(Assignee)

Comment 52

17 years ago
Oh, sure...  My life was starting to get boring without this bug to think about 
any more.  :-)

This should be easy to do...  I hadn't thought of the case where only some 
products would have groups.  I'll also add in the text from enter_bug.cgi.  I 
should be able to have this done some time in the next day or so.
(Assignee)

Comment 53

17 years ago
Created attachment 40466 [details] [diff] [review]
Fix for header-but-no-groups problem.
(Assignee)

Comment 54

17 years ago
OK.  This patch clears up the problem of showing the header but no groups, and 
adds the no-checkboxes-means-public-bug text to bug_form.pl.

This patch is against the current version in CVS, which already includes the 
earlier patch, and thus contains only the changes I've made on this today, not 
the whole shebang.

Anyone else?  :-)

Comment 55

17 years ago
Nope, that's the only thing I noticed...

r=jake on bugfix... checked in.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
Moving to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: Bugzilla 2.12 → unspecified
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.