348539 - Move CC validation out of post_bug.cgi and into Bugzilla::Bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

This should be pretty easy. We're just moving the code that converts the login_names into userids.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Once again, a very straightforward patch. Depends on the other post_bug.cgi patches I've asked you to review, but only because the patch won't apply otherwise. It doesn't actually depend on any code from the other patches.

Comment 2

[Olav Vitters]

Comment on attachment 233486 [details] [diff] [review]
v1

>diff -u post_bug.cgi post_bug.cgi

>@@ -257,12 +246,12 @@
> 
> if (Bugzilla->params->{"strict_isolation"}) {
>     my @blocked_users = ();
>-    my %related_users = %ccids;
>-    $related_users{$cgi->param('assigned_to')} = 1;
>+    my @related_users = @cc_ids;
>+    push(@related_users, $cgi->param('assigned_to'));
>     if (Bugzilla->params->{'useqacontact'} && $cgi->param('qa_contact')) {
>-        $related_users{$cgi->param('qa_contact')} = 1;
>+        push(@related_users, $cgi->param('qa_contact'));
>     }
>-    foreach my $pid (keys %related_users) {
>+    foreach my $pid (@related_users) {
>         my $related_user = Bugzilla::User->new($pid);
>         if (!$related_user->can_edit_product($product->id)) {
>             push (@blocked_users, $related_user->login);

You assume @related_users is unique, while it doesn't have to be. Although strange, I could assign it to someone/myself and also put the assignee in the CC. Same for qa_contact. Rest looks good.

Comment 3

[Olav Vitters]

Oh, and a general comment.. in some of the patches you do 'scalar $cgi->param', while here you pass $cgi->param directly. Wouldn't it be easier to have the _check functions deal with that?

Comment 4

[Olav Vitters]

PS2: Wouldn't match_field have validated the CC field?

Comment 5

[Frédéric Buclin]

If it's a scalar, you must write scalar $cgi->param('foo'). Consider this:

my_function($cgi->param('foo'), $cgi->param('bar'));

sub my_function {
    my ($foo, $bar) = @_;
    ...
}

If $cgi->param('foo') is undefined, then you will get:
 $foo = $cgi->param('bar')
 $bar = undef

which is not what you want. If you explicitly write 'scalar', then you get what you expected.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #3)
> Oh, and a general comment.. in some of the patches you do 'scalar $cgi->param',
> while here you pass $cgi->param directly. Wouldn't it be easier to have the
> _check functions deal with that?

  And yeah, it's as LpSolit said. However, I think you could be right in that what I *should* do is make the non-undef field the *first* param, and then the second param can be undef.

(In reply to comment #4)
> PS2: Wouldn't match_field have validated the CC field?

  Yes, but we shouldn't depend on that for internal functions in Bugzilla::Bug, particularly as they'll probably be used by the webservice API.

Comment 7

[Max Kanat-Alexander]

Attachment: v2

Also, the validator converts logins to IDs, where the match function only converts partial logins to full logins.

I fixed the comment.

Comment 8

[Olav Vitters]

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

>Index: post_bug.cgi

>-    foreach my $pid (keys %related_users) {
>+    # For each unique user in @related_users...
>+    foreach my $pid (keys(map {$_ => 1} @related_users)) {

#   Failed test 'post_bug.cgi --ERROR'
#   in t/001compile.t at line 101.
not ok 33 - post_bug.cgi --ERROR
Type of arg 1 to keys must be hash (not map iterator) at post_bug.cgi line 271, near "@related_users)"
post_bug.cgi had compilation errors.

Comment 9

[Max Kanat-Alexander]

Attachment: v3

Oh! :-) I fixed it. :-) (I had tested process_bug accidentally instead of post_bug, before.)

Comment 10

[Max Kanat-Alexander]

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.164; previous revision: 1.163
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.138; previous revision: 1.137
done