Completely rewrite provider login with new architecture #1189

SUPERCILEX · 2018-03-16T01:28:54Z

Built on top of #1188

Fixes #1200, fixes #1201

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2018-03-16T04:35:19Z

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

@@ -0,0 +1,153 @@
+package com.firebase.ui.auth.data.remote;


TODO(me): add tests

I think just for review sanity you should do the tests in another PR. This one is already 1k+ lines and tests will probably double it.

SGTM for the individual provider handlers since they work AFAIK, but I've actually found a bunch of issues with my ProvidersHandler implementation thanks to some lovely TDD (so I'll just add that one for now).

SUPERCILEX · 2018-03-16T04:55:02Z

@samtstern This is going to be the biggest PR by far since it requires major refactors across a large portion of the FUI API surface. The basic ideas are as follows:

Providers are now middlemen to talk between the UI layer and their "handlers".
Each handler is in charge of processing whatever work needs to be done in the background or otherwise to generate an IdpResponse which is then reported back to the provider middleman who then reports it back to the activity scoped provider handler who's then in charge of signing the user in and reporting that result back to the activity. Whew! It's quite a long chain, but it makes sure everything is cleanly separated and no memory leaks can occur.
IdpSignInContainer is dead! 👏 It's been replaced with a dead-simple activity inspired from the credentials one you wrote.

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2018-03-16T17:45:58Z

@SUPERCILEX well this is up next, after merges are resolved. Right?

# Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java # auth/src/main/java/com/firebase/ui/auth/ui/idp/WelcomeBackIdpPrompt.java # auth/src/main/java/com/firebase/ui/auth/util/signincontainer/IdpSignInContainer.java # auth/src/main/java/com/firebase/ui/auth/util/signincontainer/SignInDelegate.java # auth/src/main/java/com/firebase/ui/auth/viewmodel/AuthViewModelBase.java # auth/src/main/java/com/firebase/ui/auth/viewmodel/smartlock/SmartLockHandler.java

SUPERCILEX · 2018-03-16T17:51:10Z

@samtstern Yep, I'm working on tests right now.

Signed-off-by: Alex Saveau <[email protected]>

samtstern

I started reviewing but then i got pulled away into something else. Sending the comments I had so far, will probably pick this up on Monday.

samtstern · 2018-03-16T17:57:36Z

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

@@ -276,7 +276,7 @@ public static int getDefaultTheme() {
                .disableAutoSignIn()
                .continueWithTask(new Continuation<Void, Task<Void>>() {
                    @Override
-                    public Task<Void> then(@NonNull Task<Void> task) throws Exception {
+                    public Task<Void> then(@NonNull Task<Void> task) {


Did some linter tell you not to have throws Exception here? Does it actually change anything to remove it?

Yeah, IntelliJ has been removing these, not sure why, but it doesn't seem to matter.

samtstern · 2018-03-16T17:58:13Z

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

+import com.firebase.ui.auth.viewmodel.idp.ProviderHandler;
+import com.firebase.ui.auth.viewmodel.idp.ProvidersHandler;
+
+public final class FacebookParams extends ProviderHandler.ParamsBase {


@RestrictTo? Also class comment.

Yeah, I'm actually hoping to get rid of these. They're in a separate class because the compiler won't let me make them a static inner...

It seems extracting the base class fixes it.

samtstern · 2018-03-16T17:59:39Z

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

+import com.firebase.ui.auth.viewmodel.idp.ProviderHandler;
+import com.firebase.ui.auth.viewmodel.idp.ProvidersHandler;
+
+public final class GoogleParams extends ProviderHandler.ParamsBase {


Same comment here and on all other new classes:

Double-check visibility restrictions.

Add class comment.

samtstern · 2018-03-16T18:02:24Z

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

+                GoogleSignIn.getClient(getApplication(), getSignInOptions()).getSignInIntent()));
+    }
+
+    public LiveData<Resource<Intent>> getRequest() {


Any reason why this is a Resource? It only ever seems to be in the SUCCESS state (or null).

I'm using it for the isUsed() check, do you think we should extract it into a seperate, contained class? The main pain-point is that I'm trying to keep the previous behavior which included starting the sign-in process from onActivityResult if it's an invalid account.

Should this be a Resource<Void> with the Intent being part of an error like in other cases then?

Yeah, why not? It seems a little better than what I have.

samtstern · 2018-03-16T18:04:24Z

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

+                    .getResult(ApiException.class);
+            setResult(createIdpResponse(account));
+        } catch (ApiException e) {
+            if (e.getStatusCode() == CommonStatusCodes.INVALID_ACCOUNT) {


This took me a minute to understand, so maybe add:

// If we get INVALID_ACCOUNT it means the pre-set account was not available on the device, set email to null // and try to launch sign-in again.

samtstern · 2018-03-16T18:08:39Z

auth/src/main/java/com/firebase/ui/auth/provider/EmailProvider.java

-import com.firebase.ui.auth.data.model.FlowParameters;
-import com.firebase.ui.auth.ui.email.EmailActivity;
-
-public class EmailProvider implements Provider {


We are deleting a formerly public class. Should this have always been annotated with @RestrictTo or this is a real breaking change?

Same comment for all providers.

This was from before we used the restrict annotations, but it's always been considered private API AFAIK:

FirebaseUI-Android/auth/src/main/java/com/firebase/ui/auth/provider/package-info.java

Lines 15 to 18 in a381e13

/**

* IDP-specific interactions for signing in users. The contents of this package should be considered

* an implementation detail and not part of the main API.

*/

Also, we just broke people so these should really be marked as restricted.

SGTM let's just mark them all as restricted now

Signed-off-by: Alex Saveau <[email protected]>

…e provider Signed-off-by: Alex Saveau <[email protected]>

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2018-03-17T01:04:59Z

@samtstern Alrighty! Hope you had a great weekend, this PR is officially no longer a WIP. 👏 I've tidied a bunch of stuff up, finished implementing everything that needed to get done, and thoroughly tested everything until all those 🐛 were 💥 into oblivion.

The next PR is only a few hundred lines of code so once we get passed this, we'll be on a roll! 🥂 (My poor attempt at a motivational speech and apology for making this PR so big. 😆)

SUPERCILEX · 2018-03-22T01:40:10Z

Thanks for the great feedback, I don't mind at all! 😊 I'll address your flow questions here. Things should hopefully seem much clearer now:

ProviderBase holds the context related stuff plus a LiveData with an IdpResponse so people can subscribe to "provider responses."
ProviderResponseHandler takes in an IdpResponse and converts it to a Firebase Auth sign-in, then simply flushes the response through (though that can certainly change if need be in the future).
ProviderHandlerBase is in charge of getting a response for a specific provider implementation from their backend.

I'd recommend looking at these classes from a package perspective:

ui -> ProviderBase
viewmodel -> ProviderResponseHandler
remote -> ProviderHandlerBase

Once everything clicks, I think you'll appreciate the clean separation we have rockin! 😁

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2018-03-22T00:11:36Z

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

@@ -357,22 +359,12 @@ public static int getDefaultTheme() {
    }

    private Task<Void> signOutIdps(@NonNull Context context) {
-        try {
+        if (FacebookSignInHandler.IS_AVAILABLE) {


This is much less scary

samtstern · 2018-03-22T14:53:02Z

auth/src/main/java/com/firebase/ui/auth/data/model/Resource.java

@@ -64,20 +80,16 @@ public State getState() {

    @Nullable
    public final Exception getException() {
-        mUsed = true;
+        if (mUsable) { mState = State.USED; }


This is sort of interesting. Previously, a Resource was immutable. You'd listen to a stream and get a new resource every time the state changed. Now the state can change from the inside.

How does this affect the pattern of listening to LiveData<Resource> and checking state? Is it a problem that Observers won't get a new callback when this happens?

Yeah, technically, the "resource" part is still immutable since the value and exception can't change. Also, the class is final so only we control the state's mutability, and for now, it can only go from SUCCESS/FAILURE -> USED so I don't think it'll be a problem unless we want to listen for USED events? I'm not sure when that would ever be applicable... I think of the state as more for helping us define our conditionals rather than defining the resource—it should be focused on the data, not the state. That's my opinion though, any digressions?

Seems OK for now ... just something we may have to change in the future.

samtstern · 2018-03-22T15:02:03Z

auth/src/main/java/com/firebase/ui/auth/ui/provider/ProviderBase.java


 @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
-public abstract class ProviderBase implements Provider {
-    private final ProviderResponseHandlerBase mHandler;
+public abstract class ProviderBase {


Your github comments explaining the class hierarchy were very helpful! Let's also add class-level JavaDoc comments to these new base/handler classes to explain it to future me when I inevitably confuse myself again.

samtstern · 2018-03-22T15:04:28Z

auth/src/main/java/com/firebase/ui/auth/viewmodel/idp/LinkingProviderResponseHandler.java

-                            setResult(Resource.<IdpResponse>forFailure(e));
+                        public Task<Void> then(@NonNull Task<AuthResult> task) {
+                            if (mRequestedSignInCredential == null) {
+                                return Tasks.forResult(null);


Can you add a comment here explaining why in this case we don't care at all if the signInWithCredential operation was successful or not?

Nope, that was a mistake, thanks for catching it!

samtstern · 2018-03-22T15:07:19Z

auth/src/main/java/com/firebase/ui/auth/viewmodel/idp/LinkingProviderResponseHandler.java

+                    .addOnCompleteListener(new OnCompleteListener<Void>() {
+                        @Override
+                        public void onComplete(@NonNull Task<Void> task) {
+                            if (task.isSuccessful()) {


Seems like the way you have this set up the task is guaranteed to be successful (in the if branch you go straight to success and in the else branch you drop errors). Is that your intention?

I think this class would be improved with a healthy dose of comments since I am a little lost in what you're trying to achieve and why some error cases are ignored.

See above, my B.

samtstern · 2018-03-22T15:09:45Z

auth/src/main/res/values-zh-rTW/strings.xml

@@ -47,7 +47,6 @@
    <string name="fui_error_unknown">發生不明錯誤。</string>
    <string name="fui_error_invalid_password">密碼不正確。</string>
    <string name="fui_accessibility_logo">應用程式標誌</string>
-    <string name="fui_signed_in_with_specific_account">以 %1$s 的身分登入</string>


Thanks for reverting the tools declaration, but can we just revert all changes to the strings files? Is there some lint about an unused resource you're trying to get around?

I would much rather ignore the lint for now than have all of these strings changes cluttering up this very important PR.

Oh gotya, I'll tell lint to keep calm and let the resources be unused. 😆

samtstern · 2018-03-22T15:15:50Z

auth/src/main/java/com/firebase/ui/auth/viewmodel/idp/ProviderResponseHandlerBase.java

+        return false;
+    }
+
+    protected abstract void signIn(


Seems duplicative to have both this method and startSignIn? Couldn't this method just check the state and set the loading state in the base class and be annotated with @CallSuper?

Yeah, I've looked at this again and stuff has changed so whatever I was doing is now uncessessary. I'm going to simplify it and remove the abstractions.

samtstern · 2018-03-22T15:16:17Z

auth/src/main/java/com/firebase/ui/auth/ui/provider/ProviderBase.java

+import com.firebase.ui.auth.ui.HelperActivityBase;
+
+@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
+public abstract class ProviderBase {


Since this class is only abstract methods ... could it be an interface instead?

Good catch, I didn't notice!

samtstern · 2018-03-22T15:40:16Z

auth/src/main/java/com/firebase/ui/auth/ui/provider/FacebookProvider.java

+    }
+
+    @Override
+    public void startLogin(@NonNull HelperActivityBase activity) {


nit, but for consistency this should probably be called startSignIn since we use the term 'sign in' not 'log in' pretty much everywhere.

I find it a bit strange that this class talks directly to LoginManager. Shouldn't all of the Facebook SDK code be contained within the FacebookSignInHandler and this class merely acts as a pass-through?

Yup, fixed.

For the second part, I'm not doing that so we don't send the activity over to the view model... do you think we should?

samtstern · 2018-03-22T16:14:38Z

Ok I have spent an hour staring at things and I think we can beat a little more sense into the class hierarchy still. Here are the bits that still seem somewhat off to me.

ProviderBase

This class does a few somewhat unrelated things. The first thing it does is serve as a holder for a StringRes and a LayoutRes. These are constants per-provider. We could move these out into a separate class called ProviderView and store them in a simple static map somewhere from String --> ProviderView so you could just do map.get(PROVIDER_ID).getNameRes() and be done with it.
The second thing this class does is hold a ProviderHandlerBase, at least for IDPs. The fact that the class holds one of these is not evident from reading the ProviderBase class definition (small problem in itself) but in practice, that's what it does. It seems that the startLogin, onActivityResult and getResponseListener methods are little more than dumb proxies for the same methods in the ProviderHandlerBase.
The two classes that act differently here are EmailProvider and PhoneProvider, since rather than talking to a Handler they kick off some other activity and wait for the response. So maybe this is the justification for the whole abstraction?

ProviderHandlerBase

This is only used for social providers (Google, Facebook, etc) whereas Phone and Email are handled locally. However this is not clear from the class hierarchy because ProviderBase does not make any distinction and the class names don't either.
Could the phone and email logic be pushed into a ProviderHandlerBase? Or would it not fit that model?

ProviderResponseHandlerBase

This is basically an AuthViewModel with an extra sign in method.
Has only two subclasses, SimpleProviderResponseHandler and LinkingProviderResponseHandler.
There's only one place where we use the base class as either an argument or a return type (in `AuthMethodPickerActivity.populateIdpList)
I'm wondering if we can just remove the base layer of abstraction here at the expense of an extra if block in the auth method picker?

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2018-03-22T22:58:39Z

Thanks for the really thoughtful feedback!

ProviderBase
TBH, this was just around because it was there before. I've looked at where some of the methods are being used and just inlined stuff. I've also moved the sign-in start process to the handler with a HUGE warning not to use the activity outside of that method.

ProviderHandlerBase has been squishified into ProviderBase.

ProviderResponseHandlerBase Yup, I did that in an earlier commit.

Ok, what do you think of https://github.com/SUPERCILEX/FirebaseUI-Android/commit/e9470309dbfb68caf0b45216afed3f13d6abe17b? I think we've finally reached critical mass and there's nothing left to remove. 😆

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2018-03-22T23:32:08Z

@SUPERCILEX wooo! I have to head out a bit early today but on first pass 👍

I think this is really clear now, and also more lines deleted than added! Not bad for a refactor that makes everything work better.

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2018-03-22T23:50:41Z

Alrighty, SGTM! Thanks!

samtstern

L.
G.
T.
M.

💥 🎉 🌮 💸

samtstern · 2018-03-23T04:20:17Z

@SUPERCILEX thanks for all the back and forth, all of this code is really clear now (or as clear as the tangled auth mess that is the AuthUI flow will ever be!).

I am going to merge this PR now, if you had any TODOs we can add them to your next one.

Congrats on making this happen, get yourself a beer (or two, or orange juice, or whatever, do you).

SUPERCILEX · 2018-03-23T04:38:42Z

YASSSSSSSS! Stellar use of emojis BTW. 👌😆 You enjoy a beverage too, all of that reviewing must turn your brain into mush! 😳😊

SUPERCILEX added 5 commits March 15, 2018 16:48

Prep for final refactors

a1c7dda

Signed-off-by: Alex Saveau <[email protected]>

Perf improvements

3325fda

Signed-off-by: Alex Saveau <[email protected]>

Fix loose ends

fde1bca

Signed-off-by: Alex Saveau <[email protected]>

Start addressing review feedback

6f0e2c9

Signed-off-by: Alex Saveau <[email protected]>

Move usage updates into getters

624aa6e

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX requested a review from samtstern as a code owner March 16, 2018 01:28

SUPERCILEX commented Mar 16, 2018

View reviewed changes

Merge improvements from future PRs

3acea2d

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX mentioned this pull request Mar 16, 2018

Kill SignInDelegate ☠️ #1190

Merged

SUPERCILEX added 7 commits March 16, 2018 10:06

Address review feedback

dd9ae69

Signed-off-by: Alex Saveau <[email protected]>

Fancisize tests

887ae0d

Signed-off-by: Alex Saveau <[email protected]>

Completely rewrite provider login with new architecture

fdb9f94

Signed-off-by: Alex Saveau <[email protected]>

Cleanup and pre-testing bug fixes

5511513

Signed-off-by: Alex Saveau <[email protected]>

Make lint happy

1be0b8b

Signed-off-by: Alex Saveau <[email protected]>

Fix tests

0a65776

Signed-off-by: Alex Saveau <[email protected]>

It actually works!

f0dc341

Signed-off-by: Alex Saveau <[email protected]>

Thank you TDD!

7736254

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Mar 16, 2018

View reviewed changes

SUPERCILEX added 6 commits March 16, 2018 12:30

Start addressing review feedback

4c2eec6

Signed-off-by: Alex Saveau <[email protected]>

Restore previous linking behavior

705b9b9

Signed-off-by: Alex Saveau <[email protected]>

Fix bugs found in real-world testing plus cleanup

e17c101

Signed-off-by: Alex Saveau <[email protected]>

Add hack for googlesamples/google-services#345

27ff4d3

Signed-off-by: Alex Saveau <[email protected]>

Fix WelcomeBackIdpPrompt recursively attempting sign-ins with the sam…

8732c11

…e provider Signed-off-by: Alex Saveau <[email protected]>

Tidy loose ends

95b024c

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX changed the title ~~[WIP] Completely rewrite provider login with new architecture~~ Completely rewrite provider login with new architecture Mar 17, 2018

SUPERCILEX added 2 commits March 21, 2018 18:44

Forgot to make all exceptions be usable

748cbc5

Signed-off-by: Alex Saveau <[email protected]>

Minor cleanup

2fad171

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Mar 22, 2018

View reviewed changes

SUPERCILEX added 5 commits March 22, 2018 13:05

Address comments

e679f80

Signed-off-by: Alex Saveau <[email protected]>

Add back strings and suppress lint

6fec29c

More simplification

f4ec251

Signed-off-by: Alex Saveau <[email protected]>

Squishify the provider classes together

e947030

Signed-off-by: Alex Saveau <[email protected]>

Fix welcome back crash if response failed

36a41f2

Signed-off-by: Alex Saveau <[email protected]>

Consistency

722f9c6

Signed-off-by: Alex Saveau <[email protected]>

Fix pointless zygote logging

d111daa

Signed-off-by: Alex Saveau <[email protected]>

samtstern approved these changes Mar 23, 2018

View reviewed changes

samtstern merged commit 2ef0595 into firebase:version-3.3.0-dev Mar 23, 2018

samtstern added this to the 3.3.0 milestone Mar 23, 2018

This was referenced Mar 23, 2018

NoClassDefFoundErrors logcat spam for optional providers #1200

Closed

Fix NoClassDefFoundErrors logspam for missing providers #1201

Closed

Account Linking Phase 1 #1185

Closed

UI Flashes when singning in with one provider (phone number) #948

Closed

SUPERCILEX deleted the providers branch March 23, 2018 04:35

SUPERCILEX mentioned this pull request Mar 23, 2018

Remove unused strings #1202

Merged

	/**
	* IDP-specific interactions for signing in users. The contents of this package should be considered
	* an implementation detail and not part of the main API.
	*/

Completely rewrite provider login with new architecture #1189

Completely rewrite provider login with new architecture #1189

Conversation

SUPERCILEX commented Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented Mar 16, 2018

samtstern commented Mar 16, 2018

SUPERCILEX commented Mar 16, 2018

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

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 Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented Mar 17, 2018

SUPERCILEX commented Mar 22, 2018

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 Mar 22, 2018

SUPERCILEX commented Mar 22, 2018

samtstern commented Mar 22, 2018

SUPERCILEX commented Mar 22, 2018

samtstern left a comment

Choose a reason for hiding this comment

samtstern commented Mar 23, 2018

SUPERCILEX commented Mar 23, 2018

SUPERCILEX commented Mar 16, 2018 •

edited

Loading

SUPERCILEX Mar 16, 2018 •

edited

Loading