Country selector fixes #1345
Country selector fixes #1345
Conversation
There was a problem hiding this comment.
Overall looks good, though I do have a few concerns:
- There's too much logic in the fragment—I tried very hard to dumb it down in the phone auth refactor and I'd be sad to see it get gnarly again. Ideally some of your code could be moved to the phone utils class or another helper.
- Too many methods depend on each other being called in a very specific order. For example, the country code setup method somehow initializes the whitelist fields and also shows the Smart Lock dialog making it so if anything order wise is changed, it all blows up at runtime. That's scary for anyone touching those files. 😉
|
@SUPERCILEX I moved the logic away from the fragment but I decided against creating a manager/helper. We're not going to reuse the country spinner so I moved the logic there 😃. Also, based on the params needed & the changes I made I don't think it's easy to make a mistake editing the files now. Let me know if you disagree. |
| @@ -678,6 +678,12 @@ public PhoneBuilder setWhitelistedCountries( | |||
| "You can either whitelist or blacklist country codes for phone " + | |||
| "authentication."); | |||
| } | |||
|
|
|||
| String message = "Invalid argument: The whitelist is %s. For this case, " + | |||
There was a problem hiding this comment.
Instead of: "For this case, don't use this method" how about something like "Only non-null and non-empty whitelists are valid. To specify no whitelist, do not call this method."
| }); | ||
| } | ||
|
|
||
| private void setDefaultCountryForSpinner() { |
There was a problem hiding this comment.
This method needs to move back to where it was unfortunately (in onActivityCreated). Try clicking the phone number in your PR when you're at the submit code fragment. It will go back, but it will also show the phone picker again assuming you didn't pre-fill it in the AuthUI builder.
To see why that happens, read the comment where mCalled is used.
| } | ||
|
|
||
| private void getCountrySpinnerIsosFromParams(Bundle params) { | ||
| if (params != null) { |
There was a problem hiding this comment.
Let's:
- rename this method
init...since it returns void - remove the null check
- add nullability annotations.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: unnecessary new line
Fixed a merge issue where another PR broke the selector (we need more tests.. 😃)
Added some validation for when the default country is set in the selector via the two overloaded setDefaultNumber() methods.
When the developer does not provide a default country, we get the user’s locale based on their SIM. If the resulting country is not whitelisted or has been blacklisted, we now choose the first valid country, where the valid countries are sorted in alphabetical order. Before this, an invalid country was displayed in the picker.
When we successfully fetch the users' number, we'll input it, set the country code, and simulate a 'Verify Phone Number' button click. We now confirm that their country code is valid. If it isn't, we will still auto fill their number, but we don't continue to verify their number.
I also removed the CountryListLoadTask.