887737 - [SMS] The device is not recognizing the phone number received completely

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Leo]

1. Title:  The device does not recognize the phone number received completely
2. Precondition: SMS app should be working
3. Tester's Action: (1) Receive an sms with the phone number
4. Detailed Symptom (ENG.) : The device  is not detecting the contact
5. Expected: The handset should receive the sms with the characters properly
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia Master/v1-train: Reproduced on v1-train
8. Gaia Revision:   9f283ab5e6b2e49513057138eacaa6adaff488ac
9. Personal email id: [e-mail]

Comment 1

[Leo]

The following is the phone number where the device is not recognizing.
Number : [phone]

Comment 2

[Corey Frang [:gnarf]]

Attachment: patch v1

Must be applied on top of the pull request for #870711

Comment 3

[Corey Frang [:gnarf]]

Comment on attachment 770095 [details] [diff] [review]
patch v1

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

::: apps/sms/test/unit/utils_test.js
@@ +577,5 @@
>      });
>  
>      suite('Varied Cases', function() {
> +      MockUtils.mPhoneTestCases.forEach(function(fixture) {
> +        console.log('fixture', fixture);

already removed this in my local patch - please ignore

Comment 4

[Corey Frang [:gnarf]]

Attachment: patch v2

Fixed some stuff from the first patch that was causing units to fail

Comment 5

[Corey Frang [:gnarf]]

reminder to reviews - requires patch for bug 870711 to be applied

Comment 6

[Corey Frang [:gnarf]]

Attachment: patch v3

Fixes some issues with rebase on the url regexp bug

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 770274 [details] [diff] [review]
patch v3

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

thanks for this work !

::: apps/sms/js/link_helper.js
@@ +33,5 @@
>   */
>  var LINK_TYPES = {
>    phone: {
>      regexp: new RegExp([
> +      // sdd: space, dot, dash or parens

should you rename "sdd" to "sddp" ? :)

note that the dialer app also accepts * and # characters, for USSD stuff. Do we want to catch them too ?

@@ +36,5 @@
>      regexp: new RegExp([
> +      // sdd: space, dot, dash or parens
> +      '(\\+\\d{1,4}[\\s.()-]{0,3}|\\()?' +       // (\+<digits><sdd>|\()?
> +      '(\\d{1,4}[\\s.()-]{0,3})?' +              // <digits><sdd>*
> +      '(\\d[\\d\\s.()-]{0,100}\\d)'              // <digit><digit|sdd>+<digit>

In your last comment, I think you want "*" instead of "+".

Please use non-capturing groups if you don't use what you're capturing.

@@ +41,3 @@
>        ].join(''), 'mg'),
> +    matchFilter: function phoneMatchFilter(phone, link) {
> +      if (Utils.removeNonDialables(phone).length < 5) {

Do we want to match known emergency numbers (911, 112, 991) ? Problem is that it's not comprehensive so we may want to not do it now. Or we may want to do it for some of them. What do you think ?

also, I'd like to get rid of the removeNonDialables in bug 877718, so I think it's just easier to test if the length of "phone" is less than eg 6 or 7.

@@ +48,2 @@
>      transform: function phoneTransform(phone) {
> +      var dialable = Utils.removeNonDialables(phone);

I think we don't need to do this here, the dialer app can do this work.

::: apps/sms/test/unit/link_helper_test.js
@@ +8,5 @@
> +requireApp('sms/js/utils.js');
> +requireApp('sms/test/unit/mock_utils.js', MockUtilsLoadedForLinkHelper);
> +
> +// wait for mock utils loaded so we can use the data to do stuff
> +function MockUtilsLoadedForLinkHelper() {

you dont need to do this, suites are not ran before requires are finished. (I actually tried removing them and it worked, both the full suites and running only this file)

@@ +236,5 @@
> +      });
> +
> +      test('Phone from #887737', function() {
> +        testPhoneOK('[phone]');
> +      });

please try stuff like "[phone]" too (you can put it in the test fixtures instead of course).

::: apps/sms/test/unit/utils_test.js
@@ +6,5 @@
>  requireApp('sms/test/unit/mock_l10n.js');
> +requireApp('sms/test/unit/mock_utils.js', MockUtilsLoadedForUtils);
> +
> +// wait for mock utils loaded so we can use the data to do stuff
> +function MockUtilsLoadedForUtils() {

same here, that's useless

Comment 8

[Corey Frang [:gnarf]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> should you rename "sdd" to "sddp" ? :)

done

> note that the dialer app also accepts * and # characters, for USSD stuff. Do
> we want to catch them too ?

nope, this is clickable links, I don't think we want dialable USSDs (at least not previously supported, and support not requested yet)

> In your last comment, I think you want "*" instead of "+".
> 
> Please use non-capturing groups if you don't use what you're capturing.

done and done

> Do we want to match known emergency numbers (911, 112, 991) ? Problem is
> that it's not comprehensive so we may want to not do it now. Or we may want
> to do it for some of them. What do you think ?

Nope, I don't want a clickable 911 link in a text

> also, I'd like to get rid of the removeNonDialables in bug 877718, so I
> think it's just easier to test if the length of "phone" is less than eg 6 or
> 7.

The function will still be needed to compare dialables, so it won't be "removed" just not used in that bug yeah?

> I think we don't need to do this here, the dialer app can do this work.

It makes sense to me to do it here, I have no real tie to doing this, but I feel like a tel: style link should probably strip spaces and parens and such

> you dont need to do this, suites are not ran before requires are finished.
> (I actually tried removing them and it worked, both the full suites and
> running only this file)

You do need to do this, MockUtils won't exist when the suites run so the fixture cases won't load...  There aren't failures per se, but it doesn't load the cases.


> please try stuff like "[phone]" too (you can put it in the test
> fixtures instead of course).

Added to the fixture

Comment 9

[Corey Frang [:gnarf]]

Attachment: patch v4

Moved the test data to its own file, and fixed a bunch of responses from the first review.

Comment 10

[Corey Frang [:gnarf]]

Comment on attachment 770938 [details] [diff] [review]
patch v4

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

::: apps/sms/test/unit/link_helper_test.js
@@ +7,5 @@
>  
> +requireApp('sms/js/utils.js');
> +requireApp('sms/test/unit/mock_utils.js');
> +
> +// wait for mock utils loaded so we can use the data to do stuff

removed this dirt on my own

::: apps/sms/test/unit/utils_test.js
@@ +5,3 @@
>  requireApp('sms/test/unit/mock_contact.js');
>  requireApp('sms/test/unit/mock_l10n.js');
> +requireApp('sms/test/unit/mock_utils.js');

removed this dirt on my own

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 770938 [details] [diff] [review]
patch v4

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

r=me

thanks !

Comment 12

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/9e7768c2ccd985923e00c9d90aa3f92dfa771913
v1-train: https://github.com/mozilla-b2g/gaia/commit/496465667717dd17d9f18b884493f0d1cdabbe55

Comment 13

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 496465667717dd17d9f18b884493f0d1cdabbe55