Validate phone numbers #3149
Validate phone numbers #3149
Conversation
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?
|
@ghubstan Tests are running for me and the code looks good 👍 . Could you use it in the payment method forms so there is a clear benefit for the user for merging this PR. |
|
@ripcurlx I had incorrectly assumed this PR would be merged after approval, then GUI integration changes would go in another PR. (Why did I think that? I thought breaking the work into smaller chunks is what Bisq wants, and it is how I like to work with git.) If that's not the case, will I need to squash the new GUI side commits to this branch into one, or leave them as is, two commits in this PR? (Two commits: the one you just tested, and the commit containing the GUI integration to be added. ) Oops, fat finger closed the PR; skinny finger re-opened it. |
|
Keeping the PRs small is great, but every commit should provide direct benefit to the developer or user. I'd just add the GUI integration as a new commit to the same PR and it should be fine 👍 |
|
Hi, I have some questions about integrating the PhoneNumberValidator into the GUI. I'm starting with the Swish (Sweden) payment method. If I use the new PhoneNumberValidator in place of the SwishValidator (which just does validateIfNotEmpty(phone)), SwishValidator becomes dead code. The same is true for HalCash's (Spain & Poland) HalCashValidator, and possibly a few other payment methods' validators as well. Q1: Should I tag Validators that will 'die' in this PR as @deprecated? Q2: Should I remove unused Validators from the project, and tweak constructor signatures, etc., that include them? Another issue about the HalCash payment method (& others, I think), is the need for an iso country code to validate and normalize the phone numbers. For Swish, "SE", the country is known. For HalCash, it's ES or PL, and the user will need to input the country in the payment method form. The same will be true for relatively more country agnostic payment methods. Q3: In the country agnostic payment methods, should I include a country selector in the payment method forms that Q4: Should those new country selectors include only countries that are valid for a particular payment method? Questions 3 and 4 are based on the assumption the user's default locale != payment-method's country. For example, I'm in Brazil, but my payment method is US specific. I'm sure this is the case for many users in Europe too. The payment method's country will need to be specified for phone validation & normalization. Just trying to measure twice and cut "not much more than once" in this patch. I can't do that without advice from Bisq's veterans. Thanks. |
|
@ripcurlx When you have time, can you look at my questions above about phone # validator integration into the GUI? |
Maybe just extend the PhoneNumberValidator in e.g. SwishValidator so there is less code change to be reviewed and if in the future some specific checks have to be added it is quicker to do so
If we want to use the validator for those payment methods it will be necessary to add the country selectors. I think it would be fine to move Q3 and Q4 into a new PR as it will take more testing of this code changes than in the cases where just the PhoneNumberValidation is added. |
|
@ripcurlx The phone validator has been integrated into SwishForm. |
desktop/src/main/java/bisq/desktop/components/paymentmethods/SwishForm.java
Outdated
Show resolved
Hide resolved
PhoneNumberValidator + Test Changes: Added no-arg constuctor required for Guice injection. Added isoCountryCode setter; this field must be set before validation. Added validation of isoCountryCode. Added missing country code test. SwishValidator Changes: SwishValidator now extends PhoneValidator. Added no-arg constuctor required for Guice injection. Set isoCountryCode = SE in constructor. SwishForm Changes: Sets Swish acct phone number to normalized phone number if phone # validation is successful. Replaced Logger declaration with @slf4j annotation. Formatted source. Added 'validation.phone.missingCountryCode' property to i18n file.
180670d to
de4b590
Compare
|
@ghubstan I don't have time to test it right now, but I'll review it before the code freeze. 👍 |
|
OK, no rush. I know it's Friday night there. Thanks. |
|
@ripcurlx After you've tested this PR, and after (if) you merge it, I have HalCash phone validation ready for a new branch & PR. (FYI, HalCash required a couple of changes to pb.proto. So may others.) What do you think of grouping phone validation PRs into similar payment methods? For example, the next PR will be "Validate HalCash phone number", then a few to several (depending on similarity of changes needed) phone-validator integrations can be grouped into the next PR, and so on. Each PR would benefit users of each affected payment method, and testers wouldn't need to test everything all at once. |
Changing stuff in the proto file must be done very carefully, as this could cause backwards compatibility issues. But I'll have a look at it in your upcoming PR.
Sounds reasonable. |
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?
Replaces closed PR #3134, which contains entire history of changes.