Closed Bug 892480 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

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

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
b2g18 --- verified
b2g-v1.1hd --- fixed

People

(Reporter: isabelrios, Assigned: gnarf)

Details

(Whiteboard: [LeoVB+] )

Attachments

(4 files, 1 obsolete file)

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
Are you referring to recipients or the actual message body?
Assignee: nobody → waldron.rick
Sorry it wasn't clear, the phone numbers are in the message body.
Priority: -- → P1
Whiteboard: [leo-triage]
Target Milestone: --- → 1.1 QE5
(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
Assignee: waldron.rick → gnarf37
Just for information, I double checked this on android 4.2, it also happens there.
Summary: [SMS] Sending two phone numbers separed by space or intro in a sms are taken as only one number → [SMS] Sending two phone numbers separed by space or enter should detect two phone numbers
Attached patch patch v1 (obsolete) — Splinter Review
* 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
Attachment #774822 - Flags: review?(felash)
Status: NEW → ASSIGNED
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.
And by "this one case" i mean two nine digit numbers without spacing inside separated by a space.
Target Milestone: 1.1 QE5 → 1.1 QE4 (15jul)
Comment on attachment 774822 [details] [diff] [review]
patch v1

I'm in holiday ;)

redirecting to Francisco as he fixed one regression here lately.
Attachment #774822 - Flags: review?(felash) → review?(francisco.jordano)
Even Leo will be interested in taking this patch as it would add more usability to end user.
Discussed with Mozilla. Marking this as Leo+ as it would add good user experience. - Veeresh.
blocking-b2g: leo? → leo+
Whiteboard: [leo-triage]
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!
Attachment #774822 - Flags: review?(francisco.jordano) → review-
Attached file patch v2
patch v2

now properly handles newline by making only space and tab proper "separators"
Attachment #774822 - Attachment is obsolete: true
Attachment #775689 - Flags: review?(francisco.jordano)
Comment on attachment 775689 [details]
patch v2

\o/

Thanks a lot for fixing this super fast!

Could you rebase and merge?

Thanks!
Attachment #775689 - Flags: review?(francisco.jordano) → review+
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
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
Attached image Screenshot
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
I don't get this? "clicking on the status bar" - Do you mean the message text bubble, or some other "status bar" ?
(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.
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.
Status: RESOLVED → VERIFIED
Bug895307 describing the new case seen related to this bug
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.
Attached image 1980-01-06-21-54-45.png
Attached image 1980-01-06-21-54-53.png
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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 "+33 1 23 45 67 89" 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.
Whiteboard: [LeoVB+]
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
v1.1.0hd: 3636947a317d5ff694c66d0cd4377c549b4e4e2e
Considering QA just verified, I'm re-closing - please open a new bug with additional cases - thanks.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
(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).
You need to log in before you can comment on or make changes to this bug.