Prep for final refactors #1188

SUPERCILEX · 2018-03-15T23:56:55Z

@samtstern Alrighty, it's time to plow through that refactor!

This PR makes a few core changes:

Lots of view models will need a way to start intents so that's now natively supported through custom exceptions. This way, we don't have to subscribe to 3 different LiveDatas every time. I think that makes the most sense, what are your thoughts?
Thanks to the above, the auth view model now exposes a single LiveData for all subclasses to interact with.
Other random cleanup and changes I had in fork

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

samtstern · 2018-03-16T00:22:14Z

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

+    private String mMessage;
+
+    static {
+        boolean log;


Huh this is a neat but also kind of nasty trick. Did you see this somewhere else? Reflection in a static block seems like it could really slow down some startup times.

Maybe we should just add a FirebaseUI.setLogLevel method or something?

Yeah, I just noticed that some exceptions had sensitive info like emails and whatnot that we shouldn't be logging in production. I'll put this on the TODO list for later to figure out. 👍

samtstern · 2018-03-16T00:23:27Z

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

+        throw new AssertionError("No instance for you!");
+    }
+
+    public static boolean handleError(@NonNull HelperActivityBase activity, @NonNull Exception e) {


I am totally fine with this, but do you think it makes sense to just have all of our Resources return a common exception class with a code and a cause and then we could just switch on code instead of using instanceof?

I'd like to, but we already have code that sets the failure to task.getException(). I'm not sure if we'd want to wrap everything in a FirebaseUiException, but that would definitely make this stuff cleaner. Ideas?

samtstern · 2018-03-16T00:27:07Z

auth/src/main/java/com/firebase/ui/auth/viewmodel/AuthViewModelBase.java

    private CredentialsClient mCredentialsClient;
    private FirebaseAuth mAuth;
    private PhoneAuthProvider mPhoneAuth;

-    private SingleLiveEvent<PendingResolution> mPendingResolution = new SingleLiveEvent<>();
+    private MutableLiveData<Resource<T>> mOperation = new MutableLiveData<>();


Can you quickly explain how you're mitigating the risk that comes with removing the SingleLiveEvent for the errors?

You got it below, it's the setUsed thing. I'm totally open to going back to our SingleLiveEvent, but that felt like the greater of the two evils. Thoughts?

samtstern · 2018-03-16T00:31:39Z

This looks really great!

I wonder: can/should we enforce that Resource<> goes through states in a sane order? I am thinking that SUCCESS and FAILURE should be terminal and any attempt to go from one of those to the other or back to LOADING is some sort of problem.

But maybe that's not the case? Either way it would be good to know/define.

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

samtstern · 2018-03-16T00:33:53Z

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

+        if (!(e instanceof StatefulException) || ((StatefulException) e).isUsed()) {
+            return false;
+        }
+        ((StatefulException) e).setUsed(true);


Maybe the setUsed could be automatically called inside getPendingIntent() or getIntent() so that way you can really only get them once?

Yeah, I actually did that to start but it felt kinda weird. If you had that idea too, then it can't be that weird. 😆 SGTM!

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

SUPERCILEX · 2018-03-16T00:43:08Z

@samtstern For the resource stuff, FAILURE and LOADING states can be called any number of times since the client can try to get around the failures or let the user try again, etc. However, it does make sense to me that we would block anything after SUCCESS, do you want to go with that?

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

samtstern

Very few remaining comments! Looking good.

samtstern · 2018-03-16T15:31:24Z

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

+
+public class StatefulException extends FirebaseUiException {
+    public StatefulException() {
+        super(ErrorCodes.UNKNOWN_ERROR);


So do we need this class anymore if it doesn't actually hold any state?

Nope, good catch!

samtstern · 2018-03-16T15:32:28Z

auth/src/main/java/com/firebase/ui/auth/ui/credentials/CredentialSaveActivity.java

-        if (!mHandler.onActivityResult(requestCode, resultCode, data)) {
-            super.onActivityResult(requestCode, resultCode, data);
-        }
+        super.onActivityResult(requestCode, resultCode, data);


Are we sure there's no issue unconditionally calling both?

Yeah, as long as we have different request codes (we do: 100 and 101). I felt like we'd get nipped in the behind later if onActivityResult was called non-deterministically. (Especially since fragments rely on that call.)

samtstern · 2018-03-16T15:35:07Z

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

+    }
+
+    public static boolean handleError(@NonNull HelperActivityBase activity, @NonNull Exception e) {
+        if (!(e instanceof StatefulException)) { return false; }


Not sure that StatefulException is really a useful / meaningful base class (as stated in a comment above). Why not just return true inside each of the if blocks and then return false at the end otherwise?

Totally agree, been there, done that. 👍😆

samtstern · 2018-03-16T15:37:46Z

auth/src/test/java/com/firebase/ui/auth/viewmodel/SmartLockHandlerTest.java

-        ArgumentCaptor<PendingResolution> resolveCaptor = ArgumentCaptor.forClass(PendingResolution.class);
-        verify(mResolutionObserver).onChanged(resolveCaptor.capture());
+        ArgumentCaptor<Resource<Void>> resolveCaptor = ArgumentCaptor.forClass(Resource.class);
+        verify(mResultObserver, times(2)).onChanged(resolveCaptor.capture());


Should we change the test to actually verify the onChanged events separately? Kind of confusing to verify that the second time is a PendingIntentRequiredException but we don't assert/care what the first event is.

We do below but I got an exception without the times(2). Is there a prettier way to do this with mockito?

Just for learning purposes (I am fine with your code) you can tell Mockito you want to do events in order.

Here is some code from another project of mine which is basically the same (over there we call Resource a Loadable):

// Verify order of events InOrder inOrder = Mockito.inOrder(loadingObserver); // Start by loading liveData.postValue(Loadable.forLoading()); inOrder.verify(loadingObserver).onChanged(true); // Move to success liveData.postValue(Loadable.forSuccess("Hello")); inOrder.verify(loadingObserver).onChanged(false); // Make sure that double-success does not make the observer get called again liveData.postValue(Loadable.forSuccess("This should have never been posted")); inOrder.verify(loadingObserver, never()).onChanged(anyBoolean());

Whoa, that looks super cool!

Just for learning purposes

Says you, I definitely using that! 😆 Thanks for tip 👍

Want me to merge-as is or do you want to go and use InOrder? Up to you.

Sorry for the delay, my laptop died in the middle of running tests. 😭😆 Anyway, it's much prettier now!

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

samtstern · 2018-03-16T17:32:00Z

😆

Prep for final refactors

a1c7dda

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

SUPERCILEX requested a review from samtstern as a code owner March 15, 2018 23:56

SUPERCILEX added 2 commits March 15, 2018 17:01

Perf improvements

3325fda

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

Fix loose ends

fde1bca

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

samtstern reviewed Mar 16, 2018

View reviewed changes

Start addressing review feedback

6f0e2c9

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

samtstern reviewed Mar 16, 2018

View reviewed changes

Move usage updates into getters

624aa6e

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

SUPERCILEX mentioned this pull request Mar 16, 2018

Completely rewrite provider login with new architecture #1189

Merged

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

samtstern reviewed Mar 16, 2018

View reviewed changes

SUPERCILEX added 2 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]>

samtstern merged commit 39c85f0 into firebase:version-3.3.0-dev Mar 16, 2018

SUPERCILEX deleted the prep branch March 16, 2018 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prep for final refactors #1188

Prep for final refactors #1188

SUPERCILEX commented Mar 15, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern commented Mar 16, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

SUPERCILEX commented Mar 16, 2018

samtstern left a comment

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern Mar 16, 2018

SUPERCILEX Mar 16, 2018

samtstern commented Mar 16, 2018

Prep for final refactors #1188

Prep for final refactors #1188

Conversation

SUPERCILEX commented Mar 15, 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

samtstern commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

samtstern commented Mar 16, 2018