From ee2eb7de3865e97dd48030da6cf48606d95fe152 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Mon, 2 Jul 2007 09:16:08 +0000 Subject: Bug 384651: Make CC adding and removal use Bugzilla::Bug in process_bug Patch By Max Kanat-Alexander r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 79 ++++++++++++++++++++++++++++++++++++++ Bugzilla/User.pm | 13 +++++++ process_bug.cgi | 115 +++++++++++++------------------------------------------ 3 files changed, 119 insertions(+), 88 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index c3be53e96..56018cebd 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -471,6 +471,40 @@ sub update { return $changes; } +# XXX Temporary hack until all of process_bug uses update(). +sub update_cc { + my $self = shift; + + my $dbh = Bugzilla->dbh; + my $delta_ts = shift || $dbh->selectrow_array("SELECT NOW()"); + + my $old_bug = $self->new($self->id); + my @old_cc = map {$_->id} @{$old_bug->cc_users}; + my @new_cc = map {$_->id} @{$self->cc_users}; + my ($removed, $added) = diff_arrays(\@old_cc, \@new_cc); + + if (scalar @$removed) { + $dbh->do('DELETE FROM cc WHERE bug_id = ? AND who IN (' . + join(',', @$removed) . ')', undef, $self->id); + } + foreach my $user_id (@$added) { + $dbh->do('INSERT INTO cc (bug_id, who) VALUES (?,?)', + undef, $self->id, $user_id); + } + my $removed_users = Bugzilla::User->new_from_list($removed); + my $added_users = Bugzilla::User->new_from_list($added); + + # If any changes were found, record it in the activity log + if (scalar @$removed || scalar @$added) { + my $removed_names = join ', ', (map {$_->login} @$removed_users); + my $added_names = join ', ', (map {$_->login} @$added_users); + LogActivityEntry($self->id, "cc", $removed_names, $added_names, + Bugzilla->user->id, $delta_ts); + } + + return ($removed_users, $added_users); +} + # This is the correct way to delete bugs from the DB. # No bug should be deleted from anywhere else except from here. # @@ -1034,6 +1068,38 @@ sub set_status { # "Add/Remove" Methods # ######################## +# Accepts a User object or a username. Adds the user only if they +# don't already exist as a CC on the bug. +sub add_cc { + # XXX $product is a temporary hack until all of process_bug uses Bug + # objects for updating. + my ($self, $user_or_name, $product) = @_; + return if !$user_or_name; + my $user = ref $user_or_name ? $user_or_name + : Bugzilla::User::check($user_or_name); + + my $product_id = $product ? $product->id : $self->{product_id}; + if (Bugzilla->params->{strict_isolation} + && !$user->can_edit_product($product_id)) + { + ThrowUserError('invalid_user_group', { users => $user->login, + bug_id => $self->id }); + } + + my $cc_users = $self->cc_users; + push(@$cc_users, $user) if !grep($_->id == $user->id, @$cc_users); +} + +# Accepts a User object or a username. Removes the User if they exist +# in the list, but doesn't throw an error if they don't exist. +sub remove_cc { + my ($self, $user_or_name) = @_; + my $user = ref $user_or_name ? $user_or_name + : Bugzilla::User::check($user_or_name); + my $cc_users = $self->cc_users; + @$cc_users = grep { $_->id != $user->id } @$cc_users; +} + # $bug->add_comment("comment", {isprivate => 1, work_time => 10.5, # type => CMT_NORMAL, extra_data => $data}); sub add_comment { @@ -1185,6 +1251,19 @@ sub cc { return $self->{'cc'}; } +# XXX Eventually this will become the standard "cc" method used everywhere. +sub cc_users { + my $self = shift; + return $self->{'cc_users'} if exists $self->{'cc_users'}; + return [] if $self->{'error'}; + + my $dbh = Bugzilla->dbh; + my $cc_ids = $dbh->selectcol_arrayref( + 'SELECT who FROM cc WHERE bug_id = ?', undef, $self->id); + $self->{'cc_users'} = Bugzilla::User->new_from_list($cc_ids); + return $self->{'cc_users'}; +} + sub component { my ($self) = @_; return $self->{component} if exists $self->{component}; diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index a14549f84..f13b94fbf 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -131,6 +131,14 @@ sub new { return $class->SUPER::new(@_); } +sub check { + my ($username) = @_; + $username = trim($username); + my $user = new Bugzilla::User({ name => $username }) + || ThrowUserError('invalid_username', { name => $username }); + return $user; +} + sub update { my $self = shift; my $changes = $self->SUPER::update(@_); @@ -2072,6 +2080,11 @@ Params: login_name - B The login name for the new user. disable_mail - If 1, bug-related mail will not be sent to this user; if 0, mail will be sent depending on the user's email preferences. +=item C + +Takes a username as its only argument. Throws an error if there is no +user with that username. Returns a C object. + =item C Returns a boolean indicating whether or not the supplied username is diff --git a/process_bug.cgi b/process_bug.cgi index f0cd560cd..f2a174dee 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -27,6 +27,7 @@ # Frédéric Buclin # Lance Larsh # Akamai Technologies +# Max Kanat-Alexander # Implementation notes for this file: # @@ -107,7 +108,7 @@ sub comment_exists { # named "id_x" where "x" is the bug number. # For each bug being modified, make sure its ID is a valid bug number # representing an existing bug that the user is authorized to access. -my @idlist; +my (@idlist, @bug_objects); if (defined $cgi->param('id')) { my $id = $cgi->param('id'); ValidateBugID($id); @@ -116,12 +117,15 @@ if (defined $cgi->param('id')) { # lots of later code will need it, and will obtain it from there $cgi->param('id', $id); push @idlist, $id; + push(@bug_objects, new Bugzilla::Bug($id)); } else { foreach my $i ($cgi->param()) { if ($i =~ /^id_([1-9][0-9]*)/) { my $id = $1; ValidateBugID($id); push @idlist, $id; + # We do this until we have Bugzilla::Bug->new_from_list. + push(@bug_objects, new Bugzilla::Bug($id)); } } } @@ -130,7 +134,7 @@ if (defined $cgi->param('id')) { scalar(@idlist) || ThrowUserError("no_bugs_chosen"); # Build a bug object using the first bug id, for validations. -my $bug = new Bugzilla::Bug($idlist[0]); +my $bug = $bug_objects[0]; # Make sure form param 'dontchange' is defined so it can be compared to easily. $cgi->param('dontchange','') unless defined $cgi->param('dontchange'); @@ -613,7 +617,7 @@ if ($cgi->param('product') ne $cgi->param('dontchange')) { } my $component; -my (%cc_add, %cc_remove); +my (@cc_add, @cc_remove); if ($cgi->param('component') ne $cgi->param('dontchange')) { if (scalar(@newprod_ids) > 1) { @@ -634,7 +638,7 @@ if ($cgi->param('component') ne $cgi->param('dontchange')) { # NewCC must be defined or the code below won't insert # any CCs. $cgi->param('newcc') || $cgi->param('newcc', ""); - $cc_add{$cc->id} = $cc->login; + push(@cc_add, $cc->login); } } } @@ -710,13 +714,15 @@ if ( defined $cgi->param('id') && } } -# We need to check the addresses involved in a CC change before we touch any bugs. -# What we'll do here is formulate the CC data into two hashes of ID's involved -# in this CC change. Then those hashes can be used later on for the actual change. +# We need to check the addresses involved in a CC change before we touch +# any bugs. What we'll do here is formulate the CC data into two arrays of +# users involved in this CC change. Then those arrays can be used later +# on for the actual change. if (defined $cgi->param('newcc') || defined $cgi->param('addselfcc') || defined $cgi->param('removecc') || defined $cgi->param('masscc')) { + # If masscc is defined, then we came from buglist and need to either add or # remove cc's... otherwise, we came from bugform and may need to do both. my ($cc_add, $cc_remove) = ""; @@ -735,23 +741,15 @@ if (defined $cgi->param('newcc') } } - if ($cc_add) { - $cc_add =~ s/[\s,]+/ /g; # Change all delimiters to a single space - foreach my $person ( split(" ", $cc_add) ) { - my $pid = login_to_id($person, THROW_ERROR); - $cc_add{$pid} = $person; - } - } - if ($cgi->param('addselfcc')) { - $cc_add{$whoid} = $user->login; - } - if ($cc_remove) { - $cc_remove =~ s/[\s,]+/ /g; # Change all delimiters to a single space - foreach my $person ( split(" ", $cc_remove) ) { - my $pid = login_to_id($person, THROW_ERROR); - $cc_remove{$pid} = $person; - } - } + push(@cc_add, split(/[\s,]+/, $cc_add)) if $cc_add; + push(@cc_add, Bugzilla->user) if $cgi->param('addselfcc'); + + push(@cc_remove, split(/[\s,]+/, $cc_remove)) if $cc_remove; +} + +foreach my $b (@bug_objects) { + $b->remove_cc($_) foreach @cc_remove; + $b->add_cc($_, $product) foreach @cc_add; } # Store the new assignee and QA contact IDs (if any). This is the @@ -991,25 +989,6 @@ sub LogDependencyActivity { return 0; } -if (Bugzilla->params->{"strict_isolation"}) { - my @blocked_cc = (); - foreach my $pid (keys %cc_add) { - $usercache{$pid} ||= Bugzilla::User->new($pid); - my $cc_user = $usercache{$pid}; - foreach my $product_id (@newprod_ids) { - if (!$cc_user->can_edit_product($product_id)) { - push (@blocked_cc, $cc_user->login); - last; - } - } - } - if (scalar(@blocked_cc)) { - ThrowUserError("invalid_user_group", - {'users' => \@blocked_cc, - 'bug_id' => (scalar(@idlist) > 1) ? undef : $idlist[0]}); - } -} - if ($prod_changed && Bugzilla->params->{"strict_isolation"}) { my $sth_cc = $dbh->prepare("SELECT who FROM cc @@ -1061,6 +1040,8 @@ if ($prod_changed && Bugzilla->params->{"strict_isolation"}) { } +my %bug_objects = map {$_->id => $_} @bug_objects; + # This loop iterates once for each bug to be processed (i.e. all the # bugs selected when this script is called with multiple bugs selected # from buglist.cgi, or just the one bug when called from @@ -1432,51 +1413,9 @@ foreach my $id (@idlist) { $bug_changed = 1; } - my @ccRemoved = (); - if (defined $cgi->param('newcc') - || defined $cgi->param('addselfcc') - || defined $cgi->param('removecc') - || defined $cgi->param('masscc')) { - # Get the current CC list for this bug - my %oncc; - my $cc_list = $dbh->selectcol_arrayref( - q{SELECT who FROM cc WHERE bug_id = ?}, undef, $id); - foreach my $who (@$cc_list) { - $oncc{$who} = 1; - } + my ($cc_removed) = $bug_objects{$id}->update_cc($timestamp); + $cc_removed = [map {$_->login} @$cc_removed]; - my (@added, @removed) = (); - - my $sth_insert = $dbh->prepare(q{INSERT INTO cc (bug_id, who) - VALUES (?, ?)}); - foreach my $pid (keys %cc_add) { - # If this person isn't already on the cc list, add them - if (! $oncc{$pid}) { - $sth_insert->execute($id, $pid); - push (@added, $cc_add{$pid}); - $oncc{$pid} = 1; - } - } - my $sth_delete = $dbh->prepare(q{DELETE FROM cc - WHERE bug_id = ? AND who = ?}); - foreach my $pid (keys %cc_remove) { - # If the person is on the cc list, remove them - if ($oncc{$pid}) { - $sth_delete->execute($id, $pid); - push (@removed, $cc_remove{$pid}); - $oncc{$pid} = 0; - } - } - - # If any changes were found, record it in the activity log - if (scalar(@removed) || scalar(@added)) { - my $removed = join(", ", @removed); - my $added = join(", ", @added); - LogActivityEntry($id,"cc",$removed,$added,$whoid,$timestamp); - $bug_changed = 1; - } - @ccRemoved = @removed; - } # We need to send mail for dependson/blocked bugs if the dependencies # change or the status or resolution change. This var keeps track of that. @@ -1677,7 +1616,7 @@ foreach my $id (@idlist) { # all concerned users, including the bug itself, but also the # duplicated bug and dependent bugs, if any. - $vars->{'mailrecipients'} = { 'cc' => \@ccRemoved, + $vars->{'mailrecipients'} = { 'cc' => $cc_removed, 'owner' => $origOwner, 'qacontact' => $origQaContact, 'changer' => Bugzilla->user->login }; -- cgit v1.2.3-24-g4f1b