Closed Bug 162990 (wildcard) Opened 22 years ago Closed 22 years ago

Shorthand/wildcard entry for login names in assign, cc, qa, fields

Categories

(Bugzilla :: User Interface, enhancement, P2)

Product:
Bugzilla
Component:
User Interface
Version:
2.17
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.18

People

(Reporter: bugreport, Assigned: bugreport)

References

Details

Attachments

(1 file, 10 obsolete files)

Wildcard/substring search patch
21.82 KB, patch
bugreport
: review+
myk
: review+
Details | Diff | Splinter Review
For sites where there are too many users to use dropdown lists for picking users in CC lists, assigned, and QA fields, a good way to enter lists of users without requiring exact and complete addresses is needed. It should be possible to use an alternate form for any address so long as the alternate form matches exactly one login. Imprecise or unmatchable alternate forms should cause an error page that gives the user an opportunity to correct the address and try again. Alternate forms suggested... (suggesting + as an escape for special codes, * translating to a regexp of [^@]* ) +me --> the user doing the edit justdave@* --> any user with a username of justdave independent of domain ger*@*.org --> any user in a .org domain with a userid starting with "ger" So it should be possible for a CC list to use reviewers@mozilla.org +me justdave@syn*
Also, the scheme selected should not preclude future enhancement to match using SOUNDEX.
On some sites where one should enter a date in a form, there is a link/button that popups a new window with a calendar control. Would it be possible to do something like that for the fields asking for a login name? The window would offer a search facility where one could select one or more login names and would fill the text field in the original page with them.
and we'd use an error if more than one name matched, or something?. Maybe we should let people pick their own 'short form' (and then use that, instead of the email address, as the login/etc stuff, which is something I've been thinking of for a while)
I'm inclined to avoid javascript, which would be needed to do the pop-up style. Permitting people to specify their own "handle" could work, but there are some serious admin issues to resolve. I'm inclined to use whatever is used for userid and then piggyback on any work that changes the userid away from email addresses in the future.
Tossing this up for comments It allows wildcards to be entered on assigned_to, cc, newcc, and qa_contact fields for post_bug and process_bug and is configurable to either allow select boxes to come up for ambiguous results or to force the user to narrow their search in case the site doesn't want wildcards used for email address mining. It still doesn't solve the problem if you have no idea at all what the person's user ID is. I'll be attaching the new template file just after this. This is also my first Bugzilla patch and I'm likely to be doing something terribly wrong.
This goes in template/en/default/global -- goes with the above patch.
Comment on attachment 98040 [details] [diff] [review] (Preliminary) user wildcard patch Just a few nits from me... >+ >+# Display a confirmation screen or error if there are wildcards >+# >+# Pass it a list of field names. >+ Embellish on the definition of the hash >+# TODO: check for (and skip) redundant matches >+ >+sub userwildcards { >+ >+ my %fields = @_; >+ my $matchsuccess = 1; # did the match fail? >+ >+ # First, process any selectboxes >+ >+ for my $field (keys %::MFORM) { >+ $field =~ /^w_/ or next; >+ (my $fieldname = $field) =~ s/^w_\d+_//; >+ # feed it to the form data >+ $::FORM{$fieldname} .= " " . join (" ", @{$::MFORM{$field}}); >+ } >+ probably should use a more unmistakable prefix since this mustn't conflict with any other forms.
Comment on attachment 98040 [details] [diff] [review] (Preliminary) user wildcard patch Question: does this have the escape stuff in it, like +me? >Index: process_bug.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v >retrieving revision 1.141 >diff -u -r1.141 process_bug.cgi >--- process_bug.cgi 27 Aug 2002 09:35:06 -0000 1.141 >+++ process_bug.cgi 5 Sep 2002 22:35:29 -0000 >@@ -85,6 +85,16 @@ > scalar(@idlist) > || DisplayError("You did not select any bugs to modify.") > && exit; >+ >+# If usersearch is enabled and wildcards were used, we need to display a >+# confirmation screen before moving on >+ >+&userwildcards( >+ 'qa_contact' => 's', >+ 'newcc' => 'm', >+ 'assigned_to' => 's', >+ ); >+ This doesn't feel right to me. You have a function that does some processing and then (possibly) processes a template, gives output, and (most questionably) exits right there; note that you stuck the function in globals.pl (a comment on that below), and no other function calls template->process, nor does any other function call exit (except for... template processing stuff). There's probably a better way to do this; *if* there isn't, you need to document in your comment above it that it does indeed call exit, and script processing will stop after usewildcards() is called. Is globals.pl the right place for this? You should ask bbeatz if there's a better .pm to put this stuff into in Bugzilla:: land. >Index: globals.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v >retrieving revision 1.197 >diff -u -r1.197 globals.pl >--- globals.pl 30 Aug 2002 15:24:12 -0000 1.197 >+++ globals.pl 5 Sep 2002 22:35:29 -0000 >@@ -1397,6 +1397,126 @@ > return $str; > } > >+ >+# Display a confirmation screen or error if there are wildcards >+# >+# Pass it a list of field names. >+ >+# TODO: check for (and skip) redundant matches >+ >+sub userwildcards { >+ >+ my %fields = @_; >+ my $matchsuccess = 1; # did the match fail? >+ >+ # First, process any selectboxes >+ >+ for my $field (keys %::MFORM) { >+ $field =~ /^w_/ or next; >+ (my $fieldname = $field) =~ s/^w_\d+_//; The two above lines are unnecessarily complicated; this is not an obfuscated perl contest; if you feel the need to use shorthand, use 'next if (condition)'. >+ # feed it to the form data >+ $::FORM{$fieldname} .= " " . join (" ", @{$::MFORM{$field}}); >+ } >+ >+ return 1 if ( >+ (Param('userwildcards') eq 'off' ) || # wildcards not allowed >+ (join("\n", @::FORM{keys %fields}) !~ /\*/) ); # no wildcards used Are you sure that regex will match across the '\n'? Are you sure you don't need the /s modifier? >+ >+ # prepare default form values >+ >+ $main::vars->{'form'} = \%::FORM; >+ $main::vars->{'mform'} = \%::MFORM; Both .cgis do this; are you sure you're not clobbering any processing that's already gone on with these variables, or that you even need to do this? >+ # %usermatches is laid out the way it is so that it can accommodate >+ # multiple searches and results with as little added effort as >+ # possible. >+ my %usermatches = (); >+ >+ # replace form values with wildcards and ask for confirmation >+ >+ for my $field (keys %fields) { >+ >+ next unless $::FORM{$field} =~ /\*/; # skip non-wildcards >+ >+ my $regex = $::FORM{$field}; >+ $::FORM{$field} = ''; >+ >+ $regex =~ s/\*/\%/g; # use mysql wildcards >+ >+ my @queries = (); >+ >+ if ($fields{$field} =~ /s/i) { >+ @queries = ($regex); >+ } else { >+ $regex =~ s/[\s,]+/ /g; # Change all delimiters to a single space >+ @queries = split(' ', $regex); >+ } This check should be determinsitic; that is, mean what you say: should $fields{$field} *match* 's', or *eq* 's'? And if it doesn't *eq* 's', what else did you mean it to eq? 'm'? Ok; then check if it eqs 'm'? And if it doesn't match either, then what? Is that an error? If it is, ThrowCodeError(). >+ for my $query (@queries) { >+ >+ # Do not search if there was no wildcard >+ >+ if ($query !~ /\%/) { >+ $main::vars->{'form'}->{$field} .= $query . " "; >+ next; >+ } >+ >+ # search >+ >+ PushGlobalSQLState(); >+ SendSQL( >+ "select login_name from profiles where login_name like " . >+ SqlQuote($query) ); >+ # TODO: display real names as well >+ >+ my $name = ''; >+ my @names = (); >+ >+ while (MoreSQLData()) { >+ ($name) = &::FetchSQLData(); >+ push @names, $name; >+ } >+ >+ (my $printablename = $query) =~ s/\%/\*/g; Same obfuscation nit here. >+ %usermatches->{$field}->{$printablename} = \@names; >+ >+ # here is where it checks multiple matches and passes things >+ # accordingly >+ >+ if (@names == 1) { Say what you mean: if (scalar(@names) == 1) (and, as a good C hacker, you should always write: if (1 == scalar(@names))). There's some other minor stuff, but this should keep you busy for awhile. ;-)
Attachment #98040 - Flags: review-
Before I get started on responses, I suppose I should have explained what this patch does: This patch allows the use of the asterisk (*) as a wildcard in logins. When a wildcard is found in a specified field, it executes the search and creates a confirmation page to make sure the matches found are those intended (and to select from ambiguous matches if applicable). Then the complete form data, with the wildcards replaced by the results, are passed along and the script in question executes normally. In the case of the select boxes, a new form element is created to contain the multi-selects. Before the script checks for wildcards it looks for form elements with these prefixes and appends them to the appropriate element. Re: comment 7 Embellish on the definition of the hash It seems I left the old comment in place. It isn't just a list of field names any more. I fixed that. probably should use a more unmistakable prefix since this mustn't conflict with any other forms I changed it to 'wildcard_' instead of 'w_' -- hopefully that will be unique enough Re: comment 8 Question: does this have the escape stuff in it, like +me? No, this only has the '*' searching (so far). Aliases like '+me' can be added to this without much work, though. We should probably come up with a list of some other aliases that should be added. ...and (most questionably) exits right there When the search is done, all that is displayed is a confirmation page. No other processing is needed until the user hits 'confirm,' at which time the rest of the script will execute. This is the same as the verify-new-product screen in process_bug.cgi Throwing the template and exiting only happens when wildcards were processed. note that you stuck the function in globals.pl Admittedly, a dumb place for a CGI function. I think I'll move it to CGI.pl pending word from bbaetz (who I am told is out until Sunday). There's probably a better way to do this I guess that depends on the result of what I said above. If you mean there should be a better way than displaying an intermediate confirmation screen before sending the data to the rest of the script, I suppose I'm open to any ideas, not counting just blindly accepting the results, which I am against. *if* there isn't, you need to document in your comment above it that it does indeed call exit, and script processing will stop after usewildcards() is called Fair enough, but more specifically, it forces the script to run twice (as long as the user actually hits 'confirm'). The second time it executes normally, with the data adjusted by the results of the wildcard search. So it really kind of sort of behaves as if it never terminated. $field =~ /^w_/ or next; (my $fieldname = $field) =~ s/^w_\d+_//; The two above lines are unnecessarily complicated; this is not an obfuscated perl contest; if you feel the need to use shorthand, use 'next if (condition)'. Not intentional, I assure you. They're (bad) habits I got into with some of my personal-use scripts and they look very natural to me. I'll change it, but I just want to make sure no one thinks I'm making it hard to read on purpose. (join("\n", @::FORM{keys %fields}) !~ /\*/) ); Are you sure that regex will match across the '\n'? Are you sure you don't need the /s modifier? Actually, the "\n" was from some early test code and I forgot to change it to " ". $main::vars->{'form'} = \%::FORM; $main::vars->{'mform'} = \%::MFORM; Both .cgis do this; are you sure you're not clobbering any processing that's already gone on with these variables, or that you even need to do this? $vars are (as far as I know) only used by the template. I only throw the template and mess with $vars if I'm about to terminate, so manipulating $vars shouldn't have any effect elsewhere. This check should be determinsitic; that is, mean what you say: should $fields{$field} *match* 's', or *eq* 's'? And if it doesn't *eq* 's', what else did you mean it to eq? 'm'? Ok; then check if it eqs 'm'? And if it doesn't match either, then what? Is that an error? If it is, ThrowCodeError(). Good point. I'll fix that. (my $printablename = $query) =~ s/\%/\*/g; Same obfuscation nit here. Really I think the only reason I got into the habit of doing that was because this kind of syntax is cited in the camel book. If this is considered hard to read I'll fix it. if (@names == 1) { Say what you mean: if (scalar(@names) == 1) That's much better. (and, as a good C hacker, you should always write: if (1 == scalar(@names))). Eww! (guess I'm not a good C hacker) There's some other minor stuff, but this should keep you busy for awhile. ;-) Thanks for taking a look at this. I'm going to roll these changes in and post a new patch later today.
Attached patch wildcard code patch (obsolete) — — Splinter Review
Includes changes from the above comments
Attachment #98040 - Attachment is obsolete: true
Please remember that this is still preliminary stuff...
Just do you know I haven't fallen off the planet, I'll get this new patch reviewed shortly.
Attached patch Wildcard patch and new template (obsolete) — — Splinter Review
This patch contains both the code changes and the new patch. Note that I'm still leaving out the '+me'-like aliases. Would that feature be better tracked on its own bug?
Attachment #98043 - Attachment is obsolete: true
Attachment #98181 - Attachment is obsolete: true
Comment on attachment 98524 [details] [diff] [review] Wildcard patch and new template r=joel Need 2xr from a more independent reviewer. Addition of pronouns (like "me") will be a seperate patch.
Attachment #98524 - Flags: review+
Has this patch been deployed anywhere? If so, I'd like to see that... just to play around with it.
myk has done with for request tracker... Can't we combine the two impls?
This one is modular enough to be called from anywhere and includes the confirmation dialog page mechanism with it. I think the right thing to do would be to put this one in place, then enhance it with any additional hooks needed by the request tracker (such as limiting the matches, matching realnames as well, etc...) and then request tracker could use it as well.
From IRC discussion with bbaetz: The right thing to do is to merge this with myk's and have that be used everywhere. The userwildcards() function that handles the whole confirmation dialog should call a common match function not contained in userwildcards itself. The common match function should be callable from non-cgi things as well, such as bug_email. Ideally, when userwildcards() is called, it should be possible to specify the match function that it should call. Also, with the expanded usage, the caller to userwildacrds should pass it the field title as well as the name. i.e. something like userwildcards( MATCH => \&match_realname, qa_contact => { 's', 'QA Contact' }, cc => { 'm', 'New CC Member' }, LIMIT=10 ); Where MATCH would default to \&match_email and LIMIT would probably default to 10 or so. This will also leave room for things that would come up after the new groups system is in place where the erquest tracker may want to match only the names of reviewers, for example. myk and not_erik and, perhaps, bbaetz and I should discuss this.
I am in the process of merging this patch into the User.pm file from bug 98801 (request tracker). This will bring together the work myk has done on user freetext searching with the wildcard function in my patch. A new patch will be available soon.
Patch includes: - Bugzilla::User from bug 98801, with wildcard option added - usermatch() function in CGI.pl to parse out wildcards and freetext in login fields - Confirmation screen for ambiguous (or, optionally, all) matches - match modes: - freetext and wildcards - wildcards with multiple matches - wildcards with single matches only (to disallow users from 'discovering' other users) - off - user match limit to keep the DB from getting hit too hard - a new code-error type "bad_arg" for bad function arguments - another new tamplate: formfieldname, which translates the name of a form field to localized text Searching has been activated in post_bug and process_bug. Lots of new/changed code in this one. Have at it.
Attachment #98524 - Attachment is obsolete: true
Comment on attachment 98798 [details] [diff] [review] Wildcard and freetext user search patch A bunch of small things and one serious fail..... >+# If appropriate, this function will execute the search and display a >+# confirmation screen. The confirmation screen will terminate screipt >+# execution (exit()). Upon confirmation, the search criteria are replaced >+# with the resulting data and the script will execute again as normal. s/screipt/script/ Put a usage example in a comment here and clarify that the argument is fields => hash not just hash. This will allow future option addition, I presume? >+sub usermatch { >+ >+ my %args = @_; >+ my %fields = %{ %args->{'fields'} }; nit: remove extra spaces >+################################################################################ >+# Functions >+################################################################################ >+ >+my $user_cache = {}; >+sub new { > Let's take this up with myk.... probably the only function that this patch should actually put in user.pm is match, otherwise we have to have all of user.pm and its related data structures reviewed before they are even used. >+ >+ # Build the query. >+ my $sqlstr = &::SqlQuote($wildstr); >+ my $qry = " >+ SELECT userid, realname, login_name >+ FROM profiles >+ WHERE (login_name LIKE $sqlstr >+ "; New code should avoid putting newlines in the SQL. Use "" and . as in my $qry = "SELECT this, that" . " FROM here"; Spell out '$query' before Gerv sees this :-) Match needs to check the exact match first. Currently, if I limit the results to 5 matches and I have a userid that is a subset of 10 others, I don't see the one I specified exactly if an auto-instring search returns more than 5 earlier matches. Try this... Create users outsider1 outsider2 outsider3 sider set max matches to 2 and auto-instring enabled If you sepcify "sider", you don't even get offerred sider as an option. Ideally, the limited results would be most "relevent". As you are considering this, myk originally required the substring to be at least 3 characters. I am not sure of his reason. (BTW: Relevent probably means from shortest to longest.. you may be able to accomplish this by letting the SQL server sort by length before the limit, then sorting alphabetically in perl before the UI) >--- /dev/null Wed Sep 11 16:08:58 2002 >+++ template/en/default/global/confirm-usermatch.html.tmpl Wed Sep 11 15:41:02 2002 >@@ -0,0 +1,164 @@ > >+ # Contributor(s): Myk Melez <myk@mozilla.org> >+ # Erik Stambaugh <not_erik@dasbistro.com> >+ #%] >+ myk did?
Attachment #98798 - Flags: review-
Blocks: 145499
I haven't reviewed yet, but some notes on the previous review: > New code should avoid putting newlines in the SQL. Use "" and . I have a solution for this in the request tracker patch: a line in SendSQL that converts all whitespace to single spaces. That way the code can be structured however makes sense for readability and hackability, and the admins can see sensible-looking queries in their process lists. >Match needs to check the exact match first. Right. F.e. enter the exact match "gerv@mozilla.org" on b.m.o and you'll get both "gerv@mozilla.org" and "gerv@mozilla.org.uk". Entering an exact match should bypass all the matching code so it works the same way it does now. >Spell out '$query' before Gerv sees this :-) Surely you can't be serious, or is this a nit of his? >myk originally required the substring to be at least 3 characters. I am not >sure of his reason. It's a performance/footprint issue. One or two characters selects a bunch of data, too much to be useful (with any significant number of users).
re: comment 21 > ... and clarify that the argument is fields => hash Oops! That's twice I put an out of date usage. Let's see if I can do it again on the next patch too! > This will allow future option addition, I presume? That's the idea. I'd like to add some features in the future after all of these inter-dependent patches land. > Let's take this up with myk.... probably the only function that this patch > should actually put in user.pm is match, otherwise we have to have all of > user.pm and its related data structures reviewed before they are even used. Really the main difference between how it was being done before and the way it is now is that that data structure involved. If I take it out, it's basically the old code with the INSTR match put in. I'm not sure we've accomplished anything at that point other than pulling the SQL code out of CGI.pl and putting it into User.pm > New code should avoid putting newlines in the SQL. I know myk said his patch will fix this, but '.' concatenation works fine for me, so I changed it. > Match needs to check the exact match first. First it checks to see if there's a wildcard character. If there is, it does a wildcard search, otherwise it does the exact match before moving to INSTR. There's no reason to bother with the exact match if the address specified has a '*' in it. ...although... one of the criteria to qualify an exact match is a '@' in the address. Since we are not necessarily matching to an email address, especially not when (if?) we move away from email addresses as an identifier, I'm going to take this out. Aside from all of that, it was already doing exact matches first. > Ideally, the limited results would be most "relevent". Done. > >+ # Contributor(s): Myk Melez <myk@mozilla.org> > >+ # Erik Stambaugh <not_erik@dasbistro.com> [...] > myk did? He sure did. He probably just doesn't know it. I stole the important part of that from the confirm-duplicate template, which has his name on it. In fact, in the first patch I didn't even change any of the comments in the beginning, including the interface spec. Oops again. re: comment 22 > It's a performance/footprint issue. One or two characters selects a bunch > of data, too much to be useful (with any significant number of users). Fair enough. I have it skipping substring searches if it's <3 characters
Attached patch user search patch (obsolete) — — Splinter Review
This patch addresses the above review comments.
Attachment #98798 - Attachment is obsolete: true
Comment on attachment 99131 [details] [diff] [review] user search patch Coding structure looke good. Only one issue remains. If search mode is enabled and exact addresses are specified, (probably any condition that skips confirmation), then the $FORM variable is not reloaded. Also, the forfieldname template needs to be grabbed from the prior patch.
Attachment #99131 - Flags: review-
I did hack a fix for the one remaining issue and everything else seems to work very well. The fix is to store the values of @{$users}[0]->{'email'} in a seperate list for each field as they are returned and then reload $::FORM{$field} from each of those lists if no confirm page is needed.
re: comment 25 and comment 26 I was a little confused by this, and it's because it's not exactly doing it as described. The problem is that I had a 'next' in place when it checked whether to skip confirmation on exact matches. Exact freetext matches are the only place where the data had to be put back in. Everything else was already doing it. A new patch I'll upload in a few minutes will reflect this, but it's a matter of pasting: $vars->{'form'}->{$field} .= @{$users}[0]->{'email'} . " "; in CGI.pm line 1126, above the 'next.' ($vars->{'form'} is a ref to $::FORM, which is why this works)
Per myk's request I'm waiting for his comments before I post another patch. Just so no one thinks I forgot to send it.
I haven't done a complete review, but what I have should give you something to work on... 1. In general, I'd like to see more documentation about how this code works. I'm having a hard time figuring it out. 2. The following template is missing from the latest patch: formfieldname.html.tmpl 3. It didn't work for me when I tried to use it. I went to a bug, typed "moz" into the CC field on my test installation, selected from the list of matches, and submitted the form, but my selections weren't added to the bug's CC list. It failed when I selected both a single user and multiple users. 4. I don't see why multi-select menus are being used at all. When you type a string into the CC field, you almost always intend to add a single user to the list. The advantages of being able to add multiple users with a single match string don't outweigh the disadvantages of making the interface clumsier the vast majority of the time. 5. I like this architecture overall, but I think it would work better for usermatch() to move into User.pm and become validate(). validate() would take a list of fields (passed as a reference to a hash of hashes describing the fields), populate the hashes with the matched users, and return true if all users matched exactly, false otherwise, like so: (in process_bug.cgi) my $user_fields = { 'qa_contact' => { 'type' => 'single', value => $::FORM{'qa_contact'} } , 'newcc' => { 'type' => 'multi', value => $::FORM{'newcc'} } , 'assigned_to' => { 'type' => 'single', value => $::FORM{'assigned_to'} } , }; if (!validate($user_fields)) { # by this point the field hashes in $user_fields contain one or more # additional keys storing user objects for the matching user(s) $vars->{'user_fields'} = $user_fields; # ... other necessary $vars declarations here ... print "Content-type: text/html\n\n"; $template->process("global/confirm-usermatch.html.tmpl", $vars) || ThrowTemplateError($template->error()); } The advantages to this approach are that it moves UI generation out of a module (CGI.pl is a library now but will soon become a module) and into the CGI, where it probably belongs, puts more useful code in the module that bbaetz can use for bug_email.pl, and avoids modifying %::FORM. 6. Rather than prepending "wildcard_" to fields being matched, I suggest using the original field names (and then excluding them from the list of hidden fields generated by hidden-fields.html.tmpl). Then you don't have to worry about whether a particular form post is from the original form or a confirmation page. You only need to validate the values as usual. In order for this to work for the CC list, however, you need to modify the validation code to build the CC list from either MFORM or FORM. Index: CGI.pl 1022a1025,1176 > # ...where fieldtype can be 's' (single match only) or 'm' (multi-match) You're overloading the single/multi match distinction too much. In one sense, it means whether or not search results can contain more than one match. In the other sense, it means whether or not form fields can contain more than one match string. I suggest using single and multi to refer to the form fields and some other terminology for the other meaning. > sub usermatch { Nit: Most Bugzilla function names start with verbs. This would be better named something like "match_users" (or "MatchUsers"). > my %args = @_; > my %fields = %{ %args->{'fields'} }; Since you defined "fields" as a reference to a hash (curly braces instead of parentheses), shouldn't this be "my %fields = %{ $args->{'fields'} };"? > for my $field (keys %::MFORM) { > if ($field =~ /^usersearch_/) { > > my $fieldname = $field; > $fieldname =~ s/^usersearch_\d+_//; # the original field name > > # feed it to the form data > $::FORM{$fieldname} .= " " . join (" ", @{$::MFORM{$field}}); > > } > } Shouldn't "usersearch" be "wildcard"? > # skip confirmation for exact matches > if ((scalar(@{$users}) == 1) && > (@{$users}[0]->{'email'} eq $query)) > { > next; > } This won't work for exact matches that are subsets of other matches because those matches will return more than one user. Searching on b.m.o for "gerv@mozilla.org", for example, will return both "gerv@mozilla.org" and "gerv@mozilla.org.uk". To solve this problem, you either need to look through every search result (no matter how many) for that perfect match or (better) query the database for it beforehand. Nit: per the style guide, put "&&" next to the following condition rather than the preceding one. > %usermatches->{$field}->{$query} = \@{$users}; > > # here is where it checks for multiple matches > > if (scalar(@{$users}) == 1) { > # exactly one match > $vars->{'form'}->{$field} .= @{$users}[0]->{'email'} . " "; @{$users}[0]->{'email'} --> ${$users}[0]->{'email'} > my ($volume, $dirs, $filename) = File::Spec->splitpath($0); > $filename =~ /^(.+\.cgi)$/; > my $script = $1 > || DisplayError("Could not determine the name of the script.") > && exit; > > $vars->{'script'} = $script; # for self-referencing URLs Per our conversation on IRC, use $ENV{'SCRIPT_NAME'} or an empty action attribute. Index: defparams.pl 847a849,883 > { > name => 'usermatchmode', > desc => 'Allow match strings to be entered for user names when entering ' . > 'and editing bugs. <p>' . > '"off" disables matching,<br> ' . > '"single-wildcard" allows wildcard matching for exactly one ' . > 'address, which is useful for user privacy purposes,<br>' . > '"multi-wildcard" allows wildcards and displays a select box ' . > 'to choose from among any matches,<br> ' . > 'and "search" allows both wildcards and substring (freetext) ' . > 'matches.', > type => 's', > choices => ['off', 'single-wildcard', 'multi-wildcard', 'search'], > default => 'off' The distinction between "single" and "multi" should be embodied in the maxusermatches parameter, not this one. If maxusermatches is one, we're in single match mode, otherwise we're in multi match mode. Also, is there any reason why we need to distinguish between wildcard and substring modes? Will some installations want one but not the other? b.m.o will almost certainly want both. Anyone else feel differently? > name => 'confirmuniqueusermatch', > desc => 'Whether a confirmation screen should be displayed when only ' . > 'one user matches a search entry', > type => 'b', > default => '1', default => 1 (no quotes, as this is a boolean value) Index: template/en/default/global/code-error.html.tmpl + (Select one):<br> + (Select any matches This instructions are implicit in the field types themselves and are therefore unnecessary.
re: comment 29 > 1. In general, I'd like to see more documentation about how this code works. > I'm having a hard time figuring it out. I suppose I could plop a bunch of inline documentation into the code for now and tear it out when we get close to landing it. > 2. The following template is missing from the latest patch: > > formfieldname.html.tmpl Ack! Fortunately that's just a l10n thing, so it only provides a cosmetic function. Though there is likely to be some kind of debate about whether I'm going about it the right way. > 3. It didn't work for me when I tried to use it. I went to a bug, typed "moz" > into the CC field on my test installation, selected from the list of matches, > and submitted the form, but my selections weren't added to the bug's CC list. > It failed when I selected both a single user and multiple users. Works for me. I'll have to try this on a clean install and see what the trouble is. > 4. I don't see why multi-select menus are being used at all. When you type > a string into the CC field, you almost always intend to add a single user > to the list. The advantages of being able to add multiple users with a single > match string don't outweigh the disadvantages of making the interface clumsier > the vast majority of the time. Not much clumsier. There isn't much of a reason to have more than three selectboxes at a time-- at least for now. I don't think people are any more confused by a select box than a dropdown. Plus, with wildcards, sometimes people will mean to select everything that matches a string, which is the common use of a wildcard. You or I may not want (for example) to select *@mycompany.com, but some people might want the ability. I really don't see select boxes as any clumsier than dropdown boxes, except from a screen-real-estate point of view. > 5. I like this architecture overall, but I think it would work better > for usermatch() to move into User.pm and become validate(). validate() > would take a list of fields (passed as a reference to a hash of hashes > describing the fields), populate the hashes with the matched users, and > return true if all users matched exactly, false otherwise, like so: I kept it out of User.pm because it would have been the first CGI code plopped into there and I didn't want someone using User.pm to be stuck with the CGI interface, since it should be called from many other places in the future. Kind of for the same reason these interface bits don't belong in places like globals.pl I'm not terribly comfortable with any of the options about where to put it, but the part that generates HTML and CGI stuff seems slightly less out of place in CGI.pl Maybe under Bugzilla/User/(something) rather than directly into User.pm? Just a thought. > (in process_bug.cgi) > my $user_fields = { > 'qa_contact' => { 'type' => 'single', value => $::FORM{'qa_contact'} } , > 'newcc' => { 'type' => 'multi', value => $::FORM{'newcc'} } , > 'assigned_to' => { 'type' => 'single', value => $::FORM{'assigned_to'} } , > }; I like it. I'm going to stare at that for a while and see if there's a reason not to change it to that. > if (!validate($user_fields)) { > # by this point the field hashes in $user_fields contain one or more > # additional keys storing user objects for the matching user(s) > $vars->{'user_fields'} = $user_fields; > # ... other necessary $vars declarations here ... > print "Content-type: text/html\n\n"; > $template->process("global/confirm-usermatch.html.tmpl", $vars) > || ThrowTemplateError($template->error()); > } Hmm.... template in the calling code. I guess I don't need to complain about putting CGI and template stuff in Bugzilla/User if we do that. More staring is in order. I am concerned about how much work it will then be to generate confirmation screens in each invocation. We'll be repeating a lot of the same code in each place we call it. > 6. Rather than prepending "wildcard_" to fields being matched, > I suggest using the original field names (and then excluding them > from the list of hidden fields generated by hidden-fields.html.tmpl). > Then you don't have to worry about whether a particular form post > is from the original form or a confirmation page. You only need > to validate the values as usual. > > In order for this to work for the CC list, however, you need to modify > the validation code to build the CC list from either MFORM or FORM. Yeah. The big issue was with fields that can take multiple values, and will have some of the original values persist. If I have six selectboxes for the CC list, it just made it easier to put them into unique names and merge those names in next time the function is called. > > # ...where fieldtype can be 's' (single match only) or 'm' (multi-match) > > You're overloading the single/multi match distinction too much. In one > sense, it means whether or not search results can contain more than one > match. In the other sense, it means whether or not form fields can contain > more than one match string. I suggest using single and multi to refer to > the form fields and some other terminology for the other meaning. Search results containing more that one match is a burden for Param('usermatchmode'). This is for setting the form field types and nothing else. The effect is that you get single-select boxes rather than multi-selects. > Shouldn't "usersearch" be "wildcard"? Ack! That might explain why it wasn't populating the fields for you. I tried to take references to 'wildcard' out of it since it's not just wildcards anymore. Missed it in the template. > > # skip confirmation for exact matches > > if ((scalar(@{$users}) == 1) && > > (@{$users}[0]->{'email'} eq $query)) > > { > > next; > > } > > This won't work for exact matches that are subsets of other matches > because those matches will return more than one user. Searching on b.m.o > for "gerv@mozilla.org", for example, will return both "gerv@mozilla.org" > and "gerv@mozilla.org.uk". > > To solve this problem, you either need to look through every search result > (no matter how many) for that perfect match or (better) query the database > for it beforehand. Exact matches are already checked before substring searches. If an exact match is found, substrings are not checked. It goes: wildcard || exact || substr Wildcards should be checked first, since the search will only be executed if a wildcard character (*) is present. But in youe example, it will return only gerv@mozilla.org and not gerv@mozilla.org.uk. These are the perils of substring searches, and it's done this way so that people entering simple addresses don't get burdened with select boxes every time and that the database doesn't get hit by a substring search every time an address is entered. > @{$users}[0]->{'email'} --> ${$users}[0]->{'email'} I can try it, but I seem to remember that not working... I think I have to cast the ref in $users to an array rather than to a string. It's just ugly. > > { > > name => 'usermatchmode', [...] > > type => 's', > > choices => ['off', 'single-wildcard', 'multi-wildcard', 'search'], > > default => 'off' > > The distinction between "single" and "multi" should be embodied in > the maxusermatches parameter, not this one. If maxusermatches is one, > we're in single match mode, otherwise we're in multi match mode. I had a couple of really good reasons for that, but I'm drawing a blank right now. I'll either explain this nefarious act or change it. > Also, is there any reason why we need to distinguish between wildcard > and substring modes? Will some installations want one but not the other? > b.m.o will almost certainly want both. Anyone else feel differently? We won't be using substr on ours, I think. Forcing the user to specify that they want to search (by using *) should make accidental matches less likely. > Index: template/en/default/global/code-error.html.tmpl > > + (Select one):<br> > + (Select any matches > > This instructions are implicit in the field types themselves > and are therefore unnecessary. Actually, the fact that you have something to select at all isn't in the template until this point, and my experience has been that most (neophyte) users don't know when they're looking at a multi-select box. The hints are not vital, but I don't think they clutter the already small confirmation screen, and anyone who already understands selectboxes shouldn't be offended by getting this kind of generalized talk-down.
>I suppose I could plop a bunch of inline documentation into the code for now >and tear it out when we get close to landing it. Why not leave in those comments? After all, someone's going to have to hack on this code down the road, and they'll need to know the same things I need to know to review it. > formfieldname.html.tmpl > >Ack! Fortunately that's just a l10n thing, so it only provides a cosmetic >function. Though there is likely to be some kind of debate about whether I'm >going about it the right way. It's such a short segment of code. If your goal was to make the code in confirm-usermatch.html easier to read, then put it in a block at the bottom of that file instead, i.e.: [% PROCESS field_names field_name=field.key %] ... [% BLOCK field_names %] [% IF field_name == "foo" %]Foo ... [% END %] [% END %] >Plus, with wildcards, sometimes people will mean to select everything that >matches a string, which is the common use of a wildcard. You or I may not >want (for example) to select *@mycompany.com, but some people might want the >ability. Ok, you're right, I buy that. >I am concerned about how much work it will then be to generate confirmation >screens in each invocation. We'll be repeating a lot of the same code in each >place we call it. You could move that code to CGI.pl as something like "validate_users", passing it the $user_fields reference so it knows what to validate. >> Also, is there any reason why we need to distinguish between wildcard >> and substring modes? Will some installations want one but not the other? >> b.m.o will almost certainly want both. Anyone else feel differently? > >We won't be using substr on ours, I think. Forcing the user to specify that >they want to search (by using *) should make accidental matches less likely. Why wouldn't you want accidental matches? The likelihood of accidentally matching the right person is so much higher than the likelihood of accidentally matching the wrong one that the benefits of not making users go back and figure out someone's exact email address far outweigh the costs of making them much more rarely have to correct it. >Actually, the fact that you have something to select at all isn't in the >template until this point, and my experience has been that most (neophyte) >users don't know when they're looking at a multi-select box. It's a mistake to sacrifice power users for neophytes, and neophyte users won't be using the wildcard search to add multiple users to the list anyway. Once they discover "*" allows them to match multiple users, they'll discover how to add multiple users quickly. >The hints are not vital, but I don't think they clutter the already small >confirmation screen, and anyone who already understands selectboxes shouldn't >be offended by getting this kind of generalized talk-down. I understand selectboxes, and I'm offended, not just as a user who doesn't want to wade through obvious instructions but as a UI designer who doesn't want my users to have to do the same.
Takes into account the above requests.
Attachment #99131 - Attachment is obsolete: true
Comment on attachment 100491 [details] [diff] [review] The latest wildcard and substring userid search Looks good, works good. r=joel Need 2xr from myk
Attachment #100491 - Flags: review+
Comment on attachment 100491 [details] [diff] [review] The latest wildcard and substring userid search General issues: 1. Some discussion in the request tracker bug leads me to believe the minimum number of characters in a match string needs to be parameterized since installations with emailsuffix set to something may have usernames shorter than three characters. A simpler approach would be to require three characters unless emailsuffix contains something, in which case we waive the requirement (and assume the installation is small enough, since everyone has the same domain name on their email address, that it's not a problem). 2. If I enter one character I get the message "<that character> did not match anything". This is misleading since the problem is actually that I entered too few characters, not that the character doesn't appear in any user names. The message should explain the actual problem. 3. When I entered "bar" into the CC field on my landfill-derivative, the first matching result was an empty string. Not sure whether this is a database or a code problem. Index: template/en/default/global/code-error.html.tmpl + [% ELSIF error == "bad_arg" %] + Bad argument '<code>[% argument %]</code>' sent to + <code>[% function %]</code> function. Quotes aren't needed around the argument since you are offsetting it in a <code> section. Index: Bugzilla/User.pm +sub new { + &::SendSQL("SELECT 1, realname, login_name FROM profiles WHERE userid = $id"); + ($exists, $name, $email) = &::FetchSQLData(); + $self->{'exists'} = $exists; I'm wondering if it really makes sense to use "exists" rather than just returning "undefined" if the user doesn't exist, since that's what we return when the ID parameter is invalid, all the other information in the hash (name, email) is empty, and the undefined value is a clear indication that the user doesn't exist. + my $sqlstr = &::SqlQuote($wildstr); + my $query = "SELECT userid, realname, login_name " . + "FROM profiles " . + "WHERE (login_name LIKE $sqlstr "; + $query .= "OR realname LIKE $sqlstr) "; Might as well add this last line to the previous string. + } else { # try an exact match Nit: else on next line per style guidelines. + } else { # try an exact match ... + push(@users, new Bugzilla::User(&::FetchSQLData())) while &::MoreSQLData(); Since login_name is unique, "while &::MoreSQLData()" is unnecessary here. + # then try instr + + if ( + (scalar(@users) == 0) && + (&::Param('usermatchmode') eq 'search') && + (length($str) >= 3) + ){ Nit: first condition on same line as "if" keyword, and "&&" at beginning of next line, per style guidelines. +# match_field() is a CGI wrapper for the match() funtion. +# +# Here's what it does: +# +# 1. Accept a list of fields along with whether they may pass multiple values +# 2. Take the values of those fields from $::FORM and pass them to match() +# 3. Check the results of the match and display confirmation or failure +# messages as appropriate. Nit: "funtion" --> "function", "Accept" --> "Accepts", "pass" --> "take", "Take" --> "takes", "check" --> "checks", "display" --> "displays" +# Bugzilla::User::match_field ( +# 'field_name' => { +# 'type' => fieldtype, +# }, +# 'field_name2' => { +# 'type' => fieldtype, +# }, [...] +# ); Callers should pass a reference to a hash rather than the hash itself. Passing the hash itself makes it very difficult to add parameters to the function should they become necessary in the future, and it makes the syntax in the function inconsistent (because every other hash is a reference whose code uses -> to dereference it, while args is a hash whose code uses args{}). +# fieldtype can be either 'single' or 'multi'. +# + +sub match_field { + + my %args = @_; # arguments as a hash + my $matchsuccess = 1; # did the match fail? + my %returned_matches = (); # the values sent to the template You have defined returned_matches as a hash but are using the dereferencing operator (->) on it below. You should define it as a hash reference instead: my $returned_matches = {}; # the values sent to the template Also, is there a reason why you call it "returned_matches" here but "matches" in the template? What does "returned" mean in this context? + for my $field (keys %args) { + + # We need to move the query to $raw_form + my $raw_form = $::FORM{$field}; + $::FORM{$field} = ''; Please document why we need to move the query string to a local variable. Also, I recommend "raw_field" instead of "raw_form", since "form" connotates the entire form hash rather than one field from it. + my @queries = (); + + # set @queries depending on field type (single|multi) This could use more documentation. + } elsif ($args{$field}->{'type'} eq 'multi') { Nit: elsif on next line per style guidelines. + $raw_form =~ s/[\s,]+/ /g; # Change all delimiters to a single space + @queries = split(' ', $raw_form); There's no need to convert the delimiters first, just do the following: @queries = split(/[\s,]+/, $raw_form); + } else { Nit: else on next line per style guidelines. + # bad argument + $vars->{'argument'} = $args{$field}->{'type'}; + $vars->{'function'} = 'Bugzilla::User::match_field'; + ThrowCodeError('bad_arg'); + } + + for my $query (@queries) { + + $query =~ s/^\s+|\s+$//sg; # trim whitespace Does this apply for multi-select fields? If not, this should be made part of the code that populates the query array with the entire match string for single-select fields. + next if $query =~ /^\s*$/; # skip blank Above comment applies here as well. + my $users = ''; This variable will hold an array reference, so it shouldn't be initialized as a string. I don't think it needs to be initialized at all, actually: my $users; But if does need to be initialized, you should do the following for clarity: my $users = []; + if (&::Param('maxusermatches')) { + $users = match( + $query, # match string + &::Param('maxusermatches') + 1, # match limit + 1 # exclude_disabled + ); + } else { + $users = match($query,'',1); + } You could simplify this code with "(&::Param('maxusermatches') || 0) + 1". + # skip confirmation for exact matches + if ((scalar(@{$users}) == 1) + && (@{$users}[0]->{'email'} eq $query)) + { + $vars->{'form'}->{$field} .= @{$users}[0]->{'email'} . " "; + next; + } + + %returned_matches->{$field}->{$query}->{'users'} = $users; + %returned_matches->{$field}->{$query}->{'status'} = 'success'; + %returned_matches->{$field}->{$query}->{'selecttype'} = + $args{$field}->{'type'}; %returned_matches --> $returned_matches + # here is where it checks for multiple matches + + if (scalar(@{$users}) == 1) { + # exactly one match + $vars->{'form'}->{$field} .= @{$users}[0]->{'email'} . " "; + $need_confirm = 1 if &::Param('confirmuniqueusermatch'); If we don't need to confirm a unique user match, then this query shouldn't need to be added to the returned_matches hash, should it? If not, then this code should be moved up to the "skip confirmation for exact matches" section. + } elsif ((scalar(@{$users}) > 0) Technically this works, but "scalar(@{$users}) > *1*" would make more sense. + && (&::Param('maxusermatches') != 1)) { + $need_confirm = 1; + + if ( (&::Param('maxusermatches')) Nit: extra space. + $vars->{'matches'} = \%returned_matches; # matches that were made \%returned_matches --> $returned_matches Index: template/en/default/global/confirm-usermatch.html.tmpl Per template naming guidelines, use confirm-user-match.html.tmpl. + # matches: hash; Hierarchical hash, containing: + # names_of_fields{ patterns_given{ matches_found } } + # matches_found is an array of Bugzilla::User objects + # matches: hash; Hierarchical. The levels go like this: + # field_name { + # query_text { + # 'users' = @user_list (user objects) + # 'selecttype' = single|multi (selectbox type) + # 'status' = success|fail|trunc (result of search. + # 'trunc' (truncated) means max was reached) + # } + # } These definitions are for the same parameter and need to be combined. + # script: string; The name of the calling script, used to create a + # self-referential URL + #%] + +[% listindex = 1 %] What's the purpose of listindex? It looks like you count with it but don't do anything with the count. + Please carefully examine the following wildcard matches. If they are + incorrect, use the 'Back' button to change the fields. If they are the + correct users, press "Continue." Wildcard isn't the right word here since the user didn't necessarily use one, this message should explicitly say why the user received this page, and it's unnecessary and misleading to tell people to press "Continue" since it's the only option and since no menu items are selected by default (so pressing continue without doing anything won't submit the correct users). Something like the following: One or more of the names/email addresses you entered into fields on the previous page did not match exactly one user. Select the correct users from the lists of possible matches below, or go back to the previous page and try other names/email addresses. + One or more of your pattern matches did not succeed. Please review the + results and use the 'Back' button to go back and revise your search. It would be easier for users to understand this message if it was about what they were doing (assigning users to roles by entering their names into form fields) instead of what Bugzilla is doing (matching entered strings to user records). Something like the following: One or more of the names/email addresses you entered into fields on the previous page did not match any users. Go back to the previous page and try other names/email addresses. + [% FOREACH pattern = field.value %] Any reason why it's "pattern" here but "query" in the Perl code? + [% IF pattern.value.status == 'fail' %] + <font color="#FF0000">returned ambiguous results, which is + not allowed here.</font> This is impossible, so it's unnecessary to check for it. Even if we were worried about bugs in the code, however, we still shouldn't show a message like this, which implies that *they* messed up somehow, to end users. + <option value="[% match.email %] ">[% match.email %] + [% IF match.name.size > 0 %] + ([% match.name %]) + [% END %] + </option> Just use [% match.identity %]. + [% IF pattern.value.status == 'trunc' %] + matched + more than the maximum + of [% pattern.value.users.size %] users:<br> + [% END %] + matched:<br> If status is "trunc" then "matched" appears twice: <string> matched more than the maximum of 1000 users: matched: + <select name="[% field.key %]" id="[% field.key %]" + [% IF pattern.value.users.size > 5 %] + multiple="multiple" size="5"> + [% ELSE %] + multiple="multiple" size="[% pattern.value.users.size %]"> + [% END %] Is this really necessary? Making them all size="5" wouldn't be a bad thing, even if sometimes there was space at the bottom of some of the menus. + [% ELSE %] + matched + [% FOREACH match = pattern.value.users %] + <b>[% match.email %]</b> + [% IF match.name.size > 0 %] + ([% match.name %]) + [% END %] + <br> + [% END %] + [% END %] Here you know the number of matches is one, so no need to loop over them.
Attachment #100491 - Flags: review-
Alias: wildcard
Status: NEW → ASSIGNED
OS: other → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
Attached patch User search function (obsolete) — — Splinter Review
Here's the new patch; comments follow. re: comment 34 > 3. When I entered "bar" into the CC field on my landfill-derivative, the > first matching result was an empty string. Not sure whether this is a > database or a code problem. I couldn't make it do that, and I have no idea where that came from, but I found a couple of issues with the template that might have squashed that. Please let me know if it still does this for you. > + $self->{'exists'} = $exists; > > I'm wondering if it really makes sense to use "exists" rather than just > returning "undefined" if the user doesn't exist, since that's what we return > when the ID parameter is invalid, all the other information in the hash > (name, email) is empty, and the undefined value is a clear indication that > the user doesn't exist. Well, it's garbage in/garbage out at this point. The calling code does a search before creating the object, so it shouldn't need another mechanism for determining whether a user exists (use match for that). I don't have a problem with making it use undef, but we'd be giving it an opportunity to fail hard by trying to use undef as a hash reference. > +# Bugzilla::User::match_field ( > +# 'field_name' => { > +# 'type' => fieldtype, > +# }, > +# 'field_name2' => { > +# 'type' => fieldtype, > +# }, [...] > +# ); > > Callers should pass a reference to a hash rather than the hash itself. > Passing the hash itself makes it very difficult to add parameters to > the function should they become necessary in the future, and it makes > the syntax in the function inconsistent (because every other hash is > a reference whose code uses -> to dereference it, while args is a hash > whose code uses args{}). I have it passing a reference to an anonymous hash now. I didn't see a good reason to declare a hash that would only be used on one line. > + # here is where it checks for multiple matches > + > + if (scalar(@{$users}) == 1) { > + # exactly one match > + $vars->{'form'}->{$field} .= @{$users}[0]->{'email'} . " "; > + $need_confirm = 1 if &::Param('confirmuniqueusermatch'); > > If we don't need to confirm a unique user match, then this query shouldn't > need to be added to the returned_matches hash, should it? If not, then > this code should be moved up to the "skip confirmation for exact matches" > section. The match is added to the list in case one of the other matches triggers a confirmation. That's just so that you get a complete list while you're being pestered about it. > +[% listindex = 1 %] > > What's the purpose of listindex? It looks like you count with it but don't > do anything with the count. That's something that should have been removed in the last patch. > One or more of the names/email addresses you entered into fields on the > previous page did not match exactly one user. Select the correct users from > the lists of possible matches below, or go back to the previous page and try > other names/email addresses. We have to put something in there that will work in cases where single-match confirmation is on. Hopefully the new wording will work: One or more of the names/email addresses you entered into fields on the previous page produced results which require confirmation. Please carefully examine the matches below, selecting users from the lists if necessary, or go back to the previous page to revise the names you entered. > One or more of the names/email addresses you entered into fields > on the previous page did not match any users. Go back to the previous page > and try other names/email addresses. This one has to work with matches that go over the one-match limit, which causes a failure too, so I said 'unable to match': One or more of the names/email addresses you entered into fields on the previous page were unable to match any records. Go back to the previous page and try other names/email addresses.
Attachment #100491 - Attachment is obsolete: true
Comment on attachment 101465 [details] [diff] [review] User search function r=joel
Attachment #101465 - Flags: review+
Comment on attachment 101465 [details] [diff] [review] User search function Nit: there are a bunch of unnecessary whitespace changes in the patch. Index: Bugzilla/User.pm + if ((scalar(@users) == 0) + && (&::Param('usermatchmode') eq 'search') + && (length($str) >= 3) + ){ Nit: per style guidelines, open bracket on its own line and lined up with "if" keyword, like so: + if ((scalar(@users) == 0) + && (&::Param('usermatchmode') eq 'search') + && (length($str) >= 3)) + { +# How to call it: +# +# Bugzilla::User::match_field ({ +# 'field_name' => { +# 'type' => fieldtype, +# }, +# 'field_name2' => { +# 'type' => fieldtype, +# }, [...] +# }); Nit: actual calls to this function use a more compact and readable format, f.e.: +&Bugzilla::User::match_field ({ + 'cc' => { 'type' => 'multi' }, + 'assigned_to' => { 'type' => 'single' }, This explanation should use that same format. +sub match_field { + + my $args = shift; # arguments as a hash Nit: "fields" would probably be a better name for this hash, especially now that it's a single argument to the function. + else { + # bad argument + $vars->{'argument'} = $args->{$field}->{'type'}; + $vars->{'function'} = 'Bugzilla::User::match_field'; + ThrowCodeError('bad_arg'); + } This needs to be &::ThrowCodeError(); Index: template/en/default/global/code-error.html.tmpl + [% ELSIF error == "bad_arg" %] + Bad argument '<code>[% argument %]</code>' sent to + <code>[% function %]</code> function. Nit: unnecessary single quotes around argument. Index: template/en/default/global/confirm-user-match.html.tmpl + [% IF query.value.users.size %] + [% IF query.value.users.size > 1 %] + [% IF query.value.status == 'fail' %] + <font color="#FF0000">returned ambiguous results, which is + not allowed here.</font> Previous review comment not taken into account: "This is impossible, so it's unnecessary to check for it. Even if we were worried about bugs in the code, however, we still shouldn't show a message like this, which implies that *they* messed up somehow, to end users." + <option value="[% match.email %] ">[% match.email %] + ([% match.identity %]) + </option> The "identity" property of User objects contains either email addresses (f.e. "myk@mozilla.org") or name/address combinations (f.e. "Myk Melez <myk@mozilla.org>") depending on whether or not the user's profile includes a name, so this code just needs to be: <option value="[% match.email %] ">[% match.identity %]</option> Btw- why does [% match.email %] get a space appended to it? + [% FOREACH match = query.value.users %] + <option value="[% match.email %] ">[% match.email %] + [% IF match.name.length > 0 %] + ([% match.name %]) + [% END %] + </option> Use identity here as well. + matched + <b>[% query.value.users.0.email %]</b> + [% IF query.value.users.0.name.length > 0 %] + ([% query.value.users.0.name %]) + [% END %] And here. Note that this code currently doesn't support the Request Tracker. I think the best thing to do is get this code into the tree and then modify it to work with the Request Tracker as a separate patch. I've filed bug 172518 on that.
Attachment #101465 - Flags: review-
Question: is there any end-user documentation anywhere to explain to users how these features work?
> - another new tamplate: formfieldname, which translates the name of a form > field to localized text Note: I recently checked in a global template to do this - globals/field-descs.html.tmpl . Please use this rather than reinventing the wheel, if you haven't already done so. Gerv
>Note: I recently checked in a global template to do this - >globals/field-descs.html.tmpl . Please use this rather than reinventing the >wheel, if you haven't already done so. field-descs.html.tmpl isn't going to work in this case because requestee field names vary by flag type ID (i.e. requestee-1, requestee-2) and thus require special logic for matching and constructing their titles.
re: comment 37 > Nit: there are a bunch of unnecessary whitespace changes in the patch. Sorry. There was a lot of trailing whitespace in the checked in code and it was driving me crazy looking at it. > Nit: unnecessary single quotes around argument. Argh! I think I meant to fix that before I sent the patch in. > Previous review comment not taken into account: > > "This is impossible, so it's unnecessary to check for it. Even if we were > worried about bugs in the code, however, we still shouldn't show a message > like this, which implies that *they* messed up somehow, to end users." Actually it was, but I must have forgotten to respond to it. It is not impossible. If you have multiple results and a fail message, it means you have set the maxusermatches param to 1, which per a previous recommendation, replaces the single-wildcard match mode. This is for cases where you don't want users abusing wildcards or substring matches to mine for email addresses. > The "identity" property of User objects contains either email addresses > (f.e. "myk@mozilla.org") or name/address combinations > (f.e. "Myk Melez <myk@mozilla.org>") depending on whether or not > the user's profile includes a name, so this code just needs to be: Right, and it's being used in some places, but if someone's doing a substring or wildcard search for an email address they wouldn't necessarily recognize both the person's email address and their real name, so both should be displayed to make sure the user is selecting the person they think they're selecting. > Note that this code currently doesn't support the Request Tracker. The new match() function should be compatible with the one in Request Tracker, so I assume this means Request Tracker should be using the match_field() function instead. I thought I heard someone say this was unneccesary or undesirable. I could be imagining things. re: comment 38 > Question: is there any end-user documentation anywhere to explain to users > how these features work? I have most of the other requests finished, but I'll wait to post my latest patch until this is addressed. I agree that something should be there, but I'm not 100% sure where this should go. Suggestions are welcome. re: comment 40 > field-descs.html.tmpl isn't going to work in this case because requestee > field names vary by flag type ID (i.e. requestee-1, requestee-2) and thus > require special logic for matching and constructing their titles. I'm not sure it wouldn't be worth the trouble to put the special logic in place, though. If RT is going to be using the confirm page, it's going to be needed no matter what.
>Actually it was, but I must have forgotten to respond to it. It is not >impossible. If you have multiple results and a fail message, it means you have >set the maxusermatches param to 1, which per a previous recommendation, >replaces the single-wildcard match mode. This is for cases where you don't >want users abusing wildcards or substring matches to mine for email addresses. In that case the message should be something like: The name/email address you entered matches multiple users. Please go back and try again with a more specific name/address. >Right, and it's being used in some places, but if someone's doing a substring >or wildcard search for an email address they wouldn't necessarily recognize >both the person's email address and their real name, so both should be >displayed to make sure the user is selecting the person they think they're >selecting. As we discussed on IRC, identity contains both the name and the email address. >The new match() function should be compatible with the one in Request Tracker, >so I assume this means Request Tracker should be using the match_field() >function instead. I thought I heard someone say this was unneccesary or >undesirable. I could be imagining things. Not sure why someone said that, but yes, that's what I meant. >> Question: is there any end-user documentation anywhere to explain to users >> how these features work? > >I have most of the other requests finished, but I'll wait to post my latest >patch until this is addressed. I agree that something should be there, but I'm >not 100% sure where this should go. Suggestions are welcome. I don't think lack of user docs should block this patch. Docs are only one way users will discover this feature, and we don't have docs for most of our other features either. >I'm not sure it wouldn't be worth the trouble to put the special logic in >place, though. If RT is going to be using the confirm page, it's going to be >needed no matter what. It's not so simple. field-descs.html.tmpl defines a hash of field names and their corresponding titles, which allows no code. Your code, however, uses a conditional, which does allow the code that RT will need. If you really want to reduce the duplication, however, you could use field_descs when it works and fall back to special code.
Have at it. In addition to the above, I added 'FILTER html' where appropriate in the template and fixed a possible (but not yet manifest) issue where $::FORM was being populated with search results but $::MFORM was not. Documentation is not yet available. I'm assuming we can add that after the patch lands.
Attachment #101465 - Attachment is obsolete: true
Comment on attachment 102056 [details] [diff] [review] user wildcard and substring search Sooooo close :-) If a CC list uses a wildcard that expands to several matches and the resulting multi-select has several items selected, the routine fails to space-seperate the list of addresses.
Attachment #102056 - Flags: review-
Right back at ya. This one should properly mangle $::FORM in wildcard mode.
Attachment #102056 - Attachment is obsolete: true
Comment on attachment 102526 [details] [diff] [review] (fixed?) user wildcard and substring search r=joel
Attachment #102526 - Flags: review+
In the previous patch, if you had QA Contacts turned off and wildcard or substring searching turned on, Perl would complain about trying to use an undefined scalar as an array ref. This patch causes it to be tolerant of that situation, which unfortunately means that calling wildcard search on incorrect values will not raise any errors (though it won't actually return any results). So mind your P's and Q's when calling it and everything should be fine. No other changes. This is a one-line fix.
Attachment #102526 - Attachment is obsolete: true
Comment on attachment 103375 [details] [diff] [review] Wildcard/substring search patch r=joel (not worried that this won't abort on a missing field because subsequent code can still determine if a field was required and is missing.... this patch won't transform an undefined field into a blank one.)
Attachment #103375 - Flags: review+
Comment on attachment 103375 [details] [diff] [review] Wildcard/substring search patch There are three diff fragments in User.pm whose only change is to mangle the indentation level: @@ -101,7 +101,7 @@ @@ -122,13 +122,13 @@ @@ -153,8 +153,8 @@ Fix those and r=myk
Attachment #103375 - Flags: review+
Indent levels fixed and checked in for not_erik
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
*** Bug 146162 has been marked as a duplicate of this bug. ***
Blocks: 146162
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: