Cleanup + make sure IdpResponse is never null if resultCode == ResultCodes.OK #469

SUPERCILEX · 2016-12-22T01:43:39Z

#468

Note: this PR includes #447 to make my life easier. 😄

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

…ull if resultCode == ResultCodes.OK Signed-off-by: Alex Saveau <[email protected]>

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

samtstern · 2016-12-22T16:32:51Z

@SUPERCILEX I went ahead and merged #447 (commented there on reasoning) so you should be able to make the diff for this sane now.

branflake2267 · 2016-12-22T19:16:47Z

nice job!

…ponse # Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/ExtraConstants.java

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

samtstern · 2016-12-22T20:04:02Z

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

@@ -17,4 +13,8 @@ private ErrorCodes() {
     * An unknown error has occurred
     **/
    public static final int UNKNOWN_ERROR = 20;
+
+    private ErrorCodes() {


nit: why do we need a constructor for this class at all?

We don't, this was just formalizing the class as a "Constants" class that shouldn't be instantiated.

Got it. Had not seen that technique before but I like it.

samtstern · 2016-12-22T20:06:03Z

auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackIdpPrompt.java

-                    ExtraConstants.EXTRA_ERROR_MESSAGE,
-                    "Firebase login successful." +
-                            " Account linking failed due to provider not enabled by application");
+            Log.w(TAG, "Firebase login successful." +


nit: this error message should include which provider was not enabled.

samtstern · 2016-12-22T20:08:14Z

auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackIdpPrompt.java

+                                                TAG, "Error signing in with previous credential"))
+                                        .addOnCompleteListener(new FinishListener(idpResponse));
+                            } else {
+                                finish(ResultCodes.OK, IdpResponse.getIntent(idpResponse));


This finish() was previously for failure of the Task but not it occurs when the task succeeds but mPrevCredential == null. Is this a behavior change for developers?

I believe this was the original intention: if we have a previous credential, link it, else, finish ok. Previously, if we either didn't have a previous credential or the task failed, we would finish ok.

Because we would finish ok if we didn't have a previous credential, this isn't a behavior change.

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

SUPERCILEX · 2016-12-22T19:53:33Z

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

+            token = mIdpResponse.getIdpToken();
+            secret = mIdpResponse.getIdpSecret();
+        }
+        if (token == null) {


Only show the token/secret if we have one.

SUPERCILEX · 2016-12-22T19:54:17Z

app/src/main/res/layout/auth_ui_layout.xml


        <CheckBox
            android:id="@+id/facebook_provider"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
+            android:checked="true"


Make all providers be checked by default and use standard order: Google, Facebook, Twitter, Email.

SUPERCILEX · 2016-12-22T19:57:17Z

auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackIdpPrompt.java

 import com.google.firebase.auth.FirebaseUser;
 import com.google.firebase.auth.GoogleAuthProvider;
 import com.google.firebase.auth.TwitterAuthProvider;

-public class WelcomeBackIdpPrompt extends AppCompatBase
-        implements View.OnClickListener, IdpCallback {
+public class WelcomeBackIdpPrompt extends AppCompatBase implements IdpCallback {


This one was a mess!!! I've been struggling to understand what's going on in #309 for a while now so I decided it was time for a rewrite. If you look at the class without all the crazy diff stuff, it's much clearer what's going on now.

SUPERCILEX · 2016-12-22T19:59:03Z

auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackIdpPrompt.java

@@ -127,83 +123,71 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) {
    }

    @Override
-    public void onClick(View view) {
-        next(mPrevIdpResponse);


Ahhhhhh!!! It took me so long in #309 to "understand" what was going on only to find out I was wrong: we never even used this click listener! That kills me!

Yeah it took me a little while to see what was going on here too. Good find! This stray listener could have bitten us down the road.

SUPERCILEX · 2016-12-22T19:59:55Z

auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackIdpPrompt.java

-                    if (task.isSuccessful() && mPrevCredential != null) {
-                        FirebaseUser firebaseUser = task.getResult().getUser();
-                        firebaseUser.linkWithCredential(mPrevCredential);
-                        firebaseAuth.signOut();


SUPERCILEX · 2016-12-22T20:01:12Z

auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackIdpPrompt.java

-                                        TAG, "Error signing in with previous credential"))
-                                .addOnCompleteListener(new FinishListener(newIdpResponse));
-                    } else {
-                        finish(ResultCodes.OK, IdpResponse.getIntent(newIdpResponse));


BAD! (I think)
Here's the if: if (task.isSuccessful() && mPrevCredential != null)
What if the task fails!? We'll just finish with result ok? I'm pretty sure this was a mistake.

SUPERCILEX · 2016-12-22T20:01:34Z

auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackIdpPrompt.java

+                    .addOnFailureListener(new OnFailureListener() {
+                        @Override
+                        public void onFailure(@NonNull Exception e) {
+                            finishWithError();


On failure, we fail.

SUPERCILEX · 2016-12-22T20:03:26Z

auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackPasswordPrompt.java

                    mActivityHelper.getFlowParams(),
                    mEmail));
-            finish(ResultCodes.OK, new Intent());
+            finish(ResultCodes.CANCELED, IdpResponse.getErrorCodeIntent(ErrorCodes.UNKNOWN_ERROR));


I believe this was also a mistake, the user is not signed in so we shouldn't finish with OK.

SUPERCILEX · 2016-12-22T20:06:17Z

auth/src/main/java/com/firebase/ui/auth/ui/idp/CredentialSignInHandler.java

@@ -94,12 +95,6 @@ public void onFailure(@NonNull Exception e) {
    }

    private class StartWelcomeBackFlow implements OnSuccessListener<ProviderQueryResult> {


This class isn't static so I'm not sure why we passed in the email. Seems like extra boilerplate to me.

SUPERCILEX · 2016-12-22T20:07:31Z

auth/src/main/java/com/firebase/ui/auth/util/signincontainer/SaveSmartLock.java

        if (!mHelper.getFlowParams().smartLockEnabled
-                || !PlayServicesHelper.getInstance(getContext()).isPlayServicesAvailable()
-                || getActivity().isFinishing()) {


This was pre setRetainInstance(true), my bad.

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

samtstern · 2016-12-22T22:19:26Z

This LGTM! Gonna squash and merge.

SUPERCILEX added 3 commits December 21, 2016 16:51

Make IdpResponse#fromResultIntent(Intent) never be null (#468)

e00950d

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

Hmmm, forgot pressing back would make it null. At least it won't be n…

3e9fb89

…ull if resultCode == ResultCodes.OK Signed-off-by: Alex Saveau <[email protected]>

Bug fixes

5d2c0dc

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

SUPERCILEX mentioned this pull request Dec 22, 2016

IdpResponse.fromResultIntent(data) return null #468

Closed

SUPERCILEX changed the base branch from master to version-1.1.0-dev December 22, 2016 01:44

SUPERCILEX added 2 commits December 21, 2016 18:12

Fix broken tests

9f6605c

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

Fix some smart lock issues

bb49369

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

SUPERCILEX added 3 commits December 22, 2016 11:36

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into IdpRes…

b825b02

…ponse # Conflicts: # auth/src/main/java/com/firebase/ui/auth/ui/ExtraConstants.java

Cleanup

5d4bde8

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

Fix mistake

076e3c0

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

samtstern reviewed Dec 22, 2016

View reviewed changes

Cleanup

9541c6c

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

SUPERCILEX commented Dec 22, 2016

View reviewed changes

Address comments

04d9e36

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

samtstern merged commit 81dbbca into firebase:version-1.1.0-dev Dec 22, 2016

SUPERCILEX deleted the IdpResponse branch December 22, 2016 22:29

		@@ -94,12 +95,6 @@ public void onFailure(@NonNull Exception e) {
		}

		private class StartWelcomeBackFlow implements OnSuccessListener<ProviderQueryResult> {

Cleanup + make sure IdpResponse is never null if resultCode == ResultCodes.OK #469

Cleanup + make sure IdpResponse is never null if resultCode == ResultCodes.OK #469

Uh oh!

Conversation

SUPERCILEX commented Dec 22, 2016

Uh oh!

samtstern commented Dec 22, 2016

Uh oh!

branflake2267 commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Dec 22, 2016

Uh oh!

Uh oh!