384651 - Make CC adding and removal use Bugzilla::Bug in process_bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

So, as far as I can see, the next easiest step is to actually start using update(), add_cc, and remove_cc in process_bug. update() won't update the bugs table yet, it'll just update the cc table. The CC code in process_bug is relatively isolated, so I think it should be safe to move just it into Bugzilla::Bug.

Comment 1

[Max Kanat-Alexander]

Attachment: WIP

This is a work-in-progress. It probably doesn't even compile, although it's almost complete.

Comment 2

[Max Kanat-Alexander]

Attachment: v2

Okay, here we go!

I prefer to have add_cc add one user at a time, so that if we ever want to run it inside an eval{}, we can easily determine which user failed.

I've tested this pretty well, and it worked in all my tests.

Comment 3

[Max Kanat-Alexander]

Attachment: v2.1

I had left some debugging code in the previous patch.

Comment 4

[Max Kanat-Alexander]

Attachment: v3

I came up with an even better way of using @bug_objects.

Comment 5

[Max Kanat-Alexander]

Attachment: v4

Since we're going to have to wait a while for the product-change bug (and I'm going to have to re-write that patch anyway), I'm reversing the dependency order.

That is, you can review this one first, now, and we can review that one some time later, when that section of process_bug stops changing so much.

Comment 6

[Frédéric Buclin]

Comment on attachment 268849 [details] [diff] [review]
v4

>Index: process_bug.cgi

>+  push(@bug_objects, new Bugzilla::Bug($id));

A few lines below, you should replace:
my $bug = new Bugzilla::Bug($idlist[0]);
by:
my $bug = $bug_objects[0];


>@@ -773,6 +777,7 @@
>     || defined $cgi->param('addselfcc')
>     || defined $cgi->param('removecc')
>     || defined $cgi->param('masscc')) {

The comment right before these lines mentions hashes, but you now use arrays. You should update the comment.


>     if ($cc_add) {
>         $cc_add =~ s/[\s,]+/ /g; # Change all delimiters to a single space
>+        push(@cc_add, split(" ", $cc_add));
>     }

You could merge both lines into push(@cc_add, split([\s,]+, $cc_add)).


>+    push(@cc_add, Bugzilla->user) if $cgi->param('addselfcc');

Having $bug->{add|remove}_cc accept either a user object or a login name only because you pass an object here annoys me a bit. But I suppose that's fine.


>     if ($cc_remove) {
>         $cc_remove =~ s/[\s,]+/ /g; # Change all delimiters to a single space
>+        push(@cc_remove, split(" ", $cc_remove));
>     }

Same comment as above about merging both lines into a single one.


>+my %bug_objects = map {$_->id => $_} @bug_objects;

As all bug objects have already been created, you should loop over them a few lines below:

foreach my $id (@idlist) {
    my $old_bug_obj = new Bugzilla::Bug($id);

should be replaced by:

foreach my $old_bug_obj (@bug_objects) {
    my $id = $old_bug_obj->id;

Else we are creating bug objects again despite we already have them available.


>+    my ($cc_removed) = $bug_objects{$id}->update_cc($timestamp);

Why defining %bug_objects if all you need is the bug object? With my change above, you already have $old_bug_obj available.



>Index: Bugzilla/Bug.pm

>     my $changes = $self->SUPER::update(@_);
>-
>+    

Useless change. No need to add whitespaces here. :)


>+    my $removed_users = Bugzilla::User->new_from_list($removed);
>+    my $added_users   = Bugzilla::User->new_from_list($added);

Why creating user objects again and again? You already got them when calling $bug->cc_users. You should cache them instead. I know new_from_list() is a single SQL query, but why 1) doing a useless query, and 2) waste some more memory storing these variables?


>+sub add_cc {

>+    if (Bugzilla->params->{strict_isolation}
>+        && !$user->can_edit_product($self->{product_id}))
>+    {

Hum, no. You want to call can_edit_product() with the *new* product as argument, not the current product. This is an important distinction when moving bugs to new products.


>+sub remove_cc {

>+    # We have to operate directly on the array, so that the order of
>+    # the array is preserved.
>+    my $cc_users = $self->cc_users;
>+    my @cc_user_ids = map {$_->id} @$cc_users;
>+    my $pos = lsearch(\@cc_user_ids, $user->id);
>+    if ($pos > -1) {
>+        splice(@$cc_users, $pos, 1);
>+    }

Wouldn't @$cc_users = grep {$_->id != $user->id} @$cc_users; be easier?


>+sub cc_users {

>+    my $cc_ids ||= $dbh->selectcol_arrayref(

||= doesn't make sense. It should be = alone.



>Index: Bugzilla/User.pm

>+sub check {

We already have login_to_id($login, THROW_ERROR) which behaves in a similar way, except it returns a user ID instead of an object. Do we really need both?

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> As all bug objects have already been created, you should loop over them a few
> lines below:
> 
> foreach my $id (@idlist) {
>     my $old_bug_obj = new Bugzilla::Bug($id);

  The reason I didn't do this is that the objects in bug_objects are used very differently than old_bug_obj and new_bug_obj. Those are both exact copies of the bug in the DB, but the bugs in bug_objects have actually had their internal structures modified, and don't reflect what's in the DB. Eventually all of this compatibility code will go away, anyhow--it will all be cleaned up in the end.

> >+    my $removed_users = Bugzilla::User->new_from_list($removed);
> >+    my $added_users   = Bugzilla::User->new_from_list($added);
> 
> Why creating user objects again and again? You already got them when calling
> $bug->cc_users. You should cache them instead. I know new_from_list() is a
> single SQL query, but why 1) doing a useless query, and 2) waste some more
> memory storing these variables?

  It makes the code much simpler, for right now, and it doesn't have any significant performance impact.

> >+sub check {
> 
> We already have login_to_id($login, THROW_ERROR) which behaves in a similar
> way, except it returns a user ID instead of an object. Do we really need both?

  Yes, it'll be more and more useful as time goes on. I've always wished I had this function, so I finally just wrote it.


  Patch coming up.

Comment 8

[Max Kanat-Alexander]

Attachment: v5

Wow, I had no idea "@$cc_users =" would work. Thanks for that tip! :-)

I've fixed everything I didn't respond to below.

Comment 9

[Frédéric Buclin]

Comment on attachment 269765 [details] [diff] [review]
v5

>Index: Bugzilla/Bug.pm

>+sub update_cc {

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

Please write:

if (scalar(@$removed)) {
    $dbh->do('DELETE FROM cc WHERE bug_id = ? AND who IN (' .
              join(',', @$removed) . ')', undef, $self->id);
}

Either that or write $sth = $dbh->prepare() and then use $sth->execute() within the foreach loop.



>Index: Bugzilla/User.pm

>+sub check {

No POD for this function?


Your patch is working fine. r=LpSolit with at least my first comment addressed.

Comment 10

[Frédéric Buclin]

Comment on attachment 269765 [details] [diff] [review]
v5

>Index: Bugzilla/Bug.pm

>+sub update_cc {

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

I forgot to mention: replace ->do() by ->execute() here too and prepare the statement outside the foreach loop. r=LpSolit with this fixed as well.

Comment 11

[Max Kanat-Alexander]

I didn't do the prepare/execute as I felt it was a premature and unnecessary optimization. I did everything else.

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.368; previous revision: 1.367
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.184; previous revision: 1.183
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.153; previous revision: 1.152
done