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

Status: RESOLVED DUPLICATE of bug 823010

(Core :: DOM: Device Interfaces, defect)

Description

[Borja Salguero [:borjasalguero]]

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 '[phone]', 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

Comment 1

[Borja Salguero [:borjasalguero]]

Gregor, I've found this issue with Today's build, Do you know what it's happening here?

Comment 2

[Jason Smith [:jsmith]]

FYI - Use tef? and tracking-b2g? now. We've retired basecamp? as of EOD Friday.

Comment 3

[Gregor Wagner [:gwagner]]

(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?

Comment 4

[Borja Salguero [:borjasalguero]]

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

Comment 5

[Gregor Wagner [:gwagner]]

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 [phone] 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 [phone] 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?

Comment 6

[Borja Salguero [:borjasalguero]]

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.

Comment 7

[Gregor Wagner [:gwagner]]

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

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

Triage: bug 830629 is considered as TEF+. TEF+ on this bug as bug 830629 is closed as dup

Comment 13

[Gregor Wagner [:gwagner]]

Comment on attachment 701897 [details] [diff] [review]
patch

I don't know if vicamo is around.

Comment 14

[Alhad Purnapatre]

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 "[phone]". 

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.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

(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?

Comment 17

[Gregor Wagner [:gwagner]]

(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!

Comment 18

[Gregor Wagner [:gwagner]]

I tried it with the patch in bug 823010 and it seems to work.

Comment 19

[Alex Keybl [:akeybl]]

Let's mark as a duplicate of bug 823010 in that case.