534057 - Auto-completion no longer works in email_in.pl

Status: RESOLVED FIXED

(Bugzilla :: Incoming Email, defect)

Description

[Frédéric Buclin]

As said in bug 526158, auto-completion is now broken. We should fix it before 3.6.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

I refactored user_match() a bit to work fine either with a CGI object or with a hash. I also moved the description of the function in POD. Tested with both email_in.pl and process_bug.cgi.

When a field doesn't match a user account unambiguously, the field value is ignored rather than throwing an error.

Comment 2

[Max Kanat-Alexander]

It shouldn't ignore items if they match multiple items, it should return an error.

Comment 3

[Frédéric Buclin]

Attachment: patch, v2

Throw an error if match multiple accounts, as requested by mkanat.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 422413 [details] [diff] [review]
patch, v2

> sub match_field {
>-    my $cgi          = shift;   # CGI object to look up fields in
>+    my $cgi          = shift;   # CGI object or hash to look up fields in
>+    my $cgi_is_obj   = (ref $cgi eq 'Bugzilla::CGI') ? 1 : 0;

  Hmm. Maybe it would be better to pass Bugzilla->input_params instead of $cgi to match_field, so then it would always be a hashref? The only tricky part there is that you then have to check if things are arrayrefs, if they're "multi".

>@@ -1218,7 +1183,7 @@ sub match_field {
>                         # No need to look for a valid requestee if the flag(type)
>                         # has been deleted (may occur in race conditions).
>                         delete $expanded_fields->{$field_name};
>-                        $cgi->delete($field_name);
>+                        $cgi->delete($field_name) if $cgi_is_obj;

  Hmm, I'm mildly worried that we'll forget to update this code to actually work with flags when we update email_in to work with flags.

>+        if ($cgi_is_obj) {
>+            $raw_field = join(" ", $cgi->param($field));
>+        }
>+        elsif ($fields->{$field}->{'type'} eq 'multi' && ref $cgi->{$field}) {
>+            $raw_field = join(" ", @{$cgi->{$field}});

  Hmm, this slightly violates the "values are separated by commas" thing in email_in, though, doesn't it?

  Otherwise, this looks great. :-)

  Tell me what you think about my comments--if you think that an update of the code would be better, then feel free to post a new patch, otherwise just respond to my comments.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 422413 [details] [diff] [review]
patch, v2

Oh, I do have one r- comment--user_match_too_many needs to specify which field that it was a problem in, so it probably needs to happen inside of match_field for USAGE_MODE_EMAIL.

Comment 6

[Frédéric Buclin]

(In reply to comment #4)
>   Hmm. Maybe it would be better to pass Bugzilla->input_params instead of $cgi
> to match_field, so then it would always be a hashref? The only tricky part
> there is that you then have to check if things are arrayrefs, if they're
> "multi".

Arrayrefs are concatenated using \0, per http://search.cpan.org/~lds/CGI.pm-3.48/lib/CGI.pm#FETCHING_THE_PARAMETER_LIST_AS_A_HASH: so I would have to split strings on it. I suppose that's not a problem. If this makes the code lighter, I will do it.


>   Hmm, I'm mildly worried that we'll forget to update this code to actually
> work with flags when we update email_in to work with flags.

I'm not too worry about this, actually. :) We will fix it when it's time to do it.


>   Hmm, this slightly violates the "values are separated by commas" thing in
> email_in, though, doesn't it?

Probably, yes. But all other places in Bugzilla allow whitespaces to separate field values, email_in.pl being the only one to enforce commas. As this only affects the @cc field, and because whitespaces are illegal in email addresses, I'm not too worry about violating this rule in this very specific case. :)


>   Otherwise, this looks great. :-)

Thank you! :)

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Arrayrefs are concatenated using \0,

  No, not with how input_params works. They're arrayrefs.

> Probably, yes. But all other places in Bugzilla allow whitespaces to separate
> field values, email_in.pl being the only one to enforce commas. As this only
> affects the @cc field, and because whitespaces are illegal in email addresses,
> I'm not too worry about violating this rule in this very specific case. :)

  Okay. Usermatching works on realname too, though.

Comment 8

[Frédéric Buclin]

Attachment: patch, v3

Comment 9

[Max Kanat-Alexander]

Comment on attachment 424517 [details] [diff] [review]
patch, v3

> sub match_field {
>-    my $cgi          = shift;   # CGI object to look up fields in
>+    my $data         = shift || Bugzilla->input_params; # hash to look up fields in
>     my $fields       = shift;   # arguments as a hash

  Hmm. How about we just reverse those two parameters instead, so that we don't have to pass undef as the first argument in most cases?


  Also, instead of returning non_conclusive_fields, I think that we should just throw an error if it's populated and we're in USAGE_MODE_EMAIL. That allows us to call match_field in other places during email usage and have it not be a problem. (This would fix any problems that currently happen while updating bugs, for example.)

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
>   Also, instead of returning non_conclusive_fields, I think that we should just
> throw an error if it's populated and we're in USAGE_MODE_EMAIL.

Hum no, I much prefer to let the caller do what it wants with non conclusive fields rather than throwing an error unconditionnally. We had this problem with Bugzilla::Flag::validate() already and I had to add SKIP_REQUESTEE_ON_ERROR to do what we want. I don't want the same problem here. Also, I don't think we should look at USAGE_MODE_EMAIL here; I can imagine that we may want @non_conclusive_fields even when called from a CGI script.

I will only fix your first comment.

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> Hum no, I much prefer to let the caller do what it wants with non conclusive
> fields rather than throwing an error unconditionnally. We had this problem with
> Bugzilla::Flag::validate() already and I had to add SKIP_REQUESTEE_ON_ERROR to
> do what we want. I don't want the same problem here. Also, I don't think we
> should look at USAGE_MODE_EMAIL here; I can imagine that we may want
> @non_conclusive_fields even when called from a CGI script.

  Well, I think that in this case you're anticipating a problem that doesn't exist, and you're designing for something that you think might happen but don't know will happen. I think it would be better to simply design for the requirement at hand, and then if we discover that we need something else at a later time, we can develop the solution to fit the best way possible with the future requirement.

Comment 12

[Frédéric Buclin]

Attachment: patch, v3.1

Set $data as the 2nd parameter.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 424521 [details] [diff] [review]
patch, v3.1

Okay, after talking about it on IRC, I think that I see where you're coming from.

Ultimately, I think that there needs to be some sort of distinction between what's happening inside of Flag.pm with MATCH_SKIP_CONFIRM and what's happening in email_in.pl with MATCH_SKIP_CONFIRM, so that the error *can* be thrown within user_match when we're in email and operating normally. (For example, you could just not pass MATCH_SKIP_CONFIRM in email_in.pl and just check USAGE_MODE_EMAIL inside of match_field.)

I'm not really happy with match_field returning $non_conclusive_fields, but I think it's going to have to be acceptable for now.

Comment 14

[Frédéric Buclin]

Checking in editcomponents.cgi;                                      
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.88; previous revision: 1.87                                     
done                                                                            
Checking in editwhines.cgi;                                                     
/cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v  <--  editwhines.cgi
new revision: 1.27; previous revision: 1.26
done
Checking in email_in.pl;
/cvsroot/mozilla/webtools/bugzilla/email_in.pl,v  <--  email_in.pl
new revision: 1.31; previous revision: 1.30
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.209; previous revision: 1.208
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.427; previous revision: 1.426
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.51; previous revision: 1.50
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.127; previous revision: 1.126
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.106; previous revision: 1.105
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.202; previous revision: 1.201
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.295; previous revision: 1.294
done