B2G SMS: PhoneNumberJS it's not working properly in Gecko. Phone number it's not internationalized when sending a SMS

RESOLVED DUPLICATE of bug 823010

Status

()

Core
DOM: Device Interfaces
RESOLVED DUPLICATE of bug 823010
6 years ago
5 years ago

People

(Reporter: borjasalguero, Assigned: gwagner)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+)

Details

(Whiteboard: [EU_TPE_TRIAGED][triage:1/18][waiting on bug 823010])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Scenario:
- Send a SMS to any local non-internationalized phone number (in Spain for example '612123123').

EXPECTED:
- SMS is stored in Gecko the internationalized version '+34612123123', and it's retrieved when the status change to 'onsending', so thread view is rendered properly

CURRENTLY:
- SMS is stored with NON internationalized version '612123123', and when we try to retrieve all messages with a filter '612123123' we are not retrieve any SMS! :S

This should be blocking basecamp because it's a broken feature.

Gecko:
b75dfee
Gaia:
df38c1b
(Reporter)

Updated

6 years ago
blocking-basecamp: --- → ?
(Reporter)

Comment 1

6 years ago
Gregor, I've found this issue with Today's build, Do you know what it's happening here?
Flags: needinfo?(anygregor)
FYI - Use tef? and tracking-b2g? now. We've retired basecamp? as of EOD Friday.
blocking-b2g: --- → tef?
blocking-basecamp: ? → ---
tracking-b2g18: --- → ?
(Assignee)

Comment 3

6 years ago
(In reply to Borja Salguero [:borjasalguero] from comment #1)
> Gregor, I've found this issue with Today's build, Do you know what it's
> happening here?

Good question :) I haven't touched this code in the last few days.
Do you know how recent this regression is?
Are you using the b2g18 gecko branch or any another one like mozilla-central?
Flags: needinfo?(anygregor)
(Assignee)

Updated

6 years ago
Keywords: regressionwindow-wanted
(Reporter)

Comment 4

6 years ago
Im testing with the builds created by the manifest shared by Mozilla, based on b2g18. I've tested and it's failing in these builds since last tuesday, but I was working right with the builds that we created in Berlin... :S
(Assignee)

Comment 5

6 years ago
Oh that's the bug where we send an SMS to a local number like 987-54-3210. The actual SMS is sent in the US to +19876543210 and we stay in the thread with 987-54-3210 and no SMS is getting displayed after sending. If you go back to the main thread list you see an entry for +19876543210 with the corresponding SMS.

Was this the bug where we call the success callback with the local number or do we just not update the number in gaia?
(Reporter)

Comment 6

6 years ago
In GAIA we go to the thread once we received the 'onsending' event, retrieving the phone number directly from Gecko https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/sms.js#L27 . The problem is that the message retrieved from Gecko has not an internationalized phone number! That's why the App it's broken.

Updated

6 years ago
blocking-b2g: tef? → tef+
(Assignee)

Comment 7

6 years ago
So the problem is in RadioInterfaceLayer.js: 
https://hg.mozilla.org/mozilla-central/file/08a48d3625d2/dom/system/gonk/RadioInterfaceLayer.js#l2438

We call saveSendingMessage with the local number and in the SMS DB we change the local number to the international number with PhoneNumberJS.
But afterwards we notify all observers that we sent an sms with the non-internationalized number in sendSMS:
https://hg.mozilla.org/mozilla-central/file/08a48d3625d2/dom/system/gonk/RadioInterfaceLayer.js#l2452

We have 2 options here:
1) Return the internationalized version of the number from the call into the DB.
2) Apply PhoneNumberJS already in the sendSMS function in RadioInterfaceLayer.js
blocking-b2g: tef+ → tef?
(Assignee)

Comment 8

6 years ago
Created attachment 701894 [details] [diff] [review]
patch
Assignee: nobody → anygregor
(Assignee)

Updated

6 years ago
Keywords: regressionwindow-wanted
(Assignee)

Comment 9

6 years ago
Created attachment 701897 [details] [diff] [review]
patch
Attachment #701894 - Attachment is obsolete: true
Attachment #701897 - Flags: review?(vyang)
Duplicate of this bug: 830629
Triage: bug 830629 is considered as TEF+. TEF+ on this bug as bug 830629 is closed as dup
blocking-b2g: tef? → tef+
Whiteboard: [EU_TPE_TRIAGED]
tracking-b2g18: ? → ---
Whiteboard: [EU_TPE_TRIAGED] → [EU_TPE_TRIAGED][triage:1/16]

Updated

6 years ago
Duplicate of this bug: 831462
(Assignee)

Comment 13

6 years ago
Comment on attachment 701897 [details] [diff] [review]
patch

I don't know if vicamo is around.
Attachment #701897 - Flags: review?(bent.mozilla)

Comment 14

6 years ago
Couple of things:

1. The reason this issue was not occurring before is somewhere this internationalization WAS occurring prior to the SMS being sent, probably in Gaia. For example on my older build - Gecko(7bcf949d)/Gaia(e24d0662) where this works, when I try to send an SMS to "6502530000", after I hit "Send", the conversation view's title in SMS app shows "+16502530000". 

2. There's a possibility of the same issue occurring on incoming SMS as well. If a non TOA_INTERNATIONAL SMS arrives, then the SMS database service will still internationalize it in saveReceivedMessage, while in the notification to Gaia, the number will not be internationalized.

Updated

6 years ago
Blocks: 808607
Comment on attachment 701897 [details] [diff] [review]
patch

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

This sounds like the right solution to me. However:

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +18,5 @@
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/PhoneNumberUtils.jsm");

This will be a hit to our startup time, right? Maybe you should make this a lazy getter, or set up some kind of idle timer load to get this?

@@ +2436,5 @@
>      }
>  
> +    if (number) {
> +      let parsedNumber = PhoneNumberUtils.parse(number.toString());
> +      number = (parsedNumber && parsedNumber.internationalNumber)

This won't update the field on the 'options' object. I don't know if that's bad or not but it certainly seems weird. Can we do this parsing before we create the options object?
(In reply to Gregor Wagner [:gwagner] from comment #7)
> So the problem is in RadioInterfaceLayer.js: 
> We call saveSendingMessage with the local number and in the
> SMS DB we change the local number to the international number
> with PhoneNumberJS. But afterwards we notify all observers
> that we sent an sms with the non-internationalized number in
> sendSMS.

Hi Gregor, it seems the attachment #703142 [details] [diff] [review] will fix this as well. Philikon is going to land it tomorrow, maybe by the time this issue will be automatically fixed as well. Let's check that again after bug 823010. Shall we?
(Assignee)

Comment 17

6 years ago
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> (In reply to Gregor Wagner [:gwagner] from comment #7)
> > So the problem is in RadioInterfaceLayer.js: 
> > We call saveSendingMessage with the local number and in the
> > SMS DB we change the local number to the international number
> > with PhoneNumberJS. But afterwards we notify all observers
> > that we sent an sms with the non-internationalized number in
> > sendSMS.
> 
> Hi Gregor, it seems the attachment #703142 [details] [diff] [review] [diff] [review] will
> fix this as well. Philikon is going to land it tomorrow, maybe by the time
> this issue will be automatically fixed as well. Let's check that again after
> bug 823010. Shall we?

Sounds good to me!
Whiteboard: [EU_TPE_TRIAGED][triage:1/16] → [EU_TPE_TRIAGED][triage:1/18][waiting on bug 823010]
(Assignee)

Updated

6 years ago
Attachment #701897 - Flags: review?(vyang)
(Assignee)

Comment 18

6 years ago
I tried it with the patch in bug 823010 and it seems to work.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Depends on: 823010
Resolution: --- → WORKSFORME
Let's mark as a duplicate of bug 823010 in that case.
Resolution: WORKSFORME → DUPLICATE
Duplicate of bug: 823010
You need to log in before you can comment on or make changes to this bug.