STR: * have a contact with a number +336123456789 * in the SMS app, search for 06123456 Expected: * we should find this contact Actual: * we don't A quick fix would be to remove the leading 0 if present, either in SMS app, or in the Contacts API. I think fixing this in the Contacts API makes more sense.

Is this related to "contains" actually being a "startsWith"?

(In reply to Rick Waldron from comment #1) > Is this related to "contains" actually being a "startsWith"? Nope, the phone search is actually a "contains" ;)

(In reply to Julien Wajsberg [:julienw] from comment #0) > STR: > * have a contact with a number +336123456789 > * in the SMS app, search for 06123456 > > Expected: > * we should find this contact > > Actual: > * we don't > > A quick fix would be to remove the leading 0 if present, either in SMS app, > or in the Contacts API. I think fixing this in the Contacts API makes more > sense. Hi Julien, I am looking to take this bug and from my initial skim through the code and the implementation of a quick fix (i.e. drop the leading '0', if present, when searching, as you mentioned) I noticed that: 1. if the fix is in the contacts API (apps/sms/js/contacts.js#L182), the found contacts don't have the matching substring highlighted 2. if the fix is in the rendering of the search results (i.e. drop the leading '0' before sending the input string to the contacts API), the matching substring is highlighted without the leading '0', thus leading to cases where a search for '0654' for contacts that have local numbers, e.g. '0654455215', would only highlight '654'. These two approaches are considering a solution on Gaia's side. I guess an extra check in Gecko's implementation of ContactsManager.find (cf. https://wiki.mozilla.org/ContactsAPI#API) for the leading '0' would also do the trick. What do you think about this, should I follow one of the two points I mentioned above and eventually refactor the substring highlighting code to work correctly, or leave this for a fix in Gecko by someone else?

Mihai, what I call the "contacts API" is in gecko ;) I'd love to know what Gregor thinks about it: should this be fixed in the Gecko Contacts API as I think, or should we implement a workaround in Gaia ? Thanks !

Yeah we can handle this in the contacts api. We just have to add the national format to the search space for the contains.

Yea, bug 862250 will add support this with a "match" filterOp for telephone numbers. (In reply to Julien Wajsberg [:julienw] from comment #2) > (In reply to Rick Waldron from comment #1) > > Is this related to "contains" actually being a "startsWith"? > > Nope, the phone search is actually a "contains" ;) Not really. For "123", and a saved contact with phone "54123", it'll try to match numbers lexicographically larger than "123" and smaller than "124", where "numbers" for this contact would be the array ["54123", "4123", "123", "23", "3"]. In this case, it's a successful match, but "0123" would not work. It's more like "endsWith a string that startsWith the entire number".

Comment on attachment 741879 [details] [diff] [review] patch Review of attachment 741879 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r=me with these fixed. We should clean up that "special case telephone number" code when we don't have to worry about uplift hell anymore. ::: dom/contacts/fallback/ContactDB.jsm @@ +317,5 @@ > db.createObjectStore(SAVED_GETALL_STORE_NAME); > + } else if (currVersion == 8) { > + if (DEBUG) debug("Adding national format to contains search."); > + if (!objectStore) { > + objectStore = aTransaction.objectStore(STORE_NAME); The indentation is off by 1 space throughout the rest of this chunk, please fix this. @@ +331,5 @@ > + let parsedNumber = PhoneNumberUtils.parse(number); > + if (parsedNumber && > + parsedNumber.nationalFormat && > + number !== parsedNumber.nationalFormat) { > + let digits = parsedNumber.nationalFormat.match(/\d/g); Can you do normalize here instead of using the RegExp? That way we don't even get to the loop for the case where nationalFormat and number only differ in whitespace and formatting characters. @@ +487,5 @@ > } > } > } > + if (parsedNumber.nationalFormat && > + number.toString() !== parsedNumber.nationalFormat) { You don't need the toString() here. There's a |number = number.toString();| in the beginning of the if now. @@ +492,5 @@ > + let digits = parsedNumber.nationalFormat.match(/\d/g); > + if (digits) { > + digits = digits.join(''); > + for(let i = 0; i < digits.length; i++) { > + search[digits.substring(i, digits.length)] = 1; Again, you want to normalize before comparing instead of doing the RegExp, because this should only be added when number and nationalFormat differ in significant characters. ::: dom/contacts/tests/test_contacts_basics.html @@ +781,5 @@ > + req = mozContacts.find(options); > + req.onsuccess = function () { > + is(req.result.length, 1, "Found exactly 1 contact."); > + findResult1 = req.result[0]; > + ok(findResult1.id == sample_id1, "Same ID"); Use is() in these ID checks please. @@ +820,5 @@ > + }, > + function () { > + ok(true, "Deleting database"); > + req = mozContacts.clear(); > + req.onsuccess = function () { Nit: extra space.

Comment on attachment 741879 [details] [diff] [review] patch Review of attachment 741879 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/contacts/fallback/ContactDB.jsm @@ +344,5 @@ > + } > + } > + } > + ) > + cursor.update(cursor.value); There's no need to call this if you don't modify tel. It's just going to slow everything down. @@ +491,5 @@ > + number.toString() !== parsedNumber.nationalFormat) { > + let digits = parsedNumber.nationalFormat.match(/\d/g); > + if (digits) { > + digits = digits.join(''); > + for(let i = 0; i < digits.length; i++) { Nit: space after for.

I'm quite sure this should block 1.1 => nominating for leo.

(In reply to Julien Wajsberg [:julienw] from comment #10) > I'm quite sure this should block 1.1 => nominating for leo. Can you provide the regressing bug for posterity?

Alex> Not really, sorry. I don't know if that ever works, indeed.

*worked

Adding c=contacts whiteboard to get this into the scrumbugs backlog.

Triage - referring to FFOS Product to see if this is required? based on comment 12 unable to know if this is a regression.

I'd like UX's input on this. Personally, I see this as important, but not critical.

this is a device interfaces bug

We supported it but we had to remove it in bug 864695.

based on comment# 16 will not be blocking, but can be nominated for uplift and will be considered based on the risk

Does comment 17 (i.e. device interface bug) mean that UX is no longer needed, and that the source of the issue has been determined? Please let me know. Thanks!

I think this bug is now obsolete ?

Did this patch land in the end?

blocking koi+ bug 904515

(In reply to Julien Wajsberg [:julienw] from comment #23) > blocking koi+ bug 904515 I think you resetted to koi? by mistake :(

nope, I requested it.

Blocks a blocker

This shouldn't be needed any more.

Soooo should we close it and check if bug 904515 just works ?

Yes please retest.

(In reply to Julien Wajsberg [:julienw] from comment #0) > STR: > * have a contact with a number +336123456789 > * in the SMS app, search for 06123456 > The API supports this now with the 'match' functionality.