172518 - make request tracker use generic user matching code

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

The user matching code in bug 162990 doesn't work with the request tracker,
which has its own code for doing the same thing.  The request tracker should use
the code in bug 162990, however.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

Work-in-progress so I don't lose this code on my machine.  This is an
implementation on top of the patch for bug 162990, which is also included in
this code.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: implements feature

This patch integrates the request tracker with the generic user matching code
by:

1. Modifying User::match_field to perform user matching on fields of the form
"requestee-nnn" when code calls that function for the "requestee" field.

2. Calling User::match_field for the "requestee" field in process_bug.cgi and
attachment.cgi.

3. Modifying the "confirm matches" template to display "XXX requestee", where
"XXX" is the flag type name (f.e. "review"), when asking the user to confirm
matches for a requestee field.

4. Backing out the request tracker's own implementation of user matching.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: unrotted but otherwise equivalent

Comment 4

[Erik Stambaugh]

Comment on attachment 105141 [details] [diff] [review]
patch v2: unrotted but otherwise equivalent



Passing 'requestee' among the field names and replacing it with the actual
requestee-## fields inside the match function strikes me as a little strange,
but since I can't seem to think of a way that would be any better, I'm not
going to call it the wrong way to do it.

That all having been said, would there be any benefit to passing the field name
as a wildcard or pattern instead of a magic name, so that something that calls
match_field in the future could use it?


+    if (grep($_ eq 'requestee', keys %{$fields})) {
+	 for my $field (grep(/^requestee-(\d+)$/, keys %::FORM)) {
+	     $field =~ /^requestee-(\d+)$/;

Nit: IIRC, %{$vars->{'form}} should be used instead of %::FORM in anticipation
of the global variables being reworked.


+    $vars->{'fields'}	      = $fields; # fields being matched

I'm not sure I understand why 'fields' has to be sent as well.	Couldn't type
and flag_type go into the existing 'match' hash?


Everything else looks great.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: review fixes

>would there be any benefit to passing the field name as a wildcard or 
>pattern instead of a magic name, so that something that calls match_field 
>in the future could use it?

I thought of that, but requestee has to be special-cased anyway, since we need
the flag type name to display in the confirmation form, and other magic fields
in the future will probably be similarly specific.

Also, there's a performance hit for this, perhaps only minor, but a million 
minor hits is why we're where we are now.  Nevertheless, here's an
implementation, but I really think it doesn't make sense while it isn't being
used by anything but requestee.


>Nit: IIRC, %{$vars->{'form}} should be used instead of %::FORM 
>in anticipation of the global variables being reworked.

I hadn't heard that, and $vars is just as global as %::FORM, but changed.


>I'm not sure I understand why 'fields' has to be sent as well. Couldn't type
>and flag_type go into the existing 'match' hash?

They can't because type and flag type are field-specific, and match->field is a
list of queries, so if I add them there they look like additional queries.

Comment 6

[Erik Stambaugh]

Comment on attachment 105503 [details] [diff] [review]
patch v3: review fixes

> I thought of that, but requestee has to be special-cased anyway, since we
> need the flag type name to display in the confirmation form, and other magic
> fields in the future will probably be similarly specific.

Fair enough.

> >Nit: IIRC, %{$vars->{'form}} should be used instead of %::FORM 
> >in anticipation of the global variables being reworked.
>
> I hadn't heard that, and $vars is just as global as %::FORM, but changed.

I'm trying to remember who said it.  It makes no functional difference of
course.

> They can't because type and flag type are field-specific, and match->field
> is a list of queries, so if I add them there they look like additional
> queries.

I see.	This could probably use a re-think as far as 'matches' and 'fields'
down the road, though.	I'll have to give it some thought, but that's not a
flaw in your patch (more likely to be one in mine).

r=not_erik -- now it's Joel's turn.

Comment 7

[Joel Peshkin]

Comment on attachment 105503 [details] [diff] [review]
patch v3: review fixes

2xr=joel

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: very minor updates

These are very minor updates.  Removed "&" before a function call in
attachment.cgi since it's inconsistent with function calls around it and moved
the regexp line begin and line end matchers into the field pattern for
increased flexibility in the future.

Comment 9

[Dave Miller [:justdave]]

a=justdave

Comment 10

[Erik Stambaugh]

Comment on attachment 105509 [details] [diff] [review]
patch v4: very minor updates

r=not_erik

Comment 11

[Joel Peshkin]

Comment on attachment 105509 [details] [diff] [review]
patch v4: very minor updates

2xr=joel

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.163; previous revision: 1.162
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/global/confirm-user-match.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/confirm-user-match.html.tmpl,v
 <--  confirm-user-match.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Removing template/en/default/request/verify.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/verify.html.tmpl,v
 <--  verify.html.tmpl
new revision: delete; previous revision: 1.1
done