[ContactsAPI] mixing 'tel' with name fields in filterBy can return unexpected results

VERIFIED FIXED in Firefox 24, Firefox OS v1.1hd

Status

()

Core
DOM: Device Interfaces
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bkelly, Assigned: reuben)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [fixed-in-birch] c=)

Attachments

(1 attachment)

3.87 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Currently a request like:

  var request = navigator.mozContacts.find({
    filterBy: ['tel', 'givenName', 'familyName'],
    filterOp: 'contains',
    filterValue: 'r'
  });

Returns unexpected values.  For example:  "Kent M. Newton"

See https://gist.github.com/rwldrn/5621143

We suspect that the 'tel' logic is normalizing 'r' to '' and matching empty phone number fields.
(Reporter)

Updated

5 years ago
Status: NEW → ASSIGNED
Whiteboard: c=
(Assignee)

Updated

5 years ago
Assignee: bkelly → reuben.bmo
(Assignee)

Comment 1

5 years ago
PhoneNumberJS is normalizing 'r' to '7', we can do two things:
1) Not normalize when the search string is all letters (but in that case, we can simply skip the telephone search altogether).
2) Not normalize if filterBy contains more than just "tel".
(Reporter)

Comment 2

5 years ago
How about not normalizing if the string is all letters and filterBy contains more than just "tel"?

It would seem if they are typing in numbers, then they will want the country code normalization, etc.
(Assignee)

Comment 3

5 years ago
I believe this bug contains two problems:

1) Gaia shouldn't be calling find with "givenName" in the filterBy if the search field only has numbers, and it shouldn't be including "tel" if the search field only has letters.

2) We shouldn't be doing letter to number conversion by default in ContactDB. It's a behavior most consumers don't expect, and it should be opt-in.

Rick, any comments on 1)?

If there are no objections, I'll file the Gaia bug, and morph this bug into fixing problem 2.
Flags: needinfo?(waldron.rick)
Sure we could do this in Gaia but I really think this should be done in the API instead. The Sms app is not the only consumer, and I don't feel good with requesting to do this in all apps.

I don't compeltely agree with your proposal, here is my try :
* we should not do letter to number conversion while searching in general
* but, if the user added a number with letters in the contact list, we should match it if the letters match AND if the converted numbers match. That reverts how we look up stuff: instead of converting the input, you need to convert what's stored and store the result _and_ keep the original stuff of course.
(Assignee)

Comment 5

5 years ago
(In reply to Julien Wajsberg [:julienw] from comment #4)
> * but, if the user added a number with letters in the contact list, we
> should match it if the letters match AND if the converted numbers match.
> That reverts how we look up stuff: instead of converting the input, you need
> to convert what's stored and store the result _and_ keep the original stuff
> of course.

I think you're talking about the behavior when the user is typing digits, and we match that with contacts' names converted to numbers. We don't currently support that case, and when we do, it's likely going to be in a different filterBy or filterOp, since doing it by default on every search creates the same unexpected results that we have here.
(Assignee)

Comment 6

5 years ago
(In reply to Julien Wajsberg [:julienw] from comment #4)
> Sure we could do this in Gaia but I really think this should be done in the
> API instead. The Sms app is not the only consumer, and I don't feel good
> with requesting to do this in all apps.

There is no API bug, it's doing exactly what it was asked to do: find contacts by tel, givenName, familyName. The "tel" search works exactly like it would if you searched for tel only, and while this specific bug will be fixed if we stop doing letter-to-number conversion, what if we decide to do something different for tel searches in the future?
(In reply to Reuben Morais [:reuben] from comment #6)
> (In reply to Julien Wajsberg [:julienw] from comment #4)
> > Sure we could do this in Gaia but I really think this should be done in the
> > API instead. The Sms app is not the only consumer, and I don't feel good
> > with requesting to do this in all apps.
> 
> There is no API bug, it's doing exactly what it was asked to do: find
> contacts by tel, givenName, familyName. The "tel" search works exactly like
> it would if you searched for tel only, and while this specific bug will be
> fixed if we stop doing letter-to-number conversion, what if we decide to do
> something different for tel searches in the future?

I don't exactly get what you might do different... besides my other proposition, which would work.

(In reply to Reuben Morais [:reuben] from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #4)
> > * but, if the user added a number with letters in the contact list, we
> > should match it if the letters match AND if the converted numbers match.
> > That reverts how we look up stuff: instead of converting the input, you need
> > to convert what's stored and store the result _and_ keep the original stuff
> > of course.
> 
> I think you're talking about the behavior when the user is typing digits,
> and we match that with contacts' names converted to numbers. We don't
> currently support that case, and when we do, it's likely going to be in a
> different filterBy or filterOp, since doing it by default on every search
> creates the same unexpected results that we have here.

I agree with not supporting this now. However, putting it besides a new filter won't work, because we'll probably always use it. I feel like it should be possible to always do it if we have a correct algorithm (ie: we should _not_ convert the filter value when we search, rather we need to convert numbers-with-letters when we store it, and try to match both the origin value (with letters then) and the converted value; this would prevent matching END with 363 if END is not in the entered "number"). 

(note: my "AND" was really a "OR" in my previous sentence)
(Assignee)

Comment 8

5 years ago
Created attachment 755089 [details] [diff] [review]
Don't convert letters to numbers for tel searches
Attachment #755089 - Flags: review?(bent.mozilla)
Duplicate of this bug: 876861
dupe 876861 had leo+ => requesting leo+ here
blocking-b2g: --- → leo?
Duplicate of this bug: 874456
Comment on attachment 755089 [details] [diff] [review]
Don't convert letters to numbers for tel searches

Review of attachment 755089 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me but I'd suggest reversing the meaning of the optional argument to normalize(), from 'letterToNumber' to 'numbersOnly'. Then you wouldn't have to do this:

  if (letterToNumber === undefined) {
    letterToNumber = true;
  }
Attachment #755089 - Flags: review?(bent.mozilla) → review+

Updated

5 years ago
blocking-b2g: leo? → leo+
(Assignee)

Comment 13

5 years ago
Nice, I changed that.

https://hg.mozilla.org/projects/birch/rev/1c0a49bbb666
Whiteboard: c= → [fixed-in-birch] c=
https://hg.mozilla.org/mozilla-central/rev/1c0a49bbb666
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
https://hg.mozilla.org/releases/mozilla-b2g18/rev/812f488acb0c
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed

Updated

5 years ago
Flags: in-moztrap-
Hey, I don't see that this is fixed...

In Bug 880644 we still see the old wrong behavior, is there a possibility we're doing something else wrong in Gaia ?
Flags: needinfo?(waldron.rick)
Flags: needinfo?(reuben.bmo)
Depends on: 880644
(Assignee)

Comment 18

5 years ago
Bug 880644 will fix this, it's a stupid mistake. I'm still curious, though, how is the contacts/dialer app not hitting this in their search features?
Flags: needinfo?(reuben.bmo)
The contacts/dialer app is using a copy of the contacts app; and the contacts app is actually not using the API, it's doing a search in the DOM that is already loaded.

If you think this is silly, yes, I think the same.
(Assignee)

Comment 20

5 years ago
(In reply to Julien Wajsberg [:julienw] from comment #19)
> The contacts/dialer app is using a copy of the contacts app; and the
> contacts app is actually not using the API, it's doing a search in the DOM
> that is already loaded.
> 
> If you think this is silly, yes, I think the same.

Brilliant, so I "tested" my patch with the app that doesn't use the code…
Thanks for the info.
Resolution: FIXED → DUPLICATE
Duplicate of bug: 880644
why duplicate ? we landed something here, I think it should stay "fixed" to show this, otherwise it's hard to track :) The other possibility is to revert your patch here.
(Assignee)

Comment 22

5 years ago
Hmm, good point. Bug 880644 builds on top of this code, so I shouldn't revert it. Marking FIXED, sorry for the bugspam.
Resolution: DUPLICATE → FIXED

Comment 23

5 years ago
That's resolved finally? Wich is the definitive bug?
I'm testing with the build of TESTRUN4 and it doesn't work
bov, I'd like a full STR if that's possible, possibly in another bug (please cc me :) ) because it will be easier to handle. Thanks !
Flags: needinfo?(bov)

Comment 25

5 years ago
Hi Julien,
As we decided in https://bugzilla.mozilla.org/show_bug.cgi?id=880644(VERIFED-FIXED), the contact auto suggestions works well for Leo, taking in mind it searchs only in Name and Last Name fields, and only searchs the string typed in the beggining of the mentioned fields. So, I think we can change this bug (874501) to VERIFIED as well.

And as I told in Bug 880644, I will change the current Test Cases regarding SMS/MMS/ Contact Autosuggestions until https://bugzilla.mozilla.org/show_bug.cgi?id=890706 and https://bugzilla.mozilla.org/show_bug.cgi?id=891385(both nominated koi?) are Resolved and Fixed
Status: RESOLVED → VERIFIED
Flags: needinfo?(bov)
You need to log in before you can comment on or make changes to this bug.