Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize phone numbers #3102

Closed

Conversation

ghubstan
Copy link
Contributor

Reluctantly added new dependency: 343 kb Google libphonenumber.
See https://github.com/google/libphonenumber

Created new PhoneNumberValidator + Test. Validator hides
no arg constructor; public constructor requires two letter
ISO country code for validating national numbers.

If input passes validation, it is transformed into E.164 format
and cached in a class field, accessible via getter method.
See https://en.wikipedia.org/wiki/E.164

Five new i18n error message properties (validation.phone.*) were
added to the default and Portuguese display string property files.
A native Portuguese speaker should review/change the PT strings.

This is partial fix for Issue #3042 -- validator will be integrated
into the GUI after PR(s) approved / merged.

Reluctantly added new dependency: 343 kb Google libphonenumber.
See https://github.com/google/libphonenumber

Created new PhoneNumberValidator + Test.  Validator hides
no arg constructor; public constructor requires two letter
ISO country code for validating national numbers.

If input passes validation, it is transformed into E.164 format
and cached in a class field, accessible via getter method.
See https://en.wikipedia.org/wiki/E.164

Five new i18n error message properties (validation.phone.*) were
added to the default and Portuguese display string property files.
A native Portuguese speaker should review/change the PT strings.

This is partial fix for Issue bisq-network#3042 -- validator will be integrated
into the GUI after PR(s) approved / merged.
@battleofwizards
Copy link
Contributor

battleofwizards commented Aug 16, 2019

Thanks, looks promising! Also, very nice you provided the tests.

Short on time / quick and dirty comments:

  • have you reviewed dependencies of libphonenumber, if any?
  • have you checked the library is self-contained and never calls home?
  • can we migrate away from static fields to instance fields? if not, is it verified libphonenumber is fully thread safe?
  • validate() is a huge method, would be nice to name its fragments by extracting private methods
  • the tests beg for helpers encapsulating the intent, like assertInvalid or whatever is idiomatic for this test framework (refuteValid, assertNotValid?)
  • you cannot directly edit translations except English. See https://github.com/bisq-network/bisq/blob/master/docs/translation-process.md for details

@ManfredKarrer
Copy link
Contributor

I am not sure if the use case here is worth the risk of adding a new dependency (even if its a google library). We basically want to go the opposite way to reduce dependencies as far as possible.

@ghubstan
Copy link
Contributor Author

Closing PR. Will redo w/out another dependency.

@ghubstan ghubstan closed this Aug 17, 2019
@ghubstan ghubstan deleted the normalize-phone-numbers branch August 17, 2019 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants