Bug 401965: Move groups updating from process_bug.cgi to Bugzilla::Bug

Bugzilla/Bug.pm

@@ -499,6 +499,28 @@ sub update {
     my $old_bug = $self->new($self->id);
     my $changes = $self->SUPER::update(@_);
     
+    my %old_groups = map {$_->id => $_} @{$old_bug->groups_in};
+    my %new_groups = map {$_->id => $_} @{$self->groups_in};
+    my ($removed_gr, $added_gr) = diff_arrays([keys %old_groups],
+                                              [keys %new_groups]);
+    if (scalar @$removed_gr || scalar @$added_gr) {
+        if (@$removed_gr) {
+            my $qmarks = join(',', ('?') x @$removed_gr);
+            $dbh->do("DELETE FROM bug_group_map
+                       WHERE bug_id = ? AND group_id IN ($qmarks)", undef,
+                     $self->id, @$removed_gr);
+        }
+        my $sth_insert = $dbh->prepare(
+            'INSERT INTO bug_group_map (bug_id, group_id) VALUES (?,?)');
+        foreach my $gid (@$added_gr) {
+            $sth_insert->execute($self->id, $gid);
+        }
+        my @removed_names = map { $old_groups{$_}->name } @$removed_gr;
+        my @added_names   = map { $new_groups{$_}->name } @$added_gr;
+        $changes->{'bug_group'} = [join(', ', @removed_names),
+                                   join(', ', @added_names)];
+    }
+    
     foreach my $comment (@{$self->{added_comments} || []}) {
         my $columns = join(',', keys %$comment);
         my @values  = values %$comment;
@@ -1599,6 +1621,26 @@ sub set_product {
         $self->set_target_milestone($tm_name);
     }
     
+    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.
+        #
+        # 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})) {
+                $self->remove_group($group);
+            }
+        }
+    
+        # Make sure the bug is in all the mandatory groups for the new product.
+        foreach my $group (@{$product->groups_mandatory_for(Bugzilla->user)}) {
+            $self->add_group($group);
+        }
+    }
+    
     # XXX This is temporary until all of process_bug uses update();
     return $product_changed;
 }
@@ -1756,6 +1798,75 @@ sub modify_keywords {
     return $any_changes;
 }
 
+sub add_group {
+    my ($self, $group) = @_;
+    # Invalid ids are silently ignored. (We can't tell people whether
+    # or not a group exists.)
+    $group = new Bugzilla::Group($group) unless ref $group;
+    return unless $group;
+
+    # Make sure that bugs in this product can actually be restricted
+    # to this group.
+    grep($group->id == $_->id, @{$self->product_obj->groups_valid})
+         || ThrowUserError('group_invalid_restriction',
+                { product => $self->product, group_id => $group->id });
+
+    # OtherControl people can add groups only during a product change,
+    # and only when the group is not NA for them.
+    if (!Bugzilla->user->in_group($group->name)) {
+        my $controls = $self->product_obj->group_controls->{$group->id};
+        if (!$self->{_old_product_name}
+            || $controls->{othercontrol} == CONTROLMAPNA)
+        {
+            ThrowUserError('group_change_denied',
+                           { bug => $self, group_id => $group->id });
+        }
+    }
+
+    my $current_groups = $self->groups_in;
+    if (!grep($group->id == $_->id, @$current_groups)) {
+        push(@$current_groups, $group);
+    }
+}
+
+sub remove_group {
+    my ($self, $group) = @_;
+    $group = new Bugzilla::Group($group) unless ref $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.)
+    #
+    # 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})) {   
+        my $controls = $self->product_obj->group_controls->{$group->id};
+
+        # Nobody can ever remove a Mandatory group.
+        if ($controls->{membercontrol} == CONTROLMAPMANDATORY) {
+            ThrowUserError('group_invalid_removal',
+                { product => $self->product, group_id => $group->id,
+                  bug => $self });
+        }
+
+        # OtherControl people can remove groups only during a product change,
+        # and only when they are non-Mandatory and non-NA.
+        if (!Bugzilla->user->in_group($group->name)) {
+            if (!$self->{_old_product_name}
+                || $controls->{othercontrol} == CONTROLMAPMANDATORY
+                || $controls->{othercontrol} == CONTROLMAPNA)
+            {
+                ThrowUserError('group_change_denied',
+                               { bug => $self, group_id => $group->id });
+            }
+        }
+    }
+    
+    my $current_groups = $self->groups_in;
+    @$current_groups = grep { $_->id != $group->id } @$current_groups;
+}
+
 #####################################################################
 # Instance Accessors
 #####################################################################

Bugzilla/Product.pm

@@ -21,6 +21,7 @@ package Bugzilla::Product;
 use Bugzilla::Version;
 use Bugzilla::Milestone;
 
+use Bugzilla::Constants;
 use Bugzilla::Util;
 use Bugzilla::Group;
 use Bugzilla::Error;
@@ -101,6 +102,39 @@ sub group_controls {
     return $self->{group_controls};
 }
 
+sub groups_mandatory_for {
+    my ($self, $user) = @_;
+    my $groups = $user->groups_as_string;
+    my $mandatory = CONTROLMAPMANDATORY;
+    # For membercontrol we don't check group_id IN, because if membercontrol
+    # 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 = ?
+                AND (membercontrol = $mandatory
+                     OR (othercontrol = $mandatory
+                         AND group_id NOT IN ($groups)))",
+        undef, $self->id);
+    return Bugzilla::Group->new_from_list($ids);
+}
+
+sub groups_valid {
+    my ($self) = @_;
+    return $self->{groups_valid} if defined $self->{groups_valid};
+    
+    # Note that we don't check OtherControl below, because there is no
+    # valid NA/* combination.
+    my $ids = Bugzilla->dbh->selectcol_arrayref(
+        "SELECT DISTINCT group_id
+          FROM group_control_map AS gcm
+               INNER JOIN groups ON gcm.group_id = groups.id
+         WHERE product_id = ? AND isbuggroup = 1
+               AND membercontrol != " . CONTROLMAPNA,  undef, $self->id);
+    $self->{groups_valid} = Bugzilla::Group->new_from_list($ids);
+    return $self->{groups_valid};
+}
+
 sub versions {
     my $self = shift;
     my $dbh = Bugzilla->dbh;
@@ -286,6 +320,42 @@ below.
               a Bugzilla::Group object and the properties of group
               relative to the product.
               
+=item C<groups_mandatory_for>
+
+=over
+
+=item B<Description>
+
+Tells you what groups are mandatory for bugs in this product.
+
+=item B<Params>
+
+C<$user> - The user who you want to check.
+
+=item B<Returns> An arrayref of C<Bugzilla::Group> objects.
+
+=back
+
+=item C<groups_valid>
+
+=over
+
+=item B<Description>
+
+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.
+
+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
+groups, in this product.
+
+=item B<Params> (none)
+
+=item B<Returns> An arrayref of L<Bugzilla::Group> objects.
+
+=back
+
 =item C<versions()>
 
  Description: Returns all valid versions for that product.

process_bug.cgi

@@ -99,7 +99,7 @@ sub send_results {
 # Tells us whether or not a field should be changed by process_bug, by
 # checking that it's defined and not set to dontchange.
 sub should_set {
-    # check_defined is used for custom fields, where there's another field
+    # check_defined is used for fields where there's another field
     # whose name starts with "defined_" and then the field name--it's used
     # to know when we did things like empty a multi-select or deselect
     # a checkbox.
@@ -248,6 +248,23 @@ if (should_set('product')) {
               other_bugs => \@bug_objects,
             });
         $product_change ||= $changed;
+        
+        # strict_isolation checks mean that we should set the groups
+        # immediately after changing the product.
+        foreach my $group (@{$b->product_obj->groups_valid}) {
+            my $gid = $group->id;
+            if (should_set("bit-$gid", 1)) {
+                # Check ! first to avoid having to check defined below.
+                if (!$cgi->param("bit-$gid")) {
+                    $b->remove_group($gid);
+                }
+                # "== 1" is important because mass-change uses -1 to mean
+                # "don't change this restriction"
+                elsif ($cgi->param("bit-$gid") == 1) {
+                    $b->add_group($gid);
+                }
+            }
+        }
     }
 }
 
@@ -369,16 +386,12 @@ if (defined $cgi->param('id')) {
     }
 
     # reporter_accessible and cclist_accessible--these are only set if
-    # the user can change them and there are groups on the bug.
-    # (If the user can't change the field, the checkboxes don't appear
-    #  on show_bug, thus it would look like the user was trying to
-    #  uncheck them, which would then be denied by the set_ functions,
-    #  throwing a confusing error.)
-    if (scalar @{$bug->groups_in}) {
+    # the user can change them and they appear on the page.
+    if (should_set('cclist_accessible', 1)) {
         $bug->set_cclist_accessible($cgi->param('cclist_accessible'))
-            if $bug->check_can_change_field('cclist_accessible', 0, 1);
+    }
+    if (should_set('reporter_accessible', 1)) {
         $bug->set_reporter_accessible($cgi->param('reporter_accessible'))
-            if $bug->check_can_change_field('reporter_accessible', 0, 1);
     }
     
     # You can only mark/unmark comments as private on single bugs. If
@@ -782,80 +795,6 @@ foreach my $id (@idlist) {
         $dbh->do(q{DELETE FROM duplicates WHERE dupe = ?}, undef, $id);
     }
 
-    # First of all, get all groups the bug is currently restricted to.
-    my $initial_groups =
-      $dbh->selectcol_arrayref('SELECT group_id, name
-                                  FROM bug_group_map
-                            INNER JOIN groups
-                                    ON groups.id = bug_group_map.group_id
-                                 WHERE bug_id = ?', {Columns=>[1,2]}, $id);
-    my %original_groups = @$initial_groups;
-    my %updated_groups = %original_groups;
-
-    # Now let's see which groups have to be added or removed.
-    foreach my $gid (keys %{$new_product->group_controls}) {
-        my $group = $new_product->group_controls->{$gid};
-        # Leave inactive groups alone.
-        next unless $group->{group}->is_active;
-
-        # Only members of a group can add/remove the bug to/from it,
-        # unless the bug is being moved to another product in which case
-        # non-members can also edit group restrictions.
-        if ($group->{membercontrol} == CONTROLMAPMANDATORY
-            || ($product_change && $group->{othercontrol} == CONTROLMAPMANDATORY
-                && !$user->in_group_id($gid)))
-        {
-            $updated_groups{$gid} = $group->{group}->name;
-        }
-        elsif ($group->{membercontrol} == CONTROLMAPNA
-               || ($product_change && $group->{othercontrol} == CONTROLMAPNA
-                   && !$user->in_group_id($gid)))
-        {
-            delete $updated_groups{$gid};
-        }
-        # When editing several bugs at once, only consider groups which
-        # have been displayed.
-        elsif (($user->in_group_id($gid) || $product_change)
-               && ((defined $cgi->param('id') && Bugzilla->usage_mode != USAGE_MODE_EMAIL)
-                   || defined $cgi->param("bit-$gid")))
-        {
-            if (!$cgi->param("bit-$gid")) {
-                delete $updated_groups{$gid};
-            }
-            # Note that == 1 is important, because == -1 means "ignore this group".
-            elsif ($cgi->param("bit-$gid") == 1) {
-                $updated_groups{$gid} = $group->{group}->name;
-            }
-        }
-    }
-    # We also have to remove groups which are not legal in the new product.
-    foreach my $gid (keys %updated_groups) {
-        delete $updated_groups{$gid}
-          unless exists $new_product->group_controls->{$gid};
-    }
-    my ($removed, $added) = diff_arrays([keys %original_groups], [keys %updated_groups]);
-
-    # We can now update the DB.
-    if (scalar(@$removed)) {
-        $dbh->do('DELETE FROM bug_group_map WHERE bug_id = ?
-                  AND group_id IN (' . join(', ', @$removed) . ')',
-                  undef, $id);
-    }
-    if (scalar(@$added)) {
-        my $sth = $dbh->prepare('INSERT INTO bug_group_map
-                                 (bug_id, group_id) VALUES (?, ?)');
-        $sth->execute($id, $_) foreach @$added;
-    }
-
-    # Add the changes to the bug_activity table.
-    if (scalar(@$removed) || scalar(@$added)) {
-        my @removed_names = map { $original_groups{$_} } @$removed;
-        my @added_names = map { $updated_groups{$_} } @$added;
-        LogActivityEntry($id, 'bug_group', join(',', @removed_names),
-                         join(',', @added_names), $whoid, $timestamp);
-        $bug_changed = 1;
-    }
-
     my ($cc_removed) = $bug_objects{$id}->update_cc($timestamp);
     $cc_removed = [map {$_->login} @$cc_removed];
 

template/en/default/bug/edit.html.tmpl

@@ -498,6 +498,9 @@
         [% END %]
 
           
+      [% IF group.ingroup %]
+        <input type="hidden" name="defined_bit-[% group.bit %]" value="1">
+      [% END %]
       <input type="checkbox" value="1"
              name="bit-[% group.bit %]" id="bit-[% group.bit %]"
              [% " checked=\"checked\"" IF group.ison %]
@@ -530,11 +533,13 @@
       </p>
 
       <p>
+        <input type="hidden" name="defined_reporter_accessible" value="1">
         <input type="checkbox" value="1"
                name="reporter_accessible" id="reporter_accessible"
                [% " checked" IF bug.reporter_accessible %]
                [% " disabled=\"disabled\"" UNLESS bug.check_can_change_field("reporter_accessible", 0, 1) %]>
         <label for="reporter_accessible">Reporter</label>
+        <input type="hidden" name="defined_cclist_accessible" value="1">
         <input type="checkbox" value="1"
                name="cclist_accessible" id="cclist_accessible"
                [% " checked" IF bug.cclist_accessible %]

template/en/default/bug/process/verify-new-product.html.tmpl

@@ -145,6 +145,8 @@
     <p>These groups are optional. You can decide to restrict [% terms.bugs %] to
     one or more of the following groups:<br>
     [% FOREACH group = optional_groups %]
+      <input type="hidden" name="defined_bit-[% group.group.id FILTER html %]"
+             value="1">
       <input type="checkbox" id="bit-[% group.group.id FILTER html %]"
              name="bit-[% group.group.id FILTER html %]"
              [% ((group.membercontrol == constants.CONTROLMAPDEFAULT && user.in_group(group.group.name))

template/en/default/global/user-error.html.tmpl

@@ -600,6 +600,12 @@
     in the database which refer to it. All references to this group must
     be removed before you can remove it.
 
+  [% ELSIF error == "group_change_denied" %]
+    [% title = "Cannot Add/Remove That Group" %]
+    You tried to add or remove group id [% group_id FILTER html %]
+    from [% terms.bug %] [%+ bug.id FILTER html %], but you do not
+    have permissions to do so.
+
   [% ELSIF error == "group_exists" %]
     [% title = "The group already exists" %]
     The group [% name FILTER html %] already exists.
@@ -618,6 +624,19 @@
     In order to delete this group, you first have to change the
     [%+ param FILTER html %] to make [% attr FILTER html %] point to another group.
 
+
+  [% ELSIF error == "group_invalid_removal" %]
+    You tried to remove [% terms.bug %] [%+ bug.id FILTER html %]
+    from group id [% group_id FILTER html %], but [% terms.bugs %] in the 
+    '[% product.name FILTER html %]' product can not be removed from that
+    group.
+    
+  [% ELSIF error == "group_invalid_restriction" %]
+    You tried to restrict [% terms.bug %] [%+ bug.id FILTER html %] to
+    to group id [% group_id FILTER html %], but [% terms.bugs %] in the
+    '[% product.name FILTER html %]' product can not be restricted to
+    that group.
+
   [% ELSIF error == "group_not_specified" %]
     [% title = "Group not specified" %]
     No group was specified.