Closed Bug 862250 Opened 7 years ago Closed 7 years ago

Contacts: support exact matching for telephone numbers

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: reuben, Assigned: reuben)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(3 files, 6 obsolete files)

We need to support matching phone numbers exactly, after they're parsed (to identify calls, for example).

I don't know if using a different ContactFindOptions.filterOp value is the best way to implement this, given that it only applies to the "tel" field. Maybe a special "tel_exact" key?
Assignee: nobody → reuben.bmo
Attached patch Exact matching (obsolete) — Splinter Review
Attachment #739007 - Flags: review?(anygregor)
Attached patch Exact matching (obsolete) — Splinter Review
Attachment #739007 - Attachment is obsolete: true
Attachment #739007 - Flags: review?(anygregor)
Attachment #739070 - Flags: review?(anygregor)
Attached patch Exact matching (obsolete) — Splinter Review
Attachment #739070 - Attachment is obsolete: true
Attachment #739070 - Flags: review?(anygregor)
Attachment #739492 - Flags: review?(anygregor)
Blocks: 859185
From reading many bugs filed in the last few days I think we should support following behavior:

1) Have an "exact" search that only searches the number the user enters. We shouldn't try to be smart for the exact search and it is good to keep the number that is exposed to the user separate. (For exact matching)
2) Add a "match" search that expands the search space with all the numbers we get from PhoneNumberJS. (For incoming/outgoing calls and SMS)
3) Keep the current "contains" or "startswith" behavior for incremental search. (For incremental phonebook search)
4) Maybe add a "startswith" search for the exact number the user enters. (For incremental phonebook search)

Apps can decide if they want behavior 3 or 4 for incremental search.
(In reply to Gregor Wagner [:gwagner] from comment #4)
> From reading many bugs filed in the last few days I think we should support
> following behavior:
> 
> 1) Have an "exact" search that only searches the number the user enters. We
> shouldn't try to be smart for the exact search and it is good to keep the
> number that is exposed to the user separate. (For exact matching)

What's the use case for this in Gaia?

> 2) Add a "match" search that expands the search space with all the numbers
> we get from PhoneNumberJS. (For incoming/outgoing calls and SMS)

Seems like this and "kinda starts with" (the one we have now) are the only two that are needed.

> 3) Keep the current "contains" or "startswith" behavior for incremental
> search. (For incremental phonebook search)
> 4) Maybe add a "startswith" search for the exact number the user enters.
> (For incremental phonebook search)

What's the difference here? "startswith" would not try to match national and international versions?
(In reply to Reuben Morais [:reuben] from comment #5)
> (In reply to Gregor Wagner [:gwagner] from comment #4)
> > From reading many bugs filed in the last few days I think we should support
> > following behavior:
> > 
> > 1) Have an "exact" search that only searches the number the user enters. We
> > shouldn't try to be smart for the exact search and it is good to keep the
> > number that is exposed to the user separate. (For exact matching)
> 
> What's the use case for this in Gaia?

Gaia could try an exact search if it finds 2 contacts for one number.

> 
> > 2) Add a "match" search that expands the search space with all the numbers
> > we get from PhoneNumberJS. (For incoming/outgoing calls and SMS)
> 
> Seems like this and "kinda starts with" (the one we have now) are the only
> two that are needed.

We should give the user the option to do their own magic search function without us trying to be smart. We will never be able to get it 100% right so the user should be able to just search for numbers that are actually entered by the user without retrieving all the contacts.

> 
> > 3) Keep the current "contains" or "startswith" behavior for incremental
> > search. (For incremental phonebook search)
> > 4) Maybe add a "startswith" search for the exact number the user enters.
> > (For incremental phonebook search)
> 
> What's the difference here? "startswith" would not try to match national and
> international versions?

Bug 864695
Attached patch Exact matching (obsolete) — Splinter Review
So this will match the normalized filterValue with the normalized tel.value(s).
"match" patch incoming.
Attachment #739492 - Attachment is obsolete: true
Attachment #739492 - Flags: review?(anygregor)
Attachment #741562 - Flags: review?(anygregor)
Attachment #741562 - Attachment is obsolete: true
Attachment #741562 - Flags: review?(anygregor)
Attachment #741595 - Flags: review?(anygregor)
I forgot to qref some changes, sorry for the noise.
Attachment #741595 - Attachment is obsolete: true
Attachment #741595 - Flags: review?(anygregor)
Attachment #741600 - Flags: review?(anygregor)
Attachment #741596 - Attachment is obsolete: true
Attachment #741596 - Flags: review?(anygregor)
Attachment #741617 - Flags: review?(anygregor)
Comment on attachment 741600 [details] [diff] [review]
1 - "equals" for tel only matches the entered value (normalized)

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +316,5 @@
>          if (DEBUG) debug("Adding object store for cached searches");
>          db.createObjectStore(SAVED_GETALL_STORE_NAME);
> +      } else if (currVersion == 8) {
> +        if (DEBUG) debug("Make exactTel only contain the value entered by the user");
> +        objectStore.openCursor().onsuccess = function(event) {

How can you be sure that objectStore is valid?
        if (!objectStore) {
          objectStore = aTransaction.objectStore(STORE_NAME);
        }
Attachment #741600 - Flags: review?(anygregor) → review+
Comment on attachment 741617 [details] [diff] [review]
2 - "match" will match tel with national and international formats

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +344,5 @@
> +              cursor.value.properties.tel.forEach(
> +                function(tel) {
> +                  let parsed = PhoneNumberUtils.parse(tel.value.toString());
> +                  let normalize = PhoneNumberUtils.normalize;
> +                  cursor.value.search.parsedTel.push(normalize(parsed.nationalNumber));

No need to call normalize again.

@@ +485,5 @@
>                    }
> +
> +                  let normalize = PhoneNumberUtils.normalize;
> +                  contact.search.parsedTel.push(normalize(parsedNumber.nationalNumber));
> +                  contact.search.parsedTel.push(normalize(parsedNumber.internationalNumber));

We also need the national format here. That adds the leading zero for some countries. The numbers should be normalized again.

::: dom/contacts/tests/test_contacts_basics.html
@@ +598,5 @@
> +      ok(true, "Failed");
> +      next();
> +    }
> +  },
> +  function () {

Some more tests with the national format and national number would be great :)
Attachment #741617 - Flags: review?(anygregor) → review+
Comment on attachment 741617 [details] [diff] [review]
2 - "match" will match tel with national and international formats

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +344,5 @@
> +              cursor.value.properties.tel.forEach(
> +                function(tel) {
> +                  let parsed = PhoneNumberUtils.parse(tel.value.toString());
> +                  let normalize = PhoneNumberUtils.normalize;
> +                  cursor.value.search.parsedTel.push(normalize(parsed.nationalNumber));

And we need the national format here.
https://hg.mozilla.org/mozilla-central/rev/f8c9cbf1bc10
https://hg.mozilla.org/mozilla-central/rev/a10762d2bcd7
https://hg.mozilla.org/mozilla-central/rev/2393b185191e
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Please don't uplift. We have to fix something.
Whiteboard: [fixed-in-birch] → [fixed-in-birch][don't uplift]
Attachment #742395 - Flags: review?(anygregor)
Attachment #742395 - Flags: review?(anygregor) → review+
https://hg.mozilla.org/mozilla-central/rev/2bcb2d9ebbec
Whiteboard: [fixed-in-birch][don't uplift] → [fixed-in-birch][NO_UPLIFT]
Whiteboard: [fixed-in-birch][NO_UPLIFT] → [fixed-in-birch]
blocking a blocker
blocking-b2g: --- → leo?
(In reply to Gregor Wagner [:gwagner] from comment #21)
> blocking a blocker

leo+
blocking-b2g: leo? → leo+
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.