Bug 384664: Make keywords use Bugzilla::Bug in process_bug

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=LpSolit

2 files changed, 140 insertions, 120 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 56018cebd..10c595d94 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -388,7 +388,7 @@ sub run_create_validators {
 
     $params->{version} = $class->_check_version($product, $params->{version});
 
-    $params->{keywords} = $class->_check_keywords($product, $params->{keywords});
+    $params->{keywords} = $class->_check_keywords($params->{keywords}, $product);
 
     $params->{groups} = $class->_check_groups($product,
         $params->{groups});
@@ -496,8 +496,8 @@ sub update_cc {
    
     # 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);
+        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);
     }
@@ -505,6 +505,45 @@ sub update_cc {
     return ($removed_users, $added_users);
 }
 
+# XXX Temporary hack until all of process_bug uses update()
+sub update_keywords {
+    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_ids = map { $_->id } @{$old_bug->keyword_objects};
+    my @new_ids = map { $_->id } @{$self->keyword_objects};
+
+    my ($removed, $added) = diff_arrays(\@old_ids, \@new_ids);
+
+    if (scalar @$removed) {
+        $dbh->do('DELETE FROM keywords WHERE bug_id = ? AND keywordid IN ('
+                 . join(',', @$removed) . ')', undef, $self->id);
+    }
+    foreach my $keyword_id (@$added) {
+        $dbh->do('INSERT INTO keywords (bug_id, keywordid) VALUES (?,?)',
+                 undef, $self->id, $keyword_id);
+    }
+    
+    $dbh->do('UPDATE bugs SET keywords = ? WHERE bug_id = ?', undef,
+             $self->keywords, $self->id);
+
+    my $removed_keywords = Bugzilla::Keyword->new_from_list($removed);
+    my $added_keywords   = Bugzilla::Keyword->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 {$_->name} @$removed_keywords));
+        my $added_names   = join(', ', (map {$_->name} @$added_keywords));
+        LogActivityEntry($self->id, "keywords", $removed_names,
+                         $added_names, Bugzilla->user->id, $delta_ts);
+    }
+
+    return [$removed_keywords, $added_keywords];
+}
+
 # This is the correct way to delete bugs from the DB.
 # No bug should be deleted from anywhere else except from here.
 #
@@ -626,9 +665,7 @@ sub _check_bug_status {
 
     my @valid_statuses;
     if (ref $invocant) {
-        $invocant->{'prod_obj'} ||= 
-            new Bugzilla::Product({name => $invocant->product});
-        $product = $invocant->{'prod_obj'};
+        $product = $invocant->product_obj;
         @valid_statuses = map { $_->name } @{$invocant->status->can_change_to};
     }
     else {
@@ -846,11 +883,15 @@ sub _check_groups {
 }
 
 sub _check_keywords {
-    my ($invocant, $product, $keyword_string) = @_;
+    my ($invocant, $keyword_string, $product) = @_;
     $keyword_string = trim($keyword_string);
-    return [] if (!$keyword_string
-                  || !Bugzilla->user->in_group('editbugs', $product->id));
-
+    return [] if !$keyword_string;
+    
+    # On creation, only editbugs users can set keywords.
+    if (!ref $invocant) {
+        return [] if !Bugzilla->user->in_group('editbugs', $product->id);
+    }
+    
     my %keywords;
     foreach my $keyword (split(/[\s,]+/, $keyword_string)) {
         next unless $keyword;
@@ -1068,6 +1109,8 @@ sub set_status {
 # "Add/Remove" Methods #
 ########################
 
+# These are in alphabetical order by field name.
+
 # 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 {
@@ -1142,6 +1185,57 @@ sub add_comment {
     push(@{$self->{added_comments}}, $add_comment);
 }
 
+# There was a lot of duplicate code when I wrote this as three separate
+# functions, so I just combined them all into one. This is also easier for
+# process_bug to use.
+sub modify_keywords {
+    my ($self, $keywords, $action) = @_;
+    
+    $action ||= "makeexact";
+    if (!grep($action eq $_, qw(add delete makeexact))) {
+        $action = "makeexact";
+    }
+    
+    $keywords = $self->_check_keywords($keywords);
+
+    my (@result, $any_changes);
+    if ($action eq 'makeexact') {
+        @result = @$keywords;
+        # Check if anything was added or removed.
+        my @old_ids = map { $_->id } @{$self->keyword_objects};
+        my @new_ids = map { $_->id } @result;
+        my ($removed, $added) = diff_arrays(\@old_ids, \@new_ids);
+        $any_changes = scalar @$removed || scalar @$added;
+    }
+    else {
+        # We're adding or deleting specific keywords.
+        my %keys = map {$_->id => $_} @{$self->keyword_objects};
+        if ($action eq 'add') {
+            $keys{$_->id} = $_ foreach @$keywords;
+        }
+        else {
+            delete $keys{$_->id} foreach @$keywords;
+        }
+        @result = values %keys;
+        $any_changes = scalar @$keywords;
+    }
+    # Make sure we retain the sort order.
+    @result = sort {lc($a->name) cmp lc($b->name)} @result;
+    
+    if ($any_changes) {
+        my $privs;
+        my $new = join(', ', (map {$_->name} @result));
+        my $check = $self->check_can_change_field('keywords', 0, 1, \$privs)
+            || ThrowUserError('illegal_change', { field    => 'keywords',
+                                                  oldvalue => $self->keywords,
+                                                  newvalue => $new,
+                                                  privs    => $privs });
+    }
+
+    $self->{'keyword_objects'} = \@result;
+    return $any_changes;
+}
+
 #####################################################################
 # Instance Accessors
 #####################################################################
@@ -1339,20 +1433,20 @@ sub isunconfirmed {
 
 sub keywords {
     my ($self) = @_;
-    return $self->{'keywords'} if exists $self->{'keywords'};
-    return () if $self->{'error'};
+    return join(', ', (map { $_->name } @{$self->keyword_objects}));
+}
 
-    my $dbh = Bugzilla->dbh;
-    my $list_ref = $dbh->selectcol_arrayref(
-         "SELECT keyworddefs.name
-            FROM keyworddefs, keywords
-           WHERE keywords.bug_id = ?
-             AND keyworddefs.id = keywords.keywordid
-        ORDER BY keyworddefs.name",
-        undef, ($self->bug_id));
+# XXX At some point, this should probably replace the normal "keywords" sub.
+sub keyword_objects {
+    my $self = shift;
+    return $self->{'keyword_objects'} if defined $self->{'keyword_objects'};
+    return [] if $self->{'error'};
 
-    $self->{'keywords'} = join(', ', @$list_ref);
-    return $self->{'keywords'};
+    my $dbh = Bugzilla->dbh;
+    my $ids = $dbh->selectcol_arrayref(
+         "SELECT keywordid FROM keywords WHERE bug_id = ?", undef, $self->id);
+    $self->{'keyword_objects'} = Bugzilla::Keyword->new_from_list($ids);
+    return $self->{'keyword_objects'};
 }
 
 sub longdescs {
@@ -1368,8 +1462,7 @@ sub milestoneurl {
     return $self->{'milestoneurl'} if exists $self->{'milestoneurl'};
     return '' if $self->{'error'};
 
-    $self->{'prod_obj'} ||= new Bugzilla::Product({name => $self->product});
-    $self->{'milestoneurl'} = $self->{'prod_obj'}->milestone_url;
+    $self->{'milestoneurl'} = $self->product_obj->milestone_url;
     return $self->{'milestoneurl'};
 }
 
@@ -1383,6 +1476,14 @@ sub product {
     return $self->{product};
 }
 
+# XXX This should eventually replace the "product" subroutine.
+sub product_obj {
+    my $self = shift;
+    return {} if $self->{error};
+    $self->{prod_obj} ||= new Bugzilla::Product($self->{product_id});
+    return $self->{prod_obj};
+}
+
 sub qa_contact {
     my ($self) = @_;
     return $self->{'qa_contact'} if exists $self->{'qa_contact'};
@@ -1443,10 +1544,8 @@ sub use_votes {
     my ($self) = @_;
     return 0 if $self->{'error'};
 
-    $self->{'prod_obj'} ||= new Bugzilla::Product({name => $self->product});
-
     return Bugzilla->params->{'usevotes'} 
-           && $self->{'prod_obj'}->votes_per_user > 0;
+           && $self->product_obj->votes_per_user > 0;
 }
 
 sub groups {
@@ -1549,7 +1648,6 @@ sub choices {
     return {} if $self->{'error'};
 
     $self->{'choices'} = {};
-    $self->{prod_obj} ||= new Bugzilla::Product({name => $self->product});
 
     my @prodlist = map {$_->name} @{Bugzilla->user->get_enterable_products};
     # The current product is part of the popup, even if new bugs are no longer
@@ -1571,9 +1669,9 @@ sub choices {
        'op_sys'       => get_legal_field_values('op_sys'),
        'bug_status'   => get_legal_field_values('bug_status'),
        'resolution'   => \@res,
-       'component'    => [map($_->name, @{$self->{prod_obj}->components})],
-       'version'      => [map($_->name, @{$self->{prod_obj}->versions})],
-       'target_milestone' => [map($_->name, @{$self->{prod_obj}->milestones})],
+       'component'    => [map($_->name, @{$self->product_obj->components})],
+       'version'      => [map($_->name, @{$self->product_obj->versions})],
+       'target_milestone' => [map($_->name, @{$self->product_obj->milestones})],
       };
 
     return $self->{'choices'};
diff --git a/process_bug.cgi b/process_bug.cgi
index 4b5d0ec54..253de665f 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -882,34 +882,18 @@ $requiremilestone = $vars->{requiremilestone};
 DuplicateUserConfirm($bug, $duplicate) if $vars->{DuplicateUserConfirm};
 _remove_remaining_time() if $vars->{remove_remaining_time};
 
-my @keywordlist;
-my %keywordseen;
-
+my $any_keyword_changes;
 if (defined $cgi->param('keywords')) {
-    foreach my $keyword (split(/[\s,]+/, $cgi->param('keywords'))) {
-        if ($keyword eq '') {
-            next;
-        }
-        my $keyword_obj = new Bugzilla::Keyword({name => $keyword});
-        if (!$keyword_obj) {
-            ThrowUserError("unknown_keyword",
-                           { keyword => $keyword });
-        }
-        if (!$keywordseen{$keyword_obj->id}) {
-            push(@keywordlist, $keyword_obj->id);
-            $keywordseen{$keyword_obj->id} = 1;
-        }
+    foreach my $b (@bug_objects) {
+        my $return =
+            $b->modify_keywords(scalar $cgi->param('keywords'),
+                                scalar $cgi->param('keywordaction'));
+        $any_keyword_changes ||= $return;
     }
 }
 
-my $keywordaction = $cgi->param('keywordaction') || "makeexact";
-if (!grep($keywordaction eq $_, qw(add delete makeexact))) {
-    $keywordaction = "makeexact";
-}
-
 if ($::comma eq ""
-    && (!Bugzilla::Keyword::keyword_count()
-        || (0 == @keywordlist && $keywordaction ne "makeexact"))
+    && !$any_keyword_changes
     && defined $cgi->param('masscc') && ! $cgi->param('masscc')
     ) {
     if (!defined $cgi->param('comment') || $cgi->param('comment') =~ /^\s*$/) {
@@ -1184,25 +1168,6 @@ foreach my $id (@idlist) {
         }
     }
     
-    # When editing multiple bugs, users can specify a list of keywords to delete
-    # from bugs.  If the list matches the current set of keywords on those bugs,
-    # Bug::check_can_change_field will fail to check permissions because it thinks
-    # the list hasn't changed. To fix that, we have to call Bug::check_can_change_field
-    # again with old!=new if the keyword action is "delete" and old=new.
-    if ($keywordaction eq "delete"
-        && defined $cgi->param('keywords')
-        && length(@keywordlist) > 0
-        && $cgi->param('keywords') eq $oldhash{keywords}
-        && !$old_bug_obj->check_can_change_field("keywords", "old is not", "equal to new",
-                                                 \$PrivilegesRequired))
-    {
-        $vars->{'oldvalue'} = $oldhash{keywords};
-        $vars->{'newvalue'} = "no keywords";
-        $vars->{'field'} = "keywords";
-        $vars->{'privs'} = $PrivilegesRequired;
-        ThrowUserError("illegal_change", $vars);
-    }
-
     $oldhash{'product'} = $old_bug_obj->product;
     if (!Bugzilla->user->can_edit_product($oldhash{'product_id'})) {
         ThrowUserError("product_edit_denied",
@@ -1287,49 +1252,8 @@ foreach my $id (@idlist) {
         $bug_changed = 1;
     }
 
-    if (Bugzilla::Keyword::keyword_count() 
-        && defined $cgi->param('keywords')) 
-    {
-        # There are three kinds of "keywordsaction": makeexact, add, delete.
-        # For makeexact, we delete everything, and then add our things.
-        # For add, we delete things we're adding (to make sure we don't
-        # end up having them twice), and then we add them.
-        # For delete, we just delete things on the list.
-        my $changed = 0;
-        if ($keywordaction eq "makeexact") {
-            $dbh->do(q{DELETE FROM keywords WHERE bug_id = ?},
-                     undef, $id);
-            $changed = 1;
-        }
-        my $sth_delete = $dbh->prepare(q{DELETE FROM keywords
-                                               WHERE bug_id = ?
-                                                 AND keywordid = ?});
-        my $sth_insert =
-            $dbh->prepare(q{INSERT INTO keywords (bug_id, keywordid)
-                                 VALUES (?, ?)});
-        foreach my $keyword (@keywordlist) {
-            if ($keywordaction ne "makeexact") {
-                $sth_delete->execute($id, $keyword);
-                $changed = 1;
-            }
-            if ($keywordaction ne "delete") {
-                $sth_insert->execute($id, $keyword);
-                $changed = 1;
-            }
-        }
-        if ($changed) {
-            my $list = $dbh->selectcol_arrayref(
-                q{SELECT keyworddefs.name
-                    FROM keyworddefs
-              INNER JOIN keywords 
-                      ON keyworddefs.id = keywords.keywordid
-                   WHERE keywords.bug_id = ?
-                ORDER BY keyworddefs.name},
-                undef, $id);
-            $dbh->do("UPDATE bugs SET keywords = ? WHERE bug_id = ?",
-                     undef, join(', ', @$list), $id);
-        }
-    }
+    $bug_objects{$id}->update_keywords($timestamp);
+    
     $query .= " WHERE bug_id = ?";
     push(@bug_values, $id);
 
@@ -1546,10 +1470,8 @@ foreach my $id (@idlist) {
                 $origQaContact = $old;
             }
 
-            # If this is the keyword field, only record the changes, not everything.
-            if ($col eq 'keywords') {
-                ($old, $new) = diff_strings($old, $new);
-            }
+            # update_keywords does this for us already.
+            next if ($col eq 'keywords');
 
             if ($col eq 'product') {
                 # If some votes have been removed, RemoveVotes() returns