-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Rewrite email logic with new architecture #1214
Conversation
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
// Email address is malformed | ||
mEmailInput.setError(getString(R.string.fui_invalid_email_address)); | ||
} else if (e instanceof FirebaseAuthUserCollisionException) { | ||
// Collision with existing user email, it should be very hard for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we changing our assumption from "very hard to get here" to "impossible to get here"? Fine with me, just confirming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nevermind, I see the StartWelcomeBackFlow
in the ViewModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, moved that logic there.
} | ||
|
||
private void validateAndSignIn(final String email, final String password) { | ||
private void validateAndSignIn(String password) { | ||
// Check for null or empty password | ||
if (TextUtils.isEmpty(password)) { | ||
mPasswordLayout.setError(getString(R.string.fui_required_field)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return
if the password is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do, it looks like the diff hides it 😉
// Collision with existing user email, it should be very hard for | ||
// the user to even get to this error due to CheckEmailFragment. | ||
ProviderUtils.fetchTopProvider(getAuth(), email) | ||
.addOnSuccessListener(new StartWelcomeBackFlow(email)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about when this call fails too? Feels like we could get a case where the Resource
never gets to a final state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, yeah.
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Built on top of #1213