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

Patch by Frédéric Buclin <LpSolit@gmail.com> r/a=mkanat

10 files changed, 114 insertions, 115 deletions

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 2fa4c8ded..d33b14d31 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -790,9 +790,9 @@ sub extract_flags_from_cgi {
     my ($class, $bug, $attachment, $vars, $skip) = @_;
     my $cgi = Bugzilla->cgi;
 
-    my $match_status = Bugzilla::User::match_field($cgi, {
+    my $match_status = Bugzilla::User::match_field({
         '^requestee(_type)?-(\d+)$' => { 'type' => 'multi' },
-    }, $skip);
+    }, undef, $skip);
 
     $vars->{'match_field'} = 'requestee';
     if ($match_status == USER_MATCH_FAILED) {
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 75a4fcf1d..59324383f 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -1124,57 +1124,15 @@ sub match {
     return \@users;
 }
 
-# match_field() is a CGI wrapper for the match() function.
-#
-# Here's what it does:
-#
-# 1. Accepts a list of fields along with whether they may take multiple values
-# 2. Takes the values of those fields from the first parameter, a $cgi object 
-#    and passes them to match()
-# 3. Checks the results of the match and displays confirmation or failure
-#    messages as appropriate.
-#
-# The confirmation screen functions the same way as verify-new-product and
-# confirm-duplicate, by rolling all of the state information into a
-# form which is passed back, but in this case the searched fields are
-# replaced with the search results.
-#
-# The act of displaying the confirmation or failure messages means it must
-# throw a template and terminate.  When confirmation is sent, all of the
-# searchable fields have been replaced by exact fields and the calling script
-# is executed as normal.
-#
-# You also have the choice of *never* displaying the confirmation screen.
-# In this case, match_field will return one of the three USER_MATCH 
-# constants described in the POD docs. To make match_field behave this
-# way, pass in MATCH_SKIP_CONFIRM as the third argument.
-#
-# match_field must be called early in a script, before anything external is
-# done with the form data.
-#
-# In order to do a simple match without dealing with templates, confirmation,
-# or globals, simply calling Bugzilla::User::match instead will be
-# sufficient.
-
-# How to call it:
-#
-# Bugzilla::User::match_field($cgi, {
-#   'field_name'    => { 'type' => fieldtype },
-#   'field_name2'   => { 'type' => fieldtype },
-#   [...]
-# });
-#
-# fieldtype can be either 'single' or 'multi'.
-#
-
 sub match_field {
-    my $cgi          = shift;   # CGI object to look up fields in
     my $fields       = shift;   # arguments as a hash
+    my $data         = shift || Bugzilla->input_params; # hash to look up fields in
     my $behavior     = shift || 0; # A constant that tells us how to act
     my $matches      = {};      # the values sent to the template
     my $matchsuccess = 1;       # did the match fail?
     my $need_confirm = 0;       # whether to display confirmation screen
     my $match_multiple = 0;     # whether we ever matched more than one user
+    my @non_conclusive_fields;  # fields which don't have a unique user.
 
     my $params = Bugzilla->params;
 
@@ -1192,7 +1150,8 @@ sub match_field {
             $expanded_fields->{$field_pattern} = $fields->{$field_pattern};
         }
         else {
-            my @field_names = grep(/$field_pattern/, $cgi->param());
+            my @field_names = grep(/$field_pattern/, keys %$data);
+
             foreach my $field_name (@field_names) {
                 $expanded_fields->{$field_name} = 
                   { type => $fields->{$field_pattern}->{'type'} };
@@ -1218,7 +1177,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);
+                        delete $data->{$field_name};
                     }
                 }
             }
@@ -1227,35 +1186,19 @@ sub match_field {
     $fields = $expanded_fields;
 
     for my $field (keys %{$fields}) {
+        #Concatenate login names, so that we have a common way to handle them.
+        my $raw_field;
+        if (ref $data->{$field}) {
+            $raw_field = join(" ", @{$data->{$field}});
+        }
+        else {
+            $raw_field = $data->{$field};
+        }
+        $raw_field = clean_text($raw_field || '');
 
-        # Tolerate fields that do not exist.
-        #
-        # This is so that fields like qa_contact can be specified in the code
-        # and it won't break if the CGI object does not know about them.
-        #
-        # It has the side-effect that if a bad field name is passed it will be
-        # quietly ignored rather than raising a code error.
-
-        next if !defined $cgi->param($field);
-
-        # We need to move the query to $raw_field, where it will be split up,
-        # modified by the search, and put back into the CGI environment
-        # incrementally.
-
-        my $raw_field = join(" ", $cgi->param($field));
-
-        # When we add back in values later, it matters that we delete
-        # the field here, and not set it to '', so that we will add
-        # things to an empty list, and not to a list containing one
-        # empty string.
-        # If the field accepts only one match (type eq "single") and
-        # no match or more than one match is found for this field,
-        # we will set it back to '' so that the field remains defined
-        # outside this function (it was if we came here; else we would
-        # have returned earlier above).
-        # If the field accepts several matches (type eq "multi") and no match
-        # is found, we leave this field undefined (= empty array).
-        $cgi->delete($field);
+        # Tolerate fields that do not exist (in case you specify
+        # e.g. the QA contact, and it's currently not in use).
+        next unless ($raw_field && $raw_field ne '');
 
         my @queries = ();
 
@@ -1263,11 +1206,9 @@ sub match_field {
         # into @queries, or in the case of fields which only accept single
         # entries, we simply use the verbatim text.
 
-        $raw_field =~ s/^\s+|\s+$//sg;  # trim leading/trailing space
-
         # single field
         if ($fields->{$field}->{'type'} eq 'single') {
-            @queries = ($raw_field) unless $raw_field =~ /^\s*$/;
+            @queries = ($raw_field);
 
         # multi-field
         }
@@ -1288,35 +1229,22 @@ sub match_field {
             $limit = $params->{'maxusermatches'} + 1;
         }
 
+        my @logins;
         for my $query (@queries) {
-
             my $users = match(
                 $query,   # match string
                 $limit,   # match limit
                 1         # exclude_disabled
             );
 
-            # skip confirmation for exact matches
-            if ((scalar(@{$users}) == 1)
-                && (lc(@{$users}[0]->login) eq lc($query)))
-
-            {
-                $cgi->append(-name=>$field,
-                             -values=>[@{$users}[0]->login]);
-
-                next;
-            }
-
-            $matches->{$field}->{$query}->{'users'}  = $users;
-            $matches->{$field}->{$query}->{'status'} = 'success';
-
             # here is where it checks for multiple matches
-
             if (scalar(@{$users}) == 1) { # exactly one match
+                push(@logins, @{$users}[0]->login);
 
-                $cgi->append(-name=>$field,
-                             -values=>[@{$users}[0]->login]);
+                # skip confirmation for exact matches
+                next if (lc(@{$users}[0]->login) eq lc($query));
 
+                $matches->{$field}->{$query}->{'status'} = 'success';
                 $need_confirm = 1 if $params->{'confirmuniqueusermatch'};
 
             }
@@ -1324,6 +1252,7 @@ sub match_field {
                     && ($params->{'maxusermatches'} != 1)) {
                 $need_confirm = 1;
                 $match_multiple = 1;
+                push(@non_conclusive_fields, $field);
 
                 if (($params->{'maxusermatches'})
                    && (scalar(@{$users}) > $params->{'maxusermatches'}))
@@ -1331,23 +1260,31 @@ sub match_field {
                     $matches->{$field}->{$query}->{'status'} = 'trunc';
                     pop @{$users};  # take the last one out
                 }
+                else {
+                    $matches->{$field}->{$query}->{'status'} = 'success';
+                }
 
             }
             else {
                 # everything else fails
                 $matchsuccess = 0; # fail
+                push(@non_conclusive_fields, $field);
                 $matches->{$field}->{$query}->{'status'} = 'fail';
                 $need_confirm = 1;  # confirmation screen shows failures
             }
+
+            $matches->{$field}->{$query}->{'users'}  = $users;
         }
-        # Above, we deleted the field before adding matches. If no match
-        # or more than one match has been found for a field expecting only
-        # one match (type eq "single"), we set it back to '' so
-        # that the caller of this function can still check whether this
+
+        # If no match or more than one match has been found for a field
+        # expecting only one match (type eq "single"), we set it back to ''
+        # so that the caller of this function can still check whether this
         # field was defined or not (and it was if we came here).
-        if (!defined $cgi->param($field)
-            && $fields->{$field}->{'type'} eq 'single') {
-            $cgi->param($field, '');
+        if ($fields->{$field}->{'type'} eq 'single') {
+            $data->{$field} = $logins[0] || '';
+        }
+        else {
+            $data->{$field} = \@logins;
         }
     }
 
@@ -1363,22 +1300,24 @@ sub match_field {
     }
 
     # Skip confirmation if we were told to, or if we don't need to confirm.
-    return $retval if ($behavior == MATCH_SKIP_CONFIRM || !$need_confirm);
+    if ($behavior == MATCH_SKIP_CONFIRM || !$need_confirm) {
+        return wantarray ? ($retval, \@non_conclusive_fields) : $retval;
+    }
 
     my $template = Bugzilla->template;
+    my $cgi = Bugzilla->cgi;
     my $vars = {};
 
-    $vars->{'script'}        = Bugzilla->cgi->url(-relative => 1); # for self-referencing URLs
+    $vars->{'script'}        = $cgi->url(-relative => 1); # for self-referencing URLs
     $vars->{'fields'}        = $fields; # fields being matched
     $vars->{'matches'}       = $matches; # matches that were made
     $vars->{'matchsuccess'}  = $matchsuccess; # continue or fail
     $vars->{'matchmultiple'} = $match_multiple;
 
-    print Bugzilla->cgi->header();
+    print $cgi->header();
 
     $template->process("global/confirm-user-match.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
-
     exit;
 
 }
@@ -2296,6 +2235,49 @@ Untaints C<$passwd1> if successful.
 If a second password is passed in, this function also verifies that
 the two passwords match.
 
+=item C<match_field($data, $fields, $behavior)>
+
+=over
+
+=item B<Description>
+
+Wrapper for the C<match()> function.
+
+=item B<Params>
+
+=over
+
+=item C<$fields> - A hashref with field names as keys and a hash as values.
+Each hash is of the form { 'type' => 'single|multi' }, which specifies
+whether the field can take a single login name only or several.
+
+=item C<$data> (optional) - A hashref with field names as keys and field values
+as values. If undefined, C<Bugzilla-E<gt>input_params> is used.
+
+=item C<$behavior> (optional) - If set to C<MATCH_SKIP_CONFIRM>, no confirmation
+screen is displayed. In that case, the fields which don't match a unique user
+are left undefined. If not set, a confirmation screen is displayed if at
+least one field doesn't match any login name or match more than one.
+
+=back
+
+=item B<Returns>
+
+If the third parameter is set to C<MATCH_SKIP_CONFIRM>, the function returns
+either C<USER_MATCH_SUCCESS> if all fields can be set unambiguously,
+C<USER_MATCH_FAILED> if at least one field doesn't match any user account,
+or C<USER_MATCH_MULTIPLE> if some fields match more than one user account.
+
+If the third parameter is not set, then if all fields could be set
+unambiguously, nothing is returned, else a confirmation page is displayed.
+
+=item B<Note>
+
+This function must be called early in a script, before anything external
+is done with the data.
+
+=back
+
 =back
 
 =head1 SEE ALSO
diff --git a/editcomponents.cgi b/editcomponents.cgi
index c550f0d8c..70afd9c9f 100755
--- a/editcomponents.cgi
+++ b/editcomponents.cgi
@@ -118,7 +118,7 @@ if ($action eq 'add') {
 if ($action eq 'new') {
     check_token_data($token, 'add_component');
     # Do the user matching
-    Bugzilla::User::match_field ($cgi, {
+    Bugzilla::User::match_field ({
         'initialowner'     => { 'type' => 'single' },
         'initialqacontact' => { 'type' => 'single' },
         'initialcc'        => { 'type' => 'multi'  },
@@ -219,7 +219,7 @@ if ($action eq 'edit') {
 if ($action eq 'update') {
     check_token_data($token, 'edit_component');
     # Do the user matching
-    Bugzilla::User::match_field ($cgi, {
+    Bugzilla::User::match_field ({
         'initialowner'     => { 'type' => 'single' },
         'initialqacontact' => { 'type' => 'single' },
         'initialcc'        => { 'type' => 'multi'  },
diff --git a/editwhines.cgi b/editwhines.cgi
index 671774ef7..c59cf8be5 100755
--- a/editwhines.cgi
+++ b/editwhines.cgi
@@ -193,7 +193,7 @@ if ($cgi->param('update')) {
                 }
             }
             if (scalar %{$arglist}) {
-                &Bugzilla::User::match_field($cgi, $arglist);
+                Bugzilla::User::match_field($arglist);
             }
 
             for my $sid (@scheduleids) {
diff --git a/email_in.pl b/email_in.pl
index 044c13040..c7b11d8a7 100755
--- a/email_in.pl
+++ b/email_in.pl
@@ -152,6 +152,17 @@ sub post_bug {
         $fields->{$field} = undef if !exists $fields->{$field};
     }
 
+    my ($retval, $non_conclusive_fields) =
+      Bugzilla::User::match_field({
+        'assigned_to'   => { 'type' => 'single' },
+        'qa_contact'    => { 'type' => 'single' },
+        'cc'            => { 'type' => 'multi'  }
+      }, $fields, MATCH_SKIP_CONFIRM);
+
+    if ($retval != USER_MATCH_SUCCESS) {
+        ThrowUserError('user_match_too_many', {fields => $non_conclusive_fields});
+    }
+
     my $bug = Bugzilla::Bug->create($fields);
     debug_print("Created bug " . $bug->id);
     return ($bug, $bug->comments->[0]);
diff --git a/post_bug.cgi b/post_bug.cgi
index 749acb492..e8f9acc01 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -85,7 +85,7 @@ if ($token) {
 }    
 
 # do a match on the fields if applicable
-Bugzilla::User::match_field ($cgi, {
+Bugzilla::User::match_field ({
     'cc'            => { 'type' => 'multi'  },
     'assigned_to'   => { 'type' => 'single' },
     'qa_contact'    => { 'type' => 'single' },
diff --git a/process_bug.cgi b/process_bug.cgi
index 21cf94fc7..6c9baa3d3 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -142,7 +142,7 @@ if (defined $cgi->param('dontchange')) {
 }
 
 # do a match on the fields if applicable
-Bugzilla::User::match_field($cgi, {
+Bugzilla::User::match_field({
     'qa_contact'                => { 'type' => 'single' },
     'newcc'                     => { 'type' => 'multi'  },
     'masscc'                    => { 'type' => 'multi'  },
diff --git a/request.cgi b/request.cgi
index 594ed1833..b54477cb1 100755
--- a/request.cgi
+++ b/request.cgi
@@ -63,7 +63,7 @@ unless (defined $cgi->param('requestee')
     $fields->{'requestee'}->{'type'} = 'single';
 }
 
-Bugzilla::User::match_field($cgi, $fields);
+Bugzilla::User::match_field($fields);
 
 if ($action eq 'queue') {
     queue();
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 80172bb03..467d4a174 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -1628,6 +1628,12 @@
     <tt>[% name FILTER html %]</tt> does not exist or you are not allowed 
     to see that user.
 
+  [% ELSIF error == "user_match_too_many" %]
+    [% title = "No Conclusive Match" %]
+    [% terms.Bugzilla %] cannot make a conclusive match for one or more
+    of the names and/or email addresses you entered for
+    the [% fields.join(', ') FILTER html %] field(s).
+
   [% ELSIF error == "user_not_insider" %]
     [% title = "User Not In Insidergroup" %]
     Sorry, but you are not allowed to (un)mark comments or attachments
diff --git a/userprefs.cgi b/userprefs.cgi
index cffae38cc..e6ee8fb8a 100755
--- a/userprefs.cgi
+++ b/userprefs.cgi
@@ -247,7 +247,7 @@ sub SaveEmail {
     my $cgi = Bugzilla->cgi;
     my $user = Bugzilla->user;
 
-    Bugzilla::User::match_field($cgi, { 'new_watchedusers' => {'type' => 'multi'} });
+    Bugzilla::User::match_field({ 'new_watchedusers' => {'type' => 'multi'} });
 
     ###########################################################################
     # Role-based preferences