Bug 526184: Allow groups to be specified when creating bugs using email_in.pl

or the WebService Bug.create method.
r=LpSolit, a=LpSolit

9 files changed, 175 insertions, 152 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index ad272af22..4a90c1ce7 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -497,8 +497,8 @@ sub create {
     # Add the group restrictions
     my $sth_group = $dbh->prepare(
         'INSERT INTO bug_group_map (bug_id, group_id) VALUES (?, ?)');
-    foreach my $group_id (@$groups) {
-        $sth_group->execute($bug->bug_id, $group_id);
+    foreach my $group (@$groups) {
+        $sth_group->execute($bug->bug_id, $group->id);
     }
 
     $dbh->do('UPDATE bugs SET creation_ts = ? WHERE bug_id = ?', undef,
@@ -597,8 +597,7 @@ sub run_create_validators {
 
     $params->{keywords} = $class->_check_keywords($params->{keywords}, $product);
 
-    $params->{groups} = $class->_check_groups($product,
-        $params->{groups});
+    $params->{groups} = $class->_check_groups($params->{groups}, $product);
 
     my $component = $class->_check_component($params->{component}, $product);
     $params->{component_id} = $component->id;
@@ -1388,48 +1387,38 @@ sub _check_estimated_time {
 }
 
 sub _check_groups {
-    my ($invocant, $product, $group_ids) = @_;
-
-    my $user = Bugzilla->user;
+    my ($invocant, $group_names, $product) = @_;
 
     my %add_groups;
-    my $controls = $product->group_controls;
-
-    foreach my $id (@$group_ids) {
-        my $group = new Bugzilla::Group($id)
-            || ThrowUserError("invalid_group_ID");
-
-        # This can only happen if somebody hacked the enter_bug form.
-        ThrowCodeError("inactive_group", { name => $group->name })
-            unless $group->is_active;
-
-        my $membercontrol = $controls->{$id}
-                            && $controls->{$id}->{membercontrol};
-        my $othercontrol  = $controls->{$id} 
-                            && $controls->{$id}->{othercontrol};
-        
-        my $permit = ($membercontrol && $user->in_group($group->name))
-                     || $othercontrol;
 
-        $add_groups{$id} = 1 if $permit;
+    # In email or WebServices, when the "groups" item actually 
+    # isn't specified, then just add the default groups.
+    if (!defined $group_names) {
+        my $available = $product->groups_available;
+        foreach my $group (@$available) {
+            $add_groups{$group->id} = $group if $group->{is_default};
+        }
     }
+    else {
+        # Allow a comma-separated list, for email_in.pl.
+        $group_names = [map { trim($_) } split(',', $group_names)]
+            if !ref $group_names;
 
-    foreach my $id (keys %$controls) {
-        next unless $controls->{$id}->{'group'}->is_active;
-        my $membercontrol = $controls->{$id}->{membercontrol} || 0;
-        my $othercontrol  = $controls->{$id}->{othercontrol}  || 0;
-
-        # Add groups required
-        if ($membercontrol == CONTROLMAPMANDATORY
-            || ($othercontrol == CONTROLMAPMANDATORY
-                && !$user->in_group_id($id))) 
-        {
-            # User had no option, bug needs to be in this group.
-            $add_groups{$id} = 1;
+        # First check all the groups they chose to set.
+        foreach my $name (@$group_names) {
+            # We don't want to expose the existence or non-existence of 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);
+            $add_groups{$group->id} = $group;
         }
     }
 
-    my @add_groups = keys %add_groups;
+    # Now enforce mandatory groups.
+    $add_groups{$_->id} = $_ foreach @{ $product->groups_mandatory };
+
+    my @add_groups = values %add_groups;
     return \@add_groups;
 }
 
@@ -2099,7 +2088,7 @@ sub set_product {
         }
     
         # Make sure the bug is in all the mandatory groups for the new product.
-        foreach my $group (@{$product->groups_mandatory_for(Bugzilla->user)}) {
+        foreach my $group (@{$product->groups_mandatory}) {
             $self->add_group($group);
         }
     }
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index 46e91aa65..5c4057595 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -15,9 +15,9 @@
 # Contributor(s): Tiago R. Mello <timello@async.com.br>
 #                 Frédéric Buclin <LpSolit@gmail.com>
 
-use strict;
-
 package Bugzilla::Product;
+use strict;
+use base qw(Bugzilla::Field::ChoiceInterface Bugzilla::Object);
 
 use Bugzilla::Constants;
 use Bugzilla::Util;
@@ -32,7 +32,7 @@ use Bugzilla::Mailer;
 use Bugzilla::Series;
 use Bugzilla::Hook;
 
-use base qw(Bugzilla::Field::ChoiceInterface Bugzilla::Object);
+use Scalar::Util qw(blessed);
 
 use constant DEFAULT_CLASSIFICATION_ID => 1;
 
@@ -256,6 +256,9 @@ sub update {
                 }
             }
         }
+
+        delete $self->{groups_available};
+        delete $self->{groups_mandatory};
     }
     $dbh->bz_commit_transaction();
     # Changes have been committed.
@@ -611,9 +614,53 @@ sub group_controls {
     return $self->{group_controls};
 }
 
-sub groups_mandatory_for {
-    my ($self, $user) = @_;
-    my $groups = $user->groups_as_string;
+sub groups_available {
+    my ($self) = @_;
+    return $self->{groups_available} if defined $self->{groups_available};
+    my $dbh = Bugzilla->dbh;
+    my $shown = CONTROLMAPSHOWN;
+    my $default = CONTROLMAPDEFAULT;
+    my %member_groups = @{ $dbh->selectcol_arrayref(
+        "SELECT group_id, membercontrol
+           FROM group_control_map
+                INNER JOIN groups ON group_control_map.group_id = groups.id
+          WHERE isbuggroup = 1 AND isactive = 1 AND product_id = ?
+                AND (membercontrol = $shown OR membercontrol = $default)
+                AND " . Bugzilla->user->groups_in_sql(),
+        {Columns=>[1,2]}, $self->id) };
+    # We don't need to check the group membership here, because we only
+    # add these groups to the list below if the group isn't already listed
+    # for membercontrol.
+    my %other_groups = @{ $dbh->selectcol_arrayref(
+        "SELECT group_id, othercontrol
+           FROM group_control_map
+                INNER JOIN groups ON group_control_map.group_id = groups.id
+          WHERE isbuggroup = 1 AND isactive = 1 AND product_id = ?
+                AND (othercontrol = $shown OR othercontrol = $default)", 
+        {Columns=>[1,2]}, $self->id) };
+
+    # If the user is a member, then we use the membercontrol value.
+    # Otherwise, we use the othercontrol value.
+    my %all_groups = %member_groups;
+    foreach my $id (keys %other_groups) {
+        if (!defined $all_groups{$id}) {
+            $all_groups{$id} = $other_groups{$id};
+        }
+    }
+
+    my $available = Bugzilla::Group->new_from_list([keys %all_groups]);
+    foreach my $group (@$available) {
+        $group->{is_default} = 1 if $all_groups{$group->id} == $default;
+    }
+
+    $self->{groups_available} = $available;
+    return $self->{groups_available};
+}
+
+sub groups_mandatory {
+    my ($self) = @_;
+    return $self->{groups_mandatory} if $self->{groups_mandatory};
+    my $groups = Bugzilla->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
@@ -625,7 +672,20 @@ sub groups_mandatory_for {
                      OR (othercontrol = $mandatory
                          AND group_id NOT IN ($groups)))",
         undef, $self->id);
-    return Bugzilla::Group->new_from_list($ids);
+    $self->{groups_mandatory} = Bugzilla::Group->new_from_list($ids);
+    return $self->{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 {
+    my ($self, $group) = @_;
+    my $group_id = blessed($group) ? $group->id : $group;
+    my $is_mandatory = grep { $group_id == $_->id } 
+                            @{ $self->groups_mandatory };
+    my $is_available = grep { $group_id == $_->id }
+                            @{ $self->groups_available };
+    return ($is_mandatory or $is_available) ? 1 : 0;
 }
 
 sub groups_valid {
@@ -837,22 +897,47 @@ below.
               a Bugzilla::Group object and the properties of group
               relative to the product.
 
-=item C<groups_mandatory_for>
+=item C<groups_available>
+
+Tells you what groups are set to Default or Shown for the 
+currently-logged-in user (taking into account both OtherControl and
+MemberControl). Returns an arrayref of L<Bugzilla::Group> objects with
+an extra hash keys set, C<is_default>, which is true if the group
+is set to Default for the currently-logged-in user.
+
+=item C<groups_mandatory>
+
+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>
 
 =over
 
 =item B<Description>
 
-Tells you what groups are mandatory for bugs in this product.
+Tells you whether or not the currently-logged-in user can set a group
+on a bug (whether or not they match the MemberControl/OtherControl
+settings for a group in this product). Groups that are C<Mandatory> for
+the currently-loggeed-in user are also acceptable since from Bugzilla's
+perspective, there's no problem with "setting" a Mandatory group on
+a bug. (In fact, the user I<must> set the Mandatory group on the bug.)
 
 =item B<Params>
 
-C<$user> - The user who you want to check.
+=over
 
-=item B<Returns> An arrayref of C<Bugzilla::Group> objects.
+=item C<$group> - Either a numeric group id or a L<Bugzilla::Group> object.
 
 =back
 
+=item B<Returns>
+
+C<1> if the group is valid in this product, C<0> otherwise.
+
+=back
+
+
 =item C<groups_valid>
 
 =over
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 7dd86f301..5c6209811 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -334,7 +334,7 @@ sub queries_subscribed {
                 ON ngm.namedquery_id = lif.namedquery_id
           WHERE lif.user_id = ? 
                 AND lif.namedquery_id NOT IN ($query_id_string)
-                AND ngm.group_id IN (" . $self->groups_as_string . ")",
+                AND " . $self->groups_in_sql,
           undef, $self->id);
     require Bugzilla::Search::Saved;
     $self->{queries_subscribed} =
@@ -353,7 +353,7 @@ sub queries_available {
 
     my $avail_query_ids = Bugzilla->dbh->selectcol_arrayref(
         'SELECT namedquery_id FROM namedquery_group_map
-          WHERE group_id IN (' . $self->groups_as_string . ")
+          WHERE '  . $self->groups_in_sql . "
                 AND namedquery_id NOT IN ($query_id_string)");
     require Bugzilla::Search::Saved;
     $self->{queries_available} =
@@ -464,6 +464,14 @@ sub groups_as_string {
     return scalar(@ids) ? join(',', @ids) : '-1';
 }
 
+sub groups_in_sql {
+    my ($self, $field) = @_;
+    $field ||= 'group_id';
+    my @ids = map { $_->id } @{ $self->groups };
+    @ids = (-1) if !scalar @ids;
+    return Bugzilla->dbh->sql_in($field, \@ids);
+}
+
 sub bless_groups {
     my $self = shift;
 
@@ -524,7 +532,7 @@ sub in_group {
                               FROM group_control_map
                              WHERE product_id = ?
                                    AND $group != 0
-                                   AND group_id IN (" . $self->groups_as_string . ") " .
+                                   AND " . $self->groups_in_sql . ' ' .
                               $dbh->sql_limit(1),
                              undef, $product_id);
 
@@ -550,7 +558,7 @@ sub get_products_by_permission {
                           "SELECT DISTINCT product_id
                              FROM group_control_map
                             WHERE $group != 0
-                              AND group_id IN(" . $self->groups_as_string . ")");
+                              AND " . $self->groups_in_sql);
 
     # No need to go further if the user has no "special" privs.
     return [] unless scalar(@$product_ids);
@@ -898,10 +906,9 @@ sub visible_groups_direct {
     my $sth;
    
     if (Bugzilla->params->{'usevisibilitygroups'}) {
-        my $glist = $self->groups_as_string;
         $sth = $dbh->prepare("SELECT DISTINCT grantor_id
                                  FROM group_group_map
-                                WHERE member_id IN($glist)
+                                WHERE " . $self->groups_in_sql('member_id') . "
                                   AND grant_type=" . GROUP_VISIBLE);
     }
     else {
@@ -1957,6 +1964,13 @@ Returns a string containing a comma-separated list of numeric group ids.  If
 the user is not a member of any groups, returns "-1". This is most often used
 within an SQL IN() function.
 
+=item C<groups_in_sql>
+
+This returns an C<IN> clause for SQL, containing either all of the groups
+the user is in, or C<-1> if the user is in no groups. This takes one
+argument--the name of the SQL field that should be on the left-hand-side
+of the C<IN> statement, which defaults to C<group_id> if not specified.
+
 =item C<in_group($group_name, $product_id)>
 
 Determines whether or not a user is in the given group by name.
diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index 1d7b9f7d9..b38168602 100644
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -1802,6 +1802,15 @@ don't want it to be assigned to the component owner.
 
 =item C<cc> (array) - An array of usernames to CC on this bug.
 
+=item C<groups> (array) - An array of group names to put this
+bug into. You can see valid group names on the Permissions
+tab of the Preferences screen, or, if you are an administrator,
+in the Groups control panel. Note that invalid group names or
+groups that the bug can't be restricted to are silently ignored. If
+you don't specify this argument, then a bug will be added into
+all the groups that are set as being "Default" for this product. (If
+you want to avoid that, you should specify C<groups> as an empty array.)
+
 =item C<qa_contact> (username) - If this installation has QA Contacts
 enabled, you can set the QA Contact here if you don't want to use
 the component's default QA Contact.
@@ -1867,6 +1876,10 @@ in them. The error message will have more details.
 =item Before B<3.0.4>, parameters marked as B<Defaulted> were actually
 B<Required>, due to a bug in Bugzilla.
 
+=item The C<groups> argument was added in Bugzilla B<3.8>. Before
+Bugzilla 3.8, bugs were only added into Mandatory groups by this
+method.
+
 =back
 
 =back
diff --git a/email_in.pl b/email_in.pl
index 1f610f138..15dfcb728 100755
--- a/email_in.pl
+++ b/email_in.pl
@@ -154,26 +154,6 @@ sub post_bug {
         $fields->{$field} = undef if !exists $fields->{$field};
     }
 
-    # Restrict the bug to groups marked as Default.
-    # We let Bug->create throw an error if the product is
-    # not accessible, to throw the correct message.
-    $fields->{product} = '' if !defined $fields->{product};
-    my $product = new Bugzilla::Product({ name => $fields->{product} });
-    if ($product) {
-        my @gids;
-        my $controls = $product->group_controls;
-        foreach my $gid (keys %$controls) {
-            if (($controls->{$gid}->{membercontrol} == CONTROLMAPDEFAULT
-                 && $user->in_group_id($gid))
-                || ($controls->{$gid}->{othercontrol} == CONTROLMAPDEFAULT
-                    && !$user->in_group_id($gid)))
-            {
-                push(@gids, $gid);
-            }
-        }
-        $fields->{groups} = \@gids;
-    }
-
     my ($retval, $non_conclusive_fields) =
       Bugzilla::User::match_field({
         'assigned_to'   => { 'type' => 'single' },
diff --git a/enter_bug.cgi b/enter_bug.cgi
index a4ed7350e..79116d592 100755
--- a/enter_bug.cgi
+++ b/enter_bug.cgi
@@ -555,66 +555,14 @@ else {
     $default{'bug_status'} = ($status[0] ne 'UNCONFIRMED') ? $status[0] : $status[1];
 }
 
-my $grouplist = $dbh->selectall_arrayref(
-                  q{SELECT DISTINCT groups.id, groups.name, groups.description,
-                                    membercontrol, othercontrol
-                      FROM groups
-                 LEFT JOIN group_control_map
-                        ON group_id = id AND product_id = ?
-                     WHERE isbuggroup != 0 AND isactive != 0
-                  ORDER BY description}, undef, $product->id);
-
-my @groups;
-
-foreach my $row (@$grouplist) {
-    my ($id, $groupname, $description, $membercontrol, $othercontrol) = @$row;
-    # Only include groups if the entering user will have an option.
-    next if ((!$membercontrol) 
-               || ($membercontrol == CONTROLMAPNA) 
-               || ($membercontrol == CONTROLMAPMANDATORY)
-               || (($othercontrol != CONTROLMAPSHOWN) 
-                    && ($othercontrol != CONTROLMAPDEFAULT)
-                    && (!Bugzilla->user->in_group($groupname)))
-             );
-    my $check;
-
-    # If this is a cloned bug, 
-    # AND the product for this bug is the same as for the original
-    #   THEN set a group's checkbox if the original also had it on
-    # ELSE IF this is a bookmarked template
-    #   THEN set a group's checkbox if was set in the bookmark
-    # ELSE
-    #   set a groups's checkbox based on the group control map
-    #
-    if ( ($cloned_bug_id) &&
-         ($product->name eq $cloned_bug->product ) ) {
-        foreach my $i (0..(@{$cloned_bug->groups} - 1) ) {
-            if ($cloned_bug->groups->[$i]->{'bit'} == $id) {
-                $check = $cloned_bug->groups->[$i]->{'ison'};
-            }
-        }
-    }
-    elsif(formvalue("maketemplate") ne "") {
-        $check = formvalue("bit-$id", 0);
-    }
-    else {
-        # Checkbox is checked by default if $control is a default state.
-        $check = (($membercontrol == CONTROLMAPDEFAULT)
-                 || (($othercontrol == CONTROLMAPDEFAULT)
-                      && (!Bugzilla->user->in_group($groupname))));
-    }
-
-    my $group = 
-    {
-        'bit' => $id , 
-        'checked' => $check , 
-        'description' => $description 
-    };
-
-    push @groups, $group;        
+my @groups = $cgi->param('groups');
+if ($cloned_bug) {
+    my @clone_groups = map { $_->name } @{ $cloned_bug->groups_in };
+    # It doesn't matter if there are duplicate names, since all we check
+    # for in the template is whether or not the group is set.
+    push(@groups, @clone_groups);
 }
-
-$vars->{'group'} = \@groups;
+$default{'groups'} = \@groups;
 
 Bugzilla::Hook::process('enter_bug_entrydefaultvars', { vars => $vars });
 
diff --git a/post_bug.cgi b/post_bug.cgi
index e8f9acc01..5a1da173f 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -103,13 +103,6 @@ if (defined $cgi->param('maketemplate')) {
 
 umask 0;
 
-# Group Validation
-my @selected_groups;
-foreach my $group (grep(/^bit-\d+$/, $cgi->param())) {
-    $group =~ /^bit-(\d+)$/;
-    push(@selected_groups, $1);
-}
-
 # The format of the initial comment can be structured by adding fields to the
 # enter_bug template and then referencing them in the comment template.
 my $comment;
@@ -158,7 +151,7 @@ foreach my $field (@bug_fields) {
     $bug_params{$field} = $cgi->param($field);
 }
 $bug_params{'cc'}          = [$cgi->param('cc')];
-$bug_params{'groups'}      = \@selected_groups;
+$bug_params{'groups'}      = [$cgi->param('groups')];
 $bug_params{'comment'}     = $comment;
 
 my @multi_selects = grep {$_->type == FIELD_TYPE_MULTI_SELECT && $_->enter_bug}
diff --git a/template/en/default/bug/create/create.html.tmpl b/template/en/default/bug/create/create.html.tmpl
index d82587672..fe5f130d8 100644
--- a/template/en/default/bug/create/create.html.tmpl
+++ b/template/en/default/bug/create/create.html.tmpl
@@ -607,13 +607,14 @@ TUI_hide_default('expert_fields');
 </tbody>
 
 <tbody class="expert_fields">
-  [% IF group.size %]
+  [% IF product.groups_available.size %]
   <tr>
     <th>&nbsp;</th>
     <td colspan="3">
       <br>
         <strong>
-          Only users in all of the selected groups can view this [% terms.bug %]:
+          Only users in all of the selected groups can view this 
+          [%+ terms.bug %]:
         </strong>
       <br>
       <font size="-1">
@@ -623,12 +624,13 @@ TUI_hide_default('expert_fields');
       <br>
 
       <!-- Checkboxes -->
-      [% FOREACH g = group %]
-        &nbsp;&nbsp;&nbsp;&nbsp;
-        <input type="checkbox" id="bit-[% g.bit %]"
-          name="bit-[% g.bit %]" value="1"
-          [% " checked=\"checked\"" IF g.checked %]>
-          <label for="bit-[% g.bit %]">[% g.description FILTER html_light %]</label><br>
+      [% FOREACH group = product.groups_available %]
+        <input type="checkbox" id="groups_[% group.id FILTER html %]"
+               name="groups" value="[% group.name FILTER html %]"
+               [% ' checked="checked"' IF default.groups.contains(group.name)
+                                          OR group.is_default %]>
+        <label for="groups_[% group.id FILTER html %]">
+          [%- group.description FILTER html_light %]</label><br>
       [% END %]
     </td>
   </tr>
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index 66bb611a7..07bd4dead 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -316,7 +316,6 @@
 ],
 
 'bug/create/create.html.tmpl' => [
-  'g.bit',
   'sel.name',
   'sel.description',
   'cloned_bug_id',