JavaScript autocomplete for review request field

RESOLVED FIXED in Bugzilla 4.0

Status

()

enhancement
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: ted, Assigned: guy.pyrzak)

Tracking

unspecified
Bugzilla 4.0
Dependency tree / graph
Bug Flags:
approval +
approval4.0 +
blocking4.0 +

Details

(Whiteboard: [wanted-bmo])

Attachments

(1 attachment, 6 obsolete attachments)

I searched but didn't find a dupe of this, so I'm filing new.  I'm attaching a patch that implements dropdown autocomplete on the review request field.  the JS magic is done using jQuery and a jQuery autocomplete implementation that I found online.  I'm sure it could be swapped out for pretty much any JS toolkit implementation.  I didn't include the jQuery source or the JS autocomplete source in my patch, since they'd just make it harder to read, and I didn't write them anyway.  Presumably this could also be hooked up to the CC field, etc, but I didn't get around to that.
Comment on attachment 270598 [details] [diff] [review]
add JS autocomplete to review request fields

>                     # Certain versions are broken, development versions are
>                     # always disallowed.
>-                    blacklist => ['^3\.000[3-6]', '_'],
>+                    blacklist => ['^3\.000[3-5]', '_'],
>                 },
>                 name => 'MySQL'},

There are reasons why these MySQL DBI versions are blacklisted. This should not be included in your patch.
(In reply to comment #1)
> There are reasons why these MySQL DBI versions are blacklisted. This should not
> be included in your patch.

Yeah, oops.  That was a hack to get things working on Ubuntu.  Didn't mean to include that.
 
Removed that file from the diff.
Attachment #270598 - Attachment is obsolete: true

Comment 4

12 years ago
Ted, without the missing JS files, what do you expect us to do with your patch?
Got my bugzilla install back up and running, you can see this in action here:
http://bugzilla.rawr.homeip.net/attachment.cgi?id=1&action=edit

I loaded a bunch of random usernames into my db, so just try setting r? and typing some characters into the requestee field.
Posted patch complete patch (obsolete) — Splinter Review
Well, I hacked it together in a few hours, and I just figured bugzilla was a better home for it than sitting in my tree.  Here's a complete patch, for reference.

Comment 7

12 years ago
Comment on attachment 270604 [details] [diff] [review]
complete patch

>Index: js/jquery-latest.pack.js

>+eval(function(p,a,c,k,e,d){e=function(c){return(c<a?"":e(parseInt(c/a)))+((c=c%a)>35

Huh? What's that??
I don't know, I didn't write it, and the JS is obviously compressed, which is why I left it out of my original patch.  It's just the jQuery library, downloaded from http://jquery.com/ .

Comment 9

12 years ago
Fwiw you'd be better off using the minified+gzipped jQuery, not the packed one. It's smaller, and faster on the users' machines as it doesn't have to be unpacked before use.
Assignee

Comment 10

10 years ago
I've been thinking of doing a few things with the autocomplete. The requestee should be easy along with most of the other person fields. I'd also like to see something that integrates quicksearch into the duplicates field. I'll see if I can revive this along with the other autocomplete ideas I've had.
The patch here is pretty simple, I just took an off-the-shelf jQuery autocomplete plugin and attached it to the review request field. The other interesting part of the patch is queryuser.cgi, which is a CGI I added that takes a q=foo param, and returns one line of "login_name|realname" per user whose login name or real name matches "%foo%".
I know that this bug has a patch on it and lots of discussion, but it's actually a little too narrow. We want autocomplete for all user-based fields. Also, we don't want to use jQuery, but YUI, which is what is already in Bugzilla and which supports autocomplete.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 490923
That's fine. I just hacked it into the review field because that was what annoyed me the most. Also, I don't think any of the YUI stuff existed when I wrote this patch.
Reopened, as bug 490923 finally didn't implement it.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → NEW
Whiteboard: [wanted-bmo]
Assignee

Comment 15

9 years ago
It would take 1 line of code to implement this on 4.0 or trunk. But the problem is the field will not know if the users that show up are allowed to + the flag, so users will run into that after the save.
Assignee

Comment 16

9 years ago
Posted patch 4.0 Version (obsolete) — Splinter Review
Assignee: attach-and-request → guy.pyrzak
Attachment #270599 - Attachment is obsolete: true
Attachment #270604 - Attachment is obsolete: true
Attachment #487037 - Flags: review?(mkanat)
Attachment #487037 - Flags: review?(LpSolit)
Assignee

Updated

9 years ago
Flags: blocking4.0?
I don't see why it should block 4.0.
Severity: normal → enhancement
Status: NEW → ASSIGNED
Assignee

Comment 18

9 years ago
well mostly because adding autocomplete is a big feature of 4.0 and not having it for flags is almost a bug than an enhancement.
Comment on attachment 487037 [details] [diff] [review]
4.0 Version

>=== modified file 'template/en/default/flag/list.html.tmpl'

>                 [% ELSE %]
>+                  [% INCLUDE global/userselect.html.tmpl

What's the point to duplicate the code in the ELSE block when it's already in the IF block? Looks like both should be combined.
Attachment #487037 - Flags: review?(mkanat)
Attachment #487037 - Flags: review?(LpSolit)
Attachment #487037 - Flags: review-
Assignee

Comment 20

9 years ago
Posted patch v2 (obsolete) — Splinter Review
you were correct. I was trying to change as little code as possible b/c we're so close to rc1. Here is the simplified version.
Attachment #487037 - Attachment is obsolete: true
Attachment #487056 - Flags: review?(mkanat)
Attachment #487056 - Flags: review?(LpSolit)

Updated

9 years ago
Flags: blocking4.0? → blocking4.0+

Updated

9 years ago
Target Milestone: --- → Bugzilla 4.0
Comment on attachment 487056 [details] [diff] [review]
v2

The autocomplete parts look fine, but the rest of this is in code that LpSolit owns, so I'll let him review it.
Attachment #487056 - Flags: review?(mkanat) → review+
Comment on attachment 487056 [details] [diff] [review]
v2

>=== modified file 'template/en/default/flag/list.html.tmpl'

>+                [% END %]                  
>                   [% INCLUDE global/userselect.html.tmpl

Please fix the indentation. [% INCLUDE %] should now be vertically aligned with [% END %].


>-                             emptyok  => 1

This removal is incorrect. I can no longer remove the requestee.


>+                             classes => ["bz_userfield", "requestee"]

Is the bz_userfield class needed? Why not adding the new CSS rule to the .requestee class instead? (and why wasn't this class needed till today, when usemenuforusers was turned on?)


>+                             size => 30

30 is too large, see the current tip UI. This regresses bug 455559, see the patch there.


>           <span style="white-space: nowrap;">
>-            [% IF Param('usemenuforusers') %]
>               [% INCLUDE global/userselect.html.tmpl

Also fix the indentation here, now that [% IF %] is gone.


>+                          custom_userlist => type.grant_list

Don't call type->grant_list when usemenuforusers is turned off. This method is slow, see bug 528230, and is never used when this parameter is off.


Note also that this patch only applies cleanly to the 4.0 branch, not to 4.1.
Attachment #487056 - Flags: review?(LpSolit) → review-
Assignee

Comment 23

9 years ago
Posted patch v3 (obsolete) — Splinter Review
did what LpSolit wanted. I will provide a 4.1 patch once i get an R+
Attachment #487056 - Attachment is obsolete: true
Attachment #489363 - Flags: review?(LpSolit)
(In reply to comment #22)
> >+                             classes => ["bz_userfield", "requestee"]
> 
> Is the bz_userfield class needed? Why not adding the new CSS rule to the
> .requestee class instead? (and why wasn't this class needed till today, when
> usemenuforusers was turned on?)

Could I have an answer to this question, please?


> >+                             size => 30
> 
> 30 is too large, see the current tip UI. This regresses bug 455559, see the
> patch there.

This comment hasn't been addressed. Consider this a r-.
Assignee

Comment 25

9 years ago
the answer to both of your questions are "they are not needed". They should have beed deleted. My apologies. Assuming those strings are deleted, anything else?
Assignee

Comment 26

9 years ago
Posted patch V4Splinter Review
i don't know what happened with that previous patch. i meant to delete both of the references.
Attachment #489363 - Attachment is obsolete: true
Attachment #491593 - Flags: review?(LpSolit)
Attachment #489363 - Flags: review?(LpSolit)
Comment on attachment 491593 [details] [diff] [review]
V4

r=LpSolit
Attachment #491593 - Flags: review?(LpSolit) → review+

Updated

9 years ago
Flags: approval4.0+
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/attachment/create.html.tmpl
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/flag/list.html.tmpl
Committed revision 7607.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified template/en/default/attachment/create.html.tmpl
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/flag/list.html.tmpl
Committed revision 7486.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 182396
You need to log in before you can comment on or make changes to this bug.