-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Differentiate between user cancellation and actual errors #426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 15 commits
358e965
f3d9d72
ae29a5c
e39aaa1
093967d
ba4a1a4
197463e
da54dee
0837a6a
a2dc35e
a541fd8
761e83a
7482754
3fee673
19174f1
d56058b
e36c35d
b3c4892
22dfd43
d8d4f41
c497160
b49e43f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,8 +33,9 @@ | |
|
||
import com.firebase.ui.auth.AuthUI; | ||
import com.firebase.ui.auth.AuthUI.IdpConfig; | ||
import com.firebase.ui.auth.ErrorCodes; | ||
import com.firebase.ui.auth.IdpResponse; | ||
import com.firebase.ui.auth.ui.ResultCodes; | ||
import com.firebase.ui.auth.ResultCodes; | ||
import com.firebase.uidemo.R; | ||
import com.google.android.gms.common.Scopes; | ||
import com.google.firebase.auth.FirebaseAuth; | ||
|
@@ -199,22 +200,33 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) { | |
|
||
@MainThread | ||
private void handleSignInResponse(int resultCode, Intent data) { | ||
if (resultCode == RESULT_OK) { | ||
startActivity(SignedInActivity.createIntent(this, IdpResponse.fromResultIntent(data))); | ||
finish(); | ||
return; | ||
if (data != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of this way of handling the sign in response? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a few things here:
|
||
IdpResponse response = IdpResponse.fromResultIntent(data); | ||
|
||
if (resultCode == ResultCodes.OK) { | ||
startActivity(SignedInActivity.createIntent(this, response)); | ||
finish(); | ||
return; | ||
} | ||
|
||
// Sign in canceled | ||
if (response.getErrorCode() == ErrorCodes.NO_NETWORK) { | ||
showSnackbar(R.string.no_internet_connection); | ||
return; | ||
} | ||
|
||
if (response.getErrorCode() == ErrorCodes.UNKNOWN_ERROR) { | ||
showSnackbar(R.string.unknown_error); | ||
return; | ||
} | ||
} | ||
|
||
if (resultCode == RESULT_CANCELED) { | ||
if (resultCode == ResultCodes.CANCELED) { | ||
// User pressed back button and is not signed in | ||
showSnackbar(R.string.sign_in_cancelled); | ||
return; | ||
} | ||
|
||
if (resultCode == ResultCodes.RESULT_NO_NETWORK) { | ||
showSnackbar(R.string.no_internet_connection); | ||
return; | ||
} | ||
|
||
showSnackbar(R.string.unknown_sign_in_response); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -203,38 +203,46 @@ startActivityForResult( | |
#### Handling the sign-in response | ||
|
||
#####Response codes | ||
The authentication flow only provides three response codes: | ||
`Activity.RESULT_OK` if a user is signed in, `Activity.RESULT_CANCELLED` if | ||
sign in failed, and `ResultCodes.RESULT_NO_NETWORK` if sign in failed due to a lack of network connectivity. | ||
No further information on failure is provided as it is not | ||
typically useful; 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. | ||
The authentication flow provides several response codes of which the most common are as follows: | ||
`ResultCodes.OK` if a user is signed in, `ResultCodes.CANCELLED` if the user manually canceled the sign in, | ||
`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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's the updated README |
||
|
||
```java | ||
protected void onActivityResult(int requestCode, int resultCode, Intent data) { | ||
super.onActivityResult(requestCode, resultCode, data); | ||
if (resultCode == RESULT_OK) { | ||
// user is signed in! | ||
startActivity(new Intent(this, WelcomeBackActivity.class)); | ||
finish(); | ||
return; | ||
} | ||
if (requestCode == RC_SIGN_IN) { | ||
if (data != null) { | ||
IdpResponse response = IdpResponse.fromResultIntent(data); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ...
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's cleaner, but it doesn't work. Since we are now returning |
||
|
||
// Sign in canceled | ||
if (resultCode == RESULT_CANCELED) { | ||
showSnackbar(R.string.sign_in_cancelled); | ||
return; | ||
} | ||
if (resultCode == ResultCodes.OK) { | ||
startActivity(SignedInActivity.createIntent(this, response)); | ||
finish(); | ||
return; | ||
} | ||
|
||
// No network | ||
if (resultCode == ResultCodes.RESULT_NO_NETWORK) { | ||
showSnackbar(R.string.no_internet_connection); | ||
return; | ||
} | ||
// Sign in canceled | ||
if (response.getErrorCode() == ErrorCodes.NO_NETWORK) { | ||
showSnackbar(R.string.no_internet_connection); | ||
return; | ||
} | ||
|
||
if (response.getErrorCode() == ErrorCodes.UNKNOWN_ERROR) { | ||
showSnackbar(R.string.unknown_error); | ||
return; | ||
} | ||
} | ||
|
||
// User is not signed in. Maybe just wait for the user to press | ||
// "sign in" again, or show a message. | ||
if (resultCode == ResultCodes.CANCELED) { | ||
// User pressed back button and is not signed in | ||
showSnackbar(R.string.sign_in_cancelled); | ||
return; | ||
} | ||
|
||
showSnackbar(R.string.unknown_sign_in_response); | ||
} | ||
} | ||
``` | ||
|
||
|
@@ -250,7 +258,7 @@ Intent. | |
```java | ||
protected void onActivityResult(int requestCode, int resultCode, Intent data) { | ||
super.onActivityResult(requestCode, resultCode, data); | ||
if (resultCode == RESULT_OK) { | ||
if (resultCode == ResultCodes.OK) { | ||
IdpResponse idpResponse = IdpResponse.fromResultIntent(data); | ||
startActivity(new Intent(this, WelcomeBackActivity.class) | ||
.putExtra("my_token", idpResponse.getIdpToken())); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package com.firebase.ui.auth; | ||
|
||
/** | ||
* Error codes retrieved from {@link IdpResponse#getErrorCode()}. | ||
*/ | ||
public final class ErrorCodes { | ||
private ErrorCodes() { | ||
// We don't want people to initialize this class | ||
} | ||
|
||
/** | ||
* Sign in failed due to lack of network connection | ||
**/ | ||
public static final int NO_NETWORK = 10; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I don't think so. It should be an error code, but to keep backwards compatibility I'm returning it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
|
||
/** | ||
* An unknown error has occurred | ||
**/ | ||
public static final int UNKNOWN_ERROR = 20; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package com.firebase.ui.auth; | ||
|
||
import android.app.Activity; | ||
|
||
/** | ||
* Result codes returned when using {@link AuthUI.SignInIntentBuilder#build()} with | ||
* {@code startActivityForResult}. | ||
*/ | ||
public final class ResultCodes { | ||
private ResultCodes() { | ||
// We don't want people to initialize this class | ||
} | ||
|
||
/** | ||
* Sign in succeeded | ||
**/ | ||
public static final int OK = Activity.RESULT_OK; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to unify result codes and make I decided to use the existing activity result codes to not break anything. |
||
|
||
/** | ||
* Sign in canceled by user | ||
**/ | ||
public static final int CANCELED = Activity.RESULT_CANCELED; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library version should probably be bumped to
1.1.0
since this is slight breakage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the info! (I'm pretty sure we also deprecated
setProviders(String... providers)
)