The current product group system needs a reachitecture. It has the following problems: - You can't share a group between multiple products and update the members in one place. - You can't specify that product groups cannot be removed from any bugs in that product, yet the group will disappear off query, meaning you can possibly see that product on bugs but can't query for it. - You would expect product groups options to be on the products page, not the params page. - If you turn on entry restriction, it must be for all products. So here's the deal, what I propose: - Product groups (including entry restriction) are always available for products. The options disappear off the params page. - Every product has a place where you can specify the product's default groupset, required groupset and entry groupset. - Bugs automatically get the default groupset ORed with the required groupset when reported. - You cannot remove a restriction from a bug if its in the bug's product's required groupset. - You cannot enter a bug into a product unless the user is in the product's entry groupset. - A product will appear on the query, reports screen if the user is in the product's required groupset or entry groupset (because of reporter_accessible). - A product will appear on the entry screen if the user is in the product's entry groupset. - Because you specify groupsets explicitly, you can reuse a group between products.

See discussion in bug 68022, and partial code on the groups branch.

And let's not forget: - The current system of linking products with groups by name is non intuitive and non discoverable. To what extent does the groups branch support this? I'm pretty sure it has no concept of required groupset, or entry groupset for that matter.

I'd have to check if you want the exact state, but look for product_group_map. The sentry stuff isn't one, but buggroup stuff is, I believe.

After narrowing the scope of bug 143826, I went back through another iteration on my multicustomer requirements. It is now clear that this is the right approach. It would be best to extend it a bit further and add a permitted groupset and a canedit groupset. The permitted groupset would work much like groupsentry where a user who is in many groups would not even be offerred the option of selecting groups that are wholely inappropriate to the product. The canedit groupset could be used to identify groups that can edit a bug, thereby eliminating the assumption that users who can edit anything can edit everything they can see. This also implies that a user can only change the product of a bug to a product that the user can edit. When changing products, I presume that the sentry function would ask what to do if the required/permitted/default groupsets of the 2 products don't match. Probably it should offer the default options of..... a) Use the new product's default b) Keep the old settings, replacing the old default with the new default if it was set. c) Show the individual checkboxes for setting the bugs group restrictions in the new group.

Here's an interesting dilemma encountered in implmenting bug 157756 (should this bug be made dependent on that??)... With the old product groups, there was a notion of the bug being in its old product's group and BZ offerring to move it to its new product's group. How shoult this work?? One possibility... If old product and new product have differrent default groups AND all the default bits unique to the old product were taken, then BZ should offer to replace them with ALL of the default bits unique to the new product???

Assuming the interested parties are agreeable, I suggest that this bug be used to implement what was previosuly known as "stage3" in bug 157756. The changes from the original proposal for this bug would be... 1) Add the "permitted" groups for each product so that groups that have nothing to do with a product but are not required will not generate a checkbox. 2) Add "canedit" groups for each product so that bugs in a product can be made read-only to users that can see the bug and would be able to edit bugs in other products. 3) A new table, group_control map, would determine what combination of default/entry/required/permitted/canedit are appropriate to each combination of product and group. [called group_control_map instead of product_group_map to avoid blocking future controls by component, etc...] 4) New param buggroupentrydefault (default to former value of usebuggroupsentry) causes controls to default to requiring same-named group for entry. (if not overridden, behaves like usebuggroupsentry) 5) New param makeproductgroup (default to usebuggroups) causes same-named group to be created for each product and group control defaults to behave like usebuggroups unless overridden.

Regarding changing products: - if the group is not allowed in the new product and was there previously, remove it, give a warning on the intermediate page - if the group is required in the new product and wasnt there previously, add it, give a warning on the intermediate page - if the group can now be added to the bug, but wasn't there previously, give an option to add the group on the intermediate page I'm not sure why we need any parameters for this stuff. It seems feature overkill to me.

Regarding comment 7: Intermediate page sounds good. The only question on that is.... Should the intermediate page have give any special treatment to mapping the old default group(s) to the new default group(s) as the equivalent of the old "yes, but only if" option on the intermediate page? Presumably, that would mean that, especially on mass changes, there would be an option on the intermediate page to apply all of the new product's default groups and remove all of the old product's default groups if the bug had previously used all of the default groups. (this only really make sense in the most common case where the default for each product is a single group) The params would be a 1:1 replacement for usebuggroups and usebugroupsentry. The feedback on the original group system proposal was that a lot of sites had become accustomed to that feature and did not want to lose its "simplicity." (yeah - I know :-)

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

OK... so here's the proposal.. Comment now because I am getting my coding pencil out this week. Each product will have a map conecting it to any groups with which it is associated. That map will contain. entry - is membership required in order to enter bugs in this product? control - no/permitted/weakdefault/strongdefault/required no : Bugs in this product are never placed in this group permitted : Bugs can be placed in this group - members of this group get a checkbox weakdefault: Bugs can be placed in this group - members of this group get a checkbox that is checked by default strongdefault:Bugs can be placed in this group - members of this group get a checkbox that is checked by default and non-members restrict the bug to this group on entry required: Bugs will always be placed in this group - no checkbox will be presented canedit - Membership in this group is required in order to edit bugs in this group in any way (including CC and comment) usebuggroupsentry will be replaced with buggroupentrydefault causing the bits above to be automatically set to match the end-behavior of usebuggroupsentry as products are added if not overridden. usebuggroups will be replaced by makeproductgroup causes same-named group to be created for each product and group control defaults to behave like usebuggroups unless overridden. When a bug is moved from one product to another.... new required groups will be added non-permitted groups will be dropped newly permitted groups will be prompted on the confirm screen the option of conditionally mapping old default groups to new default groups will apply ALL of the new defaults that were not shared with the old product's defaults if ANY of the old defaults that were not shared with the old product's defaults were selected.

Additional "level" to satisfy bug 129366 Open - Even people who are not group members can set this bit on entry

This is a real early cut of the new product groups. (prior to much of my own reinspection an testing).

Fixed control problem in post_bug.cgi that kept mandatory group controls from effecting new bugs.

Comment on attachment 101036 [details] [diff] [review] Preview version 2 >Index: checksetup.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v >retrieving revision 1.193 >diff -u -r1.193 checksetup.pl >--- checksetup.pl 28 Sep 2002 18:42:24 -0000 1.193 >+++ checksetup.pl 29 Sep 2002 04:58:46 -0000 >@@ -1679,6 +1679,15 @@ > userid mediumint not null default 0, > quip text not null'; > >+$table{group_control_map} = >+ 'group_id mediumint not null, >+ product_id mediumint not null, >+ entry tinyint not null, >+ control tinyint not null, >+ canedit tinyint not null, >+ >+ unique(product_id, group_id)'; You also probably want an INDEX on group_id. > >+# 2002-09-28 - bugreport@peshkin.net - bug 147275 >+# >+# If usebuggroups was is in @oldparams, product groups are being replaced. <snip param munging> Bugzilla::Config has a section to update the old params. At this point, though, accessing the params will give you the old set. Because you can't trigger checking this on a schema change, you may have to add an exists function to Bugzilla::Config, and add it to the :admin export set. >Index: enter_bug.cgi > # If we're using bug groups to restrict bug entry, we need to know who the > # user is right from the start. >-confirm_login() if (Param("usebuggroupsentry")); >+confirm_login(); Hmmm. We don't want to do that - people shouldn't have to log in to pick a product if all the available products are not restricted. I think. >Index: globals.pl >+# CONSTANTS >+use constant ControlMapNone => 0; >+use constant ControlMapPermitted => 1; >+use constant ControlMapOpen => 2; >+use constant ControlMapWeakDefault => 3; >+use constant ControlMapStrongDefault => 4; >+use constant ControlMapRequired => 5; constants don't need () after them. Also, they're traditionally in ALLCAPS. You need comments, explaining what they mean. They should possibly go into another file though, although I'm not sure where. >+ print "so we add " . join(',',@groupstoadd) . "\n"; No... >Index: query.cgi >+ # If we're using bug groups to restrict entry on this product, >+ # and the user cannot enter bugs in this product, >+ # we don't want to include that product in this list. >+ next if (!CanEnterProduct($p)); Its actually possible for people to know about other producst, because of cclist_accessible stuff Do we want to somehow handle that? I don't think its worth it, personally. >--- template/en/default/list/table.html.tmpl 1 Jul 2002 03:28:56 -0000 1.6 >+++ template/en/default/list/table.html.tmpl 29 Sep 2002 04:58:51 -0000 >@@ -122,7 +122,7 @@ > [% tableheader %] > [% END %] > >- <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF (bug.groupset && !usebuggroups) %]"> >+ <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF bug.groupset %]"> bug.groupset?? That was just a quick review; I skipped most of the sql stuff.

>> You also probably want an INDEX on group_id. Right. >> Bugzilla::Config has a section to update the old params. At this point, though, >> accessing the params will give you the old set. Because you can't trigger >> checking this on a schema change, you may have to add an exists function to >> Bugzilla::Config, and add it to the :admin export set. Hmm.... I didn't see a very good way to do this, so when I look for these variables during the schema change below I am willing to accept EITHER the old value (from $oldhash) or the new param if checksetup is being rerun. I'm open to a better suggestion. >Index: enter_bug.cgi >> # If we're using bug groups to restrict bug entry, we need to know who the >> # user is right from the start. >>-confirm_login() if (Param("usebuggroupsentry")); >>+confirm_login(); >>Hmmm. We don't want to do that - people shouldn't have to log in to pick a >>product if all the available products are not restricted. I think. This dilemma shows up in a few places. In this place, I don't think that we woul d allow someone to anonymously submit a bug. If this is not true, I could change this to a function checking if ANY product has an entry group restriction instead. >Index: globals.pl >>+# CONSTANTS >>+use constant ControlMapNone => 0; >>+use constant ControlMapPermitted => 1; >>+use constant ControlMapOpen => 2; >>+use constant ControlMapWeakDefault => 3; >>+use constant ControlMapStrongDefault => 4; >>+use constant ControlMapRequired => 5; >>constants don't need () after them. Also, they're traditionally in ALLCAPS. You >>need comments, explaining what they mean. >>They should possibly go into another file though, although I'm not sure where. I'll ALLCAPS and comment them. I got errors in some contexts using them without () because of use strict. Bugzilla will need a better answer for constants later anyway. I will leave them in globals until someone has a concrete suggestion. In formulating a more comprehensive solution, we should bear in mind that templates will need access to the constants and that the mapping of constant values to displayed text names needs to be localizable. >>+ print "so we add " . join(',',@groupstoadd) . "\n"; >>No... heh... debug code leftover :-) >>Index: query.cgi >>+ # If we're using bug groups to restrict entry on this product, >>+ # and the user cannot enter bugs in this product, >>+ # we don't want to include that product in this list. >>+ next if (!CanEnterProduct($p)); >Its actually possible for people to know about other producst, because of >cclist_accessible stuff >Do we want to somehow handle that? I don't think its worth it, personally. Traditionally, we have not done that. This one preserves the existing functionality. >--- template/en/default/list/table.html.tmpl 1 Jul 2002 03:28:56 -0000 1.6 >+++ template/en/default/list/table.html.tmpl 29 Sep 2002 04:58:51 -0000 >@@ -122,7 +122,7 @@ > [% tableheader %] > [% END %] > >- <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF (bug.groupset && !usebuggroups) %]"> >+ <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF bug.groupset %]"> >bug.groupset?? This is actually correct. The fix to bug 170195 is that the templates receive a groupset that is now a count (rather than a map) of the restrictions. This permits templates that do cute things (like syntax highlighting) to be completely unchanged. There is an underlying philisolphical issue here. Often a schema change will make the interface to the templates into something of a misnomer. So the question is.... is it better to preserve the templates as much as possible by careful redefinition of parameters or is it better to make the naming conventions more up-to-date. Since templates tend to be customized by newbie admins, I am inclined to use the former approach and minimize breakage, but I could be easily convinced otherwise.

You missed my point with enter_bug. Obviously, we don't want anonymous bug submissions. But we do want people to be able to anonymously choose the product - then log in. OTOH, it probably doens't matter, except for the i-hate-cookies crowd, and people behind NAT/rotating proxies, and I'm trying to fix the latter now. With the constants, you probably needed to do that because of namespacing. What about a Bugzilla::Constants? Re templates, please break them. Functionality changes (eg bug 171493) are going to do so anyway, between versions. Thats what template versions are for, in theory.

For comment

>> # If we're using bug groups to restrict bug entry, we need to know who the >> # user is right from the start. >>-confirm_login() if (Param("usebuggroupsentry")); >>+confirm_login(); >>Hmmm. We don't want to do that - people shouldn't have to log in to pick a >>product if all the available products are not restricted. I think. >This dilemma shows up in a few places. In this place, I don't think that we >woul d allow someone to anonymously submit a bug. If this is not true, I >could >change this to a function checking if ANY product has an entry group >restriction instead. Here at RH we would actually prefer to have logins checked before receiving a list of possible products due to some products that have been created for private groups. Sometimes product names can give away NDA type information. Also definitely would never see a reason to anonymously submit a report.

WRT Comment 18: I am setting it so that the only products that a non-logged-in user could see are those that are "enterable" by everyone. Any product with ANY entry restriction would be invisible to logged-out users as well as to logged-in users who fail to satisft the entry criteria. I think that this should cover the NDA issue that is, I believe, exactly the same for RedHat's situation and my own. Am I on track?

WRT comment 19: I think we are on the same page if you mean taking a users current permission set and generating a list of viewable products (world enterable included) when hitting enter_bug.cgi. We would not have a need for having a list of products being presented for someone who has not logged in yet though. You would need to log in as the next step before getting a bug form anyway so why not go ahead and do it before the list. But I think we mean the same thing.

WRT: Comment 20 I think we are in sync. bbaetz: I think users who begin to enter a bug prior to logging in would be upset if they lost the opportunity to see certain products because they had forgotten to log in first. I'm inclined to make the login check unconditional at the beginning of enter_bug. Is there a sufficiently compelling argument to not do this?

From IRC: enter_bug will require login at the beginning. The only remaining problem is that the names for the control values stink. Throwing the question to developers..... please suggest names that don't stink for each of the 6 control values.

Tonight's Names that don't stink from IRC Members and Nonmembers have each have .... NA / Shown / Default / Mandatory Member Nonmember Implication NA -anything- Bug would never be in this group Shown NA Group members can put bug in this group, others cannot Shown Shown Anyone can put bugs in this group, only members can remove them Default NA Group members put bugs in this group by default on entry or on product change. Nonmembers do not. Default Default Same as Default/NA but nonmembers have option on entry or product change, but cannot remove the bit. Default Shown Illegal Shown Default Members have full control over group bit, nonmembers have the option, but cannot remove the bit once set. Shown Mandatory Members have full contol over group bit, nonmembers are forced to put bug in group on entry or product change Mandatory Shown Illegal Mandatory Default Illegal Mandatory NA Illegal Mandatory Mandatory Bug is always in this group Default Mandatory Same as shown/Mandatory, but members default to set.

NA/<anything-but-NA> should be illegal Shown/Default, add: non-members can only set on entry Default/NA - s/do/can/, ie non-members cannot file This makes a lot more sense if you write it out as a table in the order NA/Shown/Default/Mandatory, for members vs non members, mainly because all the 'illegal' cases become clear, as does the algorithm for determining it.

Member Nonmember Implication NA NA Bug would never be in this group NA Shown Illegal NA Default Illegal NA Mandatory Illegal Shown NA Group members can put a bug in this group, others cannot Shown Shown Group members can put a bug in this group, others can put a bug in this group on entry only and cannot remove it. Shown Default Group members can put a bug in this group, others can put a bug in this group on entry only, do so by default, and cannot remove it. Shown Mandatory Members have full contol over group bit, nonmembers are forced to put bug in group on entry or product change. Default NA Group members put bugs in this group by default on entry or on product change and can add or remove it at will. Nonmembers can not. Default Shown Illegal Default Default Same as Default/NA but nonmembers have option on entry or product change, but cannot remove the bit. Default Mandatory Same as shown/Mandatory, but members default to set. Mandatory Shown Illegal Mandatory Default Illegal Mandatory NA Illegal Mandatory Mandatory Bug is always in this group So the underlying principles are.... A non-member can never remove a group's restriction. A non-member can only add a group's restriction on entry or product change. A user with access to a bug, but not in all of its groups, can not move the bug out of the current product. Traditional non-product groups would be shown/na in all products. Traditional product groups would be default/na in the same-named product. Question: In the UI, 2 select boxes and a lot of error-checks or 1 select box with the 9 legal combinations shown??

> A user with access to a bug, but not in all of its groups, can not move the bug > out of the current product ...if doing so would require the removal of any group the user is not a member of. Although even that restriction is more than we currently have.

Not ready for full review yet. Has new UI and classifications. Still needs error checks during update of group controls. (and lots of testing)

This is the new UI

This is a basically operational patch, but I still have to complete my own testing ang inspections. Please test if possible.

In order to avoid a big mess when product groups become AND/OR, each product will only be permitted to specify a single default group. This takes a big source of confusion out of the verify behavior when a bug moves from one product to another and is still more flexible than the current product groups. In a seperate bug later, this can be made more elborate.

OK... this should be the right thing. We are only supporting one default group per product for now

Comment on attachment 102545 [details] [diff] [review] Patch ready now >@@ -282,16 +282,24 @@ > # (b) The group name isn't a product name > # This means that all product groups will be skipped, but > # non-product bug groups will still be displayed. >- if($ison || >- ($isactive && ($ingroup && (!Param("usebuggroups") || ($name eq $bug{'product'}) || >- (!defined $::proddesc{$name}))))) >+ # This is changing to.... >+ # (1) The bit is set and not required, or >+ # (2) The group is Shown or Default for members and >+ # the user is a member of the group. You don't need to keep the previous set of comments, which no longer apply >+ if (($ison && ($membercontrol != ControlMapMandatory())) || >+ ($isactive && $ingroup >+ && (($membercontrol == ControlMapDefault()) >+ || ($membercontrol == ControlMapShown())) >+ )) > { > $user{'inallgroups'} &= $ingroup; > >- push (@groups, { "bit" => $groupid, >- "ison" => $ison, >- "ingroup" => $ingroup, >- "description" => $description }); >+ if (($ison) || ($ingroup)) { >+ push (@groups, { "bit" => $groupid, >+ "ison" => $ison, >+ "ingroup" => $ingroup, >+ "description" => $description }); >+ } Hmm. Thats ugly :) In my Bug.pm rewrite, the 'inallgroups' stuff moves to the template, but for now... >Index: checksetup.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v >retrieving revision 1.194 >diff -u -r1.194 checksetup.pl >--- checksetup.pl 29 Sep 2002 23:01:11 -0000 1.194 >+++ checksetup.pl 11 Oct 2002 04:49:22 -0000 >@@ -1688,6 +1688,17 @@ > userid mediumint not null default 0, > quip text not null'; > >+$table{group_control_map} = >+ 'group_id mediumint not null, >+ product_id mediumint not null, >+ entry tinyint not null, >+ membercontrol tinyint not null, >+ othercontrol tinyint not null, >+ canedit tinyint not null, >+ >+ unique(product_id, group_id), primary key, not unique, perhaps? I don't think it makes a difference, although it may for pgsql. dkl? >@@ -3427,6 +3438,65 @@ > print "done.\n"; > } > >+# 2002-09-28 - bugreport@peshkin.net - bug 147275 >+# >+# If usebuggroups was is in @oldparams, product groups are being replaced. >+my %oldhash; >+foreach my $j (@oldparams) { >+ $oldhash{@{$j}[0]} = @{$j}[1]; Oh. That won't necessarily work, because the second argument is a string, not a value. For a string param, they're the same,but not all of them are. Hmm.... >+} >+if (defined $oldhash{'usebuggroups'}) { >+ # product groups are being replaced >+ SetParam('makeproductgroups', $oldhash{'usebuggroups'}); >+ SetParam('useentrygroupdefault', $oldhash{'usebuggroupsentry'}); >+ WriteParams(); Do we really want to keep these old options? If so, you're better off having the stuff in Bugzilla::Config handle the updates. >+} >+if ($oldhash{'usebuggroups'} || Param('makeproductgroups')) { >+ # If makeproductgroups is enabled and group_control_map is empty, >+ # backward-compatbility usebuggroups-equivalent records should >+ # be created. >+ my $entry = $oldhash{'usebuggroupsentry'} || Param('useentrygroupdefault'); >+ $sth = $dbh->prepare("SELECT COUNT(*) FROM group_control_map"); >+ $sth->execute(); >+ my ($mapcnt) = $sth->fetchrow_array(); >+ if ($mapcnt == 0) { >+ # Initially populate group_control_map. >+ # First, get all the existing products and their groups. >+ $sth = $dbh->prepare("SELECT groups.id, products.id, groups.name, " . >+ "products.name FROM groups, products " . >+ "WHERE isbuggroup != 0 AND isactive != 0"); >+ $sth->execute(); You're getting the cross-product of products x groups? Why?? >+ while (my ($groupid, $productid, $groupname, $productname) >+ = $sth->fetchrow_array()) { >+ } else { >+ # See if this group is a product group at all. >+ my $sth2 = $dbh->prepare("SELECT id FROM products WHERE name = " . >+ $dbh->quote($groupname)); Yeah, this is ugly. What you should do, is for each group, try to find the product. That cuts down on the queries you need. ie use a nested loop, instead of the cross product + inner loop >Index: defparams.pl > > { >- name => 'usebuggroups', >+ name => 'makeproductgroups', > desc => 'If this is on, Bugzilla will associate a bug group with each ' . > 'product in the database, and use it for querying bugs.', > type => 'b', Do we still want this option? Now that stuff is flexable, its a lot less useful. >@@ -233,9 +233,9 @@ > }, > > { >- name => 'usebuggroupsentry', >+ name => 'useentrygroupdefault', > desc => 'If this is on, Bugzilla will use product bug groups to restrict ' . >- 'who can enter bugs. Requires usebuggroups to be on as well.', >+ 'who can enter bugs. Requires makeproductgroups to be on as well.', > type => 'b', > default => 0 ditto. >Index: editgroups.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v >retrieving revision 1.22 >diff -u -r1.22 editgroups.cgi >--- editgroups.cgi 28 Sep 2002 22:54:44 -0000 1.22 >+++ editgroups.cgi 11 Oct 2002 04:49:23 -0000 >@@ -384,6 +384,16 @@ > VALUES ($admin, $gid, 0)"); > SendSQL("INSERT INTO group_group_map (member_id, grantor_id, isbless) > VALUES ($admin, $gid, 1)"); >+ # Permit all existing products to use the new group if makeproductgroups. >+ if (Param("makeproductgroups")) { >+ SendSQL("INSERT INTO group_control_map " . >+ "(group_id, product_id, entry, membercontrol, " . >+ "othercontrol, canedit) " . >+ "SELECT $gid, products.id, 0, " . >+ ControlMapShown() . ", " . >+ ControlMapNa() . ", 0 " . >+ "FROM products"); Cool, I didn't realise mysql supported that >Index: editproducts.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v >retrieving revision 1.31 >diff -u -r1.31 editproducts.cgi >--- editproducts.cgi 28 Sep 2002 18:51:52 -0000 1.31 >+++ editproducts.cgi 11 Oct 2002 04:49:24 -0000 >@@ -189,7 +189,17 @@ > my $action = trim($::FORM{action} || ''); > my $localtrailer = "<A HREF=\"editproducts.cgi\">edit</A> more products"; > >- >+# Initialize map of 9 legal combinations of member and monmember group controls. >+my %legal_controls; >+$legal_controls{ControlMapNa()}{ControlMapNa()} = 1; >+$legal_controls{ControlMapShown()}{ControlMapNa()} = 1; >+$legal_controls{ControlMapShown()}{ControlMapShown()} = 1; >+$legal_controls{ControlMapShown()}{ControlMapDefault()} = 1; >+$legal_controls{ControlMapShown()}{ControlMapMandatory()} = 1; >+$legal_controls{ControlMapDefault()}{ControlMapNa()} = 1; >+$legal_controls{ControlMapDefault()}{ControlMapDefault()} = 1; >+$legal_controls{ControlMapDefault()}{ControlMapMandatory()} = 1; >+$legal_controls{ControlMapMandatory()}{ControlMapMandatory()} = 1; eww. :) >Index: globals.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v >retrieving revision 1.211 >diff -u -r1.211 globals.pl >--- globals.pl 10 Oct 2002 19:02:22 -0000 1.211 >+++ globals.pl 11 Oct 2002 04:49:25 -0000 >@@ -631,6 +631,80 @@ > return $password; > } > >+# >+# This function checks if there are any entry groups defined. >+# If called with no arguments or a zero argument, it identifies Lose the 'or a zero argument' bit. Even if it is needed to work (Which it isn't - change line 2 to set it to undef instead of 0), its not something which people should need to rely on. >+ my $query = "SELECT COUNT(*) FROM group_control_map WHERE entry != 0"; >+ $query .= " AND product_id = $product_id" if ($product_id); SELECT id FROM ... WHERE ... LIMIT 1 is better. >+sub AnyDefaultGroups { >+ return $::CachedAnyDefaultGroups if defined($::CachedAnyDefaultGroups); >+ PushGlobalSQLState(); >+ SendSQL("SELECT COUNT(*) FROM group_control_map WHERE " . >+ "membercontrol = " . ControlMapDefault() . >+ " OR othercontrol = " . ControlMapDefault()); ditto. >+ $::CachedAnyDefaultGroups = FetchOneColumn(); >+ PopGlobalSQLState(); >+ return $::CachedAnyDefaultGroups; >+} >+sub CanEditProductId { Comment? What does this routine do? >+ my ($productid) = @_; >+ my $query = "SELECT COUNT(DISTINCT group_control_map.group_id), " . >+ "COUNT(DISTINCT user_group_map.group_id) " . >+ "FROM group_control_map " . >+ "LEFT JOIN user_group_map " . >+ "ON group_control_map.group_id = user_group_map.group_id " . >+ "AND user_group_map.user_id = $::userid " . >+ "AND isbless = 0 " . >+ "WHERE group_control_map.product_id = $productid " . >+ "AND group_control_map.canedit != 0"; >+ PushGlobalSQLState(); >+ SendSQL($query); >+ my ($editgroups, $editgroupsok) = FetchSQLData(); >+ PopGlobalSQLState(); >+ return ($editgroups == $editgroupsok); So why can't you SELECT COUNT(foo) = COUNT(bar) ? > >+# CONSTANTS Please move these to Bugzilla::Constants, and ALLCAPS them (and export them, I guesS). Then you won't need the () form everywhere. |use constant| at runtime (whcih anything in globals.pl is) is undefined, anyway. >+# >+# ControlMap constants for group_control_map. >+# membercontol:othercontrol => meaning Maybe these docs should go somewhere else, then? >+SendSQL("SELECT DISTINCT groups.id, groups.name, " . >+ "membercontrol, othercontrol " . >+ "FROM groups LEFT JOIN group_control_map " . >+ "ON group_id = id AND product_id = $product_id " . >+ " WHERE isbuggroup = 1 AND isactive = 1 ORDER BY description"); In other places you used !=0, and I assumed that that was on purpose - does it matter? These fields should be boolean, but maybe its better to be safe... >+ # Depending on the "addtonewgroup" variable, groups with >+ # defaults will change. >+ SendSQL("SELECT DISTINCT groups.id, isactive, " . >+ "oldcontrolmap.membercontrol, newcontrolmap.membercontrol, " . >+ "user_group_map.user_id IS NOT NULL, " . >+ "bug_group_map.group_id IS NOT NULL " . >+ "FROM groups " . >+ "LEFT JOIN group_control_map AS oldcontrolmap " . >+ "ON oldcontrolmap.group_id = groups.id " . >+ "AND oldcontrolmap.product_id = " . $oldhash{'product_id'} . >+ " LEFT JOIN group_control_map AS newcontrolmap " . >+ "ON newcontrolmap.group_id = groups.id " . >+ "AND newcontrolmap.product_id = $newproduct_id " . >+ "LEFT JOIN user_group_map " . >+ "ON user_group_map.group_id = groups.id " . >+ "AND user_group_map.user_id = $::userid " . >+ "AND user_group_map.isbless = 0 " . >+ "LEFT JOIN bug_group_map " . >+ "ON bug_group_map.group_id = groups.id " . >+ "AND bug_group_map.bug_id = $id " >+ ); OK, that one _really_ needs comments. >Index: reports.cgi This file is dead, or will be dead RSN >+ [% ELSIF error == "multiple_default_groups" %] >+ [% title = "Multiple Default Groups Not Permitted" %] >+ Multiple default groups are not permitted. >+ Each product may have only a single default group. >+ I don't like this, as we discussed on IRC. >--- template/en/default/list/table.html.tmpl 1 Jul 2002 03:28:56 -0000 1.6 >+++ template/en/default/list/table.html.tmpl 11 Oct 2002 04:49:28 -0000 >- <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF (bug.groupset && !usebuggroups) %]"> >+ <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF bug.groupset %]"> Why is this still called bug.groupset?

> > > >+$table{group_control_map} = > >+ 'group_id mediumint not null, > >+ product_id mediumint not null, > >+ entry tinyint not null, > >+ membercontrol tinyint not null, > >+ othercontrol tinyint not null, > >+ canedit tinyint not null, > >+ > >+ unique(product_id, group_id), > > primary key, not unique, perhaps? I don't think it makes a difference, although > it may for pgsql. dkl? > Only a single key can be a primary key. This is an index on a tuple of 2 fields. There can be non-unique product IDs and non-unique group IDs, but there is a unique record for each product, group pair. > >+# > >+# If usebuggroups was is in @oldparams, product groups are being replaced. > >+my %oldhash; > >+foreach my $j (@oldparams) { > >+ $oldhash{@{$j}[0]} = @{$j}[1]; > > Oh. That won't necessarily work, because the second argument is a string, not a > value. For a string param, they're the same,but not all of them are. Hmm.... > That's a keyword and value in the hash where the keyword is always first and the value second. Why won't this work? > >+} > >+if (defined $oldhash{'usebuggroups'}) { > >+ # product groups are being replaced > >+ SetParam('makeproductgroups', $oldhash{'usebuggroups'}); > >+ SetParam('useentrygroupdefault', $oldhash{'usebuggroupsentry'}); > >+ WriteParams(); > > Do we really want to keep these old options? If so, you're better off having > the stuff in Bugzilla::Config handle the updates. > Exactly how would Bugzilla::Config do this (from checksetup)? > > > You're getting the cross-product of products x groups? Why?? > > Yeah, this is ugly. What you should do, is for each group, try to find the > product. That cuts down on the queries you need. ie use a nested loop, instead > of the cross product + inner loop > Actually, for each group, I need to find either the same-name product or, if that doesn't exist, ALL the products. Is that any simpler? > >Index: defparams.pl > > > > { > >- name => 'usebuggroups', > >+ name => 'makeproductgroups', > >- name => 'usebuggroupsentry', > >+ name => 'useentrygroupdefault', > > Do we still want this option? Now that stuff is flexable, its a lot less > useful. > > There was a pretty strong sentiment in the NG when this was first proposed that the functionality should be preserved. It also simplifies knowing when to do the conversion for updating old sites. > >- > >+# Initialize map of 9 legal combinations of member and monmember group controls. > >+my %legal_controls; > >+$legal_controls{ControlMapNa()}{ControlMapNa()} = 1; > >+$legal_controls{ControlMapShown()}{ControlMapNa()} = 1; > >+$legal_controls{ControlMapShown()}{ControlMapShown()} = 1; > >+$legal_controls{ControlMapShown()}{ControlMapDefault()} = 1; > >+$legal_controls{ControlMapShown()}{ControlMapMandatory()} = 1; > >+$legal_controls{ControlMapDefault()}{ControlMapNa()} = 1; > >+$legal_controls{ControlMapDefault()}{ControlMapDefault()} = 1; > >+$legal_controls{ControlMapDefault()}{ControlMapMandatory()} = 1; > >+$legal_controls{ControlMapMandatory()}{ControlMapMandatory()} = 1; > > eww. :) > Care to suggest an altenative syntax that works? I tried a number that didn't.

No, mysql allows primary keys to be more than one key. Don't worry about it though, I suspect > > >+foreach my $j (@oldparams) { > > >+ $oldhash{@{$j}[0]} = @{$j}[1]; Entries in @oldparams are an arrayref, with the first entry the name of the param,and the second entry a printable (ie Data::Dumper'd) string. I guess that you're not looking at any multiple-value fields, though. Hmm > Exactly how would Bugzilla::Config do this (from checksetup)? Modify UpdateParams in B::C. Yeah, this isn't the nicest solution, but it works... > Actually, for each group, I need to find either the same-name product or, if > that doesn't exist, ALL the products. Is that any simpler? Well, you code has G*P lookups. Instead, try: get group list foreach group { if (get_product_id($group_name)) { update for found case } else { insert for all products. } } That gives you 2xG queries, which is more effiecient. No, I can't think of a better way of doing that. Did you ever draw that 2x2 grid, though? IOW, I'd like to see if that is condensable to some if statements, explicitly seting out the rules.

The grid... NA SH DE MA NA + - - - SH + + + + DE + - + + MA - - - + (mem == SH) || (mem == other) || ((mem == DE) && (other != SH))

Patch has most of the items noted by bbaetz covered and a number of fixes to mass-change. Still needs cleanup in the Config migration area of checksetup. Remainder should be ready for testing.

oops

1) Shouldn't CanEnterProduct() take a second argument $userid like CanSeeBug does so that $::userid can go away eventually. sub CanEnterProduct { my ($productname) = @_; my $userid = $::userid | 0; should be sub CanEnterProduct { my ($productname,$userid) = @_; if (!$userid) { $userid = $::userid | 0; } 2) In bug_form.pl the following lines do not work with PostgreSQL and I suspect others. SendSQL("SELECT DISTINCT groups.id, name, description," . " bug_group_map.group_id IS NOT NULL," . " user_group_map.group_id IS NOT NULL," . With PostgreSQL, in order to emulate the proper behaviour I have to re-write it as: SendSQL("SELECT DISTINCT groups.id, name, description," . " CASE WHEN bug_group_map.group_id IS NOT NULL THEN 1 ELSE 0 END," " CASE WHEN user_group_map.group_id IS NOT NULL THEN 1 ELSE 0 END," This is because when using IS NOT NULL in the SELECT, Pg treats it as a boolean TRUE or FALSE and returns 't' or 'f' respectively and not 1 or 0 as Mysql does. This is interpretted incorrectly in the logic that follows this SQL statement. I will leave it as an IF/ELSE until a better solution is found.

WRT comment 38 Yes, I should add the $userid argument in a number of places. On the IS NOT NULL issue, the CASE WHEN conditionalize to pgsql could work, but perhaps the right thing to do is just to skip the IS NOT NULL on these and deal with the value of the variable it returns. I assume that actually assigning a variable to a fieled that returns a NULL would result in an undef variable. Is that something that we should do across the board???

WRT comment 39 This should only be necessary in places where you are doing IS NOT NULL/NULL in the SELECT clause. Otherwise shouldnt matter. Doesnt seem like this is happening many places. Also refrain from using IFNULL(), this is mysql specific and COALESCE () has to be used for Pg.

<dkl> joel_desk: one thing i did have to change in enter_bug.cgi is the following <dkl> if (AnyEntryGroups()) { <dkl> confirm_login(); <dkl> } else { <dkl> quietly_check_login(); <dkl> } <dkl> joel_desk: otherwise as soon as I chose a product it would log me out. Funny that it didnt happen with stock bugzilla

Thats going to move to a user object, I think. OR maybe we need a product/group object, Not sure about that. Whilst postgres returns t/f, doens't DBD::Pg convert that into perl true/false? Or does that not happen for expressions in the SELECT?

Excerpt from the DBD::Pg man page. Data-Type bool The current implementation of PostgreSQL returns 't' for true and 'f' for false. From the perl point of view a rather unfortunate choice. The DBD-Pg module translates the result for the data-type bool in a perl- ish like manner: 'f' -> '0' and 't' -> '1'. This way the application does not have to check the database-specific returned values for the data-type bool, because perl treats '0' as false and '1' as true. PostgreSQL Version 6.2 considers the input 't' as true and anything else as false. PostgreSQL Version 6.3 considers the input 't', '1' and 1 as true and anything else as false. PostgreSQL Version 6.4 considers the input 't', '1' and 'y' as true and any other character as false. So you were right. I think I may have had some other boolean flipped somewhere that was giving me incorrect results. I removed the CASE statements.

Right. The only thing I'm not sure of is if DBD:Pg knows that |foo IS NOT NULL| is indeed a boolean return value (similarly, foo != 0, and so on)

The pg issues are still open and I am leaving changing all the CanDoXYZ functions to accept a userid to be a seperate bug. That said, this is the patch.

The appropriate way to handle this is to place this on the intermediate product change page: (a) a list of previously set restrictions that are now unavailable and will be forced to be removed (b) a list of the previously unset or unavailable restrictions that will be forced to added (c) a list of the previously unavailable restrictions that are now available, but not forced to be added, with checkboxes allowing you to set each optionally: (d) a list of unset restrictions where the old product was default off and the new product was default on, with checkboxes allowing you to set each (e) a list of set restrictions where the old product was default on and the new product was default off, with checkboxes allowing you to unset each

WRT Comment 46: This can generally be done for single changes, but it does not really extend to mass-changes. The mass change will have to stay pretty much as it is, but the single change can get fancier.

I don't think the mass change page will allow you to mass move bugs into a product if they are in different products to start with, so this shouldn't be a problem. If it does, that needs to get fixed. Or alternatively, I don't see why the product change page can't get extended to cope with multiple source products.

Enhancments to intermediate verify screens beyond what is needed to maintain parity with the existing scheme will be handled as a seperate landing after this one. This patch is already reachine the limits of reviewable size. See bug 174992

Comment on attachment 102999 [details] [diff] [review] patch v8 usebuggroups params and other relate old product group functions still exist in multiple files: queryhelp.cgi bug_form.pl describecomponents.cgi queryhelp.cgi

Updated to head. Fixed old reference in queryhelp.cgi Could not find additional references ... dkl??

Adding dependency on bug 114696

Fixed ininitialized variable errors where no group_control_map entry exists

Comment on attachment 104604 [details] [diff] [review] revised needs-work :edit attachment as comment is broken. I'll comment in a moment from the main bug window :-)

>--- Bugzilla/Config.pm 16 Oct 2002 10:49:56 -0000 1.5 >+++ Bugzilla/Config.pm 30 Oct 2002 04:14:50 -0000 >@@ -163,6 +163,18 @@ > delete $param{'usequip'}; > } > >+ # Change from old product groups to controls for group_control_map >+ # 2002-10-14 bug 147275 bugreport@peshkin.net >+ if (exists $param{'usebuggroups'} && !exists $param{'makeproductgroups'}) >+ $param{'makeproductgroup'} = $param{'usebuggroups'}; spelling error here, makeproductgroups needs an 's' on the end >+ delete $param{'usebuggroups'}; >+ } >+ if (exists $param{'usebuggroupsentry'} >+ && !exists $param{'useentrygroupdefault'}) { >+ $param{'useentrygroupdefault'} = $param{'usebuggroupsentry'}; >+ delete $param{'usebuggroupsentry'}; The two delete $param{} lines need to go away, so that checksetup.pl can finish cleaning up and stash the old values into old-params.txt. Otherwise the admin receives no notification that the params are gone, and no way to recover the old values if they want to revert for some reason.

I'm still working on reviewing this, not waiting for those nits to get fixed (though they're pretty important nits - that missing 's' caused us about 45 minutes of grief trying to figure out why the old permissions weren't converting when I applied this on Syndicomm). This patch gives us so much damn flexibility it's going to take me an entire day to test it thoroughly.

This patch has the data/params conversion fixed.

Update to HEAD Fixes editproducts problems for sites not previosly using groups (from tm's testing)

I tested the patch on my bugzilla (15 products, 80 users, 20 bugs/per day) for 4 days now, and everything seems to work fine. Patch applied smoothly and since i patched it, nothing has changed an nobody from my users has complained about missing functionality or errors when using bugzilla. After 2 days of using it without groups, i added group for each product with canedit checked (which means - only users in that group can edit bugs). Everything works fine. Also i tested on several combination of list items (made one secure group and added some bugs to it). Also that worked fine. Of course, this is not something you can call a test, but it prooves that everything works fine as in old, so in new functionality. Also - at first i patched my test_bugzilla, which works with same bugs database and the unpatched-real system kept going without no problems. One nit - when only canedit is checked, users who are not in that group, can not append comments. I think this must be changed, because comment appending will bring no harm and it's the normal way how bugzilla with users without "canedit" worked before (i don't know how it was with groups, because i've only now started to use them, because the new version is exactly what i need).

>> One nit - when only canedit is checked, users who are not in that group, can not >> append comments. I think this must be changed, because comment appending will >> bring no harm and it's the normal way how bugzilla with users without "canedit" >> worked before (i don't know how it was with groups, because i've only now >> started to use them, because the new version is exactly what i need). That is by design and is crucial for the situation where preventing cross-contamination is an overriding concern. However, if it is important to have a product-level equivalent of the systemwide editbugs group, this could be added as (yet another) feature. There are a few approaches that could be used. It could be done either as a systemwide param (cancommentanyway) or a distinct column in the group controls (canedit versus cancomment) for each product/group combination. Since the old behavior is unchanged so long as the admin does not choose to change it, I'd suggest we not try to slide this in before this patch lands. If this is important for some sites, please open an additional bug blocked by this one, specifying the preference on how that should be implemented.

If you have a lot of groups, the product group permissions page gets big fast. Especially if you upgraded from a system with lots of products that used the old product groups system. Here's what I'd like to propose for the UI on that page: Any group which has empty settings (NA for both popups and both checkboxes are empty) would not be displayed in the list. Only groups which had relevant settings in them would be displayed. Then you would have a single row at the bottom of the list with a popup menu for the group name, which would contain the names of all of the groups not yet listed, and the usual controls to the right of it, so you could add an additional group to the list.

Resynced to HEAD at 2.17.1 UI no longer clutters the edit group controls page with irrelevent groups

Now keeps groups in alphabetical order during edits

Update to head. Take care of one warning.

OK.... I installed the latest version on Syndicomm's system... which uses Product Groups for everything... I always hated all those product groups because there's 8 different products that everyone who has access at all, can see. :) So what I like about this patch is that I can create a single group to encompass all Syndicomm Online members, and make that mandatory on all of the products, thereby eliminating the existing product groups and make my group list more manageable. So... 1. I create a group called syndicommusers, with a regexp of .*@syndicomm\.com$ 2. I edit the Chat product, remove Entry from the Chat group, and put N/A for both popups for the Chat group, and add Entry for syndicommusers, and set both popups to Mandatory. 3. I visit a bug in the Chat product. I see a checkbox for the Chat group and it's checkmarked. I Commit the bug, making no changes to it. I get an email that shows the Chat group was removed from the bug, and the syndicommusers group was added to it. 4. I visit a bug in the Login Shell product (which hasn't had the groups changed yet) and add one of my test users to the CC on the bug. I then visit the groups editor and remove the syndicomm.com regexp from the Login Shell group, and visit editusers to ensure my test user no longer has access to it. 5. I visit the product editor, and repeat step 2 above for the Login Shell group. 6. Log out. Log in as the test user. 7. Visit the same bug listed in step 4. I don't have access to the Login Shell group, but I can see the bug because I'm on the CC List. Commit the bug with no changes. No error received saying I'm not allowed to remove the Login Shell group (good). I get an email showing the group was changed. So far so good. 1. Go to Query page, run a query that pulls all bugs (open or closed) in the Chat product. Click Change Several. Checkmark 2 or 3 bugs. 2. Don't touch anything, click Commit. Get an error that I didn't change anything. OK, same as before :) 3. Go back to the change several, same three are still selected. Choose "Remove bugs from this group" for the Chat group. I do NOT choose to add them to the syndicommusers group. Click commit. Get an email showing Chat was removed and syndicommusers was added. (woohoo!) :) Looks like the transition on this will run pretty smooth. Just need to do a change-several on the entire buglist removing the obsolete groups and the rest will take care of itself. :-) nit: on the product group permissions edit page, the list box where you can choose other groups to add needs a size="" constraint. I'd suggest size="5". Apparently Mozilla defaults to making the list box as tall as the number of options, so mine is REALLY tall. :-) Also, on the editproducts page, it would be useful to have a summary of group settings on the main product page, just like it shows summaries for the components, milestones and versions. Edit Group Access Controls: Users must be a member of these groups to enter bugs in this product: Syndicomm Users Users must be a member of these groups to edit bugs in this product: Engineering The Syndicommusers group is mandatory. The Forum Sysops group is shown to members. The Engineering group is shown to members. Something along those lines.... thoughts?

Hmm, creating a new active group appears to make it "Shown to members" in all products. This is a good default, but it would be nice if there was an option to create the new group without making it visible in any products. Like in my case, I have a dozen or two products, and I'm making a new group which replaces 6 of the existing product groups, and now I have to go through and remove the Shown marker on the other 12 products...

Comment on attachment 106324 [details] [diff] [review] Update Nov 14 ok, besides the other nits I mentioned in the previous comments, I finally found something worth calling a showstopper on the review. :) Nowhere on the Edit Group Access Controls page does it tell you what product you're editing the group access for. Other than what I've brought up so far, this looks pretty cooked, and these should be pretty easy fixes, so this looks like fast-track for check-in to me. :)

OK, this identifies the product and summarizes the settings on the main product edit page.

Comment on attachment 106465 [details] [diff] [review] Patch with UI tweaks r= justdave requesting a 2nd review

Comment on attachment 106465 [details] [diff] [review] Patch with UI tweaks >Index: checksetup.pl >+# 2002-09-28 - bugreport@peshkin.net - bug 147275 >+# >+if (Param('makeproductgroups')) { >+ # If makeproductgroups is enabled and group_control_map is empty, >+ # backward-compatbility usebuggroups-equivalent records should >+ # be created. >Index: defparams.pl > { >- name => 'usebuggroups', >+ name => 'makeproductgroups', Surely this can't be right? You're triggering a checksetup schema update on the new param (which is probably ok, because of the order we convert stuff), but theres no other restriction. Why can't someone have an empty map, then change the setting, then rerun checkseutp and have unexpected changes made? >Index: editparams.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v >retrieving revision 1.18 >diff -u -r1.18 editparams.cgi >--- editparams.cgi 29 Aug 2002 09:25:44 -0000 1.18 >+++ editparams.cgi 16 Nov 2002 05:40:37 -0000 >@@ -26,7 +26,7 @@ > use lib "."; > > use Bugzilla::Config qw(:DEFAULT :admin); >- >+use Bugzilla::Constants; > require "CGI.pl"; > > ConnectToDatabase(); Why do you need this here? >Index: globals.pl >+sub AnyDefaultGroups { >+ return $::CachedAnyDefaultGroups if defined($::CachedAnyDefaultGroups); >+ PushGlobalSQLState(); >+ SendSQL("SELECT * FROM group_control_map WHERE " . you can SELECT 1, rather than SELECT *, if we don't care about the result >+sub CanEditProductId { >+ my ($productid) = @_; >+ my $query = "SELECT (COUNT(DISTINCT group_control_map.group_id) = " . >+ "COUNT(DISTINCT user_group_map.group_id)) " . >+ "FROM group_control_map " . >+ "LEFT JOIN user_group_map " . >+ "ON group_control_map.group_id = user_group_map.group_id " . >+ "AND user_group_map.user_id = $::userid " . >+ "AND isbless = 0 " . >+ "WHERE group_control_map.product_id = $productid " . >+ "AND group_control_map.canedit != 0"; Can this be modified to use the New Improved way of doing the group restrictions? Its the same basic idea, so it should work >+sub CanEnterProduct { Ditto > Index: process_bug.cgi >@@ -972,6 +971,7 @@ > if ($::comma eq "" > && (! @groupAdd) && (! @groupDel) > && (! @::legal_keywords || (0 == @keywordlist && $keywordaction ne "makeexact")) >+ && (!@groupAdd) && (!@groupDel) Wy is this added - its the same query as two lines up? >+ while (MoreSQLData()) { >+ my ($groupid, $isactive, $oldcontrol, $newcontrol, >+ $useringroup, $bugingroup) = FetchSQLData(); >+ # An undefined newcontrol is none. >+ $newcontrol = &CONTROLMAPNA unless $newcontrol; >+ $oldcontrol = &CONTROLMAPNA unless $oldcontrol; You don't need the & here and the next few lines >Index: sanitycheck.cgi You should sanity check for the values being in range, and possibly for the combinations being valid, too. >Index: Bugzilla/Config.pm >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v >retrieving revision 1.6 >diff -u -r1.6 Config.pm >--- Bugzilla/Config.pm 9 Nov 2002 01:59:22 -0000 1.6 >+++ Bugzilla/Config.pm 16 Nov 2002 05:40:42 -0000 >@@ -163,6 +163,16 @@ > delete $param{'usequip'}; > } > >+ # Change from old product groups to controls for group_control_map >+ # 2002-10-14 bug 147275 bugreport@peshkin.net >+ if (exists $param{'usebuggroups'} && !exists $param{'makeproductgroups'}) { >+ $param{'makeproductgroups'} = $param{'usebuggroups'}; >+ } >+ if (exists $param{'usebuggroupsentry'} >+ && !exists $param{'useentrygroupdefault'}) { >+ $param{'useentrygroupdefault'} = $param{'usebuggroupsentry'}; >+ } >+ ick. I regret putting the conversion code in there... > Index: Bugzilla/Constants.pm >+require Exporter; >+ >+@Bugzilla::Constants::ISA = qw(Exporter); use base qw(Exporter) seems to be the prevailing style. Is there a large test db somewhere I can use to test this? I've played with it before on my installation (Although not this round; I'll do that tomorrow) but thats not really a large db. review- based mainly on the CanEditProductId perf issue, called once per product all the time. We probably want a GetEnterableProducts sub which does all teh sql queries in one go. If thats too much, then don't worry about it - its unlikyl to be noticable on our ~10 products.

Took care of all the items from the previous comment except... checksetup: In the schema conversion, the only way that checksetup will populate the group_control_map is if.... a) It never existed AND makeproductgroups (was usebuggroups) is set, indicating this is definietly being upgraded. b) The user wants to makeproductgroups now AND has NEVER set a single group relationship (this would happen if the user copied the DB without data/params, ran checksetup, changed the params, and reran checksetup) c) The user changed his mind, set the param, dropped the group_control_map, and reran checksetup. The key item is that this will never run if the user is in any situation where groups were doing anything at all. sanitycheck: Since the constants approach we finally settled on does not result in a reversable hash, this would be quite a mess. There is really no situation where the admin interface would show the admin they something is happening different from what is actually happening, so the consequences of an "illegal" combination are minor. Note that illegal combinations are nonsensical, but not damaging. I did add GetEnterableProducts(). This should be a boost to the speed of query.cgi (EVERY TIME) bacuse it used to have to look for same-named groups with one query for each and every legal product it found in versioncache. As an added bonus, it removes one dependency on versioncache.

Comment on attachment 106497 [details] [diff] [review] With changes from bbaetz review Incidentally, I tested the prior version on my DB (40 products * 1100 bugs * 150 users, Justdave has been trying it on syndicomm, and then there are the results from comment 62. Naturally, the latest changes require me to repeat some of my testing.

Just in case it is needed, this is a patch that can be applied to a FRESH sanitycheck.cgi after applying attachment 106497 [details] [diff] [review] to the whole tree.

Comment on attachment 106497 [details] [diff] [review] With changes from bbaetz review As discussed on irc earlier, when the permitted groups changes, we need to change existing bugs, after displaying a confirmation screen.

OK... this version uses templates for the new product admin pages and has a confirm for changes that need bugs updated. If the user confirms, it will move bugs in and out of groups as required.

Comment on attachment 106791 [details] [diff] [review] Patch with enhanced admin screens You need to fix the date in checksetup just before checkin (Actually, some of your previous checkins haven't done that - r=bbaetz on fixing those using cvs log) Also, when moving the product, I get the 'Verify Bug Group' stuff, even when teh bug group was mandatory. I'm not sure what to do with this screen now that its not just a boolean option - maybe show a list of removed groups, list of added, and list of can-now-add-but-couldn't-before? +my %ctl = ( + &::CONTROLMAPNA => 'NA', Can you use +CONTROLMAPNA to do the numeric forcing? &:: won't work when editproducts isn't in the main:: namespace. Not that thats going to happen before its rewritten, mind you. the editproducts removing/adding stuff should use the group name if possible. Capitalise the first char in the sentances, too +sub AnyEntryGroups { + my $product_id = shift; + $product_id = 0 unless ($product_id); Why is a product_id of 0 valid? an undef value passed to this function definately isn't. (As a side note, these non-user-specific product-type functions are going to need their own module. Suggestions on names?) +sub CanEnterProduct { + my ($productname) = @_; + my $query = "SELECT group_id IS NULL " . sybase doesn't like those sort of things in the SELECT. Either move the constraint to the WHERE, or jsut select it and return (defined(FetchOneColumn())) When I move to a new product, where do you check canedit on the new product? In fact, where do you check canenter? I can see where you check it, but that massive SQL statement looks like its going to just ingore anything the user isn't in. In fact, I just tested - I can file a bug in a product I'm allowed to, modify the html locally to include a product I can't, and then I can submit, bypassing canenter. --- reports.cgi 16 Oct 2002 23:09:26 -0000 1.61 +++ reports.cgi 19 Nov 2002 14:52:22 -0000 +push( @myproducts, "-All-"); +foreach my $this_product (@legal_product) { + push(@myproducts, $this_product) if CanEnterProduct($this_product); } Why not use GetEnterableProducts here? Can you then (later) drop the security check, since you test for presence in @myproducts earlier, returning 'invalid_product_name'. The sanitycheck alert should give a group id/name and product id/name, too, so that the admin knows what to fix. Anyway, with the exception of that secuiryt issue, this is looking good

OK, fixed the list of issues from bbaetz Left the &::CONSTANT stuff, I'm getting tired of breaking this.

Comment on attachment 106958 [details] [diff] [review] Patch v(I lost count long ago) +++ template/en/default/admin/products/groupcontrol/confirm-edit.html.tmpl +[% PROCESS global/header.html.tmpl title="Confirm Group Control Change for product \'$product\'" %] Needs to html filter the product editproducts and editgroups also need to filter the group name in various places Similarly later on in that file, and for the group name too - probably easier to just set filtered_product at the top, and use that everywhere template/en/default/admin/products/groupcontrol/edit.html.tmpl needs html attribute quoting in a few places, and its |checked="checked"|, not |checked="1"| It also needs FILTER html on some of the names Similarly for all the templates you added - please check those The user-error error you added looks like its going to have extra whitespacing because of where the &quot; are in relation to the word wrapping You also need a sanitycheck test that all bugs are in their REQURIED groups, but none of the N/A groups Fix those, and this'll be ready, I think...

doh! this...

OK - this has all the new CGI changes (and even some older ones) value_quote()'d

replaced value_quote with html_quote

Comment on attachment 106995 [details] [diff] [review] Patch rev (lostcount+4) In editgroups, the |changeform| needs its description changed in the text field, not just from the hidden input r=bbaetz with that

Comment on attachment 106995 [details] [diff] [review] Patch rev (lostcount+4) One single nit: You should use &amp; not & in: + print " <TH ALIGN=\"right\" VALIGN=\"top\"><A HREF=\"editproducts.cgi?action=editgroupcontrols&product=", url_quote($product), "\">Edit Group Access Controls</A></TH>\n";

Yeah, but I think we've given up on anything approaching html compliance for edit* (there was an &amp which needed to be &amp;, too) The html quoting I wanted were correctness fixes, not compliance ones.

That doesn't mean we shouldn't fix it anyway as long as we're looking at it. Less to go back and fix later.

Its going to be totally rewritten when its templatised, though. Its not worth spending time on in its current form

Comment on attachment 106995 [details] [diff] [review] Patch rev (lostcount+4) >+ print "<P>note: Any group controls Set to NA/NA with no other checkboxes "; >+ print "will automatically be removed from the panel the next time "; >+ print "update is clicked.\n"; This text can go away. You're not removing things from the panel anymore, and there's no Update button to click :) denying review because I moved a bug to a different product... each of these two products has different Mandatory groups, and what's Mandatory for one is N/A for the other. The groups didn't change. It left the original product's group on the bug, and didn't add the new one. I moved it back. It removed the destinatino products group, and added the source product's group (backwards). I never get a group confirmation screen on a product change, either.

Oh, I forgot to mention, I had a group on that bug with "Use for bugs" disabled, which got removed from that bug during that product move. Since that checkbox is effectively replacing the "disabled" on the old system, those should *not* get automatically removed. Think "Netscape Confidential", which is disabled on bugzilla.mozilla.org, because they aren't supposed to use it here, but I'm sure they wouldn't want those existing bugs accidently opened up if someone happens to touch them. That "use for bugs" really needs to be split into two separate functions. The "role group" vs "bug group" and the "enabled" vs "disabled" should be two different things. Bug that's "another bug".

OK, cleaned up the text Both of the group-change problems were the result of the same issue... if there are NO deafult groups defined, the code was being skipped.

Comment on attachment 107219 [details] [diff] [review] Patch rev (lostcount+5) product group changes are working as designed now. However, inactive groups are still getting removed on a product change. That needs to be fixed (inactive groups shouldn't be touched except for manual intervention - we can always add another link on the editgroups page for "remove all bugs from this group" or something, but that's Another Bug)

OK, forced changes resulting from product changes now only apply to groups that are still used for bugs.

Comment on attachment 107221 [details] [diff] [review] patch rev (lostcount+6) Identical behavior to the previous patch. Did you upload the right one? This still removes an inactive group on a product change.

doh! Try this

Comment on attachment 107222 [details] [diff] [review] patch rev (lostcount+7) yay, this one works.

Comment on attachment 107222 [details] [diff] [review] patch rev (lostcount+7) ok, Brad, your turn :)

Comment on attachment 107222 [details] [diff] [review] patch rev (lostcount+7) Groups which are marked as 'not for bugs' still affect bug entry. If I have a 'default' group, then disable it, it still shows (and is applied for) enter_bug. Isuspect mandatory has teh same issue. If I disable amd then reenable it, I'm seeing the ui on ediprodducts redisplay. I think we should drop entries from group_control_map when we disable a bug, rather than testing for is_active everywhere. I'm not sure if that would have any side effects, though. Thoughts?

grr... I can't just whack the controls when a group becomes inactive for a number of reasons. Most importantly, someone who disables a group and then reenables it 5 minutes later would not expect to see all of the settings reset (and enforced at odd times). So, I am adding the checks in the few places I missed. If the admin were to delete the group instead of just making taking away the used for bugs status, then all the cleanup happens. In order to make it clear that a control is still hanging around, when a product has controls pointing at a disabled group, the edit group controls screens will show the group as involved but disabled. None of the controls will be enforced on the group unless it is re-enabled.

Given the magnitude of what happens when you disable a group, maybe it should remove everything from the group control map for it and give a big warning confirmation screen that it's going to do so if you disabled it. I can't think of any reason outside of testing that you'd want to disable a group for 5 minutes and change it back unless you did it on accident, and an accident would get stopped by the confirmation screen.

I have this working well with leaving the group restore-able. I don't want to get into a situation where the bugs later get removed from the group if the group is later re-enabled and any other edit is done to the product. This would also play havoc with sanitycheck because it would get just as confused as the admins.

Updates handling of disabled groups. Disabled groups that still have bugs in them show up greyed out in the edit group controls page. Shows counts of bugs in each group when editing group controls Makes good on the original promise that sites using the old scheme won't even have to touch the new controls.

OK, this will keep the disabled group shown greyed out if there are bugs remaining in the group even if the group controls were NA/NA when the group was disabled. The decision regarding displaying such things is now made in the template.

groups now passed to the edit bugs template even if mandatory, though it will not present them unless it is altered. This restores cclist_accessible on bugs with only mandatory groups.

adding this to the approval queue for post-2.17.1

Comment on attachment 107253 [details] [diff] [review] patch rev (lostcount+10) If the only grpoup for a product is a mandatroy group, then I get the 'Only users in all of the selected groups can view this bug:' text with no checkboxes. Fix that, and you can take r=bbaetz

Tweak to template to supress "following groups" stuff if nothing is printed.

Comment on attachment 107279 [details] [diff] [review] patch rev (lostcount+11) '(The assignee can always see a bug, and this section does not take effect unless the bug is restricted to at least one group.)' Its still not obvious that the bug is restricted to a group. Lop off the 'to at least one group' bit, or something r=bbaetz with or without that change (or any other similar wording change you/justdave/etc come up with)

everything that's left is cosmetic, let's land this baby and file bugs on the cosmetic stuff if we care :-)

Checked in

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

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