-
Notifications
You must be signed in to change notification settings - Fork 493
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
[SDK-1405] Added support for new generic error details #1084
Conversation
I think this looks good. @abbaspour, you had mentioned needing a new callback here? |
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
Thanks @adamjmcgrath. I've just read this is due a further change so I will hold off merging for now. Will need another review once those changes are made. |
src/helper/response-handler.js
Outdated
@@ -58,6 +59,17 @@ function wrapCallback(cb, options) { | |||
errObj.error_description = errObj.description; | |||
} | |||
|
|||
if ( | |||
errObj.code === 'blocked_user' && |
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.
A question - do we need to have this hardcoded check here on 'blocked_user'.
There might be benefits in making this code generic. If there are other cases in future that need to extend error responses via codes and details, then generic code will require no new code changes in Auth0.js.
Do you think it makes sense to check if error_codes and error_details are present, then just include them in the response object in generic manner ( and not wrap it in blockedUser object)?
{
code: 'blocked_user',
description: 'Blocked user.',
codes: [ 'reason-1', 'reason-2' ],
details: {
'reason-1': {
timestamp: 123786498
},
'reason-2': {
timestamp: 127836389
}
}
}
or
{
code: 'blocked_user',
description: 'Blocked user.',
errorDetails: {
codes: [ 'reason-1', 'reason-2' ],
details: {
'reason-1': {
timestamp: 123786498
},
'reason-2': {
timestamp: 127836389
}
}
}
}
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.
@madsharm I like the second option here, primarily because the first one has both code
and codes
at the same level on the object, which could be confusing.
Agreed with making it more generic though. Let me make some changes.
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.
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 and thanks for doing the change
* Removed check for `blocked_users` code * Changed property name on error object from `blockUser` to `errorDetails`
Changes
This PR adds support for new "blocked_reasons" error type, which includes more detail behind why a user has been blocked.
The details are surfaced in the error object as follows:
References
Support for this was generated from an internal service desk ticket.
Testing
Checklist