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.

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.

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?

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.

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.

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.

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.

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. :)

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.

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.

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? :)

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?

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.

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.

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.

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.

*** Bug 85384 has been marked as a duplicate of this bug. ***

Joe was working on this...

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.

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

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?

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

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.

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.

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.

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.

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? :-)

Nope, that's the only thing I noticed... r=jake on bugfix... checked in.

Moving to Bugzilla product