384664 - Make keywords use Bugzilla::Bug in process_bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

Now that CCs are done, the next-easiest thing to do is to make Keywords use the bug objects in process_bug.cgi.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Here we go! Works very nicely. :-)

Comment 2

[Max Kanat-Alexander]

Attachment: v1.1

Updated to apply against v3 of the CC bug.

Comment 3

[Max Kanat-Alexander]

Attachment: v1.2

I fixed the bitrot.

By the way, I forgot to mention it, but you'll notice I cleaned up the {prod_obj} stuff in Bugzilla::Bug in this patch.

Comment 4

[Frédéric Buclin]

Comment on attachment 269293 [details] [diff] [review]
v1.2

>diff -u process_bug.cgi process_bug.cgi

>@@ -108,7 +108,7 @@
>-my @idlist;
>+my (@idlist, @bug_objects);

>@@ -117,12 +117,15 @@
>+  push(@bug_objects, new Bugzilla::Bug($id));


>@@ -136,7 +139,7 @@
>-my (@idlist, @bug_objects);
>+my @idlist;

>@@ -145,15 +148,12 @@
>-  push(@bug_objects, new Bugzilla::Bug($id));


No idea how I'm supposed to read this patch. It contains blocks which undo what has been done above. Looks like 2 patches have been mixed.

Comment 5

[Max Kanat-Alexander]

Attachment: v1.3

Oh, weird! Sorry about that! Here's the right patch.

Comment 6

[Frédéric Buclin]

Comment on attachment 269767 [details] [diff] [review]
v1.3

>diff -u process_bug.cgi process_bug.cgi

>+    foreach my $b (@bug_objects) {
>+        $any_keyword_changes =
>+            $b->modify_keywords($cgi->param('keywords'),
>+                                scalar $cgi->param('keywordaction'));
>     }

$any_keyword_changes takes the value of the last bug being processed. With keywordaction = makeexact, if the returned value is 0 for the last bug, then you will incorrectly assume no changes have been made about keywords, even if there were some for other bugs.

Also, you should write |scalar $cgi->param()| as it's really a scalar.



>diff -u Bugzilla/Bug.pm Bugzilla/Bug.pm

>+sub update_keywords {

>+    foreach my $keyword_id (@$removed) {
>+        $dbh->do('DELETE FROM keywords WHERE bug_id = ? AND keywordid = ?',
>+                 undef, $self->id, $keyword_id);
>+    }

Either prepare the statement outside the foreach loop or write
|keywordid IN (...)|.


>+    foreach my $keyword_id (@$added) {
>+        $dbh->do('INSERT INTO keywords (bug_id, keywordid) VALUES (?,?)',
>+                 undef, $self->id, $keyword_id);
>+    }

Prepare the statement outside the foreach loop.


>+        my $removed_names = join ', ', (map {$_->name} @$removed_keywords);
>+        my $added_names   = join ', ', (map {$_->name} @$added_keywords);

Nit: please add parens when using join(). This is clearer.


>+    return [$removed_keywords, $added_keywords];

Be consistent with update_cc() and return an array instead of an arrayref.

>+# 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 {

I don't see why this comment is useful. Just remove it!


>+        my @all_changes = map { @$_ } diff_arrays(\@old_ids, \@new_ids);
>+        $any_changes = scalar @all_changes;

I don't like map {@$_}. You could as well write:
my ($removed, $added) = diff_arrays(\@old_ids, \@new_ids);
$any_changes = scalar(@$removed) || scalar(@$added);


>+sub keyword_objects {

>+    $self->{'keyword_objects'} = Bugzilla::Keyword->new_from_list($ids);
> }

You have to return $self->{'keyword_objects'}.

Comment 7

[Max Kanat-Alexander]

Just a few responses:

* Good idea about removing @all_changes.
* Oh, all the functions are supposed to return an arrayref, since they'll eventually be part of $changes in update()! I guess I wasn't paying attention when I made update_cc return an array. We'll have to fix that later, when we combine all the update() functions.
* I really don't see the point of optimizing update_keywords with prepared statements.

Comment 8

[Max Kanat-Alexander]

Oh, and the comment is useful to explain why that one function is different from all the rest, so that somebody doesn't try to refactor it in the future. So it's staying.

Comment 9

[Max Kanat-Alexander]

Attachment: v2

Here--this should fix everything I didn't respond to.

Comment 10

[Frédéric Buclin]

(In reply to comment #7)
> * I really don't see the point of optimizing update_keywords with prepared
> statements.

We do it everywhere else within the Bugzilla code. Everytime we have a loop, we prepare the statements outside the loop. I don't see why we should do an exception here. Moreover, the docs say it's faster to prepare the statement only once that calling ->do() everytime.

Comment 11

[Frédéric Buclin]

Comment on attachment 270538 [details] [diff] [review]
v2

>Index: Bugzilla/Bug.pm

>+sub update_keywords {

>+    if (scalar @$removed || scalar @$added) {
>+        my $removed_names = join ', ', (map {$_->name} @$removed_keywords);
>+        my $added_names   = join ', ', (map {$_->name} @$added_keywords);

Nit: please add parens -> join().


>+sub modify_keywords {

>+        my $new = join(', ', \@result);

This should be:
    my $new = join(', ', map {$_->name} @result);


Your patch is working fine. r=LpSolit with the changes above.

Comment 12

[Max Kanat-Alexander]

Hey, thanks for catching that error, there! :-) I fixed both the things you mentioned.

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.370; previous revision: 1.369
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.185; previous revision: 1.184
done