Bug 526189: Refactor the way that groups are checked for being validly

settable in a particular product, to eliminate the possibility of ever
setting an inactive or invalid group on a product.
r=LpSolit, a=LpSolit

2 files changed, 33 insertions, 16 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index d435d5442..9282bc3d2 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -1410,7 +1410,7 @@ sub _check_groups {
             # so instead of doing check(), we just do "next" on an invalid
             # group.
             my $group = new Bugzilla::Group({ name => $name }) or next;
-            next if !$product->group_is_valid($group);
+            next if !$product->group_is_settable($group);
             $add_groups{$group->id} = $group;
         }
     }
@@ -2074,15 +2074,14 @@ sub set_product {
     }
     
     if ($product_changed) {
-        # Remove groups that aren't valid in the new product. This will also
-        # have the side effect of removing the bug from groups that aren't
-        # active anymore.
+        # Remove groups that can't be set in the new product, or that aren't
+        # active.
         #
         # We copy this array because the original array is modified while we're
         # working, and that confuses "foreach".
         my @current_groups = @{$self->groups_in};
         foreach my $group (@current_groups) {
-            if (!grep($group->id == $_->id, @{$product->groups_valid})) {
+            if (!$group->is_active or !$product->group_is_valid($group)) {
                 $self->remove_group($group);
             }
         }
@@ -2326,8 +2325,8 @@ sub add_group {
     return if !$group->is_active or !$group->is_bug_group;
 
     # Make sure that bugs in this product can actually be restricted
-    # to this group.
-    grep($group->id == $_->id, @{$self->product_obj->groups_valid})
+    # to this group by the current user.
+    $self->product_obj->group_is_settable($group)
          || ThrowUserError('group_invalid_restriction',
                 { product => $self->product, group_id => $group->id });
 
@@ -2355,12 +2354,14 @@ sub remove_group {
     return unless $group;
     
     # First, check if this is a valid group for this product.
-    # You can *always* remove a group that is not valid for this product, so
-    # we don't do any other checks if that's the case. (set_product does this.)
+    # You can *always* remove a group that is not valid for this product
+    # or that is not active, so we don't do any other checks if either of
+    # those are the case. (Users might remove inactive groups, and set_product
+    # removes groups that aren't valid for this product.)
     #
     # This particularly happens when isbuggroup is no longer 1, and we're
     # moving a bug to a new product.
-    if (grep($_->id == $group->id, @{$self->product_obj->groups_valid})) {   
+    if ($group->is_active and $self->product_obj->group_is_valid($group)) {
         my $controls = $self->product_obj->group_controls->{$group->id};
 
         # Nobody can ever remove a Mandatory group.
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index 5c4057595..9416d3eef 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -666,8 +666,10 @@ sub groups_mandatory {
     # is Mandatory, the group is Mandatory for everybody, regardless of their
     # group membership.
     my $ids = Bugzilla->dbh->selectcol_arrayref(
-        "SELECT group_id FROM group_control_map
-          WHERE product_id = ?
+        "SELECT group_id 
+           FROM group_control_map
+                INNER JOIN groups ON group_control_map.group_id = groups.id
+          WHERE product_id = ? AND isactive = 1
                 AND (membercontrol = $mandatory
                      OR (othercontrol = $mandatory
                          AND group_id NOT IN ($groups)))",
@@ -677,8 +679,8 @@ sub groups_mandatory {
 }
 
 # We don't just check groups_valid, because we want to know specifically
-# if this group is valid for the currently-logged-in user.
-sub group_is_valid {
+# if this group can be validly set by the currently-logged-in user.
+sub group_is_settable {
     my ($self, $group) = @_;
     my $group_id = blessed($group) ? $group->id : $group;
     my $is_mandatory = grep { $group_id == $_->id } 
@@ -688,6 +690,11 @@ sub group_is_valid {
     return ($is_mandatory or $is_available) ? 1 : 0;
 }
 
+sub group_is_valid {
+    my ($self, $group) = @_;
+    return grep($_->id == $group->id, @{ $self->groups_valid }) ? 1 : 0;
+}
+
 sub groups_valid {
     my ($self) = @_;
     return $self->{groups_valid} if defined $self->{groups_valid};
@@ -910,7 +917,7 @@ is set to Default for the currently-logged-in user.
 Tells you what groups are mandatory for bugs in this product, for the
 currently-logged-in user. Returns an arrayref of C<Bugzilla::Group> objects.
 
-=item C<group_is_valid>
+=item C<group_is_settable>
 
 =over
 
@@ -946,7 +953,9 @@ C<1> if the group is valid in this product, C<0> otherwise.
 
 Returns an arrayref of L<Bugzilla::Group> objects, representing groups
 that bugs could validly be restricted to within this product. Used mostly
-by L<Bugzilla::Bug> to assure that you're adding valid groups to a bug.
+when you need the list of all possible groups that could be set in a product
+by anybody, disregarding whether or not the groups are active or who the
+currently logged-in user is.
 
 B<Note>: This doesn't check whether or not the current user can add/remove
 bugs to/from these groups. It just tells you that bugs I<could be in> these
@@ -958,6 +967,13 @@ groups, in this product.
 
 =back
 
+=item C<group_is_valid>
+
+Returns C<1> if the passed-in L<Bugzilla::Group> or group id could be set
+on a bug by I<anybody>, in this product. Even inactive groups are considered
+valid. (This is a shortcut for searching L</groups_valid> to find out if
+a group is valid in a particular product.)
+
 =item C<versions>
 
  Description: Returns all valid versions for that product.