Email link sign in (Phase 1) #1486

lsirac · 2018-10-15T18:47:20Z

Most of email link sign in (phase 1, meaning the flow needs to be completed on the same device)

samtstern · 2018-10-15T21:09:13Z

@lsirac let me know when you want code review to start (even if it's just on some parts)

auth/src/main/java/com/firebase/ui/auth/ui/email/EmailLinkCatcherActivity.java

lsirac · 2018-10-15T22:02:10Z

Things to look for:

All UI stuff (seriously, I never really do any UI stuff, so there may be bugs)
EmailLinkSignInHandler - here's where everything happens (signing in, linking, anonymous upgrade)
EmailActivity - added some new fragments to this, just something to look at when I'm registering them
When catching the link, we open a lot of activities e.g. AuthUI->KickOff->Catcher, etc, I don't see a way around that
TroubleSigningInFragment - I made sure that we don't resend the emails on rotations, but maybe I missed something

…ady been used)

samtstern

First pass. Mostly small comments, whatever formatter you used is very strange and throwing me off / creating a lot of noise.

I did not look at layouts or tests yet.

app/src/main/java/com/firebase/uidemo/ChooserActivity.java

app/src/main/java/com/firebase/uidemo/auth/AnonymousUpgradeActivity.java

app/src/main/java/com/firebase/uidemo/auth/AuthUiActivity.java

app/src/main/java/com/firebase/uidemo/auth/SignedInActivity.java

auth/src/main/java/com/firebase/ui/auth/viewmodel/email/EmailLinkSignInHandler.java

samtstern · 2018-10-17T20:18:49Z

auth/src/main/java/com/firebase/ui/auth/viewmodel/email/EmailLinkSignInHandler.java

+                                    Exception {
+                                persistenceManager.clearAllData(getApplication());
+                                if (!task.isSuccessful()) {
+                                    return Tasks.forResult(task.getResult(Exception.class));


If the task is not successful then calling getResult() will throw right? Shouldn't this be Tasks.forException(task.getException())? Maybe I am missing something,

Yeah, but can't we just return the original task here anyway?

auth/src/main/java/com/firebase/ui/auth/viewmodel/email/EmailLinkSignInHandler.java

auth/src/main/java/com/firebase/ui/auth/viewmodel/email/EmailProviderResponseHandler.java

samtstern · 2018-10-18T16:43:39Z

Some notes from playing around with this PR within the sample app:

The button says "sign in with email" but shouldn't it say "sign in with email link" or something to distinguish it?
When I tried to use email link sign in without the provider enabled in my FIrebase project, I saw the EmailLinkFragment very briefly flicker into existence which left me confused. I wonder if we're not waiting for the email sending to finish before showing that?
Once I did enable the provider but did no other config, I ran into "UNAUTHORIZED_DOMAIN:Domain not whitelisted by project" however that was only higher up in the logs. The error that bubbled up was com.firebase.ui.auth.data.model.UserCancellationException: Unknown error but I believe we should be sending back some sort of "developer error" message, not user cancellation.
This is more of a product issue, but I was surprised to get com.google.firebase.auth.FirebaseAuthException: Please activate Dynamic Links in the Firebase Console and agree to the terms and conditions. [ FDL domain is not configured ]. We should probably do that for the developer!

In the end the flow does work (woot!) but there was a big flickering activity stack. I think you mentioned that in one of your self-review comments and I'll dig in and see if we can try to make that look nicer.

samtstern · 2018-10-18T16:46:52Z

I was able to get a crash by doing the following:

Sign in with email link
Go back try to sign in with the same email, but with email+password

2018-10-18 09:46:08.085 25713-25713/com.firebase.uidemo E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.firebase.uidemo, PID: 25713
    java.lang.IllegalStateException: User has no providers even though we got a collision.
        at com.firebase.ui.auth.viewmodel.email.EmailProviderResponseHandler$StartWelcomeBackFlow.onSuccess(EmailProviderResponseHandler.java:101)
        at com.firebase.ui.auth.viewmodel.email.EmailProviderResponseHandler$StartWelcomeBackFlow.onSuccess(EmailProviderResponseHandler.java:91)

Not sure if that's expected? I assume the other way around should work (password first, then link the next time) right?

Edit it does seem to work if I start with a password and then use a link.

samtstern · 2018-10-18T16:52:01Z

Some UI feedback on the "trouble signing in" flow:

The "trouble signing in" link doesn't seem to show any sort of pressed/clicked state when I tap it, which seems janky. It's also styled differently than the ToS/PP links (no underline, different color) which makes me think it's not clickable. Maybe we should make it a borderless button?
There's no transition to the "Trouble getting email?" screen, which felt strange.
Hitting the back button at the "Trouble getting email" screen takes me back to the NASCAR screen rather than back to the "email sent" screen, which seems too harsh.

auth/src/main/AndroidManifest.xml

app/src/main/java/com/firebase/uidemo/ChooserActivity.java

samtstern

Some more very small comments. I think we're getting close to merge, any TODOs on your side?

auth/src/main/java/com/firebase/ui/auth/ui/email/EmailLinkCatcherActivity.java

samtstern · 2018-10-19T19:23:18Z

auth/src/main/java/com/firebase/ui/auth/ui/email/EmailLinkFragment.java

    public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) {
        mProgressBar = view.findViewById(R.id.top_progress_bar);
-        mListener = (TroubleSigningInListener) getActivity();
+        getView().setVisibility(View.GONE);


So what goes it look like at this point? Blank screen?

auth/src/main/java/com/firebase/ui/auth/util/data/EmailLinkParser.java

auth/src/main/java/com/firebase/ui/auth/util/ui/TextHelper.java

samtstern · 2018-10-19T20:29:13Z

@lsirac by the way:

com.firebase.ui.auth.viewmodel.EmailLinkEmailHandlerTest > testSendSignInLinkToEmail_expectSuccess FAILED
    org.mockito.exceptions.verification.TooManyActualInvocations at EmailLinkEmailHandlerTest.java:77
com.firebase.ui.auth.viewmodel.EmailLinkEmailHandlerTest > testSendSignInLinkToEmail_expectFailure FAILED
    org.mockito.exceptions.verification.TooManyActualInvocations at EmailLinkEmailHandlerTest.java:98

…kFragment

samtstern

New progress bar looks great, 🚢 it!

samtstern · 2018-10-23T17:49:58Z

(review assuming tests pass)

lsirac · 2018-10-23T17:58:22Z

Some notes from playing around with this PR within the sample app:

The button says "sign in with email" but shouldn't it say "sign in with email link" or something to distinguish it?

When I tried to use email link sign in without the provider enabled in my FIrebase project, I saw the EmailLinkFragment very briefly flicker into existence which left me confused. I wonder if we're not waiting for the email sending to finish before showing that?

Once I did enable the provider but did no other config, I ran into "UNAUTHORIZED_DOMAIN:Domain not whitelisted by project" however that was only higher up in the logs. The error that bubbled up was com.firebase.ui.auth.data.model.UserCancellationException: Unknown error but I believe we should be sending back some sort of "developer error" message, not user cancellation.

This is more of a product issue, but I was surprised to get com.google.firebase.auth.FirebaseAuthException: Please activate Dynamic Links in the Firebase Console and agree to the terms and conditions. [ FDL domain is not configured ]. We should probably do that for the developer!

In the end the flow does work (woot!) but there was a big flickering activity stack. I think you mentioned that in one of your self-review comments and I'll dig in and see if we can try to make that look nicer.

Thanks Sam!

To quickly respond:

From the DD, looks like we're keeping the same button
This should be fixed now
This should also be fixed

…is not supported

lsirac · 2018-10-24T00:12:08Z

Done. 👍

lsirac · 2018-10-24T01:19:50Z

I will add the documentation in a separate PR

Most of the implementation and tests for email link sign in phase 1

45e1e21

lsirac requested a review from samtstern as a code owner October 15, 2018 18:47

lsirac commented Oct 15, 2018

View reviewed changes

auth/src/main/java/com/firebase/ui/auth/ui/email/EmailLinkCatcherActivity.java Show resolved Hide resolved

lsirac added 4 commits October 15, 2018 15:19

Fix auth:checkstyle error

aba1fcf

Added loading animation

e6e2fb3

Handle the invalid link flow (link is malformed, expired, or has alre…

c4c43e6

…ady been used)

Fix broken tests

fe06a27

samtstern reviewed Oct 17, 2018

View reviewed changes

samtstern reviewed Oct 18, 2018

View reviewed changes

auth/src/main/AndroidManifest.xml Show resolved Hide resolved

samtstern reviewed Oct 18, 2018

View reviewed changes

app/src/main/java/com/firebase/uidemo/ChooserActivity.java Show resolved Hide resolved

Changes from code review

b4d45dc

samtstern reviewed Oct 19, 2018

View reviewed changes

lsirac added 3 commits October 19, 2018 14:01

More changes from code review

6641ba5

Adds circular loading progress bar while sending an email in EmailLin…

ff4a0f2

…kFragment

Removed redundant declarations and added annotations

63fcb62

lsirac force-pushed the emaillink branch from abfd657 to 63fcb62 Compare October 23, 2018 17:41

samtstern approved these changes Oct 23, 2018

View reviewed changes

samtstern changed the title ~~[WIP] Email link sign in~~ Email link sign in Oct 23, 2018

Fixes InvisibleFragment

65f4bd1

lsirac closed this Oct 23, 2018

lsirac reopened this Oct 23, 2018

Add transition from EmailLinkFragment to TroubleSigningInFragment

0292dbf

lsirac added 2 commits October 23, 2018 15:45

Show an error message when trying to sign in with email link when it …

e37f42f

…is not supported

Fix anonymous upgrade demo

d5027c4

lsirac changed the title ~~Email link sign in~~ Email link sign in (Phase 1) Oct 24, 2018

samtstern added this to the 4.3.0 milestone Oct 24, 2018

samtstern merged commit 150f1fa into version-4.3.0-dev Oct 24, 2018

samtstern mentioned this pull request Oct 24, 2018

Add translations and documentation for email link sign in (Phase 1) #1501

Closed

Email link sign in (Phase 1) #1486

Email link sign in (Phase 1) #1486

Uh oh!

Conversation

lsirac commented Oct 15, 2018

Uh oh!

samtstern commented Oct 15, 2018

Uh oh!

Uh oh!

lsirac commented Oct 15, 2018

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samtstern Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

lsirac Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samtstern commented Oct 18, 2018

Uh oh!

samtstern commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samtstern commented Oct 18, 2018

Uh oh!

Uh oh!

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samtstern Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

samtstern commented Oct 19, 2018

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

samtstern commented Oct 23, 2018

Uh oh!

lsirac commented Oct 23, 2018

Uh oh!

lsirac commented Oct 24, 2018

Uh oh!

lsirac commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

samtstern commented Oct 18, 2018 •

edited

Loading