-
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 11 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 |
---|---|---|
|
@@ -203,38 +203,41 @@ 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(); | ||
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. nit: let's check |
||
// User pressed back button and is not signed in | ||
showSnackbar(R.string.sign_in_cancelled); | ||
return; | ||
} | ||
|
||
// Sign in canceled | ||
if (resultCode == RESULT_CANCELED) { | ||
showSnackbar(R.string.sign_in_cancelled); | ||
IdpResponse response = IdpResponse.fromResultIntent(data); | ||
|
||
if (resultCode == ResultCodes.OK) { | ||
startActivity(SignedInActivity.createIntent(this, response)); | ||
finish(); | ||
return; | ||
} | ||
|
||
// User is not signed in. Maybe just wait for the user to press | ||
// "sign in" again, or show a message. | ||
|
||
// No network | ||
if (resultCode == ResultCodes.RESULT_NO_NETWORK) { | ||
if (response.getErrorCode() == ResultCodes.NO_NETWORK) { | ||
showSnackbar(R.string.no_internet_connection); | ||
return; | ||
} | ||
|
||
// User is not signed in. Maybe just wait for the user to press | ||
// "sign in" again, or show a message. | ||
if (response.getErrorCode() == ResultCodes.UNKNOWN_ERROR) { | ||
showSnackbar(R.string.unknown_error); | ||
} | ||
} | ||
``` | ||
|
||
|
@@ -250,7 +253,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,33 @@ | ||
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; | ||
|
||
/** | ||
* 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. I also removed the |
||
|
||
/** | ||
* An unknown error has occurred | ||
**/ | ||
public static final int UNKNOWN_ERROR = 20; | ||
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. 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. 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. 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. 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 what I was thinking: we can expand later, but for now at least we have something to work with. 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 think this should now be moved to another file like |
||
} |
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)
)