323239 - Move CC insertion from post_bug.cgi to Bugzilla::Bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

Bugzilla::Bug needs a way to add an array of usernames to the CC list. The function should not do the User-matching itself, the caller should do that.

Then, post_bug.cgi needs to use this code instead of doing it manually as it does now.

Comment 1

[André Batosti]

Attachment: batosti_v1

Comment 2

[Frédéric Buclin]

Comment on attachment 216749 [details] [diff] [review]
batosti_v1

>Index: Bugzilla/Bug.pm
>+sub add_cc_list {
>+    foreach my $person (@cclist) {
>+        next if (lsearch($self->cc, $person) != -1);
>+        my $ccid = login_to_id($person);
>+        $dbh->do("INSERT INTO cc (bug_id, who) VALUES (?, ?)", 
>+                 undef, $self->bug_id, $ccid);
>+        push (@{$self->{'cc'}}, $person);
>+    }

Prepare the SQL query outside the FOREACH loop.

Comment 3

[André Batosti]

Attachment: batosti_v1_fix

Comment 4

[Max Kanat-Alexander]

Comment on attachment 217475 [details] [diff] [review]
batosti_v1_fix

  Looks good. Except for bitrot, and a few nits:

>@@ -260,6 +261,7 @@ if (defined $cgi->param('cc')) {
>         my $ccid = DBNameToIdAndCheck($person);
>         if ($ccid && !$ccids{$ccid}) {
>            $ccids{$ccid} = 1;
>+           $cclist{$person} = 1;
>         }
>     }
> }

  Both process_bug.cgi and post_bug.cgi need that code, we'll have to put it into User.pm or Bug.pm somehow. Perhaps a login_to_id({list => \@list}, THROW_ERROR) function. (That's just a note for the future.)

  Also, do we really still need $ccids?

>+sub add_cc_list {
>+    my $self = shift;
>+    my @cclist = @_;

  Hrm. We have to get the ids of users anyway, in the calling script, right? Why not just have the function take a list of ids by default, and then we can add the ability for the function to take names later, if we need it.

  We could also make $self->cc return a list of ids, if that makes more sense in the code. Or perhaps make $self->cc into a hash of {id => name}.

  I could just be crazy, though. You can feel free to ignore me, if you want. :-)

>+
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $sth = $dbh->prepare("INSERT INTO cc (bug_id, who) VALUES (?, ?)");
>+    foreach my $person (@cclist) {
>+        next if (lsearch($self->cc, $person) != -1);

  Nit: I'd prefer you used grep, since it's built-in and probably faster. But you don't have to.

>+        my $ccid = login_to_id($person);
>+        $sth->execute($self->bug_id, $ccid);
>+        push (@{$self->{'cc'}}, $person);
>+    }
>+
>+    return $self->cc;

  Don't return anything. I don't want people relying on that.

Comment 5

[André Batosti]

Attachment: batosti_v2

i'm using the add_cc_list and cc with users objects instead the e-mails. I think its is better than ids.

Comment 6

[Frédéric Buclin]

Comment on attachment 220998 [details] [diff] [review]
batosti_v2

>Index: enter_bug.cgi

>     if (defined $cloned_bug->cc) {
>+        my @cloned_cc;
>+        foreach my $user (@{$cloned_bug->cc}) {
>+            push (@cloned_cc, $user->login);
>+        }
>+        $vars->{'cc'}         = join (" ", @cloned_cc);

Nit: you should use map() here instead of the foreach loop.



>Index: post_bug.cgi

> # Create the ccid hash for inserting into the db
> # use a hash rather than a list to avoid adding users twice
> if (defined $cgi->param('cc')) {
>     foreach my $person ($cgi->param('cc')) {
>         next unless $person;
>-        my $ccid = DBNameToIdAndCheck($person);
>-        if ($ccid && !$ccids{$ccid}) {
>-           $ccids{$ccid} = 1;
>+        my $user = Bugzilla::User->new_from_login($person);
>+        if ($user) {
>+           push (@cclist, $user);
>         }
>     }
> }

First of all, DBNameToIdAndCheck() no longer exists. Then the comment explains why we use a hash instead of an array: to avoid duplicated entries in the DB. I know you check this in Bug.pm but I think it's a good idea to do some triage here. Moreover, this would also avoid calling Bugzilla::User->new() several times for the same user (in case you enter several times the same name).


>-    $related_users{$cgi->param('assigned_to')} = 1;
>+    my @related_users = @cclist;
>+    push (@related_user, new Bugzilla::User($cgi->param('assigned_to'));
>     if (Param('useqacontact') && $cgi->param('qa_contact')) {
>-        $related_users{$cgi->param('qa_contact')} = 1;
>+        push (@related_user, new Bugzilla::User($cgi->param('qa_contact'));

Same comment here.


>-# Insert the cclist into the database
>-my $sth_cclist = $dbh->prepare(q{INSERT INTO cc (bug_id, who) VALUES (?,?)});
>-foreach my $ccid (keys(%ccids)) {
>-    $sth_cclist->execute($id, $ccid);
>-}

>+# Insert the cclist into the database
>+$bug->add_cc_list(@cclist);

Please update the DB while tables are locked! There is a good reason to do so, see my comment below about add_cc_list().



>Index: Bugzilla/Bug.pm

>+    foreach my $id (@$ccids) {
>+        push (@cclist, new Bugzilla::User($id));
>+    }

We really have to make sure we want to return a list of user objects. If yes, we should have a new_from_list(\@ids) function in User.pm.


>+sub add_cc_list {

>+    foreach my $user (@cclist) {
>+        next if (lsearch($self->cc, $user->login) != -1);
>+        $sth->execute($self->bug_id, $user->id);
>+        push (@{$self->{'cc'}}, $user->login);
>+    }

Huh? AFAIK, $bug->cc returns, with your patch applied, a list of user objects, not a list of login names. Moreover, in case two users are altering the same bug at the same time, you could end with a race condition if tables aren't locked while updating $bug->cc. This is the reason you have to call this method while tables are locked in post_bug.cgi.

IMO, add_cc_list() should be rename as edit_cc_list() so that it can work with both post_bug.cgi and process_bug.cgi. Having the whole schema in mind is easier to make the right decisions, IMO. But I know mkanat disagrees with me about this point.

Comment 7

[André Batosti]

Attachment: batosti_v3

Comment 8

[Vlad Dascalu]

Comment on attachment 223728 [details] [diff] [review]
batosti_v3

This no longer applies to the trunk cleanly. There are 2 hunks (out of 5) that are failing for post_bug.cgi.

The rest looks ok, so probably it would take minimal work to update it to trunk.

Comment 9

[André Batosti]

Attachment: batosti_v3_fix

Comment 10

[Vlad Dascalu]

Comment on attachment 229542 [details] [diff] [review]
batosti_v3_fix

I'm not a proper reviewer for this right now.

Comment 11

[Frédéric Buclin]

Comment on attachment 229542 [details] [diff] [review]
batosti_v3_fix

Bitrotten, because post_bug.cgi now uses Bugzilla::Bug::_check_cc() (among others). Also, the bug object creation is now much lighter which doesn't require to lock tons of tables anymore.

Comment 12

[Max Kanat-Alexander]

Also, we're going to implement this is a completely different way than it's implemented here, so hold off for a while.

Comment 13

[Max Kanat-Alexander]

*** Bug 351312 has been marked as a duplicate of this bug. ***

Comment 14

[Max Kanat-Alexander]

Attachment: v4

Okay. You'll see that I had to refactor some parts of Bugzilla::Object in order to get this to work elegantly. It works well, though.

Comment 15

[Olav Vitters]

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

>Index: Bugzilla/Bug.pm

>+sub create {

>+    my $cc_ids = $params->{cc};
>+    delete $params->{cc};

Nit: Would like a comment on why $params->{cc} should be deleted (even if I do know the answer know).

Otheriwse: Works fine. See nothing wrong with it (code wise), but would like opinion of LpSolit on the design (how the CCs are created).

Comment 16

[Frédéric Buclin]

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

>Index: Bugzilla/Object.pm

>+    $class->check_required_create_fields();
>+    my ($field_names, $values) = $class->run_create_validators($params);
>+    return $class->insert_create_data($field_names, $values);
>+}
>+
>+sub check_required_create_fields {
>+    my ($class, $params) = @_;


> sub run_create_validators {

>+    return \%field_values;


>+sub insert_create_data {
>+    my ($class, $field_values) = @_;


Several things:

1. check_required_create_fields() in create() should pass $params.
2. run_create_validators() returns a hashref while create() expects to get two arrayrefs.
3. insert_create_data($field_names, $values) pass field names and values separately while this method expects to get a single hashref.


The rest of the code looks good, despite I didn't test it yet.

Comment 17

[Max Kanat-Alexander]

Attachment: v5

Okay, here's a patch with your comments addressed. I also did a slight refactoring of run_create_validators in Bugzilla::Bug. (It was something I'd already done in a future patch, but I figured I might as well just do it now.)

Comment 18

[Frédéric Buclin]

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

>Index: Bugzilla/Bug.pm

> sub run_create_validators {

>+    _check_strict_isolation($product, $params->{cc},
>+    $params->{assigned_to}, $params->{qa_contact});

It must be $class->_check_strict_isolation().



>Index: Bugzilla/Object.pm

>+    my ($field_values) = $class->run_create_validators($params);

It returns a hashref, not an array. Don't use parens.


r=LpSolit with these two comments fixed.

Comment 19

[Max Kanat-Alexander]

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.173; previous revision: 1.172
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.148; previous revision: 1.147
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.8; previous revision: 1.7
done