Closed Bug 1352324 Opened 9 years ago Closed 8 years ago

Store phone numbers in E.164 format

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Version:
52 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: MattN, Assigned: lchang)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [form autofill:M3])

Attachments

(6 files)

Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1352324 - (Part 6) Store tel in E.164 format and calculate its components while reading.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1352324 - (Part 2) Update the metadata for PhoneNumber utility to v8.4.1.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1352324 - (Part 5) Add unit tests for PhoneNumber.jsm and PhoneNumberNormalizer.jsm.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1352324 - (Part 4) Fix eslint errors of PhoneNumber utility.
59 bytes, text/x-review-board-request
MattN
: review+
Details
There is discussion about using E.164 format for phone numbers in the web payments specs so we should consider using that for storage if it aligns with competitors.
Priority: -- → P3
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Whiteboard: [form autofill] → [form autofill:M3]
Hi Matt, I retrieved the phonenumberutils, which was removed in bug 1310863, to our codebase and updated the library a bit to fulfill our need. Unit tests are not yet ready but it would be nice if you can take a look in advance and see if I'm on the right track. Thanks.
Depends on: 1368008
Comment on attachment 8871615 [details] Bug 1352324 - (Part 6) Store tel in E.164 format and calculate its components while reading. https://reviewboard.mozilla.org/r/143094/#review150738 ::: browser/extensions/formautofill/ProfileStorage.jsm:438 (Diff revision 3) > + // Set "US" as the default region as we only support "en-US" for now. > + let tel = PhoneNumber.Parse(profile.tel, profile.country || "US"); You could fallback to the region of `Services.prefs.getCharPref("browser.search.countryCode")` instead though I really don't understand why it was put under the "search" namespace when it's generally useful… ::: browser/extensions/formautofill/ProfileStorage.jsm:441 (Diff revision 3) > + if (tel.countryName) { > + profile["tel-country-name"] = tel.countryName; > + } I would rather we just not expose this until we need it so it doesn't cause confusion. I also think the API was poorly named since it doesn't actually return the country name, only the country ISO code from what I can tell. ::: browser/extensions/formautofill/ProfileStorage.jsm:453 (Diff revision 3) > + // to figure out how to parse numbers from other regions when we support > + // new countries in the future. > + if (tel.nationalNumber && tel.countryName == "US") { You should be able to do this as long as the country code is 1: "The country calling code for all countries participating in the NANP is 1. In international format, an NANP number should be listed as +13015550100" https://en.wikipedia.org/wiki/North_American_Numbering_Plan#Modern_plan
Attachment #8871615 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8871613 [details] Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5. https://reviewboard.mozilla.org/r/143090/#review150748 ::: commit-message-f81bc:1 (Diff revision 1) > +Bug 1352324 - (Part 1) Retrieve PhoneNumberUtils from ad87ed1bbed5, r=MattN Nit: "Retrieve PhoneNumber utility JSMs from ad87ed1bbed5"
Attachment #8871613 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8871613 [details] Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5. https://reviewboard.mozilla.org/r/143090/#review150752 ::: browser/extensions/formautofill/PhoneNumber.jsm:1 (Diff revision 1) > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ We should bring the tests back too…
Comment on attachment 8871613 [details] Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5. https://reviewboard.mozilla.org/r/143090/#review150756 ::: browser/extensions/formautofill/PhoneNumber.jsm:2 (Diff revision 1) > +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */ > + > +// Don't modify this code. Please use: > +// https://github.com/andreasgal/PhoneNumber.js > + > +"use strict"; > + > +this.EXPORTED_SYMBOLS = ["PhoneNumber"]; These JSMs are also missing license headers. Github says it's Apache 2.0 so it's not just a matter of adding the MPL 2.0 header I think?
Comment on attachment 8871614 [details] Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill. https://reviewboard.mozilla.org/r/143092/#review150758 ::: browser/extensions/formautofill/PhoneNumber.jsm:4 (Diff revision 1) > // Don't modify this code. Please use: > // https://github.com/andreasgal/PhoneNumber.js What is our plan with respect to maintaing this library? Are we planning to submit pull requests upstream for your non-autofill-specific changes?
Comment on attachment 8871614 [details] Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill. https://reviewboard.mozilla.org/r/143092/#review150758 > What is our plan with respect to maintaing this library? Are we planning to submit pull requests upstream for your non-autofill-specific changes? As we discussed, we'll maintain this library by our own in Form Autofill codebase. I'll update this comment.
Comment on attachment 8871615 [details] Bug 1352324 - (Part 6) Store tel in E.164 format and calculate its components while reading. https://reviewboard.mozilla.org/r/143094/#review150738 > I would rather we just not expose this until we need it so it doesn't cause confusion. I also think the API was poorly named since it doesn't actually return the country name, only the country ISO code from what I can tell. Yeah, it's a bit confusing that the PhoneNumber library use `countryName` to stand for the country code. Agree with you that we don't need it currently so I'll remove it. Thanks. > You should be able to do this as long as the country code is 1: > > "The country calling code for all countries participating in the NANP is 1. In international format, an NANP number should be listed as +13015550100" https://en.wikipedia.org/wiki/North_American_Numbering_Plan#Modern_plan Thanks for pointing it out.
Comment on attachment 8871613 [details] Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5. https://reviewboard.mozilla.org/r/143090/#review150752 > We should bring the tests back too… The old tests are not suit our test framework so I based the tests from the original repo and created our own tests in a separated commit (part 4).
Comment on attachment 8871615 [details] Bug 1352324 - (Part 6) Store tel in E.164 format and calculate its components while reading. Hi Matt, I added some unit tests to this commit. Although you've already reviewed it, would you mind taking a look again?
Comment on attachment 8871613 [details] Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5. Hi Gervase, In this patch, we are going to bring back some PhoneNumber libraries, which existed in the central but was later removed in bug 1310863, and maintain them in the near future. We noticed that these files lack the licensing header. It was in our codebase so I suppose they're under MPL v2.0. However, these files were originally imported from GitHub [1], where they are declared Apache License v2.0. Could you please help us to figure out which license we can use? Thanks a lot. [1] https://github.com/andreasgal/PhoneNumber.js
Flags: needinfo?(gerv)
We should use the Apache License 2.0, and (given that upstream is unlikely to be maintaining the code) add license headers ourselves. Gerv
Flags: needinfo?(gerv)
Gervase, I see. Thanks.
Comment on attachment 8877028 [details] Bug 1352324 - (Part 2) Update the metadata for PhoneNumber utility to v8.4.1. https://reviewboard.mozilla.org/r/148360/#review157594
Attachment #8877028 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8871614 [details] Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill. https://reviewboard.mozilla.org/r/143092/#review157596 ::: .eslintignore:82 (Diff revision 6) > +# imported from 3rd-party library > +browser/extensions/formautofill/PhoneNumber.jsm > +browser/extensions/formautofill/PhoneNumberNormalizer.jsm > +browser/extensions/formautofill/PhoneNumberMetaData.jsm Perhaps we should put them in their own directory to make it more clear they were a separate project ::: .eslintignore:82 (Diff revision 6) > # generated or library files in activity-stream > browser/extensions/activity-stream/data/content/activity-stream.bundle.js > browser/extensions/activity-stream/vendor/** > # imported from chromium > browser/extensions/mortar/** > +# imported from 3rd-party library If we're going to maintain them now then why not update them to current eslint style in a separate commit?
Comment on attachment 8871614 [details] Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill. https://reviewboard.mozilla.org/r/143092/#review157596 > Perhaps we should put them in their own directory to make it more clear they were a separate project Do you mean put them in `browser/extensions/formautofill/PhoneNumberUtils/`? > If we're going to maintain them now then why not update them to current eslint style in a separate commit? Makes sense. I'll fix them in a separate commit.
Comment on attachment 8871614 [details] Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill. https://reviewboard.mozilla.org/r/143092/#review157596 > Do you mean put them in `browser/extensions/formautofill/PhoneNumberUtils/`? Yes, though maybe lowercase since I don't think we often use uppercase in paths > Makes sense. I'll fix them in a separate commit. Thanks
Comment on attachment 8871614 [details] Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill. https://reviewboard.mozilla.org/r/143092/#review157596 > Yes, though maybe lowercase since I don't think we often use uppercase in paths Updated. Thanks. > Thanks Fixed in the next commit. I left PhoneNumberMetaData.jsm in .eslintignore as it is generated by script.
Comment on attachment 8871614 [details] Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill. https://reviewboard.mozilla.org/r/143092/#review162316
Attachment #8871614 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8882026 [details] Bug 1352324 - (Part 4) Fix eslint errors of PhoneNumber utility. https://reviewboard.mozilla.org/r/153098/#review162318 ::: .eslintignore:82 (Diff revision 1) > +# imported from 3rd-party library > +browser/extensions/formautofill/phonenumberutils/PhoneNumberMetaData.jsm Nit: Maybe "Generated data files" would be clearer
Attachment #8882026 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877091 [details] Bug 1352324 - (Part 5) Add unit tests for PhoneNumber.jsm and PhoneNumberNormalizer.jsm. https://reviewboard.mozilla.org/r/148422/#review162322 Thanks!
Attachment #8877091 - Flags: review?(MattN+bmo) → review+
Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b27e73bea124 (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5. r=MattN https://hg.mozilla.org/integration/autoland/rev/5ebb632fa7bb (Part 2) Update the metadata for PhoneNumber utility to v8.4.1. r=MattN https://hg.mozilla.org/integration/autoland/rev/e27d693ddcc4 (Part 3) Update PhoneNumber utility to fulfill Form Autofill. r=MattN https://hg.mozilla.org/integration/autoland/rev/27971d8110df (Part 4) Fix eslint errors of PhoneNumber utility. r=MattN https://hg.mozilla.org/integration/autoland/rev/b69772478582 (Part 5) Add unit tests for PhoneNumber.jsm and PhoneNumberNormalizer.jsm. r=MattN https://hg.mozilla.org/integration/autoland/rev/7aec8351b1db (Part 6) Store tel in E.164 format and calculate its components while reading. r=MattN
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: