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

Validate phone numbers #3134

Closed
wants to merge 1 commit into from
Closed

Conversation

ghubstan
Copy link
Contributor

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

After successful validation, inputs are transformed into E.164
format and cached in a field, accessible via a getter method.
However, area and region codes are not checked for correctness.
End users are responsible for correctness of area/region codes.

Four i18n properties (validation.phone.*) were added to
displayStrings.properties

This is a partial fix for Issue #3042. The validator
will be integrated into the GUI if PR is approved & patch is
merged.

Question for Bisq devs:
Another new pkg protected class, CountryCallingCodes, contains
an immutable map of ISO country codes -> country calling codes;
it is the only non-validator class in its package. The map
declaration is too large to live in the PhoneNumberValidator class,
even after tweaking idea.properties in an attempt to prevent
the IDE from freezing. Is it OK to leave CountryCallingCodes
where it is, or is there a more appropriate home for it?

Copy link
Contributor

@battleofwizards battleofwizards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big plus for the test cases and nice meaningful names!

@ghubstan ghubstan force-pushed the validate-phone branch 2 times, most recently from e452fe8 to 2c7a948 Compare August 24, 2019 15:08
@ghubstan ghubstan force-pushed the validate-phone branch 2 times, most recently from 5573b9f to 1ea6cb8 Compare August 26, 2019 15:07
Created new PhoneNumberValidator + Test.  Validator hides
no arg constructor; public constructor requires two letter
ISO country code for validating inputs.

After successful validation, inputs are transformed into E.164
format and cached in a field, accessible via a getter method.
However, area and region codes are not checked for correctness.
End users are responsible for correctness of area/region codes.

Four i18n properties (validation.phone.*) were added to
displayStrings.properties

This is a partial fix for Issue bisq-network#3042.  The validator
will be integrated into the GUI if PR is approved & patch is
merged.

Question for Bisq devs:
Another new pkg protected class, CountryCallingCodes, contains
an immutable map of ISO country codes -> country calling codes;
it is the only non-validator class in its package.  The map
declaration is too large to live in the PhoneNumberValidator class,
even after tweaking idea.properties in an attempt to prevent
the IDE from freezing.  Is it OK to leave CountryCallingCodes
where it is, or is there a more appropriate home for it?
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NACK - based on my comments. Please update your PR with the required changes and I'll give it a spin 👍

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