-
Notifications
You must be signed in to change notification settings - Fork 82
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
Update LockCallback and AuthenticationCallback [SDK-2480] #621
Conversation
/** | ||
* Called when the user goes back and closes the activity, without using an Authentication flow. | ||
*/ | ||
public abstract void onCanceled(); |
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.
AuthenticationCallback
is a subclass of LockCallback
. The onCanceled
is now merged into onError
for those developers passing an AuthenticationCallback
implementation (what we document and recommend).
case LockEvent.SIGN_UP: | ||
break; | ||
if (data.hasExtra(Constants.EXCEPTION_EXTRA)) { | ||
onError((AuthenticationException) data.getSerializableExtra(Constants.EXCEPTION_EXTRA)); |
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.
any exception available would be raised through onError
immediately
Credentials credentials = extractCredentials(data); | ||
onAuthentication(credentials); | ||
} else if (event == LockEvent.CANCELED) { | ||
onError(new AuthenticationException("a0.authentication_canceled", "The user pressed back")); |
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.
If the event is "canceled", then raise an exception with the specific code that represents this. This is only for AuthenticationException
. LockCallback
would still receive this event and is up to the dev to handle (this advanced scenario remains available)
@@ -168,7 +167,7 @@ private void processEvent(@NonNull Intent data) { | |||
break; | |||
case Constants.INVALID_CONFIGURATION_ACTION: |
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 don't have a "public" event type for this. So we raise an exception instead.
@@ -68,5 +69,5 @@ | |||
* | |||
* @param error describing what happened. | |||
*/ | |||
void onError(@NonNull LockException error); | |||
void onError(@NonNull AuthenticationException error); |
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.
Changes the type of exception. Helps to deliver the actual exception and not wrapping everything again.
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.
LGTM, just have a question regarding the Readme.
MIGRATION_GUIDE.md
Outdated
|
||
### Removed methods | ||
|
||
#### From class `AuthenticationCallback` | ||
- Removed `public void onCanceled()`. Instead, an exception will be raised with this code. Check the property `AuthenticationException#isCanceled()` to handle it. |
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.
Says "with this code" but doesn't include the code.
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.
Good catch. Rephrased this and also noted that this is a method and not a property. Ready for review.
d54f080
to
48117f5
Compare
Changes
With this change, the
AuthenticationCallback
is reduced from 3 methods down to 2. Before, the "canceled" event would trigger aonCanceled
method call. This method no longer exists and instead, an exception now is raised with the code "a0.authentication_canceled". This is easily testable checking theAuthenticationException#isCanceled()
method.The immediate benefit is accessing the
AuthenticationException
directly, without having to cast the cause wrapped in the (previously existing)LockException
. This helps to consume different error codes and messages that are now going to be received after #618This PR includes a breaking change.
References
See SDK-2480