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

RESOLVED FIXED in Bugzilla 4.0

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: christian, Assigned: christian)

Tracking

unspecified
Bugzilla 4.0
Bug Flags:
approval +
approval4.0 +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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

8 years ago
This seems sensible to me. LpSolit?
Target Milestone: --- → Bugzilla 4.0

Comment 2

8 years ago
Sure, this seems sensible to me as well.

Comment 3

8 years ago
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

8 years ago
Created attachment 464223 [details] [diff] [review]
v1
Attachment #464223 - Flags: review?(mkanat)

Comment 5

8 years ago
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.
Attachment #464223 - Flags: review?(mkanat) → review-
Uh, what about those of us who use spaces already when adding CCs? Won't that break us?

Comment 7

8 years ago
(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.
(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

8 years ago
", " is valid. Only " " is not anymore.
(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.
(Assignee)

Comment 11

8 years ago
Created attachment 464251 [details] [diff] [review]
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.
Attachment #464223 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #464251 - Flags: review?(mkanat)

Comment 12

8 years ago
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.
Attachment #464251 - Flags: review-

Updated

8 years ago
Attachment #464251 - Flags: review?(mkanat)
(Assignee)

Comment 13

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

Updated

8 years ago
Assignee: ui → clegnitto
How do such broken features get into the product? No one testing upfront?? *frustrated*
(Assignee)

Comment 15

8 years ago
I don't think this is inherently broken, arguments could be made for for both behaviors.
(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

8 years ago
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.
(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

8 years ago
(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.
(Assignee)

Comment 20

8 years ago
Created attachment 470945 [details] [diff] [review]
Don't split on spaces (backend and frontend)
Attachment #464251 - Attachment is obsolete: true
(Assignee)

Comment 21

8 years ago
Also found bug 592480 when testing this change.
(Assignee)

Comment 22

8 years ago
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...
Attachment #470945 - Flags: review?(mkanat)

Comment 23

8 years ago
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.
Attachment #470945 - Flags: review?(mkanat) → review-
(Assignee)

Comment 24

8 years ago
Created attachment 482405 [details] [diff] [review]
Updated to latest trunk
Attachment #470945 - Attachment is obsolete: true
Attachment #482405 - Flags: review?(mkanat)
(Assignee)

Comment 25

8 years ago
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

8 years ago
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.
Attachment #482405 - Flags: review?(mkanat) → review+

Updated

8 years ago
Flags: approval4.0+
Flags: approval+
(Assignee)

Comment 27

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

Comment 28

8 years ago
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.
Status: NEW → ASSIGNED

Comment 29

8 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/User.pm
modified js/field.js
Committed revision 7580.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Bug.pm
modified Bugzilla/User.pm
modified js/field.js
Committed revision 7465.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

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