From a404e6c3e3d777f09ef2ab8100ad01a20a3c885c Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Sat, 12 Jan 2008 22:23:10 +0000 Subject: Bug 401965: Move groups updating from process_bug.cgi to Bugzilla::Bug Patch By Max Kanat-Alexander r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 113 ++++++++++++++++++++- Bugzilla/Product.pm | 70 +++++++++++++ process_bug.cgi | 105 ++++--------------- template/en/default/bug/edit.html.tmpl | 5 + .../bug/process/verify-new-product.html.tmpl | 2 + 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 + +=over + +=item B + +Tells you what groups are mandatory for bugs in this product. + +=item B + +C<$user> - The user who you want to check. + +=item B An arrayref of C objects. + +=back + +=item C + +=over + +=item B + +Returns an arrayref of L objects, representing groups +that bugs could validly be restricted to within this product. Used mostly +by L to assure that you're adding valid groups to a bug. + +B: 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 these +groups, in this product. + +=item B (none) + +=item B An arrayref of L objects. + +=back =item C 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 %] + + [% END %]

+ + These groups are optional. You can decide to restrict [% terms.bugs %] to one or more of the following groups:
[% FOREACH group = optional_groups %] +