Can't find any docs anywhere for how to set up groups. There's a comment in the CHANGES file from when the groups capability was added, that says the documentation is in the comments in the makegroupstable.sh file, however, that file is no longer in the distribution, and the documentation did not get moved to the relavent section of checksetup.pl, or into the README, either.

ok, you're right, there's not much in there. It does explain a couple of the fields in the table, but it doesn't explain how to use them in bugzilla, which is what I'm trying to figure out. As far as I can tell, the current implementation permits you to move individual users in and out of groups, and also to move individual bugs in and out of groups. The fact that there is a flag for "user has the ability to create and destroy groups" implies that there is somewhere to do this, but I can't find it. The only way I can find to create and destroy groups is to log into mySQL and do it yourself. What I'm really looking for is a way to automatically set a group bit on a bug based on what product/version combo the bug is reported under, but if that doesn't already exist, that's worthy of a separate bug report for a feature request, rather than discussing it here.

OK, from more investigating on this stuff, it looks like there's something missing in the current Bugzilla distribution. As stated above, there is a group bit available for "Has the ability to create and destroy groups". There is nowhere in Bugzilla to excercise this privilege. Did there used to be an editgroups.cgi that somehow disappeared out of the tree? Or was this an intended feature that never got implemented?

It never got implemented.

ok, in that case, maybe I'll implement it. Sounds like an easy enough thing to do, just didn't want to duplicate any effort.

Adding Terry to the CC so he sees this. I just attached my first draft of editgroups.cgi to this bug. Yes, it's been thoroughly tested this time. :) Everything works except for deleting a group. I figured people would be mostly interested in adding and editing groups, so I did that first. I'll keep working and post a new one as soon as I get the delete stuff in. (If you try to delete one in this version, it'll tell you it's not implimented yet, and explain the way I intend to have it work).

This looks very cool, and I'd go ahead and check it in, except for one thing. jmrobins@tgix.com (who is now CC'd on this bug, and for whom i just created a bugzilla.mozilla.org account) just submitted a big patch that optionally allows groups to automatically be created for each product. I just checked it in. I'm afraid that if his stuff is turned on, then this new editgroups.cgi script might break some of his things. I'd like one or both of you to check it out and make sure that your code will interact well with each other.

A few comments, from a first glance at the code for editgroups.cgi: 1) The TestGroup method does the same thing as the GroupExists method I implemented in globals.pl. You may want to modify this script to just use that instead of duplicating the method. 2) When you add a group, you're not matching existing users against the userregexp, so people who probably should get put in the group won't be. Same for when you edit a group, and change the regexp. To see how I'm handling it for product bug groups, take a look at editproducts.cgi, where I'm creating and editing the groups for products. (You may or may not want to handle it this way, but I thought it worked nicely...) 3) If you rename a bug group associated with a product, it will break my patch. There is a simple fix for this, though: When a user tries to rename a group, check and see if there is a product by the same name as the group being modified. If so, disallow the change, with a message to the effect of "Cannot change the name of a bug group associated with a product." Changing the description is OK, though. 4) One note for the upcoming delete function: When you're deleting group permissions from the users, be careful not to delete permissions from any superusers, since they should always have membership in all groups. I handled this with a bit of a kludge: Merely add a check for whether the user's groupset is the superuser groupset: SendSQL("UPDATE profiles " . "SET groupset = groupset - $bit " . "WHERE (groupset & $bit) " . "AND (groupset != 9223372036854710271)"); Otherwise, it looks pretty nice. Let me know your thoughts on these comments.

OK, yeah, I don't see much that would break anything other than what Joe already mentioned. I do have comments about Joe's patch though: 1) If you turn this on, it does not create groups for any existing products. Each of those products now becomes closed to new bugs because the group-check against that user's login fails, not because they aren't in the group, but because the group doesn't exist. 2) In the Enter A New Bug page, the popup menu to choose the group state for that product appears twice (once defaulting to "only those..." and the other defaulting to "those outside of...") along with one popup for each of the other groups the user has access to. This might be from the patch I submitted earlier this week, and maybe it didn't get taken into account when Joe did the same thing, so now they duplicate each other. 3) If you create a product with spaces in the name, it creates a group name with spaces in it. This breaks bug_email.pl, because it uses a whitespace-separated list of group names for the groupset to set the bug to. There may be other things that break because of a space in that name, but I couldn't tell you for sure.

I got to thinking about this, and maybe a better way to handle the name of the group for a product group, is instead of naming the group to match the product, let the user name the group (this will require adding a column to the products table). This could have a number of benefits: 1) If the group already exists, it's used. 2) If the group doesn't exist, it's created. 3) More than one product could conceivably belong to the same group. 4) If the group number is stored in the table instead of the group name, then we don't have to worry about the user later renaming the group, although it will cause one more thing to check for when deleting a group. A thought on this... how about storing the group number in the versions table instead of the product table? Then you could put automatic restrictions on visibility to unreleased private beta versions of a product that already has a public release. I actually already submitted this idea in Bug 30189, which comes awfully close to this patch you just submitted, but with one more level of precision. (In fact, when I discovered your patch, I almost went and marked that bug FIXED, until I realized it couldn't split between versions.) I'll be happy to work with you on implementing it this way if you're game.

Oh, one thing I forgot: Some products I like using this with, some I'd rather have open to the public and unrestricted. The present system doesn't allow this. it's all or nothing. (thus, my note above about existing products becoming closed to new bugs by means of access restriction to everyone when you turn it on). So, if the user leaves the group name blank when creating a product (or version, if we decide to go that route), then no group check would be performed when deciding the visibility of that product.

Another thought on this: if we stored the group number with the product or version, the usebuggroups and usebuggroupsentry prefs would no longer be needed, as this would essentially be set on a product by product basis. i.e. if you didn't want it for that product, just leave the group name blank when you set it up.

The diffs I just attached fix the duplicate select-box problem caused by the conflict between my patch and Joe's. My patch from last week was already generating a list of select-boxes for each group the user had access to. Joe's patch was creating yet another one if the product had a group defined. I moved Joe's group detection into my list of select-boxes so that if the current select- box matched the group for the current product, it automatically got set to "only people in...". Consequently, his patch to add the 'groupset' parameter to post_bug.cgi got removed, since the 'bit-$bit' parameters were already being checked for. I also fixed the problem here where products without a defined group were being denied bug entry. (it was missing the '&& GroupExists($p)' on the initial access check).

A whole bunch of responses to the various thoughts: > If you create a product with spaces in the name, it creates a group name > with spaces in it. This breaks bug_email.pl, because it uses a > whitespace-separated list of group names for the groupset to set the > bug to. There may be other things that break because of a space in > that name, but I couldn't tell you for sure. I've taken a look at bug_email.pl, and you're right that this would cause a problem. However, (1) bug_email.pl has several other issues already, it would appear, (for example, the default group for bugs doesn't exist at all), and (2) this is easily remedied with only a minor functional change so that it requires a comma or + delimiter, and no longer accepts spaces. (In fact, I think that it should only use commas, but that's another issue entirely.) I can patch this up if you think that might be a good idea. I don't think that the spaces matter anywhere else. I did a whole bunch of testing on this, including groups with spaces, and didn't notice it anywhere. I didn't test the stuff in contrib, though, which is why I missed this... > I got to thinking about this, and maybe a better way to handle the name > of the group for a product group, is instead of naming the group to > match the product, let the user name the group (this will require adding > a column to the products table). I actually dislike the idea of having more than one product in the same product group. It's quite possible to create groups that are not associated with products, and put bugs from different products into those groups if you so desire. However, the idea behind this patch was that each product is, in its fashion, a separate entity, and should have separate access possible. If you have pieces that are very closely grouped together, maybe they're different components of one product? Similarly, WRT > how about storing the group number in the versions table instead of > the product table? I don't like this as much. Again, you could easily create an additional access group for a version you wanted to keep private. I think that the product level is a more reasonable level for the default permission separation than the version level. > Oh, one thing I forgot: Some products I like using this with, some I'd > rather have open to the public and unrestricted. The present system > doesn't allow this. it's all or nothing. (etc.) Ooh... You're right. I think that this could be easily remedied by the addition of a simple flag in editproducts.cgi, for when you add or edit a product, for whether or not to create a bug group. I think it should be possible to have a bug group without a userregexp, so just leaving it blank doesn't cover it, but a simple yes/no toggle would handle that. The code already can handle products without groups, so being able to continue on with products without groups should work fine. I'll add this in and post the diffs here when I get a chance. (I'm pretty busy at the moment, so it might be a couple of days.) (Although a possible workaround would be to set the userregexp to something that would match all users, like "." or "@" or something... That way, everyone would have access to that group.) > I also fixed the problem here where products without a defined group > were being denied bug entry. (it was missing the '&& GroupExists($p)' > on the initial access check). Yeah, when I saw your first comment, I looked back at it and realized that that was somehow missing... Not sure how I managed to skip that one, but the simple additional check you put in fixes it up just fine.

by the way, Terry, the most-recent fix I posted here needs to be checked in NOW, whether we do other stuff here or not. My previous patch to do the group select at bug submission time and Joe's patch for the product groups that have both already been checked in break each other, and the most-recent attachment on this bug patches up the immediate breakage. Joe and I are still hashing out the editgroups thing so don't worry about that yet. I have other comments for Joe, but I'm a bit busy here at the moment, and will likely get that written up and posted here in an hour or two...

A whole bunch of responses to the various thoughts (back at ya ;) : > I've taken a look at bug_email.pl, and you're right that this would > cause a problem. (In fact, I think that it should only use commas, > but that's another issue entirely.) I can patch this up if you think > that might be a good idea. I agree here. Better to fix bug_email to use something more logical. :) > I actually dislike the idea of having more than one product in the > same product group. You do, I don't. The way I suggested would make it work either way. It would be a preference of the site maintainer. > It's quite possible to create groups that are not associated with > products, and put bugs from different products into those groups if > you so desire. But it doesn't happen automatically without support from the system we're talking about. The users of these products would then have to explicitly set the group every time they report a bug. > However, the idea behind this patch was that each product is, in its > fashion, a separate entity, and should have separate access possible. > If you have pieces that are very closely grouped together, maybe > they're different components of one product? No, these are obviously separate products. I just have the same people doing internal testing on them, and I don't want group overload with a billion select-boxes at the bottom of the enter-bug page. Perhaps what we really need is a third classification for "isbuggroup". Right now it's just 0 or 1. What if we added a 2 to that possibility and made it a product group if it's a 2? Any time you are viewing or entering a bug, the only #2 select box that would show up is the one related to that product, but any #1 select boxes they have access to would show up. (I still favor letting the maintainer pick which group that product gets set to, personally). > I don't like this as much. Again, you could easily create an > additional access group for a version you wanted to keep private. I > think that the product level is a more reasonable level for the > default permission separation than the version level. And once again, the user has to explicitly set the visibility every time they report a bug, instead of it being set for them, and you don't have the ability to hide a version number from being visible to the public. > I think that this could be easily remedied [the all or nothing > problem] by the addition of a simple flag in editproducts.cgi, for > when you add or edit a product, for whether or not to create a bug > group. Agreed. Sounds like a good way to do it. However, if we made it so the maintainer could choose the name of the group being associated with it, then just leaving the group name blank would do it. > I think it should be possible to have a bug group without a > userregexp, It is. I haven't used any regexps on any groups yet. Not everyone in any given domain automatically should have access to it in my situation, so I've just been setting them individually when they tell me their account has been created. All my regexps are just blank.

Seconds on the comment that the patch fix needs checked in. I applied the stuff in the tree through today, and you can't post a bug with groups turned on, you get an error in globals.pl @ line 134. The diffs from dave got line-wrapped and patch chokes on the file, btw.

Please check in the duplicate select-box conflict patch fix.

OK, just a status update for the people who are paying attention to this bug (by the way, Terry, the patch to fix the immediate stuff that broke before STILL hasn't been checked in!) the major project I was working on is out the door now, so I'll be having time available to play with this again. Sometime in the next week or so I'll be revising editgroups.cgi to follow Joe's protocol for group definitions, and get the delete stuff added to it.

If you set usebuggroups, play around, and then unset usebuggroups, you'll still get prompted for the bug's scope. With the bug_form.pl diff I attached, you'll no longer get prompted. The spirit of that diff should probably also be followed in enter_bug.cgi.

Ok, per request from Tara Hernandez I've reviewed and tested and checked in the patches from 03/09/00 03/11/00 and 04/12/00 and a change to CGI.pl to make it easy for folks to reach the editgroups.cgi.

Cool, thanks. :) editgroups.cgi is still a work in progress. There's still stuff I need to do to it to get it to be a little more cooperative with the Product Bug Groups stuff, but as long as you're careful, it doesn't actually break anything. I never did get time to work on it again last week. Life is rough. I'll keep trying.

Was the pref "usebuggroups" added by this feature? After Don checked in the patch I had to back it out because it broke bugzilla.mozilla.org by not displaying the "People not in the X group can see this bug" element any more in the bug display. During a post mortem I discovered two different buggroup items in our prefs file, both are turned off and things work just fine. According to editparams.cgi, the editbuggroups pref associates a bug group with each product in the database and uses it for querying bugs. By this definition, it seems correct for us to have it off. I'm trying to figure out if we need to turn on this pref or if the code needs changing or what. All we want to do with groups is to restrict certain bugs so only certain people can view them, but this isn't related at all to products.

Yep, he's right. The 4/12/00 patch on this bug should not have been applied, and needs to be backed out. It breaks what he says here. Those popups are supposed to be there for people that have more than just the product bug groups. FYI, those popups only show up if you have access to that group. Those who have access to nothing won't see them.

Ok, finally the proper stuff is checked in.

Should the link to editgroups.cgi appear in the footer menu if usebuggroups is turned off? If so, should editgroups.cgi check to see if usebuggroups is off and if so display a message instead of allowing groups to be created?

That's a thought. I like that idea. I think it's worthy of a new bug report though because it can be extended to other circumstances as well (for example, I have keywords disabled, and edit keywords still shows up in the footer for me).

OK, believe it or not, I *finally* got a free day to work on this again. :) The following two attachments are the diffs for editusers.cgi and editgroups.cgi to get them up-to-date. My only changes to editusers.cgi is to add a query parameter for the list operation, to which allows you to provide the contents of the WHERE clause in the query that generates the list of users. This way I can call it from editgroups.cgi and have it generate a list of users that are members of that group. The above patch for the == string comparison is included in this (I already applied it on mine), so that one will not need to be checked in, it's included in this one. editgroups.cgi now allows you to delete groups. It double-checks the user list and the bug list for any users or bugs that have that group bit set, and gives you override checkboxes to have it nuke them all for you. It also will generate a user list or a bug list showing the items that are still in that group. It will also warn you if you try to delete a group that belongs to a product (usebuggroups methodology), but also has an override checkbox for that (warns you that the product will become publicly visible). It does grab a list of people with maintainer privs before it does a global "remove this bit from all users", and restores the opblessgroupset value to those people afterwards. This completes the feature list for the scope of this bug. As soon as these patches are checked in, this bug can be marked FIXED (I don't have checkin privs, so someone else will have to do that for me). Any additional features needed for this file can go in new bug reports.

My part is done here (for the time being). Reassigning this to Tara so she can make sure the attached patches get checked in, or delegate it. (I don't have checkin privs).

Chris you wanna take a look at this and get it in?

Oof, there are a lot patches here. Should I start applying from 6/3 or from 3/11? Where should I start?

The two that were added on 6/17 are the only ones that need to be applied (this has been an ongoing project for a while). Everything above that has either already been applied or is included within the last two.

great. i'll toss them up on landfill either tonight or tomorrow.

patches applied to http://landfill.bluemartini.com dave, i am going to enable your create/destroy groups bit so you can play around with this and test.

OK, now that I've had the chance to play with it from an almost-user perspective... got some concept issues here now that we need to work out. The editusers and editgroups bits are two separate privileges... except that due to the interrelationship between records in the database, they kind of go hand in hand. 1) I would assume that if a user has the authority to edit groups, but not the authority to edit users, that they should probably be denied access to delete a group if there are users that are members of it? [unrelated point - I created a test group on landfill. Then I went to delete it, and got a confirmation dialog with no warnings. The current code doesn't check to exclude the maintainer except when it does the final delete (it probably should, but it doesn't yet). I should have gotten a warning saying there were users in the group because the maintainer would automatically be in it. So you apparently don't have a maintainer set up on landfill] 2) If a user has the ability to create groups, a) should they automatically become a member of any group they create? b) should they automatically be able to move users in and out of that group? [can you set me for "able to move others in and out of this group" for the test group I just created?] 3) Should a user with the authority to create and destroy groups be allowed to delete a group that (s)he's not a member of? Should they not be allowed to delete a group that has members if they don't have the authority to move members in and out of the group? 4) If a user has the ability to create groups, but not the ability to edit any aspect of a bug, should they be able to delete a group that still has bugs belonging to it? I just realized that the user regexps aren't being handled yet, either (applying to existing matching users at the time of group creation - they'll still be applied to new signups). None of this is a showstopper to checking it in (it's still useful as-is). Just stuff that still needs to be done to it.

A few comments: 1) I would assume that if a user has the authority to edit groups, but not the authority to edit users, that they should probably be denied access to delete a group if there are users that are members of it? If I read this right, there should be seperation of powers: 1) edit groups is just that, the ability to add and remove users from a group 2) create/destroy groups, the ability to add and remove groups, edit group permissions are implied > So you apparently don't have a maintainer set up on landfill Actually it was set, and I am the maintainer. >2) If a user has the ability to create groups, a) should they automatically >become a member of any group they create? No. They must explicitly add themselves to any group they create. >b) should they automatically be able >to move users in and out of that group? Yes, group creation and destruction equates to "super-user priviledges with respect to groups" in my way of thinking. This includes setting who is in a group or not. >[can you set me for "able to move others in and out of this group" for the >test group I just created?] Done. >3) Should a user with the authority to create and destroy groups be allowed to >delete a group that (s)he's not a member of? Should they not be allowed to >delete a group that has members if they don't have the authority to move >members >in and out of the group? Yes. If someone has the authority to create groups, they should have the authority to destroy it and membership for all users in that group. >4) If a user has the ability to create groups, but not the ability to edit any >aspect of a bug, should they be able to delete a group that still has bugs >belonging to it? I'm thinking that some form of warning should come up, letting them know exactly how many bugs belong to the group that is about to be destroyed (in fact, this would be a good idea in general) >I just realized that the user regexps aren't being handled yet, either (applying >to existing matching users at the time of group creation - they'll still be >applied to new signups). It would be nice to have this ability, but it would need to be tempered with the appropriate warnings as any massive db operation would. >None of this is a showstopper to checking it in (it's still useful as-is). >Just stuff that still needs to be done to it. I agree completely. I think there's a good case here to check in into the trunk and then spawn sub-bugs of this so that other people can work on these issues as so inclined. There's good work here that can be built upon. I'm going to play with it some more later on today and probably commit tomorrow.

>If I read this right, there should be seperation of powers: >1) edit groups is just that, the ability to add and remove users from a group >2) create/destroy groups, the ability to add and remove groups, edit group >permissions are implied OK, so all of the places throughout bugzilla that test for "caneditusers" or "can add/remove other users from this group" should also be checking for "can create/ destroy groups" as well... or I could just automatically set the blessgroupset bits for any group in existence on someone with group create/destroy privs when they enter the group editor... that would save having to edit the rest of bugzilla to compensate... >> So you apparently don't have a maintainer set up on landfill > >Actually it was set, and I am the maintainer. Have you edited your account? There's a bug in the edituser.cgi that will remove your maintainer status if you edit the maintainer account (any bits that aren't defined on the form that is shown to you get cleared when you update the account). You still have access to everything that exists at that time, but since it cleared the other bits, you won't have access to anything new unless you go back in and add yourself to it. >>4) If a user has the ability to create groups, but not the ability to edit any >>aspect of a bug, should they be able to delete a group that still has bugs >>belonging to it? > >I'm thinking that some form of warning should come up, letting them know exactly how many bugs belong to the group that is about to be destroyed (in fact, this would be a good idea in general) right now it just has a warning saying "there are still bugs in this group" and it has a link to "show me which bugs" which calls buglist.cgi with a query to look them up. Problem here is that above, you said the person should not automatically become a member of the new groups they create. Problem with this is if they go to delete said group later, it can't show them which bugs are in it, because they aren't a member of the group, so they can't see them.

>I could just automatically set the blessgroupset >bits for any group in existence on someone with group create/destroy privs when >they enter the group editor... that would save having to edit the rest of >bugzilla to compensate... That sounds good. >>Actually it was set, and I am the maintainer. >Have you edited your account? Yes. This is probably what happened. >Problem with this >is if they go to delete said group later, it can't show them which bugs are in >it, because they aren't a member of the group, so they can't see them. <insert circus music here> IEEEE. Okay, so if we go so far as above, we should probably add the person who can create/destroy groups as a member of each group they create by default.

committed changes to trunk. Thanks again Dave.

Verif feature is present. All bugs and enhancements should be filed separately.

In search of accurate queries.... (sorry for the spam)

Moving closed bugs to Bugzilla product