490923 - Autocomplete assignee, qa contact, and cc on the client side

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

I can't believe this bug isn't already filed.

We should have an auto-complete drop-down for the assigned_to, qa_contact, cc, and requestee fields that uses XML-RPC or JSON-RPC to get the names of users. We already have the necessary WebService function, User.get, and all we need to do is implement the client side.

Performance should be taken somewhat into account, only in the sense of "let's try to avoid overwhelming the server", but usability is the most important concern.

Note that Everything Solved wrote some code that does this that we could possibly re-use parts of.

Comment 2

[Max Kanat-Alexander]

Attachment: Work In Progress

Comment 3

[Frédéric Buclin]

We no longer accept new features for Bugzilla 3.6. Retargetting to 3.8.

Comment 4

[Guy Pyrzak]

Attachment: V1

Here is my version of the Work In Progress. 

It works really well. However... to implement this I had to edit userselect.html.tmpl
which is basically used all over the place but i only added the JS and CSS to the edit bug pages. 

It would be easy to copy the css/js includes to multiple places or just include them in global (which i don't recommend). mkanat and others take a look at the patch and let me know what you guys think.

btw. wow it was easy to do this!

Comment 5

[Guy Pyrzak]

after thinking this over i'm not sure if i should have added the autocomplete to userselect.html.tmpl. I could have just done it on edit/create. what is better? Adding autocomplete everywhere userselect is or just on edit/create? 

Do we add this to the flag UI as well?

Comment 6

[Guy Pyrzak]

something i just noticed with this. the @ sign gets converted to %40 by the YUI json stringify and then the backend throws an error. Do we not want it to convert the %40's and other encode characters or is this a bug in the webservice?

Comment 7

[Guy Pyrzak]

I'm not sure we should add autocomplete for the requestee field without being able to pass in the flag information. Otherwise we'll introduce another complication to the user. The UI will encourage them to fill in the autocomplete with a full name which might not have authority to set the flag. This could be especially frustrating for when a user has 2 emails for bugzilla (i'm looking at you BMO folks!) ;).

Comment 8

[Jesse Ruderman]

If you show the "real name" field in addition to the email address, the ambiguity shouldn't be a problem.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 427739 [details] [diff] [review]
V1

  Awesome!! :-)

>=== modified file 'js/util.js'
>--- js/util.js	2009-02-11 19:41:14 +0000
>+++ js/util.js	2010-02-19 09:59:39 +0000
>@@ -271,3 +271,48 @@
>         YAHOO.util.Dom.addClass(anElement, aClass);
>     }
> }
>+
>+/**
>+ * The Autoselect
>+ */
>+YAHOO.bugzilla.userAutocomplete = {

  This should probably go in field.js instead of util.js. Util is for short utility functions.

  What does this do on error? Is there any way of debugging it if it fails for a user?

>+    jsonRpcCallCounter : 0,

  I'd suggest either using this for the id, or not having it.


>+    generateRequest : function ( sQuery ){ 

  Any particular reason to call it sQuery? I'm not quite sure what that means.

>+      var json_object = {
>+          method : "User.get",
>+          id : 1,
>+          params : [ { 
>+            match : [ sQuery ]
>+          } ]

  You should also set include_fields to limit the data transfered over the wire.

>=== modified file 'template/en/default/bug/edit.html.tmpl'

  Looks like you included the "fix space in summary" patch here.

>=== modified file 'template/en/default/bug/show-header.html.tmpl'
>+[% javascript_urls = [ "js/yui/datasource.js", "js/yui/connection.js", 
>+                       "js/yui/json.js", "js/yui/autocomplete.js",

  Wow, there's no bundle for all of those, eh? We could just make our own....

>@@ -44,6 +47,7 @@
>                   "bz_product_$bug.product",
>                   "bz_component_$bug.component",
>                   "bz_bug_$bug.bug_id",
>+                  "yui-skin-sam"

  Usually I do this only on particular elements. If we're going to do it globally on show_bug, we might as well do it globally on all pages.

>=== modified file 'template/en/default/global/userselect.html.tmpl'
>+  [% IF id %]
>+    <div id="[% id FILTER html %]_AutoComplete" >

  Unless it's required, don't CamelCase the id.

>+  [% IF id %]
>+      <div id="[% id FILTER html %]_AutoCompleteContainer"></div>

  Same there.

>+    <script type="text/javascript">
>+      YAHOO.bugzilla.userAutocomplete.init( "[% id FILTER html %]", 
>+                    "[% id FILTER html %]_AutoCompleteContainer"
>+                    [% IF multiple %], true[% END%]);         

  Those filters should be "js" instead of "html".

  And yeah, userselect is definitely the right place to put this.

Comment 10

[Max Kanat-Alexander]

Oh, also, you'll also have to either check feature_enabled('jsonrpc') or make all the json-rpc stuff into required modules.

Comment 11

[Guy Pyrzak]

which direction do you prefer, checking or requiring it? I'm indifferent.

I think it fails silently. I can add some tooling to make it fail noisily or let users add a fail function listener.

it's called sQuery b/c that's what they called it in YUI, i think it stands for string Query, if you have a preferred term let me know, I can call it textEntered?

I can make a bundle of the javascript if you like, but if we're going to do that we should probably do it for EVERYTHING in edit bugs or global.

It seems like you're suggesting just move it to global, so i can combine them there.

No worries with the camel case, i was following YUI's convention and I couldn't remember bz's convention.

js filter, got it.

let me know how you want to handle the questions i had. I can move the code and make the tiny updates but the big question is:

Do we require it or do we just check for feature_enabled?

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> which direction do you prefer, checking or requiring it? I'm indifferent.

  Hmmm...for now let's check feature_enabled. I think the only place we need to do that is when adding the listener in userselect.

> I think it fails silently. I can add some tooling to make it fail noisily or
> let users add a fail function listener.

  I think probably the simplest would be to do like I did in the original patch, so people can look at the console if they have one and want to see what's going on.

> it's called sQuery b/c that's what they called it in YUI, i think it stands for
> string Query, if you have a preferred term let me know, I can call it
> textEntered?

  Hmm, yeah, textEntered sounds better.

> I can make a bundle of the javascript if you like, but if we're going to do
> that we should probably do it for EVERYTHING in edit bugs or global.

  Well, no, not everything, because we use those items separately. I think that we'll always be using this other stuff together, at least for now. I just don't want to slow down the show_bug page that much for people with high-latency connections.

> It seems like you're suggesting just move it to global, so i can combine them
> there.

  No, I was only suggesting moving the CSS class.

  However, userselect.html.tmpl *is* used in other places, so we might have to account for that somehow.

Comment 13

[Guy Pyrzak]

Attachment: v2

Comment 14

[Frédéric Buclin]

Please fix the indentation in default/global/userselect.html.tmpl, especially for <input> which should be vertically aligned with the previous [% END %] and the 2nd [% IF %] block where the indentation is totally confusing. Especially, its corresponding [% END %] should be aligned with [% IF %].

Comment 15

[Guy Pyrzak]

nits fixed. not bothering to repost until i get further fixes, but I'll repost it if that's the only thing you want me to fix.

Comment 16

[Guy Pyrzak]

Attachment: v3

Comment 17

[Guy Pyrzak]

Attachment: v4

Comment 18

[Guy Pyrzak]

Attachment: v5

added all the stuff necessary to get this to work in all the UIs that use userselect.html.tmpl

Comment 19

[Guy Pyrzak]

please note we should make sure the down arrow autocomplete still works for those pros who love it. Not sure how we do that thought

Comment 20

[Max Kanat-Alexander]

Comment on attachment 427995 [details] [diff] [review]
v5

>=== modified file 'js/field.js'
>--- js/field.js	2010-02-18 18:53:30 +0000
>+    debug_helper : function ( ){
>+        /* used to help debug any errors that might happen */
>+        if( typeof(console) !== 'undefined' && console != null && arguments.length > 0 ){

  Not that this hugely matters, but you might want to check if (console &&) first, which I *think* works even for undefined variables...but maybe not?

>=== modified file 'skins/standard/show_bug.css'
>+/* custom styles for inline instances */

  Inline instances of what?

>=== modified file 'template/en/default/global/header.html.tmpl'
>+               [% END %] yui-skin-sam">

  Cool. We should probably remove any other yui-skin-sam classes remaining, no?

>=== modified file 'template/en/default/global/userselect.html.tmpl'
>+    <div id="[% id FILTER html %]_autocomplete" class="">

  Why class=""?

>+    <script type="text/javascript">
>+      YAHOO.bugzilla.userAutocomplete.init( "[% id FILTER js %]", 
>+                    "[% id FILTER js %]_autocomplete_container"
>+                    [% IF multiple %], true[% END%]);         
>+    </script>

  If you want, theoretically this is the only thing that needs the jsonrpc check.


  Everything else looks cool! This is r+ from me, although I'm still going to test it, which I haven't yet.

Comment 21

[Max Kanat-Alexander]

Works great!

It's really slow when we're not running under mod_perl, though. At least part of that is contributed to by the queryDelay--setting it to 0 causes a bit too much churn in JavaScript land, but making it 0.05 seems better to me, since we already have the 3-character limit set. Any thoughts on that?

Comment 22

[Guy Pyrzak]

The console check is the right way to do it across all browsers. I used to have checking for just if( console) and it threw errors for LpSolit

the comment refers to Inline  instances of the autocomplete I can add that to the comment.

I can remove the other yui-skin-sam if you like or it can be part of another bug

the class="" was left over. I'll remove that. 

I'd rather the whole thing be in the check instead of just the javascript.

I'll set it to 0.05, that's fine with me, I'll add a comment so people to know it is editable.

I still haven't handled the autocomplete problem which is what caused this feature to get yelled at last time.

Specifically the BMO folks REALLY like using the built in browser autocomplete. By that i mean the fact that the browser remembers what you've typed in previously instances of a text box and shows it in a drop down. 

To see what i'm talking about try pressing the down arrow on the quicksearch and it will show you everything you've ever typed. When we did this with the keyword some users got VERY upset, specifically because they would prefer it over the autocomplete which required more typing. @samuel.sidler 

So I don't know how we want to handle it. Any suggestions on this point max or LpSolit? 

I'm happy to take another bug that lets users turn off this feature via personal prefs OR something else, but i do think having a way to turn off this and other autocompletes is important for the experts out there who really just prefer to not have autocompletes.

Also I just noticed that bug 455810 is already fixed on BMO so i'm happy to fix it the EXACT same way on bugzilla.

Comment 23

[Guy Pyrzak]

removing requestee from the title since that requires webservices we don't have yet.

Comment 24

[Guy Pyrzak]

Attachment: V6

hopefully the last version. Cleaned up some stuff and also moved the inline css from show_bug.css to global.css since there are at least 3 other UI's that use the userselect.html.tmpl stuff.

I added more comments, set the query throttle to 0.05 and tried to address anything that was left. Basically if mkanat r+'s this we can check this puppy in i think. 

I also noticed that the keyword autocomplete is already on keywords for BMO and i guess people like it so I'm going to assume they'll like this one as well.

Comment 25

[Guy Pyrzak]

I was testing this fix and it turns out it still doesn't work because if you go to the edit component page this doesn't work. I'm tempted to move the autocomplete into the rollup what do you think max?

Comment 26

[Guy Pyrzak]

Attachment: v7

after some testing it looks like adding this capability to some pages (specifically the edit component page) is more trouble than it is worth. So instead I made sure that if the JS required to generate the autocomplete isn't on the page the user does not see any errors instead it just skips it. i use the same technique I use for the console.

for those playing along at home this still adds the autocomplete to the edit/create and edit multiple UIs just not any of the admin UIs

Comment 27

[Frédéric Buclin]

(In reply to comment #26)
> after some testing it looks like adding this capability to some pages
> (specifically the edit component page) is more trouble than it is worth.

What's the problem with adding/editing components? Why is it harder?

Comment 28

[Guy Pyrzak]

There is some strange layout issue with tables (i can post a screen shot). I will investigate it further if that is desired. 

The other problem is that I would have to include fields.js on the edit components page or move it to somewhere else and including fields.js seemed like a lot for just 1 thing. I'll start working on it and have a patch ready if you guys would like me to add it.

Comment 29

[Guy Pyrzak]

Attachment: V9

this is a version that better supports the inline text elements. It also supports create and edit components

Comment 30

[Frédéric Buclin]

Comment on attachment 428071 [details] [diff] [review]
V9

Except my name is mangled by JSON-RPC, which I guess is a separate bug, everything else is working fine. So r=LpSolit

Comment 31

[Max Kanat-Alexander]

(In reply to comment #22)
> I can remove the other yui-skin-sam if you like or it can be part of another
> bug

  It's all the same to me.

> I'd rather the whole thing be in the check instead of just the javascript.

  Why?

> I still haven't handled the autocomplete problem which is what caused this
> feature to get yelled at last time.

  This feature has never been implemented.

> Specifically the BMO folks REALLY like using the built in browser autocomplete.

   I haven't seen that feedback.

> To see what i'm talking about try pressing the down arrow on the quicksearch
> and it will show you everything you've ever typed. When we did this with the
> keyword some users got VERY upset, specifically because they would prefer it
> over the autocomplete which required more typing. @samuel.sidler 

  So just ss mentioned it?

> So I don't know how we want to handle it. Any suggestions on this point max or
> LpSolit? 

  YUI's autocomplete has a mechanism for including that in the drop-down, I believe, if you really want it.

> I'm happy to take another bug that lets users turn off this feature via
> personal prefs OR something else, but i do think having a way to turn off this
> and other autocompletes is important for the experts out there who really just
> prefer to not have autocompletes.

  I disagree--I think that such a preference would only accommodate a very small percentage of our userbase, and we don't add preferences that only a very small percentage needs. Here's a good essay on why:

  http://www106.pair.com/rhp/free-software-ui.html

> Also I just noticed that bug 455810 is already fixed on BMO so i'm happy to fix
> it the EXACT same way on bugzilla.

  That UI is problematic, we can talk about it in that bug.

Comment 32

[Max Kanat-Alexander]

(In reply to comment #23)
> removing requestee from the title since that requires webservices we don't have
> yet.

  Just add it to the field, for now--we can spiff it up and limit it later.

Comment 33

[Max Kanat-Alexander]

(In reply to comment #28)
> There is some strange layout issue with tables (i can post a screen shot). I
> will investigate it further if that is desired. 

  Is it still a problem with your latest patch? A screenshot would be good.

> The other problem is that I would have to include fields.js on the edit
> components page or move it to somewhere else and including fields.js seemed
> like a lot for just 1 thing. I'll start working on it and have a patch ready if
> you guys would like me to add it.

  That's fine, you can include fields.js on the editcomponents page.

Comment 34

[Frédéric Buclin]

(In reply to comment #33)
>   Is it still a problem with your latest patch? A screenshot would be good.

Not a problem anymore. The UI is fine.

Comment 35

[Guy Pyrzak]

> (In reply to comment #28)
the strange table layout is fixed in the v9 patch. And i did include field.js as part of the v9 patch.

I think we should do requestee stuff in another patch since this one is almost done and requestee appears on the attachment page and other places. I will do the requestee stuff immediately but there is a bunch of flag and attachment stuff I'd have to test/check and right now v9 is SOOOOOOO ready to be committed.

But if you insist, I'll do the requestee stuff.

Comment 36

[Frédéric Buclin]

(In reply to comment #35)
> But if you insist, I'll do the requestee stuff.

IMO, a separate bug is fine.

Comment 37

[Max Kanat-Alexander]

Yeah, doing the requestee stuff somewhere else is totally fine.

Comment 38

[Guy Pyrzak]

bzr commit --fixes mozilla:490923
Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                       
modified js/field.js
added js/yui/bz_autocomplete_bundle.js
modified skins/standard/global.css
added skins/standard/yui/autocomplete.css
modified template/en/default/admin/components/create.html.tmpl
modified template/en/default/admin/components/edit.html.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/bug/show-header.html.tmpl
modified template/en/default/bug/create/create.html.tmpl
modified template/en/default/global/header.html.tmpl
modified template/en/default/global/userselect.html.tmpl
modified template/en/default/list/list.html.tmpl
Committed revision 7018.

Comment 39

[Max Kanat-Alexander]

Added to the release notes in bug 604256.

Comment 40

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #37)
> Yeah, doing the requestee stuff somewhere else is totally fine.

Was a (new) bug ever filed for doing that?  I've searched and not found anything :(

(If so, bug 386600 should be re-duped to the new bug, since 386600 was explicitly about the review requestee fields and this seems not to have fixed that.  If a new bug wasn't filed, perhaps re-open bug 386600?)