Remove unnecessary (?) SignInActivity #443
Conversation
…factor # Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
# Conflicts: # auth/src/main/java/com/firebase/ui/auth/provider/FacebookProvider.java
…-unnecessary-sign-in-activity # Conflicts: # auth/src/main/AndroidManifest.xml # auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java # auth/src/main/java/com/firebase/ui/auth/ui/email/SignInActivity.java # auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java # auth/src/main/res/values/strings.xml # auth/src/test/java/com/firebase/ui/auth/ui/email/WelcomeBackPasswordPromptTest.java # auth/src/test/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivityTest.java
Signed-off-by: Alex Saveau <[email protected]>
…-unnecessary-sign-in-activity # Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/ActivityHelper.java # auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackPasswordPrompt.java # auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java # auth/src/main/java/com/firebase/ui/auth/ui/email/SignInActivity.java # auth/src/test/java/com/firebase/ui/auth/ui/email/RegisterEmailActivityTest.java
Signed-off-by: Alex Saveau <[email protected]>
…-leak # Conflicts: # auth/src/main/java/com/firebase/ui/auth/provider/FacebookProvider.java
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
|
I will take a deeper look at this tomorrow but I agree with your initial assessment, those two Activities seem to do basically the same job and SignInActivity can be removed. After this goes in I am going to refactor the email flow a little further, I played with it a lot today and I found some cases where it seems to be jumping around. But let's get through this change first, just a warning that if you do decide to do any more email-flow changes after this one give me a heads up since we'll almost certainly conflict. |
Signed-off-by: Alex Saveau <[email protected]>
There was a problem hiding this comment.
@samtstern nope, I'm not planning on changing anything after this so you should be good to go!
| AuthCredential authCredential = | ||
| AuthCredentialHelper.getAuthCredential(mIdpResponse); | ||
| authResult.getUser().linkWithCredential(authCredential); | ||
| firebaseAuth.signOut(); |
There was a problem hiding this comment.
@samtstern Do you know why we were doing this? It seems unnecessary and just using linkWithCredential works fine.
There was a problem hiding this comment.
Looks like it was added in this commit
The only reason I can think to do this is that known issue about FirebaseUser changes not taking effect until signout/signin, but I believe that's only for explicit profile information change requests.
One thing I am seeing now is that the linkWithCredential call is done as fire-and-forget, which will almost certainly create some strange race conditions.
Seems like the original author should have been chaining the link-->signout-->sign in operations together.
There was a problem hiding this comment.
Humm, so should I revert and use a continuation or leave it at linkWithCredential?
There was a problem hiding this comment.
I think we leave it with your new implementation. I can't see a reason for the sign out thing.
| <string name="trouble_signing_in">Trouble signing in?</string> | ||
| <string name="sign_in">Sign in</string> | ||
| <string name="sign_in_default">Sign in</string> |
There was a problem hiding this comment.
This was a nasty one! WelcomeBackPasswordPrompt kept using the word "Start" everywhere which was extremely confusing. Turns out our sample was accidentally overriding this string resource. Since I think this could be a common occurrence, I've changed the name to sign_in_default.
There was a problem hiding this comment.
Ah I just noticed this too, thanks for sorting that out.
Signed-off-by: Alex Saveau <[email protected]>
…-unnecessary-sign-in-activity # Conflicts: # auth/src/main/java/com/firebase/ui/auth/provider/TwitterProvider.java
|
@samtstern merged! |
| .withIntent(startIntent) | ||
| .create() | ||
| .visible() | ||
| .get(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSignInButton_validatesFields() { |
There was a problem hiding this comment.
Shouldn't WelcomeBackPasswordPrompt validate fields now that we're removing SignInActivity?
There was a problem hiding this comment.
@samtstern Great catch!!! Error handling was completely broken: the loading dialog was started before we checked for errors and never dismissed if there was an error so the user ended up being stuck with a loading dialog. Also, we were setting the error on the editText instead of the TextInputLayout. Whew!
Signed-off-by: Alex Saveau <[email protected]>
|
This whole change LGTM, merging into 1.1.0-dev |
Hi @samtstern, this is the last PR in my "simplify email auth" series.
I'm confused about
SignInActivity: why is it around?WelcomeBackPasswordPromptbasically does the same thing asSignInActivitybut better. It doesn't let the user edit their email mid sign-in and gives them a friendlyWelcome back!message. Was there a reason for keepingSignInActivityaround?