Closed Bug 885280 Opened 6 years ago Closed 6 years ago

[SMS] The sms app does not group together correctly the numbers with prefix

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: rafael.marquez, Assigned: vicamo)

References

Details

(Keywords: regression, Whiteboard: [fixed-in-birch] [LeoVB+])

Attachments

(5 files, 7 obsolete files)

14.24 KB, image/png
Details
19.22 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
19.19 KB, patch
Details | Diff | Splinter Review
1.49 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
1.49 KB, patch
Details | Diff | Splinter Review
V1-train
Gecko: 856e202
Gaia: aea6834
Ril: Moz Ril

*Procedure
1. Open sms app
2. Send a sms to the number 666 666 666
3. Send a sms to the number 0034 666 666 666

*Expected Result
Only a conversation is created

*Actual Result
Two conversation are created
blocking-b2g: --- → leo?
The same bug is reproduced if you have a contact with the number 0034 666 666 666 and you send an SMS to the number 0034 666 666 666 and another to the number 666 666 666(two conversations are created)
Updating the component. This should be handled by mozMobileMessage when determining the threadId for the newly created message.
Component: Gaia::SMS → DOM: Device Interfaces
Product: Boot2Gecko → Core
Triage suspects this is a regression - if it is that helps lean more towards blocking - can you confirm?  Only if this is a regression from 1.0.1 can this be a blocker at this time.
Flags: needinfo?(fbsc)
I am trying to reproduce this issue within US, unable to reproduce
 1. send an SMS to a number 650-933-6961
 2. send an SMS to a number 1-650-933-6961
--> one SMS thread is created

or it's not the case... ?

Leo
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/62049a1c5d36
Gaia   885b874029d8143035765ae903c147a9415fcc52
Build  20130624070224
Version 18.1
@nkot this is correct behaviour(In reply to nkot from comment #4)
> I am trying to reproduce this issue within US, unable to reproduce
>  1. send an SMS to a number 650-933-6961
>  2. send an SMS to a number 1-650-933-6961
> --> one SMS thread is created
> 
> or it's not the case... ?

This is the same behaviour I see.

> 
> Leo
> Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/62049a1c5d36
> Gaia   885b874029d8143035765ae903c147a9415fcc52
> Build  20130624070224
> Version 18.1
This is reproducible:
v1-train
Gecko-26944ea
Gaia-05ff7b4

Please see screenshot attached.
For the same number with and without country code, two threads are created
I tried with v1.0.1 and cannot reproduce that. Messages to the same number with and without country code are grouped in the same thread
Gregor, is this the same thing as bug 883770?
Flags: needinfo?(anygregor)
(In reply to Isabel Rios [:isabel_rios] from comment #7)
> Created attachment 767130 [details]
> number with and without country code

Hi Isabel,

Thanks for the screen shot! Have your ever tried to killed the Message App and launched that again to see if this issue is still present?

I'd believe the backend should be able to put these two messages into the same thread, because the Gecko didn't modify the logic for creating threads recently. I'm wondering the Gaia end doesn't call the getThread(...) again to refresh the thread list.
Blocks: b2g-sms, b2g-mms
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(In reply to Gene Lian [:gene] from comment #10)
> (In reply to Isabel Rios [:isabel_rios] from comment #7)
> > Created attachment 767130 [details]
> > number with and without country code
> 
> Hi Isabel,
> 
> Thanks for the screen shot! Have your ever tried to killed the Message App
> and launched that again to see if this issue is still present?
> 
> I'd believe the backend should be able to put these two messages into the
> same thread, because the Gecko didn't modify the logic for creating threads
> recently. I'm wondering the Gaia end doesn't call the getThread(...) again
> to refresh the thread list.

Hi Gene, 
I tried killing the app and even powering the device off and on, the messages are not grouped.
blocking-b2g: leo? → leo+
Thanks Check for taking this! Really appreciate!
s/Check/Chuck/
I have tried to compare the difference of codes between central and b2g18 1.0.1, it shows that there is no difference in code involving message grouping, the call flow is [1] -> [2] -> [3].
And according the flow, "666-666-666" and "0034-666-666-666" will be treated as different number. Because the |defaultRegion| is null and these numbers are not start with '+', the |endsWidth()| test will never be applied.
Maybe the grouping issue is handled in gaia or elsewhere.

But I found in ITU-T E.164, Clause 12[6], shows that except prefix '+', prefix '00' also can be used as international prefix, which is not handled in PhoneNumber.jsm now.

So I think adding support for prefix '00' in PhoneNumber.jsm can solve the issue.

[1] MobileMessageDatabaseService.js : findParticipantRecordByAddress()
    central : http://hg.mozilla.org/mozilla-central/file/3b955f306226/dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
    b2g18 1.0.1 : http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/sms/src/ril/SmsDatabaseService.js
[2] PhoneNumberUtils.jsm : parseWithMCC()
    central : http://hg.mozilla.org/mozilla-central/file/3b955f306226/dom/phonenumberutils/PhoneNumberUtils.jsm
    b2g18 1.0.1 : http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/phonenumberutils/PhoneNumberUtils.jsm
[3] PhoneNumber.jsm : parsenumber()
    central : http://hg.mozilla.org/mozilla-central/file/3b955f306226/dom/phonenumberutils/PhoneNumber.jsm
    b2g18 1.0.1 : http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/phonenumberutils/PhoneNumber.jsm
[4] http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/phonenumberutils/PhoneNumber.jsm#l331
[5] http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/file/9c62297d11b0/dom/sms/src/ril/SmsDatabaseService.js#l864
[6] http://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-E.164-201011-I!!PDF-E&type=items
Treat prefix "00" in address as prefix of international number as mentioned in comment 14.
Attachment #768230 - Flags: feedback?(vyang)
Attachment #768230 - Flags: feedback?(gene.lian)
Comment on attachment 768230 [details] [diff] [review]
WIP - Treat prefix "00" in address as prefix of international number.

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

Looks great!
Attachment #768230 - Flags: feedback?(gene.lian) → feedback+
Comment on attachment 768230 [details] [diff] [review]
WIP - Treat prefix "00" in address as prefix of international number.

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

Thank you :)
Attachment #768230 - Flags: feedback?(vyang) → review+
Please add tests and also create an upstream PR for https://github.com/andreasgal/PhoneNumber.js

Thanks!
Flags: needinfo?(anygregor)
Looks like we've got a r+ here, so we have the information we need to move forward to fix this bug. So we probably don't need a regression window now. Feel free to set the flag again if you still need it.
Try : https://tbpl.mozilla.org/?tree=Try&rev=d52f185609dd
Test failed while parsing number "0011-650-333-6000"

21:14:18     INFO -  try: 0011-650-333-6000, SG
21:14:18     INFO -  4142 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/phonenumberutils/tests/test_phonenumber.xul | got: 0011-650-333-6000 SG

It seems that some carriers provide special prefixes, '001' in the test case, for dialing international number for special rate or service.
In such case, we have to replace the whole prefix, '001' in the test case, into '+' instead of replace '00' only.
So the solution of replacing prefix '00' into '+' directly can't work for all and might cause other problem.

We are looking for other solutions. :(
Based on the result of this try run : https://tbpl.mozilla.org/?tree=Try&rev=017a08e470cb
The test cases passes with patch 0002 along, which means we can handle the prefix correctly without patch 0001.

The difference between test case and SMS DB is that, SMS DB[1] doesn't assign a default MCC, so the Phone Number Utility can't handle prefix properly.

The new solution is get MCC from RIL and provide it to |PhoneNumberUtils.parseWithMCC()| in SMS DB.

But there might be a problem if user changes SIM to different country with different prefix pattern(maybe on business trip or something). This will affect the phone number parsing and the grouping will be wrong again.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js#846
Phone Number Utility already has the ability to handle IDD, but requires a country code.
So if MCC is not assigned, use the MCC of device.

This doesn't solve the issue of user changing SIM card with different MCC(apply different IDD rule based on SIM card) or no SIM card in device(apply IDD of default MCC, now it's hard-coded to Brazil).
The only way I can come up with to resolve this is, save the MCC of every SMS, and parse number with it.
This solution needs lots of work, and considering this is a rare use case, I prefer we can ignore it for now.
Attachment #770615 - Attachment is obsolete: true
Attachment #772009 - Flags: review?(vyang)
Rebase.
Attachment #770616 - Attachment is obsolete: true
Attachment #770616 - Flags: review?(vyang)
Attachment #772011 - Flags: review?(vyang)
Flags: needinfo?(fbsc)
(In reply to Chuck Lee [:chucklee] from comment #24)
> Created attachment 772009 [details] [diff] [review]
> 0001. Use country of device in parseWithMCC() if no MCC is provided.

This is exactly what PhoneNumberUtils.Parse does. No?
Attached patch patch (obsolete) — Splinter Review
Use 'PhoneNumberUtils.parse(number)' instead of 'PhoneNumberUtils.parseWithMCC(number, null)' for phone number parsing.  Parsed international number of 01115555211014 is +15555211014.  Address this possible difference as well.

Also add test cases for various phone number match cases in MobileMessageDatabaseService.  Originated from bug 849739 attachment 734592 [details] [diff] [review].
Attachment #772009 - Attachment is obsolete: true
Attachment #772011 - Attachment is obsolete: true
Attachment #772009 - Flags: review?(vyang)
Attachment #772011 - Flags: review?(vyang)
Attachment #773180 - Flags: review?(anygregor)
Attached patch patch : v2 (obsolete) — Splinter Review
Remove one redundant comment line.  Sorry for the noises.
Attachment #773180 - Attachment is obsolete: true
Attachment #773180 - Flags: review?(anygregor)
Attachment #773184 - Flags: review?(anygregor)
Attachment #773184 - Flags: review?(anygregor) → review+
Attached patch patch : v3Splinter Review
Rebase.
Attachment #773184 - Attachment is obsolete: true
Attachment #775483 - Flags: review+
Whiteboard: [fixed-in-birch]
Depends on: 871433
For b2g18 patch, I think we'd at least need changes from bug 871433, which normalize phone numbers before searching.
https://hg.mozilla.org/mozilla-central/rev/f471c8057f90
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Re-opening since looks like this patch is causing a problem in b2g18:

Trying to send a message to '123123123' does not work. After tapping on send, the 'To' and the 'Text' field are removed. Nothing is sent, there is not any error message shown.

This is working fine with 07/15 build: Gecko-2b310a1-Gaia-d52ce22

But not with 07/16 one: Gecko-ea7cfd4-Gaia-c8f432c

Log:
E/GeckoConsole(  461): Content JS LOG at app://sms.gaiamobile.org/js/message_manager.js:389 in onError: Error Sending: {}
E/GeckoConsole(  109): [JavaScript Error: "[Exception... "Data provided to an operation does not meet requirements."  code: "0" nsresult: "0x80660005 (DataError)"  location: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js Line: 850"]" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 850}]
E/GeckoConsole(  109): [JavaScript Error: "aMessageRecord is undefined" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 751}]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Confirmed. Object returned from |PhoneNumberUtils.parse()| may have a null |internationalNumber|, so we may have |needles = ['123123123', null]|, which skips storing that message into object store in following steps.

See: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js#837
Attached patch m-c follow-upSplinter Review
Check nullity of |parsedAddress.internationalNumber|.
Attachment #777643 - Flags: review?(gene.lian)
Attached patch b2g18 follow-upSplinter Review
Attachment #777643 - Flags: review?(gene.lian) → review+
https://hg.mozilla.org/mozilla-central/rev/2df208471aed
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Cannot reproduce the original bug or the problem described in comment 37.

Verified with Unagi device 07/18 v1-train build:
Gecko-4bc3f2f
Gaia-f1d2e3f
ref ril
Status: RESOLVED → VERIFIED
Whiteboard: [fixed-in-birch] → [fixed-in-birch] [LeoVB+]
Duplicate of this bug: 897309
You need to log in before you can comment on or make changes to this bug.