839094 - PhoneNumberJS: handle countries sharing the same meta-data correctly

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Gregor Wagner [:gwagner]]

+++ This bug was initially created as a clone of Bug #818507 +++

I did an adb logcat, and I'm noticing this a lot:

[JavaScript Error: "formats is undefined" {file: "resource://gre/modules/PhoneNumber.jsm" line: 78}]


Lets start a new bug since this seems to be another bug and all the old comments don't apply.

Comment 1

[Gregor Wagner [:gwagner]]

Mike, what gecko version are you using, mc, b2g18, b2g18_v1*?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

According to the More Information panel in "Device Information", I'm on 1.0.1.0-prerelease.

Does that tell you what I need? If not, where can I find this information?

Comment 3

[Gregor Wagner [:gwagner]]

Seems like we have a problem in PhoneNumberJS with mcc 302. Investigating

Comment 4

[Gregor Wagner [:gwagner]]

Mcc in question is "mcc":302. I can't find it in our international country code table :(

Comment 5

[Jose Manuel Cantera]

(In reply to Gregor Wagner [:gwagner] from comment #4)
> Mcc in question is "mcc":302. I can't find it in our international country
> code table :(

that's unfortunate. It seems the library is not complete enough. At least SIM importation should not break when mcc is not available for some reason in phonenumber.js

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: patch

Stupid mistake :)
The first entry doesn't have the same country initials of course.

Comment 7

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 8

[Andreas Gal :gal]

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

Nice catch. Dump bug.

Comment 9

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d60526b97a17

Comment 10

[Gregor Wagner [:gwagner]]

(In reply to Jose M. Cantera from comment #5)
> (In reply to Gregor Wagner [:gwagner] from comment #4)
> > Mcc in question is "mcc":302. I can't find it in our international country
> > code table :(
> 
> that's unfortunate. It seems the library is not complete enough. At least
> SIM importation should not break when mcc is not available for some reason
> in phonenumber.js

It was another mistake. I was looking in the wrong table.

Comment 11

[Gregor Wagner [:gwagner]]

This most likely fixes bug 834755 and should be tef+

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d60526b97a17

Comment 14

[Gregor Wagner [:gwagner]]

Followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5e030a2763

Comment 15

[Andrew Overholt [:overholt]]

FWIW I think this should block :)

Comment 16

[Gregor Wagner [:gwagner]]

Yes this should definitely block!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b5e030a2763

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Error messages without clarifying the user impact here do not make this a blocker - can you please nominate for uplift with an explanation of the risk/reward here and we'll look at approving this week if low risk.

Comment 19

[Andrew Overholt [:overholt]]

The user impact for me is that I can't send SMSes without an internationalized phone #.  And we just drop back to an empty compose window with no feedback.  That was bug 834755 which was duped to this one.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Comment 21

[Gregor Wagner [:gwagner]]

I missed the comments here because of all the bugzilla spam we get recently. I thought this was uplifted last week and now we lost one week of testing :(

User impact is that people in random countries outside of the US can't send SMS or maybe even can't make phone calls to local numbers at all if we add this to the phoneNumber validation path as suggested by a few other blocking bugs.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

This bug is tracking-b2g18+ please go ahead with an uplift nomination and we can review the risks in taking this (as mentioned in comment 18). We've got time still to land fixes to v1.0.1 for tef so this isn't a blocker based on not being a partner request at this time.

Comment 23

[Andrew Overholt [:overholt]]

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

Approved for uplift.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/48475dda7060

FYI, I folded the two csets into one for the uplift.

Also, it's not clear to me whether this was approved to land on b2g18_v1_0_1 at this time or not, so I didn't land it there. Please set checkin-needed if it needs uplift there as well and I'll get it.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

This does need to uplift to v1.0.1

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e