Closed Bug 585802 Opened 11 years ago Closed 11 years ago

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

Categories

(Bugzilla :: User Interface, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: christian, Assigned: christian)

References

Details

Attachments

(1 file, 3 obsolete files)

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).
This seems sensible to me. LpSolit?
Target Milestone: --- → Bugzilla 4.0
Sure, this seems sensible to me as well.
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.
Attached patch v1 (obsolete) — Splinter Review
Attachment #464223 - Flags: review?(mkanat)
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?
(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.
", " 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.
Attached patch Backend and frontend changes (obsolete) — Splinter Review
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
Attachment #464251 - Flags: review?(mkanat)
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-
Attachment #464251 - Flags: review?(mkanat)
Ok, I'll clean this up and do more hunting around.
Assignee: ui → clegnitto
How do such broken features get into the product? No one testing upfront?? *frustrated*
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...
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.
(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.
Attachment #464251 - Attachment is obsolete: true
Also found bug 592480 when testing this change.
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 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-
Attachment #470945 - Attachment is obsolete: true
Attachment #482405 - Flags: review?(mkanat)
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 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+
Flags: approval4.0+
Flags: approval+
Thanks! Yeah, I noticed that after I started digging into it for the CC count bug.
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
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
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 634243
You need to log in before you can comment on or make changes to this bug.