237683 - editusers.cgi: Allow for more flexible user searching such as real name, email, and userid

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[David Lawrence [:dkl]]

editusers.cgi: Allow for more flexible user searching such as real name, email,
and userid. Patch coming that adds a drop down menu on the search screen
selecting which type of search desired.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add more flexible searching in edituser.cgi

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch to add more flexible searching in edituser.cgi

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 144058 [details] [diff] [review]
Patch to add more flexible searching in edituser.cgi

okay, backing up one page after submitting an attachment is not a good idea.
Please disregard the dual submissions error.

Comment 4

[Asa Dotzler [:asa]]

Myk, with the size of bmo's user list, this would be a huge help for admins
trying to locate a user. Can you take a look at this patch?

Comment 5

[Marc Schumann [:Wurblzap]]

Might this perhaps be an easier change after bug 119485 has landed?

Comment 6

[Marc Schumann [:Wurblzap]]

Comment on attachment 144059 [details] [diff] [review]
Patch to add more flexible searching in edituser.cgi

This doesn't apply any more since bug 119485.

Comment 7

[Joost de Valk]

I would really love this to be implemented, someone care to fix the patch?

Comment 8

[David Lawrence [:dkl]]

Attachment: Updated patch to add more flexible searching in edituser.cgi (v2)

Updated to HEAD

Comment 9

[GavinS]

Comment on attachment 211946 [details] [diff] [review]
Updated patch to add more flexible searching in edituser.cgi (v2)

1) The text still has 'login name' in it, so you get 'List users with {login name} login name matching'

2) Not providing any text to search on should find all users, I think,  like it did before. It does do this for a real name search, but not for a user id search.

3) The new param is not stored, so that a 'back to list' after viewing a user does not work properly. (Need to store 'matchvalue' in editusers.cgi:mirrorListSelectionValues(), I think)

4) (NIT) Could reject a non numeric user id search string with an error

Comment 10

[David Lawrence [:dkl]]

Attachment: Updated patch to add more flexible searching in edituser.cgi (v3)

1. Removed login name from text after matchvalue selection drop down.
2. Created error message to be displayed if user id is entered and not valid integer format.
3. Added matchvalue to mirrorListSelectionValues().
4. Empty matchstr should bring up entire user list regardless of matchvalue.
5. Reworked section of code to be fit better into the old way of forming the query.

Comment 11

[Frédéric Buclin]

Comment on attachment 212253 [details] [diff] [review]
Updated patch to add more flexible searching in edituser.cgi (v3)

>Index: editusers.cgi

>+    my $matchvalue    = $cgi->param('matchvalue');

Instead of checking everytime if $matchvalue is defined, you should write:
my $matchvalue    = $cgi->param('matchvalue') || '';


>+            if (defined($matchvalue) && $matchvalue eq 'userid') {
>+                if ($matchstr) {
>+                    ThrowUserError('illegal_user_id', {userid => $matchstr})
>+                        if $matchstr !~ /^\d+$/;
>+                }

Use detaint_natural() instead of this regexp.



>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "illegal_user_id" %]
>+    [% title = "Illegal User ID" %]
>+    User ID '[% userid %]' is not valid integer.
>+

Unfiltered directive! userid may contain JS code! Run ./runtests.pl before posting a patch.

Comment 12

[David Lawrence [:dkl]]

Attachment: Updated patch to add more flexible searching in edituser.cgi (v4)

Careless errors. Hopefully better this time.

1. Changed to my $matchvalue = $cgi->param('matchvalue') || ''; and removed
defined checks.
2. Changed to use detaint_natural instead of regex to check for valid integer.
3. Add FILTER html to userid directive in error message.

Thanks!

Comment 13

[GavinS]

Comment on attachment 212258 [details] [diff] [review]
Updated patch to add more flexible searching in edituser.cgi (v4)

-<p><label for="matchstr">List users with login name matching</label>
+<p><label for="matchstr">List users with</label>

I think this <label> is wrong. We currently have 2 labels for "matchstr", which I doubt is a good thing. Make the first one a label for "matchvalue", maybe, or remove one of them. Apart from that issue, this patch is OK with me.

Comment 14

[David Lawrence [:dkl]]

(In reply to comment #13)
> (From update of attachment 212258 [details] [diff] [review] [edit])
> -<p><label for="matchstr">List users with login name matching</label>
> +<p><label for="matchstr">List users with</label>
> 
> I think this <label> is wrong. We currently have 2 labels for "matchstr", which
> I doubt is a good thing. Make the first one a label for "matchvalue", maybe, or
> remove one of them. Apart from that issue, this patch is OK with me.
> 

I thought it might not work either, but in testing clicking on both groups of
text did the right thing from what I could tell. It would both put the cursor
into the matchstr text entry box. I can change the label to the left of matchvalue to focus the matchvalue select drop down just to be sure. Now that
I think about it that is better form anyway.


Dave

Comment 15

[Marc Schumann [:Wurblzap]]

http://www.w3.org/TR/html4/interact/forms.html#h-17.9 says: "More than one LABEL may be associated with the same control by creating multiple references via the for attribute", so having two <label>s for one form control is legal. A different question is whether we want to do it -- we're currently in the process of fixing bug 281691, which cares about screen readers. I suspect screen readers might take offense on multiple <label>s for one form control, because they need to decide which one to read.

I'm currently inclined to say that screen readers need to cope with what's legal -- what's your thoughts?

Comment 16

[David Lawrence [:dkl]]

Attachment: Updated patch to add more flexible searching in edituser.cgi (v5)

Gave 'matchvalue' it's own label for now to allow patch to be submitted. We should discuss whether we should allow multiple labels to point to a single form control for future use but I felt for now it was just best to keep them separate.

Comment 17

[David Lawrence [:dkl]]

Thanks for the approval. I am not sure I am set up anymore to commit to CVS so if someone else has the time to check this in I would appreciate it.

Comment 18

[Vlad Dascalu]

Thanks, David, keep these coming :)

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.114; previous revision: 1.113
done
Checking in template/en/default/admin/users/search.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/search.html.tmpl,v  <--  search.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.147; previous revision: 1.146
done