147275 - Rearchitect product groups.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P2)

Description

[Matthew Tuck [:CodeMachine]]

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.

Comment 1

[Bradley Baetz (:bbaetz)]

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

Comment 2

[Matthew Tuck [:CodeMachine]]

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.

Comment 3

[Bradley Baetz (:bbaetz)]

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.

Comment 4

[Joel Peshkin]

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.

Comment 5

[Joel Peshkin]

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

Comment 6

[Joel Peshkin]

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.

Comment 7

[Matthew Tuck [:CodeMachine]]

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.

Comment 8

[Joel Peshkin]

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

Comment 9

[Joel Peshkin]

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

Comment 10

[Joel Peshkin]

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.

Comment 11

[Joel Peshkin]

Additional "level" to satisfy bug 129366

Open - Even people who are not group members can set this bit on entry

Comment 12

[Joel Peshkin]

Attachment: Sneak Preview

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

Comment 13

[Joel Peshkin]

Attachment: Preview version 2

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

Comment 14

[Bradley Baetz (:bbaetz)]

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 - [e-mail] - 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.

Comment 15

[Joel Peshkin]

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

Comment 16

[Bradley Baetz (:bbaetz)]

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.

Comment 17

[Joel Peshkin]

Attachment: UI for controlling group access to a product

For comment

Comment 18

[David Lawrence [:dkl]]

>> # 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.

Comment 19

[Joel Peshkin]

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?

Comment 20

[David Lawrence [:dkl]]

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.

Comment 21

[Joel Peshkin]

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?

Comment 22

[Joel Peshkin]

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.

Comment 23

[Joel Peshkin]

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.

Comment 24

[Bradley Baetz (:bbaetz)]

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.

Comment 25

[Joel Peshkin]

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

Comment 26

[Bradley Baetz (:bbaetz)]

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

Comment 27

[Joel Peshkin]

Attachment: Still premature patch

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

Comment 28

[Joel Peshkin]

Attachment: UI for a specific product to control group associations

This is the new UI

Comment 29

[Joel Peshkin]

Attachment: Patch - for early testing

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

Please test if possible.

Comment 30

[Joel Peshkin]

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.

Comment 31

[Joel Peshkin]

Attachment: Patch ready now

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

Comment 32

[Bradley Baetz (:bbaetz)]

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 - [e-mail] - 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?

Comment 33

[Joel Peshkin]

> > 
> >+$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.

Comment 34

[Bradley Baetz (:bbaetz)]

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.

Comment 35

[Joel Peshkin]

The grid...  

   NA SH DE MA
NA  +  -  -  -
SH  +  +  +  +
DE  +  -  +  +
MA  -  -  -  +

(mem == SH) || (mem == other) || ((mem == DE) && (other != SH))

Comment 36

[Joel Peshkin]

Attachment: Revised patch with most items covered

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.

Comment 37

[Joel Peshkin]

Attachment: Same patch with Constants.pm included this time

oops

Comment 38

[David Lawrence [:dkl]]

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.

Comment 39

[Joel Peshkin]

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

Comment 40

[David Lawrence [:dkl]]

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. 

Comment 41

[David Lawrence [:dkl]]

<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

Comment 42

[Bradley Baetz (:bbaetz)]

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?

Comment 43

[David Lawrence [:dkl]]

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.

Comment 44

[Bradley Baetz (:bbaetz)]

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)

Comment 45

[Joel Peshkin]

Attachment: patch v8

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.

Comment 46

[Matthew Tuck [:CodeMachine]]

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

Comment 47

[Joel Peshkin]

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.

Comment 48

[Matthew Tuck [:CodeMachine]]

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.

Comment 49

[Joel Peshkin]

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 50

[David Lawrence [:dkl]]

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

Comment 51

[Joel Peshkin]

Attachment: v9

Updated to head.  Fixed old reference in queryhelp.cgi

Could not find additional references ... dkl??

Comment 52

[Joel Peshkin]

Attachment: doh! try this one

Comment 53

[Joel Peshkin]

Adding dependency on bug 114696

Comment 54

[Joel Peshkin]

Attachment: Fixes items noted by bbaetz

Comment 55

[Joel Peshkin]

Attachment: Update to HEAD and fix unitialized rows problem

Comment 56

[Joel Peshkin]

Attachment: revised

Fixed ininitialized variable errors where no group_control_map entry exists

Comment 57

[Dave Miller [:justdave]]

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

Comment 58

[Dave Miller [:justdave]]

>--- 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 [e-mail]
>+    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.

Comment 59

[Dave Miller [:justdave]]

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.

Comment 60

[Joel Peshkin]

Attachment: Patch with params conversion fixed

This patch has the data/params conversion fixed.

Comment 61

[Joel Peshkin]

Attachment: Updated and editproducts fixes

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

Comment 62

[Toms Baugis]

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

Comment 63

[Joel Peshkin]

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

Comment 64

[Dave Miller [:justdave]]

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.

Comment 65

[Joel Peshkin]

Attachment: Updated to unclutter edit group controls

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

Comment 66

[Joel Peshkin]

Attachment: Update to HEAD and clean up editproducts UI

Now keeps groups in alphabetical order during edits

Comment 67

[Joel Peshkin]

Attachment: Update Nov 14

Update to head.  Take care of one warning.

Comment 68

[Dave Miller [:justdave]]

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?

Comment 69

[Dave Miller [:justdave]]

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 70

[Dave Miller [:justdave]]

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

Comment 71

[Joel Peshkin]

Attachment: Patch with UI tweaks

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

Comment 72

[Dave Miller [:justdave]]

Comment on attachment 106465 [details] [diff] [review]
Patch with UI tweaks

r= justdave
requesting a 2nd review

Comment 73

[Bradley Baetz (:bbaetz)]

Comment on attachment 106465 [details] [diff] [review]
Patch with UI tweaks

>Index: checksetup.pl

>+# 2002-09-28 - [e-mail] - 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 [e-mail]
>+    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.

Comment 74

[Joel Peshkin]

Attachment: With changes from bbaetz review

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 75

[Joel Peshkin]

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.

Comment 76

[Joel Peshkin]

Attachment: Distinct patch for sanitycheck.cgi

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 77

[Bradley Baetz (:bbaetz)]

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.

Comment 78

[Joel Peshkin]

Attachment: Patch with enhanced admin screens

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 79

[Bradley Baetz (:bbaetz)]

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

Comment 80

[Joel Peshkin]

Attachment: Patch v(I lost count long ago)

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

Comment 81

[Bradley Baetz (:bbaetz)]

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 " 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...

Comment 82

[Joel Peshkin]

Attachment: Patch ver (lostcount)+1

Comment 83

[Joel Peshkin]

Attachment: Patch rev (lostcount)+2

doh!  this...

Comment 84

[Joel Peshkin]

Attachment: Patch rev (lostcount+3)

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

Comment 85

[Joel Peshkin]

Attachment: Patch rev (lostcount+4)

replaced value_quote with html_quote

Comment 86

[Bradley Baetz (:bbaetz)]

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 87

[Tobias Burnus]

Comment on attachment 106995 [details] [diff] [review]
Patch rev (lostcount+4)

One single nit: You should use & 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";

Comment 88

[Bradley Baetz (:bbaetz)]

Yeah, but I think we've given up on anything approaching html compliance for
edit* (there was an &amp which needed to be &, too)

The html quoting I wanted were correctness fixes, not compliance ones.

Comment 89

[Dave Miller [:justdave]]

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.

Comment 90

[Bradley Baetz (:bbaetz)]

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

Comment 91

[Dave Miller [:justdave]]

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.

Comment 92

[Dave Miller [:justdave]]

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". 

Comment 93

[Joel Peshkin]

Attachment: Patch rev (lostcount+5)

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 94

[Dave Miller [:justdave]]

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)

Comment 95

[Joel Peshkin]

Attachment: patch rev (lostcount+6)

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

Comment 96

[Dave Miller [:justdave]]

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.

Comment 97

[Joel Peshkin]

Attachment: patch rev (lostcount+7)

doh! Try this

Comment 98

[Dave Miller [:justdave]]

Comment on attachment 107222 [details] [diff] [review]
patch rev (lostcount+7)

yay, this one works.

Comment 99

[Dave Miller [:justdave]]

Comment on attachment 107222 [details] [diff] [review]
patch rev (lostcount+7)

ok, Brad, your turn :)

Comment 100

[Bradley Baetz (:bbaetz)]

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?

Comment 101

[Joel Peshkin]

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.

Comment 102

[Dave Miller [:justdave]]

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.

Comment 103

[Joel Peshkin]

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.

Comment 104

[Joel Peshkin]

Attachment: patch rev (lostcount+8)

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.

Comment 105

[Joel Peshkin]

Attachment: patch rev (lostcount+9)

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.

Comment 106

[Joel Peshkin]

Attachment: patch rev (lostcount+10)

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.

Comment 107

[Dave Miller [:justdave]]

adding this to the approval queue for post-2.17.1

Comment 108

[Bradley Baetz (:bbaetz)]

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

Comment 109

[Joel Peshkin]

Attachment: patch rev (lostcount+11)

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

Comment 110

[Bradley Baetz (:bbaetz)]

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)

Comment 111

[Dave Miller [:justdave]]

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

Comment 112

[Joel Peshkin]

Checked in

Comment 113

[Bradley Baetz (:bbaetz)]

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

Comment 114

[Dave Miller [:justdave]]

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