diff options
| -rwxr-xr-x | Bugzilla/Bug.pm | 113 | ||||
| -rw-r--r-- | Bugzilla/Product.pm | 70 | ||||
| -rwxr-xr-x | process_bug.cgi | 105 | ||||
| -rw-r--r-- | template/en/default/bug/edit.html.tmpl | 5 | ||||
| -rw-r--r-- | template/en/default/bug/process/verify-new-product.html.tmpl | 2 | ||||
| -rw-r--r-- | template/en/default/global/user-error.html.tmpl | 19 |
6 files changed, 230 insertions, 84 deletions
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index bfd110ad0..e9274466d 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -498,7 +498,29 @@ 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 ##################################################################### diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm index 728bd0652..45c489973 100644 --- a/Bugzilla/Product.pm +++ b/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; @@ -285,6 +319,42 @@ below. Returns: A hash with group id as key and hash containing 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()> diff --git a/process_bug.cgi b/process_bug.cgi index 91d430aa9..dcea4ffba 100755 --- a/process_bug.cgi +++ b/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]; diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl index 1f20cdb52..c37f93838 100644 --- a/template/en/default/bug/edit.html.tmpl +++ b/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 %] diff --git a/template/en/default/bug/process/verify-new-product.html.tmpl b/template/en/default/bug/process/verify-new-product.html.tmpl index ee87ef818..d1ce9652b 100644 --- a/template/en/default/bug/process/verify-new-product.html.tmpl +++ b/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)) diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 2848ab5a4..46ed901a2 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/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. |