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

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Leo, Assigned: gnarf)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

Details

(Whiteboard: [TD-19700], [u=commsapps-user c=messaging p=0],[LeoVB+])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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: sasikala.paruchuri8@gmail.com
(Reporter)

Comment 1

4 years ago
The following is the phone number where the device is not recognizing.
Number : +5511 98907-6047
Assignee: nobody → gnarf37
Whiteboard: [TD-19700]
(Reporter)

Updated

4 years ago
blocking-b2g: --- → leo+
Depends on: 870711
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 770095 [details] [diff] [review]
patch v1

Must be applied on top of the pull request for #870711
Attachment #770095 - Flags: review?(felash)
(Assignee)

Comment 3

4 years ago
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
(Assignee)

Comment 4

4 years ago
Created attachment 770115 [details] [diff] [review]
patch v2

Fixed some stuff from the first patch that was causing units to fail
Attachment #770095 - Attachment is obsolete: true
Attachment #770095 - Flags: review?(felash)
Attachment #770115 - Flags: review?(felash)
(Assignee)

Comment 5

4 years ago
reminder to reviews - requires patch for bug 870711 to be applied
(Assignee)

Comment 6

4 years ago
Created attachment 770274 [details] [diff] [review]
patch v3

Fixes some issues with rebase on the url regexp bug
Attachment #770115 - Attachment is obsolete: true
Attachment #770115 - Flags: review?(felash)
Attachment #770274 - Flags: review?(felash)
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('+5511 98907-6047');
> +      });

please try stuff like "+33 (0) 612345678" 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
Attachment #770274 - Flags: review?(felash)
(Assignee)

Comment 8

4 years ago
(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 "+33 (0) 612345678" too (you can put it in the test
> fixtures instead of course).

Added to the fixture
(Assignee)

Comment 9

4 years ago
Created attachment 770938 [details] [diff] [review]
patch v4

Moved the test data to its own file, and fixed a bunch of responses from the first review.
Attachment #770274 - Attachment is obsolete: true
Attachment #770938 - Flags: review?(felash)
(Assignee)

Comment 10

4 years ago
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
Whiteboard: [TD-19700] → [TD-19700], [u=commsapps-user c=messaging p=0]
Comment on attachment 770938 [details] [diff] [review]
patch v4

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

r=me

thanks !
Attachment #770938 - Flags: review?(felash) → review+
(Assignee)

Comment 12

4 years ago
master: https://github.com/mozilla-b2g/gaia/commit/9e7768c2ccd985923e00c9d90aa3f92dfa771913
v1-train: https://github.com/mozilla-b2g/gaia/commit/496465667717dd17d9f18b884493f0d1cdabbe55
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g18: --- → fixed
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Whiteboard: [TD-19700], [u=commsapps-user c=messaging p=0] → [TD-19700], [u=commsapps-user c=messaging p=0],[LeoVB+]
v1.1.0hd: 496465667717dd17d9f18b884493f0d1cdabbe55
status-b2g-v1.1hd: --- → fixed
You need to log in before you can comment on or make changes to this bug.