From f6e24acaeb1e9b0f7f4eafb27bc7adf81d5dd9e2 Mon Sep 17 00:00:00 2001 From: Max Kanat-Alexander Date: Mon, 22 Feb 2010 16:24:06 -0800 Subject: Bug 526184: Allow groups to be specified when creating bugs using email_in.pl or the WebService Bug.create method. r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 67 +++++++-------- Bugzilla/Product.pm | 107 +++++++++++++++++++++--- Bugzilla/User.pm | 26 ++++-- Bugzilla/WebService/Bug.pm | 13 +++ email_in.pl | 20 ----- enter_bug.cgi | 66 ++------------- post_bug.cgi | 9 +- template/en/default/bug/create/create.html.tmpl | 18 ++-- template/en/default/filterexceptions.pl | 1 - 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 # Frédéric Buclin -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 +=item C + +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 objects with +an extra hash keys set, C, which is true if the group +is set to Default for the currently-logged-in user. + +=item C + +Tells you what groups are mandatory for bugs in this product, for the +currently-logged-in user. Returns an arrayref of C objects. + +=item C =over =item B -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 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 set the Mandatory group on the bug.) =item B -C<$user> - The user who you want to check. +=over -=item B An arrayref of C objects. +=item C<$group> - Either a numeric group id or a L object. =back +=item B + +C<1> if the group is valid in this product, C<0> otherwise. + +=back + + =item C =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 + +This returns an C 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 statement, which defaults to C if not specified. + =item C 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 (array) - An array of usernames to CC on this bug. +=item C (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 as an empty array.) + =item C (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 were actually B, due to a bug in Bugzilla. +=item The C 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'); - [% IF group.size %] + [% IF product.groups_available.size %]  
- 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 %]:
@@ -623,12 +624,13 @@ TUI_hide_default('expert_fields');
- [% FOREACH g = group %] -      - -
+ [% FOREACH group = product.groups_available %] + +
[% END %] 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', -- cgit v1.2.3-24-g4f1b