905927 - [Contacts API] fail in matching digits test.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Comment 1

[leo.bugzilla.gecko]

1. Perform all the phonebook matching tests from attached files.
2. Test Place - VENEZUELA

EXPECTED:
The handset must makes the matching in all conditions on file attached.
Should follow the rules.

ACTUAL:
The phone does not matching correctly.

Comment 2

[leo.bugzilla.gecko]

Attachment: Matching Venezuela_Movistar 10 DIGITS_0816.xlsx

Comment 3

[leo.bugzilla.gecko]

Attachment: matchingError0816.xlsm

Comment 4

[leo.bugzilla.gecko]

I tried to change the value of 'substringMatching' for Venezuela.
The value of 'substringMatching' is set to '7'.
I changed the value to '10'.

Ex> B2g.js
pref("dom.phonenumber.substringmatching.VE", 10);

Fail items decreased.
But, bug is still reproduced.
I'll attach the test results.
Please check this.

In addition, Please let us know the value of "ES"(Spain).

Comment 5

[leo.bugzilla.gecko]

Attachment: changePref_matchingError0820.xlsm

Comment 6

[leo.bugzilla.gecko]

Attachment: changePref_Matching Venezuela_Movistar 10 DIGITS_0820.xlsx

Comment 7

[Wayne Chang [:wchang]]

Hi Gregor, did you work on the phone number matching logic?

Can you check if all we need is to add customisation for Venezuela or if something is broken?

Thanks

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 10

[Gregor Wagner [:gwagner]]

Leo, can you test with this patch?

Comment 11

[leo.bugzilla.gecko]

I tested this patch to leo device.
But, bug is still reproduced.
I'll attach log file.

Comment 12

[leo.bugzilla.gecko]

Attachment: log_matchingDigits_VE_0822.txt

1. Save Contact
(Name: Aaa Aaa, Tel: [phone]
2. MO call
(Number: 04143727591)

Result>
 -.EXPECTED: Match
 -.ACTUAL: No match

Comment 13

[leo.bugzilla.gecko]

I would like to know the progress of patch work.
Please check.

And, I have another question.
According to the test case...
The same scenario, but the result should be differnt.(Depending on operators)
Ex>
1. Contact:"[phone]", MO Call:"123-4567"
-> EXPECTED: TELEFONICA-NO, MOVILNET-YES

2. Contact:"[phone]", MO Call:"945-6771"
-> EXPECTED: TELEFONICA-NO, MOVILNET-YES
(Yellow highlight in attached file)

Is it possible to pass complete including these items?
Please check as soon as possible.
Thanks.

Comment 14

[leo.bugzilla.gecko]

Attachment: fulltest_matchingError0826xlsm.xlsm

Added yellow highlight and the rest of the test results.

Comment 15

[Gregor Wagner [:gwagner]]

(In reply to leo.bugzilla.gecko from comment #13)
> I would like to know the progress of patch work.
> Please check.
> 
> And, I have another question.
> According to the test case...
> The same scenario, but the result should be differnt.(Depending on operators)


What does that mean? We need to apply the substring matching based on the operator within a certain country?

Comment 16

[Gregor Wagner [:gwagner]]

(In reply to leo.bugzilla.gecko from comment #12)
> Created attachment 793830 [details]
> log_matchingDigits_VE_0822.txt
> 
> 1. Save Contact
> (Name: Aaa Aaa, Tel: [phone]
> 2. MO call
> (Number: 04143727591)
> 
> Result>
>  -.EXPECTED: Match
>  -.ACTUAL: No match

Oh I see what happened here. '77' is not a valid country code and we couldn't parse it.

Comment 17

[Gregor Wagner [:gwagner]]

Attachment: patch

Can you test with the updated patch?

Comment 18

[Gregor Wagner [:gwagner]]

Attachment: patch

Now with the new testfile.

Comment 19

[leo.bugzilla.gecko]

(In reply to Gregor Wagner [:gwagner] from comment #18)
> Created attachment 795663 [details] [diff] [review]
patch

Now with the new
> testfile.

I tested this patch.
Most of the fail cases are fixed.
In addition to this patch, I had set the value of substringmatching set to '10'.
Right?
Please confirm.

And, There are some issues I mentioned above(comment #13) remain.
As your question(comment #15)...
  "We need to apply the substring matching based on the operator within a certain country?"
-> Is this possible?

I'll attach test result.
Check please.

Comment 20

[leo.bugzilla.gecko]

Attachment: TestResult_matchingDigits_VE_0827.zip

Comment 21

[Gregor Wagner [:gwagner]]

(In reply to leo.bugzilla.gecko from comment #19)
> (In reply to Gregor Wagner [:gwagner] from comment #18)
> > Created attachment 795663 [details] [diff] [review]
> patch
> 
> Now with the new
> > testfile.
> 
> I tested this patch.
> Most of the fail cases are fixed.
> In addition to this patch, I had set the value of substringmatching set to
> '10'.
> Right?
> Please confirm.
> 
> And, There are some issues I mentioned above(comment #13) remain.
> As your question(comment #15)...
>   "We need to apply the substring matching based on the operator within a
> certain country?"
> -> Is this possible?

No we don't support that. But how can we fix the problem you stated in comment #13?

1. Contact:"[phone]", MO Call:"123-4567"
-> EXPECTED: TELEFONICA-NO, MOVILNET-YES

They would need a separate entry of substringmatching?

Comment 22

[leo.bugzilla.gecko]

(In reply to Gregor Wagner [:gwagner] from comment #21)
> (In reply to leo.bugzilla.gecko from comment #19)
> (In reply to Gregor
> Wagner [:gwagner] from comment #18)
> > Created attachment 795663 [details] [diff] [review]
> [diff] [review]
> patch
> 
> Now with the new
> > testfile.
> 
> I tested
> this patch.
> Most of the fail cases are fixed.
> In addition to this patch,
> I had set the value of substringmatching set to
> '10'.
> Right?
> Please
> confirm.
> 
> And, There are some issues I mentioned above(comment #13)
> remain.
> As your question(comment #15)...
>   "We need to apply the
> substring matching based on the operator within a
> certain country?"
> ->
> Is this possible?

No we don't support that. But how can we fix the problem
> you stated in comment #13?

1. Contact:"[phone]", MO Call:"123-4567"
> -> EXPECTED: TELEFONICA-NO, MOVILNET-YES

They would need a separate entry
> of substringmatching?

The matching rules as follows:
(Ref. "matchingError0816.xlsm")
----------------------------------
1 - Search all contacts with 7 digits. (match 7 digits)
2 - Search all contacts with 11 digits and match the last 10 digits, considering the network code + number dialed of 7 digits. (Example: In Digitel Network, dialing 1234567 it would match with the contact 0412 1234567)
For Movistar the prefix are 0414 and 0424 and for Movilnet are 0416 and 0426.
----------------------------------

I can't test by the network right now.
But, matching contact doesn't seem to use the 'MNC' or and so on.
So, I wondered if it supports it.

First of all, Check my question.
> In addition to this patch, I had set the value of 'substringmatching' set to
> '10'.
> Right?
> Please confirm.

Thanks.

Comment 23

[Gregor Wagner [:gwagner]]

(In reply to leo.bugzilla.gecko from comment #22)
> (In reply to Gregor Wagner [:gwagner] from comment #21)
> > (In reply to leo.bugzilla.gecko from comment #19)
> > (In reply to Gregor
> > Wagner [:gwagner] from comment #18)
> > > Created attachment 795663 [details] [diff] [review]
> > [diff] [review]
> > patch
> > 
> > Now with the new
> > > testfile.
> > 
> > I tested
> > this patch.
> > Most of the fail cases are fixed.
> > In addition to this patch,
> > I had set the value of substringmatching set to
> > '10'.
> > Right?
> > Please
> > confirm.
> > 
> > And, There are some issues I mentioned above(comment #13)
> > remain.
> > As your question(comment #15)...
> >   "We need to apply the
> > substring matching based on the operator within a
> > certain country?"
> > ->
> > Is this possible?
> 
> No we don't support that. But how can we fix the problem
> > you stated in comment #13?
> 
> 1. Contact:"[phone]", MO Call:"123-4567"
> > -> EXPECTED: TELEFONICA-NO, MOVILNET-YES
> 
> They would need a separate entry
> > of substringmatching?
> 
> The matching rules as follows:
> (Ref. "matchingError0816.xlsm")
> ----------------------------------
> 1 - Search all contacts with 7 digits. (match 7 digits)
> 2 - Search all contacts with 11 digits and match the last 10 digits,
> considering the network code + number dialed of 7 digits. (Example: In
> Digitel Network, dialing 1234567 it would match with the contact 0412
> 1234567)
> For Movistar the prefix are 0414 and 0424 and for Movilnet are 0416 and 0426.
> ----------------------------------
> 
> I can't test by the network right now.
> But, matching contact doesn't seem to use the 'MNC' or and so on.
> So, I wondered if it supports it.
> 
> First of all, Check my question.
> > In addition to this patch, I had set the value of 'substringmatching' set to
> > '10'.
> > Right?
> > Please confirm.
> 
> Thanks.

No we don't support substring matching based on carrier. Lets just use the 7 digit matching and this should work 99.9% of the time.
Or do people tend to have the same phone number with different prefix?
So is it a common case that people have for example a 0414-1234567 and a 0416-1234567 number?

Comment 24

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 25

[leo.bugzilla.gecko]

I tested this patch.

What I want to know is the value of 'substringmatching' for VE.
It is not included in your patch.
But, test result depends on value of 'substringmatching'.
The value of 'substringMatching' is set to '7'-> FAIL
The value of 'substringMatching' is set to '10'-> PASS
Example> 
-. Contact: 7414 3727591
-. Dial: 0414 3727591
-. EXPECTED: Match
-. ACTUAL: 
   The value of 'substringMatching' is set to '7'-> No match
   The value of 'substringMatching' is set to '10'-> Match
Other cases are the same results.
I want to know if I could change the value to '10'.

Comment 26

[Gregor Wagner [:gwagner]]

(In reply to leo.bugzilla.gecko from comment #25)
> I tested this patch.
> 
> What I want to know is the value of 'substringmatching' for VE.
> It is not included in your patch.
> But, test result depends on value of 'substringmatching'.
> The value of 'substringMatching' is set to '7'-> FAIL
> The value of 'substringMatching' is set to '10'-> PASS
> Example> 
> -. Contact: 7414 3727591
> -. Dial: 0414 3727591
> -. EXPECTED: Match
> -. ACTUAL: 
>    The value of 'substringMatching' is set to '7'-> No match
>    The value of 'substringMatching' is set to '10'-> Match
> Other cases are the same results.
> I want to know if I could change the value to '10'.

The value is already set to 7:
http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#395

How do you test this?
We don't support changing the value during runtime. So it's important that you reboot the device after you change substringMatching and also delete and re-insert the contact.

Comment 27

[leo.bugzilla.gecko]

I don't change the value during runtime.
I just changed it in the source code.

-----------------------
[b2g.js]
pref("dom.phonenumber.substringmatching.VE", 7);
->
pref("dom.phonenumber.substringmatching.VE", 10);
-----------------------

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 796325 [details] [diff] [review]
patch

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

r=me with this stuff addressed:

::: dom/contacts/fallback/ContactDB.jsm
@@ +662,5 @@
>                    }
>                  }
>                }
>                for (let num in containsSearch) {
> +                if (num && num != "null") {

Ick, can "null" really happen?

@@ +1015,5 @@
>  
>      let filter_keys = fields.slice();
>      for (let key = filter_keys.shift(); key; key = filter_keys.shift()) {
>        let request;
> +      let substringResult = {};

Shouldn't this be an array?

@@ +1048,5 @@
>            dump("ContactDB: normalized filterValue is empty, can't perform match search.\n");
>            return txn.abort();
>          }
>  
> +        let substringRequest;

Why is this outside the block below? You don't use it again do you?

@@ +1096,5 @@
>          txn.result = {};
>  
>        request.onsuccess = function (event) {
>          if (DEBUG) debug("Request successful. Record count: " + event.target.result.length);
> +        if (substringResult && Object.keys(substringResult).length > 0) {

'substringResult' will always evaluate to true here.

@@ +1097,5 @@
>  
>        request.onsuccess = function (event) {
>          if (DEBUG) debug("Request successful. Record count: " + event.target.result.length);
> +        if (substringResult && Object.keys(substringResult).length > 0) {
> +          for (var attrname in substringResult) { 

This will make attrname '1', '2', etc, right?

Also trailing whitespace.

::: dom/phonenumberutils/PhoneNumberUtils.jsm
@@ +84,5 @@
>  
>    parse: function(aNumber) {
>      if (DEBUG) debug("call parse: " + aNumber);
>      let result = PhoneNumber.Parse(aNumber, this.getCountryName());
> +    

Nit: whitespace

@@ +111,2 @@
>        }
> +    } else {

else if (DEBUG)

Comment 29

[Gregor Wagner [:gwagner]]

(In reply to Gregor Wagner [:gwagner] from comment #26)
> (In reply to leo.bugzilla.gecko from comment #25)
> > I tested this patch.
> > 
> > What I want to know is the value of 'substringmatching' for VE.
> > It is not included in your patch.
> > But, test result depends on value of 'substringmatching'.
> > The value of 'substringMatching' is set to '7'-> FAIL
> > The value of 'substringMatching' is set to '10'-> PASS
> > Example> 
> > -. Contact: 7414 3727591
> > -. Dial: 0414 3727591
> > -. EXPECTED: Match
> > -. ACTUAL: 
> >    The value of 'substringMatching' is set to '7'-> No match
> >    The value of 'substringMatching' is set to '10'-> Match
> > Other cases are the same results.
> > I want to know if I could change the value to '10'.
> 
> The value is already set to 7:
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#395
> 
> How do you test this?
> We don't support changing the value during runtime. So it's important that
> you reboot the device after you change substringMatching and also delete and
> re-insert the contact.

Is 7414 3727591 a valid number in VE? '7' is the country code for Russia and we interpret it as a Russian number.

Comment 30

[Gregor Wagner [:gwagner]]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #28)
> Comment on attachment 796325 [details] [diff] [review]
> patch
> 
> Review of attachment 796325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with this stuff addressed:
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +662,5 @@
> >                    }
> >                  }
> >                }
> >                for (let num in containsSearch) {
> > +                if (num && num != "null") {
> 
> Ick, can "null" really happen?
> 

Yes :(

> @@ +1015,5 @@
> >  
> >      let filter_keys = fields.slice();
> >      for (let key = filter_keys.shift(); key; key = filter_keys.shift()) {
> >        let request;
> > +      let substringResult = {};
> 
> Shouldn't this be an array?

No we use it as a dictionary.

> 
> @@ +1048,5 @@
> >            dump("ContactDB: normalized filterValue is empty, can't perform match search.\n");
> >            return txn.abort();
> >          }
> >  
> > +        let substringRequest;
> 
> Why is this outside the block below? You don't use it again do you?

fixed

> 
> @@ +1096,5 @@
> >          txn.result = {};
> >  
> >        request.onsuccess = function (event) {
> >          if (DEBUG) debug("Request successful. Record count: " + event.target.result.length);
> > +        if (substringResult && Object.keys(substringResult).length > 0) {
> 
> 'substringResult' will always evaluate to true here.

fixed

> 
> @@ +1097,5 @@
> >  
> >        request.onsuccess = function (event) {
> >          if (DEBUG) debug("Request successful. Record count: " + event.target.result.length);
> > +        if (substringResult && Object.keys(substringResult).length > 0) {
> > +          for (var attrname in substringResult) { 
> 
> This will make attrname '1', '2', etc, right?

fixed


> 
> Also trailing whitespace.

my trademark...

> 
> ::: dom/phonenumberutils/PhoneNumberUtils.jsm
> @@ +84,5 @@
> >  
> >    parse: function(aNumber) {
> >      if (DEBUG) debug("call parse: " + aNumber);
> >      let result = PhoneNumber.Parse(aNumber, this.getCountryName());
> > +    
> 
> Nit: whitespace
> 
> @@ +111,2 @@
> >        }
> > +    } else {
> 
> else if (DEBUG)

fixed.

Comment 31

[Gregor Wagner [:gwagner]]

Attachment: final patch

Comment 32

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/dea5249f90e8

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dea5249f90e8

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 35

[jongsoo.oh]

leo has to take this patch.
Please uplift to b2g18. Thank you.

Comment 36

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/307824edadd7

Comment 37

[Gregor Wagner [:gwagner]]

(In reply to jongsoo.oh from comment #35)
> leo has to take this patch.
> Please uplift to b2g18. Thank you.

The patch landed on b2g18. Please test it from your side again.

Comment 38

[leo.bugzilla.gecko]

I tested the patch.
This issue is fixed.
But, The following was deleted.

-----------------------------------------------------------------------------
--- a/dom/contacts/fallback/ContactDB.jsm
+++ b/dom/contacts/fallback/ContactDB.jsm

@@ -982,17 +983,27 @@ ContactDB.prototype = {
         }
         
         let index = store.index("telMatch");
         let normalized = PhoneNumberUtils.normalize(options.filterValue,
                                                     /*numbersOnly*/ true);
 
+        if (!normalized.length) {
+          dump("ContactDB: normalized filterValue is empty, can't perform match search.\n");
+          return txn.abort();
+        }
         // Some countries need special handling for number matching. Bug 877302
         if (this.substringMatching && normalized.length > this.substringMatching) {
-----------------------------------------------------------------------------
Is that right?
Please confirm.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/307824edadd7