Differentiate between user cancellation and actual errors #426

SUPERCILEX · 2016-11-28T23:36:36Z

SUPERCILEX · 2016-11-28T23:37:29Z

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

@@ -34,7 +34,7 @@
 import com.firebase.ui.auth.AuthUI;
 import com.firebase.ui.auth.AuthUI.IdpConfig;
 import com.firebase.ui.auth.IdpResponse;
-import com.firebase.ui.auth.ui.ResultCodes;
+import com.firebase.ui.auth.ResultCodes;


The library version should probably be bumped to 1.1.0 since this is slight breakage.

I think this would technically require a major version (if you're going by semver) since old code would break when updated.

Could we keep the .ui.ResultCodes class but have it be all @Deprecated constants that are just symlinks to the new class? Then it's fully backwards compatible.

Awesome idea!

PS: how does deprecation work? When the library hits 2.0, will we remove the deprecated stuff or are we stuck with it for life?

We can remove deprecated things after a major version change (if we want to). Or we can leave it for life. This will be the first example of deprecation in this library so there's no precedent for us, and every library does it a bit differently.

Ok, thanks for the info! (I'm pretty sure we also deprecated setProviders(String... providers))

SUPERCILEX · 2016-11-28T23:40:52Z

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

+    /**
+     * Sign in succeeded
+     **/
+    public static final int OK = Activity.RESULT_OK;


I decided to unify result codes and make ResultCodes a central place for those.

I decided to use the existing activity result codes to not break anything.

SUPERCILEX · 2016-11-28T23:42:20Z

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

+    /**
+     * Sign in failed due to lack of network connection
+     **/
+    public static final int NO_NETWORK = 10;


I also removed the RESULT_ bit since the class is named ResultCodes and it looks nicer without the extra RESULT_.

SUPERCILEX · 2016-11-28T23:46:09Z

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

+    /**
+     * An unknown error has occurred
+     **/
+    public static final int UNKNOWN_ERROR = 20;


Should we be more specific here? The errors finished with this code vary widely and we log each of them so I didn't think it was necessary. The only thing people want to know is if the user cancelled the sign in or an error occurred.

This is fine for now, it would be nice to be more expressive in the long term but I don't want to overwhelm people with 50 error codes.

That's what I was thinking: we can expand later, but for now at least we have something to work with.

I think this should now be moved to another file like ErrorCodes.java or something since it's no longer a "response code" which should only be codes returned to onActivityResult directly and not the codes contained by IdpResponse.

SUPERCILEX · 2016-11-29T00:43:00Z

I also added an isNewUser method to IdpResponse in this PR because I thought it would have something to do with ResultCodes. Turns out it didn't and I'm not quite sure why I thought that in the first place, but here it is, crammed into this PR.

SUPERCILEX · 2016-11-29T01:59:43Z

Turns out there's no way to know if an IDP is a new user so I changed the method name to isNewEmailUser. If I'm wrong, I'd love hear a solution!

samtstern · 2016-11-29T17:29:30Z

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

+    /**
+     * @return true if the user was created in the sign in flow, false if it already existed.
+     */
+    public boolean isNewEmailUser() {


I think we should delay this feature because with a little more effort (which would be out of scope here) we could do a more complete job with the "new or existing user" problem.

At the time when we acquire the users email (either directly or through an IDP response) we could do a query for providers from email and use that to determine if it's a new or existing user. It comes at the cost of an extra request but at least it would be correct.

The other option is just to recommend that developers keep an index of known users in the Realtime Database, which is what ~90% of devs are doing anyway (for various reasons). We could even provide a FBUI helper function to manage this.

In any case, we should discuss it thoroughly.

I learned something new today! I thought fetchProvidersForEmail only worked for email/password accounts.

I agree, since this is going to be much more complicated, I'll save it for another PR.

samtstern · 2016-11-29T19:41:57Z

app/src/main/java/com/firebase/uidemo/storage/ImageActivity.java

@@ -82,7 +83,7 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) {
        super.onActivityResult(requestCode, resultCode, data);

        if (requestCode == RC_CHOOSE_PHOTO) {
-            if (resultCode == RESULT_OK) {
+            if (resultCode == ResultCodes.OK) {


This seems like an IntelliJ refactor bug, in this case I am not using AuthUI I want to see if the Camera returned Activity.RESULT_OK

samtstern · 2016-11-29T19:42:46Z

auth/README.md

@@ -204,8 +204,8 @@ startActivityForResult(

 #####Response codes
 The authentication flow only provides three response codes:


This sentence is no longer true ("only three response codes..."). Maybe we should just point to the ResponseCodes file here and rely on JavaDoc comments so we don't have to keep updating this.

samtstern · 2016-11-29T19:45:58Z

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

@@ -69,7 +69,7 @@ public void onCreate(@Nullable Bundle savedInstanceState) {

        if (providerConfig == null) {
            // we don't have a provider to handle this
-            finish(Activity.RESULT_CANCELED, new Intent());
+            finish(ResultCodes.UNKNOWN_ERROR, new Intent());


Changing all of the CANCELED to UNKNOWN_ERROR is still a breaking API change, as people checking for CANCELED in code will no longer see the same behavior.

This makes me think maybe we should simplify to three cases:

OK

CANCELED (with optional extras describing the error)

NO_NETWORK (just to keep backwards compat, otherwise I'd say we lump this into CANCELED)

I like that, what about getErrorCode in IdpResponse?

That seems like the right answer 👍

samtstern · 2016-11-29T21:12:39Z

auth/README.md

-        // user is signed in!
-        startActivity(new Intent(this, WelcomeBackActivity.class));
-        finish();
+    if (data == null) {


nit: let's check if (requestCode != ResultCodes.OK && data == null) here to more clearly indicate what happened.

SUPERCILEX

I extracted ErrorCodes so ResultCodes now only has the activity codes so should we just remove that class altogether or keep it in case we want to add another result code?

SUPERCILEX · 2016-11-29T20:38:40Z

auth/README.md

+`ResultCodes.NO_NETWORK` if sign in failed due to a lack of network connectivity,
+and `ResultCodes.UNKNOWN_ERROR` for all other errors.
+Typically, the only recourse for most apps if sign in fails is to ask
+the user to sign in again later, or proceed with anonymous sign-in if supported.


Here's the updated README

SUPERCILEX · 2016-11-29T21:43:44Z

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

-            startActivity(SignedInActivity.createIntent(this, IdpResponse.fromResultIntent(data)));
-            finish();
-            return;
+        if (data != null) {


What do you think of this way of handling the sign in response?

There are a few things here:

I'd like checking for RESULT_OK to be a top-level check. That's our contract, not the nullity of data.

When the resultCode is CANCELED we will have two quick snackbars right? One because the IdpResponse contains UNKNOWN_ERROR and one because it was canceled.

samtstern · 2016-11-29T22:20:26Z

auth/README.md

-    }
+    if (requestCode == RC_SIGN_IN) {
+        if (data != null) {
+            IdpResponse response = IdpResponse.fromResultIntent(data);


How about:

if (requestCode == RC_SIGN_IN) { IdpResponse response = (data == null) ? null : IdpResponse.fromResultIntent(data); if (resultCode == ResultCodes.OK) { startActivity(SignedInActivity.createIntent(this, response)); finish(); return } else if (resultCode == ResultCodes.CANCELED) { // Canceled... } else if (response != null) { // All the error handling ... }

That's cleaner, but it doesn't work. Since we are now returning ResultCodes.CANCELED with an added error code, it's a little different. So how about this?

samtstern · 2016-11-29T22:21:45Z

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

+    /**
+     * Sign in failed due to lack of network connection
+     **/
+    public static final int NO_NETWORK = 10;


Isn't this one actually a result code and not an error? We still need to return this via onActivityResult in order to honor our previous API contract.

No, I don't think so. It should be an error code, but to keep backwards compatibility I'm returning it in SignInDelegate which the old ResultCodes points to.

Ah ok i see what you did here. So you're returning it in both places for SignInDelegate, with the recommendation being that from here on out people use the errorCode to make decisions when the result is not OK. That makes sense!

samtstern · 2016-11-29T23:35:30Z

Everything LGTM. I need to figure out what the next milestone will look like before I merge this into master, we might want a dev branch instead.

SUPERCILEX · 2016-11-30T01:02:07Z

@samtstern Sounds good to me!

SUPERCILEX · 2016-12-02T01:36:04Z

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

+            return resultIntent.getParcelableExtra(ExtraConstants.EXTRA_IDP_RESPONSE);
+        } else {
+            return null;
+        }


Since fromResultIntent is annotated as @Nullable, I think this was the original intention.

SUPERCILEX · 2016-12-02T01:37:08Z

@samtstern I added 1 commit since you last approved my PR.

# Conflicts: # auth/README.md

samtstern · 2016-12-06T17:10:19Z

Your added commit looks good to me, thanks for flagging it.

See #418 (comment)

…codes # Conflicts: # app/src/main/res/values/strings.xml # auth/src/main/java/com/firebase/ui/auth/ui/ResultCodes.java # auth/src/main/java/com/firebase/ui/auth/ui/accountlink/WelcomeBackIdpPrompt.java # auth/src/main/java/com/firebase/ui/auth/ui/email/EmailHintContainerActivity.java # auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

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

SUPERCILEX · 2016-12-06T19:34:39Z

@samtstern looks like we're good to go!

SUPERCILEX added 3 commits November 28, 2016 15:16

Refactor result codes

358e965

Move ResultCodes.java to base package

f3d9d72

Differentiate various failures from user cancel events

ae29a5c

SUPERCILEX commented Nov 28, 2016

View reviewed changes

SUPERCILEX mentioned this pull request Nov 28, 2016

Possibility to differentiate between canceled & failed sign in #405

Closed

Add isNewUser to IdpResponse.java

e39aaa1

SUPERCILEX added 2 commits November 28, 2016 16:45

Cleanup

093967d

Bug fixes and sample updates

ba4a1a4

samtstern reviewed Nov 29, 2016

View reviewed changes

SUPERCILEX added 3 commits November 29, 2016 11:28

Remove isNewEmailUser

197463e

Add backwards compatibility

da54dee

Cleanup

0837a6a

samtstern reviewed Nov 29, 2016

View reviewed changes

SUPERCILEX added 4 commits November 29, 2016 12:32

Address review comments

a2dc35e

Update README.md

a541fd8

Cleanup

761e83a

Refactor IdpResponse.java intents

7482754

SUPERCILEX changed the title ~~Differentiate between user cancellation and actual errors~~ [DONT MERGE] Differentiate between user cancellation and actual errors Nov 29, 2016

samtstern reviewed Nov 29, 2016

View reviewed changes

SUPERCILEX added 2 commits November 29, 2016 13:32

Fix README

3fee673

Fix README and extract ErrorCodes.java

19174f1

SUPERCILEX changed the title ~~[DONT MERGE] Differentiate between user cancellation and actual errors~~ Differentiate between user cancellation and actual errors Nov 29, 2016

SUPERCILEX commented Nov 29, 2016

View reviewed changes

samtstern reviewed Nov 29, 2016

View reviewed changes

SUPERCILEX added 2 commits November 29, 2016 15:16

Cleanup handling sign in response

d56058b

Comment tweaks

e36c35d

samtstern approved these changes Nov 29, 2016

View reviewed changes

Remove unnecessary null checks

b3c4892

SUPERCILEX commented Dec 2, 2016

View reviewed changes

Merge remote-tracking branch 'firebase/master' into error-codes

22dfd43

# Conflicts: # auth/README.md

samtstern mentioned this pull request Dec 6, 2016

Complete rewrite/refactor of email sign in flow #418

Merged

Merge remote-tracking branch 'firebase/master' into error-codes

d8d4f41

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

samtstern mentioned this pull request Dec 6, 2016

Add and enforce checkstyle, lint, findbugs, and pmd #420

Merged

SUPERCILEX added 2 commits December 6, 2016 11:15

Fix merge mistakes

b49e43f

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

samtstern merged commit 44a4f27 into firebase:version-1.1.0-dev Dec 6, 2016

SUPERCILEX deleted the error-codes branch December 6, 2016 19:36

		@@ -204,8 +204,8 @@ startActivityForResult(

		#####Response codes
		The authentication flow only provides three response codes:

Differentiate between user cancellation and actual errors #426

Differentiate between user cancellation and actual errors #426

Conversation

SUPERCILEX commented Nov 28, 2016 • 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

SUPERCILEX commented Nov 29, 2016

SUPERCILEX commented Nov 29, 2016

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

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 Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

SUPERCILEX Nov 29, 2016 • 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

samtstern commented Nov 29, 2016

SUPERCILEX commented Nov 30, 2016

Choose a reason for hiding this comment

SUPERCILEX commented Dec 2, 2016

samtstern commented Dec 6, 2016

SUPERCILEX commented Dec 6, 2016

SUPERCILEX commented Nov 28, 2016 •

edited

Loading

samtstern Nov 29, 2016 •

edited

Loading

SUPERCILEX Nov 29, 2016 •

edited

Loading