585802 - Change the cc/user autocomplete (and backend) usermatching to ignore spaces / search on space separated names

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[LegNeato]

Searching for people to CC on bmo is somewhat annoying. This is mainly due to the fact that searching for "Jim b" returns all results with "Jim" in it as well as all results with "b" in it, which results in tons you need to sort though. I would imagine most people would expect to get only the records with "Jim b" in it.

On line 715 of js/field.js, there is:

userAutoComp.delimChar = [","," "];

which basically instructs YUI to act this way (for the frontend case). The space should be removed so only commas are used to delineate queries.

I don't believe this is as much as a problem for keywords because bmo keywords do not contain spaces. When you search for keywords using space it behaves as you would expect (does a different query and lumps those results in).

Comment 1

[Max Kanat-Alexander]

This seems sensible to me. LpSolit?

Comment 2

[Frédéric Buclin]

Sure, this seems sensible to me as well.

Comment 3

[Max Kanat-Alexander]

Okay. LegNeato, do you want to provide the patch? You'll want to change both the YUI code and everywhere that receives a CC list and splits it on spaces.

Comment 4

[Guy Pyrzak]

Attachment: v1

Comment 5

[Max Kanat-Alexander]

Comment on attachment 464223 [details] [diff] [review]
v1

We also need to change the backend, so that the frontend and backend are consistent. So, everywhere that currently splits the cc field on a space should be splitting it only on a comma.

Comment 6

[Reed Loden [:reed]]

Uh, what about those of us who use spaces already when adding CCs? Won't that break us?

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Uh, what about those of us who use spaces already when adding CCs? Won't that
> break us?

  Yes, it will. You'll have to use commas instead.

  I just don't think that the frontend and backend should work differently in this respect.

Comment 8

[Reed Loden [:reed]]

(In reply to comment #7)
> (In reply to comment #6)
> > Uh, what about those of us who use spaces already when adding CCs? Won't that
> > break us?
> 
>   Yes, it will. You'll have to use commas instead.

That's pretty uncool, especially considering ", " has been accepted for years. You're going to cause some very unhappy people, especially if you change the backend to not accept ", " as a valid delimiter format.

Comment 9

[Frédéric Buclin]

", " is valid. Only " " is not anymore.

Comment 10

[Reed Loden [:reed]]

(In reply to comment #9)
> ", " is valid. Only " " is not anymore.

Ah, if that's so, then I have no issues. I always use commas, but I add a space, as it looks weird to me without the extra space.

Comment 11

[LegNeato]

Attachment: Backend and frontend changes

I'm not sure if this is the correct way to do it (and my "trim" function feels hacky)...but it works so far. I kept the current behavior (splitting on spaces) for every other field as I am not familiar enough with bugzilla to know the ramifications of that change. I don't like hardcoding those cc fields though, and I am not sure I found every place to change this.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 464251 [details] [diff] [review]
Backend and frontend changes

We have a "trim" function available. Also, I think you mean "map" there, not "grep".

Also, you don't need to check explicitly for "cc" or "newcc" or "masscc" or anything like that. Just remove spaces from that match.

Also, you need to fix editcomponents.cgi, at least. Look for everywhere that uses the userselect template.

Comment 13

[LegNeato]

Ok, I'll clean this up and do more hunting around.

Comment 14

[Thomas D. (:thomas8)]

How do such broken features get into the product? No one testing upfront?? *frustrated*

Comment 15

[LegNeato]

I don't think this is inherently broken, arguments could be made for for both behaviors.

Comment 16

[Thomas D. (:thomas8)]

(In reply to comment #15)
> I don't think this is inherently broken, arguments could be made for for both
> behaviors.

With the difference that the current behaviour is practically completely useless as soon as you have a couple of hundred users, it will just show you a list of 100 people you don't want because it's only matching the surname (bar) of the full name (foo bar) that you enter. Try autocompleting "Wayne Mery" (which is probably a unique full name), to see the effect...

Comment 17

[Max Kanat-Alexander]

Thomas: First off, making such statements in Bugzilla is inappropriate. If you're frustrated with something, you're welcome to talk about it on IRC.

However, to respond, user-side autocomplete is not in any stable released version of Bugzilla. It was backported to bugzilla.mozilla.org for their 3.6 upgrade from the untested and unreleased 4.0 branch.

Comment 18

[Reed Loden [:reed]]

(In reply to comment #17)
> Thomas: First off, making such statements in Bugzilla is inappropriate. If
> you're frustrated with something, you're welcome to talk about it on IRC.

Indeed, please read the Bugzilla Etiquette -- https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

> However, to respond, user-side autocomplete is not in any stable released
> version of Bugzilla. It was backported to bugzilla.mozilla.org for their 3.6
> upgrade from the untested and unreleased 4.0 branch.

As far as I know, this isn't a "new problem" anyway... just one that is amplified and noticed due to the ajax-based autocomplete.

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> As far as I know, this isn't a "new problem" anyway... just one that is
> amplified and noticed due to the ajax-based autocomplete.

  Yeah, very much true.

Comment 20

[LegNeato]

Attachment: Don't split on spaces (backend and frontend)

Comment 21

[LegNeato]

Also found bug 592480 when testing this change.

Comment 22

[LegNeato]

Comment on attachment 470945 [details] [diff] [review]
Don't split on spaces (backend and frontend)

I think I meant to get this review, dunno why I didn't stick the flag on it...

Comment 23

[Max Kanat-Alexander]

Comment on attachment 470945 [details] [diff] [review]
Don't split on spaces (backend and frontend)

The process_bug part of the patch has bitrotted.

When I try to do a JS autocomplete search using a name with a space in it (like "Max Kan" I get JS errors in my console. I'm not sure if that's just some local problem though.

Comment 24

[LegNeato]

Attachment: Updated to latest trunk

Comment 25

[LegNeato]

FWIW I ran:

$ prove -vf xt/search.t :: --add-custom-fields

All tests successful.
Files=1, Tests=71643, 108 wallclock secs (14.75 usr  1.84 sys + 27.20 cusr  2.30 csys = 46.09 CPU)
Result: PASS

$

Comment 26

[Max Kanat-Alexander]

Comment on attachment 482405 [details] [diff] [review]
Updated to latest trunk

Cool, this looks right to me.

FWIW, xt/search.t doesn't test any of this code.

Comment 27

[LegNeato]

Thanks! Yeah, I noticed that after I started digging into it for the CC count bug.

Comment 28

[Frédéric Buclin]

We should commit it before 4.0rc1, to have a better testing of this feature, and to avoid a behavior change between rc1 and final.

Comment 29

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/User.pm
modified js/field.js
Committed revision 7580.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified Bugzilla/Bug.pm
modified Bugzilla/User.pm
modified js/field.js
Committed revision 7465.