892480 - [SMS] Sending two phone numbers separed by space or enter should detect two phone numbers

Status: RESOLVED FIXED

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

Description

[isabelrios]

Unagi device 07/11 v1-train build, ref ril:
Gecko-bd579ea
Gaia-680c72c

STR
There are two wrong situations:
A)
1. Create a new message with two phone numbers separed by space: 123456789 987654321
2. Send the message
3. Once the message has ben sent, open it an tap on for example the first number underlined

B) Create a new message with two phone numbers separed by intro: 
123456789
987654321
2. Send the message
3. Once the message has ben sent, open it an tap on for example the first number underlined

EXPECTED
In both cases in the dialer only the number that has been tapped should appear

ACTUAL
In both cases the dialer appears pre-filled with the two numbers as one: 123456789987654321

See bug 890342 which has already been fixed, but these two scenarios are still wrong

Comment 1

[Rick Waldron [:rwaldron]]

Are you referring to recipients or the actual message body?

Comment 2

[isabelrios]

Sorry it wasn't clear, the phone numbers are in the message body.

Comment 3

[Rick Waldron [:rwaldron]]

(In reply to Isabel Rios [:isabel_rios] from comment #2)
> Sorry it wasn't clear, the phone numbers are in the message body.

Thanks!

I'm going to send this along to Corey

Comment 4

[Corey Frang [:gnarf]]

Just for information, I double checked this on android 4.2, it also happens there.

Comment 5

[Corey Frang [:gnarf]]

Attachment: patch v1

* Shorten the greediness of the match to not accept as many numbers
* Added a check to ensure that the character after the match is not a digit

- All units still pass

Comment 6

[Corey Frang [:gnarf]]

There are a lot of edge cases here that can't be properly solved - for instance, neither iPhone or Android properly detects the two numbers for "815 555 1212 815 555 1212"

This solves this one case, and adds a unit test to ensure it doesn't regress.

Comment 7

[Corey Frang [:gnarf]]

And by "this one case" i mean two nine digit numbers without spacing inside separated by a space.

Comment 8

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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

I'm in holiday ;)

redirecting to Francisco as he fixed one regression here lately.

Comment 9

[Leo]

Even Leo will be interested in taking this patch as it would add more usability to end user.

Comment 10

[Leo]

Discussed with Mozilla. Marking this as Leo+ as it would add good user experience. - Veeresh.

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

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

Hi,

had to apply this by hand, but managed to test it.

I understand that we are reducing the number of character to detect a phone, I'm happy with reducing to 12 the number of digits (instead of 100) but this still doesn't solve the problem of adding two numbers with an enter between them.

Probably cause the Regex has the 'm' modifier. Is there any other technical reason to add the 'm' modifier to the regexp?

Thanks a lot!

Comment 12

[Corey Frang [:gnarf]]

Attachment: patch v2

patch v2

now properly handles newline by making only space and tab proper "separators"

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 775689 [details]
patch v2

\o/

Thanks a lot for fixing this super fast!

Could you rebase and merge?

Thanks!

Comment 14

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/70de7c7971bbb2bdc4bd9e7ddc3f0ae3a40a9f9f
v1-train: https://github.com/mozilla-b2g/gaia/commit/3636947a317d5ff694c66d0cd4377c549b4e4e2e

Comment 15

[Leo]

Even after applying the exsiting patch,able to reproduce the first STR
STR
There are two wrong situations:
A)
1. Create a new message with two phone numbers separed by space: 123456789 987654321
2. Send the message
3. Once the message has ben sent, open it an tap on for example the first number underlined

.Tested in v1-train as well.
Please check

Comment 16

[Corey Frang [:gnarf]]

Two nine digit numbers "123456789 987654321" works correctly and is detected as two links on my v1-train.  There is also a unit test specifically for this case: https://github.com/mozilla-b2g/gaia/blob/773a197b4e18377f366a0815778789aa4f25d412/apps/sms/test/unit/link_helper_test.js#L266-L271

Comment 17

[dkumar]

Attachment: Screenshot

Comment 18

[dkumar]

Issue reproduces on LEO 1.1 Mozilla RIL
Environmental Variables
Build ID: 20130717070237
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/582e3a7018b0
Gaia: c506c50adaaebcf729ac3c27887ba2931ab79040
Platform Version: 18.1
RIL Version: 01.01.00.019.158 

The problem is fixed from the UI point of view which is separating the numbers but fuctionality is still broken,clicking on the status bar where the numbers are present takes the user to a separate page where the numbers are considered as one number.

See attachemnet.

Issue still reproduces on COMM RIL
Build ID: 20130717070237
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/582e3a7018b0
Gaia: c506c50adaaebcf729ac3c27887ba2931ab79040
Platform Version: 18.1
RIL Version: 01.01.00.019.164

Comment 19

[Corey Frang [:gnarf]]

I don't get this? "clicking on the status bar" - Do you mean the message text bubble, or some other "status bar" ?

Comment 20

[dkumar]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #19)
> I don't get this? "clicking on the status bar" - Do you mean the message
> text bubble, or some other "status bar" ?

Sorry for the confusion, I meant header that displays the number.

Comment 21

[isabelrios]

Tested with Unagi device v1-train 07/17 build:
Gecko-2d17cfb
Gaia-c506c50
ref. ril

The problem is solved in both situations described above.

But, another case has been seen:
Sending: 'test1 600123123', the number is not detected as a link when the word ahead of it ends with a number. 

Opening a new bug for following up that new case.

Comment 22

[isabelrios]

Bug895307 describing the new case seen related to this bug

Comment 23

[bov]

Unagi device 2013/07/19 v1-train build, 
Both with Commercial Ril:AU164 and with Reference RIL
Gecko-6b60535.
Gaia-4abea64

STR
Tested five situations:

A)
1. Create a new message with two valid phone numbers (8 digits)separated by space: 12345678 12345678
2. Send the message
3. Once the message has ben sent, open it: both numbers should be shown underlined separated.
4.Tap on for example the first number underlined

B)
1. Create a new message with two valid phone numbers (9 digits)separated by space: 123456789 987654321
2. Send the message
3. Once the message has ben sent, open it: both numbers should be shown underlined separated.
4.Tap on for example the first number underlined

C)
Create a new message with two valid phone numbers (8 digits)separated by intro: 
12345678
87654321
2. Send the message
3. Once the message has ben sent, open it: both numbers should be shown underlined separated
4.Tap on for example the first number underlined

D)
Create a new message with two valid phone numbers (9 digits)separated by intro: 
123456789
987654321
2. Send the message
3. Once the message has ben sent, open it: both numbers should be shown underlined separated
4.Tap on for example the first number underlined

E)
Create a new message with two phone numbers separated by intro, first invalid (4 digits) and second valid (9 digits)separated by a space: 
1234 123456789
2. Send the message
3. Once the message has ben sent, open it: only the second number(valid), should be shown underlined 
4.Tap on the number underlined

EXPECTED
Only valid numbers (7 digits or more?) should be shown underlined, and tapping on them, in the dialer only the number that has been tapped should appear

ACTUAL
A),B)and E) Both numbers appear underlined as a unique valid phone number and tapping on it, the dialer appears pre-filled with the two numbers as one (Commercial RIl), or with no number pre-filled (Reference RIL)
C) and D) Both number are shown underlined separately and tapping on any of them, the dialer appears pre-filled with the last number pre-filled, not the selected one)(Commercial RIl), or with no number pre-filled (Reference RIL).

So, it seems that:
* In SMS only recognize valid numbers over 8 digits. PLEASE, clarify if this is the final agreement in order to change the existing test cases. Note that in previous builds (e.g. the one used in TESTRUN4), these test cases worked well and valid numbers were over 6 digits.
*Any case the dialer doesn't pre-filled the selected number.

Carol in copy cause Commercial RIL is also affected.

Comment 24

[bov]

Attachment: 1980-01-06-21-54-45.png

Comment 25

[bov]

Attachment: 1980-01-06-21-54-53.png

Comment 26

[Corey Frang [:gnarf]]

So here's the thing.

People separate numbers by spaces sometimes.

is "815 555 1212" a legal phone number?  is "1815 5551212" 4 then 7?

There are many edge cases here and the more we keep throwing at it the more impossible this will get to implement.  We have to draw the line somewhere.

If someone can help me understand the rules you want implemented we can make sure it is implemented correctly.

RE:

C) and D) Both number are shown underlined separately and tapping on any of them, the dialer appears pre-filled with the last number pre-filled, not the selected one)(Commercial RIl), or with no number pre-filled (Reference RIL).

This sounds like a problem outside of the sms app - I know for a fact that the B, and D cases are being unit tested and we are firing the proper activity.  The C case should be handled correctly (two separate links)

The A case gets a little tricky because of the way we detect separators.  "8 + (space) + 8" happens to be around the length needed to get "[phone]" to be detected.  I can add another case for the 8 + 8 and try to solve it.

The E case gets a little trickier yet: "0451 491934" needs to be detected as one number, 4+7, vs 4+9 - do we know that no locales use this numbering format?

Either way - we need to figure out why the links for B,C,D are working right on my unagi, but not your phone.

Comment 27

[sbogati]

Issue no longer repros.
Environmental  Variables:
Build ID: 20130723070209
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/68fb0a2e0114
Gaia: ffe25bfdf0e71c820ca710cb61fb8306564a8f4e
Platform Version: 18.1
Sending two phone numbers separated by space or enter is detecting as two phone numbers

Comment 28

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

v1.1.0hd: 3636947a317d5ff694c66d0cd4377c549b4e4e2e

Comment 29

[Corey Frang [:gnarf]]

Considering QA just verified, I'm re-closing - please open a new bug with additional cases - thanks.

Comment 30

[Julien Wajsberg [:julienw] (PTO -> may 13)]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #26)

> If someone can help me understand the rules you want implemented we can make
> sure it is implemented correctly.

plus, we still want to have this fast enough.

I'd pefer to have more false positives than false negatives (as we can't copy/paste for now).