Closed
Bug 172518
Opened 22 years ago
Closed 22 years ago
make request tracker use generic user matching code
Categories
(Bugzilla :: Attachments & Requests, defect, P2)
Bugzilla
Attachments & Requests
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: myk, Assigned: myk)
References
Details
Attachments
(1 file, 4 obsolete files)
14.38 KB,
patch
|
erik
:
review+
bugreport
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Blocks: rt-clean-up
Assignee | ||
Comment 2•22 years ago
|
||
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.
Attachment #102036 -
Attachment is obsolete: true
Assignee | ||
Comment 3•22 years ago
|
||
Attachment #104754 -
Attachment is obsolete: true
Updated•22 years ago
|
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
Comment 4•22 years ago
|
||
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.
Attachment #105141 -
Flags: review-
Assignee | ||
Comment 5•22 years ago
|
||
>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.
Attachment #105141 -
Attachment is obsolete: true
Comment 6•22 years ago
|
||
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.
Attachment #105503 -
Flags: review+
Comment 7•22 years ago
|
||
Comment on attachment 105503 [details] [diff] [review] patch v3: review fixes 2xr=joel
Attachment #105503 -
Flags: review+
Assignee | ||
Comment 8•22 years ago
|
||
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.
Attachment #105503 -
Attachment is obsolete: true
Comment 9•22 years ago
|
||
a=justdave
Comment 10•22 years ago
|
||
Comment on attachment 105509 [details] [diff] [review] patch v4: very minor updates r=not_erik
Attachment #105509 -
Flags: review+
Comment 11•22 years ago
|
||
Comment on attachment 105509 [details] [diff] [review] patch v4: very minor updates 2xr=joel
Attachment #105509 -
Flags: review+
Assignee | ||
Comment 12•22 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•