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

RESOLVED FIXED in Firefox OS v1.1hd

Status

Firefox OS
Gaia::SMS
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: isabelrios, Assigned: gnarf)

Tracking

unspecified
1.1 QE4 (15jul)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

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

Details

(Whiteboard: [LeoVB+] )

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 2

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

Updated

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

Comment 4

5 years ago
Just for information, I double checked this on android 4.2, it also happens there.
(Assignee)

Updated

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

Comment 5

5 years ago
Created attachment 774822 [details] [diff] [review]
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
Attachment #774822 - Flags: review?(felash)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

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

Comment 7

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

Updated

5 years ago
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)

Comment 9

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

Comment 10

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

Comment 12

5 years ago
Created attachment 775689 [details]
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+
(Assignee)

Comment 14

5 years ago
master: https://github.com/mozilla-b2g/gaia/commit/70de7c7971bbb2bdc4bd9e7ddc3f0ae3a40a9f9f
v1-train: https://github.com/mozilla-b2g/gaia/commit/3636947a317d5ff694c66d0cd4377c549b4e4e2e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-b2g18: --- → fixed
status-b2g-v1.1hd: --- → affected
Resolution: --- → FIXED

Comment 15

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

Comment 16

5 years ago
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

5 years ago
Created attachment 777349 [details]
Screenshot

Comment 18

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

Comment 19

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

Comment 20

5 years ago
(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.
(Reporter)

Comment 21

5 years ago
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
(Reporter)

Comment 22

5 years ago
Bug895307 describing the new case seen related to this bug

Comment 23

5 years ago
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

5 years ago
Created attachment 778474 [details]
1980-01-06-21-54-45.png

Comment 25

5 years ago
Created attachment 778476 [details]
1980-01-06-21-54-53.png

Updated

5 years ago
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

5 years ago
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.

Updated

5 years ago
Whiteboard: [LeoVB+]

Comment 27

5 years ago
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
status-b2g18: fixed → verified
v1.1.0hd: 3636947a317d5ff694c66d0cd4377c549b4e4e2e
status-b2g-v1.1hd: affected → fixed
(Assignee)

Comment 29

5 years ago
Considering QA just verified, I'm re-closing - please open a new bug with additional cases - thanks.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 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.