Closed
Bug 885280
Opened 12 years ago
Closed 12 years ago
[SMS] The sms app does not group together correctly the numbers with prefix
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
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
Reporter | ||
Updated•12 years ago
|
blocking-b2g: --- → leo?
Reporter | ||
Comment 1•12 years ago
|
||
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)
Comment 2•12 years ago
|
||
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
Comment 3•12 years ago
|
||
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)
Keywords: regression,
regressionwindow-wanted
Comment 4•12 years ago
|
||
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
Comment 5•12 years ago
|
||
@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
Comment 6•12 years ago
|
||
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
Comment 7•12 years ago
|
||
Comment 8•12 years ago
|
||
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
Updated•12 years ago
|
status-b2g18:
--- → affected
status-b2g18-v1.0.1:
--- → unaffected
Gregor, is this the same thing as bug 883770?
Flags: needinfo?(anygregor)
Comment 10•12 years ago
|
||
(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.
Updated•12 years ago
|
Comment 11•12 years ago
|
||
(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.
Updated•12 years ago
|
blocking-b2g: leo? → leo+
Assignee: nobody → chulee
Comment 12•12 years ago
|
||
Thanks Check for taking this! Really appreciate!
Comment 13•12 years ago
|
||
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 16•12 years ago
|
||
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+
Assignee | ||
Comment 17•12 years ago
|
||
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+
Comment 18•12 years ago
|
||
Please add tests and also create an upstream PR for https://github.com/andreasgal/PhoneNumber.js
Thanks!
Flags: needinfo?(anygregor)
Comment 19•12 years ago
|
||
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.
Keywords: regressionwindow-wanted
Attachment #768230 -
Attachment is obsolete: true
Attachment #770616 -
Flags: review?(vyang)
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)
Updated•12 years ago
|
Flags: needinfo?(fbsc)
Comment 26•12 years ago
|
||
(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?
Assignee | ||
Comment 27•12 years ago
|
||
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)
Assignee | ||
Comment 28•12 years ago
|
||
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)
Assignee: chulee → vyang
Assignee | ||
Comment 29•12 years ago
|
||
Updated•12 years ago
|
Attachment #773184 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 30•12 years ago
|
||
Rebase.
Attachment #773184 -
Attachment is obsolete: true
Attachment #775483 -
Flags: review+
Assignee | ||
Comment 31•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Whiteboard: [fixed-in-birch]
Assignee | ||
Comment 32•12 years ago
|
||
For b2g18 patch, I think we'd at least need changes from bug 871433, which normalize phone numbers before searching.
Assignee | ||
Comment 33•12 years ago
|
||
Comment 34•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 35•12 years ago
|
||
status-b2g18-v1.0.0:
--- → wontfix
status-b2g-v1.1hd:
--- → affected
status-firefox23:
--- → wontfix
status-firefox24:
--- → wontfix
status-firefox25:
--- → fixed
Comment 36•12 years ago
|
||
Comment 37•12 years ago
|
||
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 → ---
Assignee | ||
Comment 38•12 years ago
|
||
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
Assignee | ||
Comment 39•12 years ago
|
||
Check nullity of |parsedAddress.internationalNumber|.
Attachment #777643 -
Flags: review?(gene.lian)
Assignee | ||
Comment 40•12 years ago
|
||
Updated•12 years ago
|
Attachment #777643 -
Flags: review?(gene.lian) → review+
Assignee | ||
Comment 41•12 years ago
|
||
Updated•12 years ago
|
Comment 42•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 43•12 years ago
|
||
Comment 44•12 years ago
|
||
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
Updated•12 years ago
|
Whiteboard: [fixed-in-birch] → [fixed-in-birch] [LeoVB+]
You need to log in
before you can comment on or make changes to this bug.
Description
•