Adds continue as guest #1375

lsirac · 2018-06-26T21:54:41Z

samtstern

Mostly LGTM! Just a few comments.

samtstern · 2018-06-26T21:58:36Z

auth/src/main/java/com/firebase/ui/auth/data/remote/AnonymousSignInHandler.java

+                .build();
+    }
+
+    private void showTopProgressBar() {


I think we should either rename this to setLoading() or just inline it at all call sites. The handlers should not have UI logic, it's up to the observer to decide how to display (or ignore) progress events. Top progress bar is our current favorite pattern but it was a dialog before that and could easily change again.

samtstern · 2018-06-26T21:59:06Z

auth/src/main/java/com/firebase/ui/auth/data/remote/AnonymousSignInHandler.java

+
+    }
+
+    public IdpResponse getResponse(boolean isNewUser) {


Is this used in more than one place? Could we just inline it? Also does it need to be public?

Definitely should be private.

samtstern · 2018-06-26T22:00:38Z

auth/src/main/java/com/firebase/ui/auth/data/remote/AnonymousSignInHandler.java

+        setResult(Resource.<IdpResponse>forLoading());
+    }
+
+    private FirebaseAuth getAuth() {


I think you can get this from AuthUI right? The other handlers don't duplicate this logic afaik.

Other handlers extend from AuthViewModelBase. Added a todo for this.

samtstern · 2018-06-26T22:00:56Z

auth/src/main/java/com/firebase/ui/auth/data/remote/AnonymousSignInHandler.java

+    }
+
+    @Override
+    public void onActivityResult(int requestCode, int resultCode, @Nullable Intent data) {}


nit: move this up with the other @Override methods.

samtstern · 2018-06-26T22:02:19Z

build.gradle.kts

@@ -101,7 +101,8 @@ fun Project.configureAndroid() {
                    "IconExpectedSize",
                    "InvalidPackage", // Firestore uses GRPC which makes lint mad
                    "NewerVersionAvailable", "GradleDependency", // For reproducible builds
-                    "SelectableText", "SyntheticAccessor" // We almost never care about this
+                    "SelectableText", "SyntheticAccessor", // We almost never care about this
+                    "InvalidVectorPath" // false positive for fui_ic_anonymous_white_24dp


I think we may just want to add a single exception for this to the lint baseline rather than ignoring all InvalidVectorPath errors since they could be serious in the future.

SUPERCILEX

Looks good, mostly have some comments about the provider handler stuff.

PS: @samtstern why are we deciding to show UI? I still feel like devs should sign in anonymously on boot as opposed to letting users think they signed in and then lose all their data when they switch devices.

SUPERCILEX · 2018-06-26T23:40:13Z

auth/src/main/java/com/firebase/ui/auth/AuthUI.java

+        /**
+         * {@link IdpConfig} builder for the Anonymous provider.
+         */
+


Nit: remove line

SUPERCILEX · 2018-06-26T23:41:19Z

auth/src/main/java/com/firebase/ui/auth/AuthUI.java

+
+    public class AnonymousAuthProvider {
+        public static final String PROVIDER_ID = "anonymous";
+    }


Not quite sure I see why we need this... Can't we just use AnonymousAuthProvider.PROVIDER_ID?

Where is this AnonymousAuthProvider that you speak of? 😄

Oh lol, you named it so well I thought you were joking. 😊 Anyway... since Firebase doesn't have a built-in AnonymousAuthProvider, shouldn't the FirebaseAuthProvider essentially be the same thing? @samtstern Am I mixing it up or is FirebaseAuthProvider intended to represent anon auth?

No idea, @lsirac might know better than I do!

I don't think FirebaseAuthProvider is intended to represent anon auth. Every signed in user has a "firebase" provider.

Gotya, then I think we should stick to a field in AuthUI directly. We used to have stuff like this:

- /** - * Provider identifier for email and password credentials, for use with {@link - * SignInIntentBuilder#setAvailableProviders(List)}. - */ - public static final String EMAIL_PROVIDER = EmailAuthProvider.PROVIDER_ID;

So we could do ANONYMOUS_PROVIDER. Also, does it need to be publicly accessible? I think we could mark it with @RestrictTo.

So we'd do AuthUI.ANONYMOUS_PROVIDER instead of AuthUI.AnonymousAuthProvider.PROVIDER_ID? and yeah we can mark it with @RestrictTo

SUPERCILEX · 2018-06-26T23:50:55Z

auth/src/main/res/layout/fui_provider_button_anonymous.xml

+    android:id="@+id/anonymous_button"
+    style="@style/FirebaseUI.Button.AccountChooser.AnonymousButton"
+    android:text="@string/fui_sign_in_anonymously">
+</com.firebase.ui.auth.util.ui.SupportVectorDrawablesButton>


Nit: unnecessary tag (could just be />)

SUPERCILEX · 2018-06-26T23:52:17Z

library/quality/lint-baseline.xml

+        id="InvalidVectorPath"
+        message="Use 0.94 instead of .94 to avoid crashes on some devices">
+        <location file="src/main/res/drawable/fui_ic_anonymous_white_24dp.xml"></location>
+    </issue>


I don't think we want this to be in our baseline since we're purposefully ignoring it. The baseline is meant to be a baseline (for lack of a better word 😆) that you're trying to slowly shrink over time.

Do you know how to purposefully ignore a warning like this? Can we add something in the vector XML itself?

Yup, should be the same tools:ignore="InvalidVectorPath" we all know and love. 😆

SUPERCILEX · 2018-06-26T23:56:20Z

auth/src/main/java/com/firebase/ui/auth/data/remote/AnonymousSignInHandler.java

+                .build();
+    }
+
+    // TODO: Change ProviderSignInBase to extend from AuthViewModelBase


ProviderSignInBase classes are meant to only retrieve remote provider data, hence making it difficult to get an auth instance as you've noticed. It was also supposed to be UI focused which is why there's a bunch of activity related stuff.

I feel like we should make this class be a bare bones middle man that sends an IdpResponse to an AnonymousProviderResponseHandler with the auth stuff. @samtstern What do you think?

@SUPERCILEX in this version which class actually would call signInAnonymously?

signInAnonymously would be called in the AnonymousProviderResponseHandler. And AnonymousSignInHandler would be something as simple as:

// ... @Override public void startSignIn(@NonNull HelperActivityBase activity) { setResult(Resource.<IdpResponse>forSuccess(new IdpResponse.Builder( new User.Builder(AuthUI.AnonymousAuthProvider.PROVIDER_ID, null).build()) .build())); }

Not sure if that's actually better though since having AnonymousSignInHandler becomes kinda pointless. I'm just not a huge fan of doing auth stuff in a non-auth view model. 🤷‍♂️

@samtstern thoughts?

Maybe we compromise and only have the ResponseHandler but call it immediately, rather than having the useless ViewModel. I agree we shouldn't do the signing in in the non-auth view model if we can avoid it.

samtstern · 2018-06-27T00:13:00Z

@SUPERCILEX agree, but enough devs have asked for this (across all platforms) that we just decided to give them the option. Apparently some people like the UX of the "continue as guest" button. I am not one of those people but hey, free choice!

SUPERCILEX · 2018-06-27T00:17:54Z

@samtstern Seems weird to me, but alrighty then! 👍

samtstern · 2018-06-28T17:34:09Z

@SUPERCILEX after talking to Leo, we think the right move is to stick with the original organization and have the anon sign-in code be part of the SignInHandler rather than a ResponseHandler.

I know this breaks your class hierarchy a bit, but I don't think adding a useless class is any better. If we just pretend that the Anonymous sign in is some IDP called "Anonymous, Incorporated" it's not hard to imagine why the Firebase Auth code belongs there.

Maybe we can find a future way to bring the two ViewModel class hierarchy trees closer together in the future?

samtstern · 2018-06-29T16:25:42Z

Ok I am going to be out all of next week so I am going to merge this now as I think it LGTM. But not opposed to future discussion on the organization if we're not all on the same page.

austinhodak · 2018-10-09T16:27:21Z

Not sure if this is related to this pull. When using a dark theme for the FirebaseUI using .setTheme(), the shadow underneath the anon login is still white whereas the others are the correct dark theme.

` <style name="SignInTheme" parent="Theme.AppCompat">
@color/timelineBG
@color/timelineDark
@color/md_orange_500

    <item name="colorControlNormal">@color/md_orange_500</item>
    <item name="colorControlActivated">@color/md_orange_A700</item>
    <item name="colorControlHighlight">@color/md_orange_A200</item>
    <item name="android:windowBackground">@color/timelineBG</item>
</style>`

samtstern · 2018-10-09T16:28:29Z

@austinhodak could you file a separate bug to track that issue? Thanks!

Adds continue as guest

0701683

lsirac requested a review from samtstern as a code owner June 26, 2018 21:54

lsirac changed the title ~~Adds continue as guest functionality~~ Adds continue as guest Jun 26, 2018

samtstern mentioned this pull request Jun 26, 2018

Auth - add anonymous provider as login #900

Closed

samtstern added this to the 4.2.0 milestone Jun 26, 2018

Updated documentation

9665e12

samtstern reviewed Jun 26, 2018

View reviewed changes

Changes based on code review

be3ec5b

lsirac requested a review from SUPERCILEX June 26, 2018 23:38

samtstern approved these changes Jun 26, 2018

View reviewed changes

SUPERCILEX reviewed Jun 27, 2018

View reviewed changes

lsirac added 2 commits June 26, 2018 17:51

Changes based on Alex's suggestions

f416125

Small changes

ebe7650

samtstern approved these changes Jun 28, 2018

View reviewed changes

samtstern merged commit 2631d09 into version-4.2.0-dev Jun 29, 2018

samtstern mentioned this pull request Jun 29, 2018

[Translations] Get translations for continue as guest #1382

Closed

Adds continue as guest #1375

Adds continue as guest #1375

Conversation

lsirac commented Jun 26, 2018 • edited by samtstern Loading

samtstern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samtstern commented Jun 27, 2018

SUPERCILEX commented Jun 27, 2018

samtstern commented Jun 28, 2018

samtstern commented Jun 29, 2018

austinhodak commented Oct 9, 2018

samtstern commented Oct 9, 2018

lsirac commented Jun 26, 2018 •

edited by samtstern

Loading

SUPERCILEX left a comment •

edited

Loading