feat: Render okta errors based on error param #3196
Conversation
Bundle ReportChanges will increase total bundle size by 1.97kB ⬆️
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3196 +/- ##
================================================
+ Coverage 98.13000 98.14000 +0.01000
================================================
Files 935 932 -3
Lines 14510 14473 -37
Branches 3968 3871 -97
================================================
- Hits 14240 14204 -36
+ Misses 265 264 -1
Partials 5 5
... and 24 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 267 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3196 +/- ##
=======================================
Coverage 98.13% 98.14%
=======================================
Files 935 932 -3
Lines 14510 14473 -37
Branches 3968 3871 -97
=======================================
- Hits 14240 14204 -36
+ Misses 265 264 -1
Partials 5 5
... and 24 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3196 +/- ##
=======================================
Coverage 98.13% 98.14%
=======================================
Files 935 932 -3
Lines 14510 14473 -37
Branches 3886 3952 +66
=======================================
- Hits 14240 14204 -36
+ Misses 265 264 -1
Partials 5 5
... and 24 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3196 +/- ##
=======================================
Coverage 98.13% 98.14%
=======================================
Files 935 932 -3
Lines 14510 14473 -37
Branches 3973 3957 -16
=======================================
- Hits 14240 14204 -36
+ Misses 265 264 -1
Partials 5 5
... and 24 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
| if (error in errorMessages) { | ||
| return errorMessages[error as keyof typeof errorMessages] | ||
| } | ||
| return 'An unknown error occurred. Please try again or contact support.' |
There was a problem hiding this comment.
We could log this to Sentry via Sentry.captureMessage and include the error type so we can keep tabs on cases we might have missed (or if they change for some reason)
There was a problem hiding this comment.
Another thing you may want to add in alongside the fingerprint is a tag. I added one to the prismLanguageMapper capture message and this is what it provides in the Sentry UI:
And the code is pretty simple to add in:
Sentry.captureMessage(`Unsupported language type for filename ${fileName}`, {
fingerprint: ['unsupported-prism-language'],
tags: {
'file.extension': fileExtension,
},
})| const ErrorType = { | ||
| UnauthorizedClient: 'unauthorized_client', | ||
| AccessDenied: 'access_denied', | ||
| UnsupportedResponseType: 'unsupported_response_type', | ||
| UnsupportedResponseMode: 'unsupported_response_mode', | ||
| InvalidScope: 'invalid_scope', | ||
| ServerError: 'server_error', | ||
| TemporarilyUnavailable: 'temporarily_unavailable', | ||
| InvalidClient: 'invalid_client', | ||
| LoginRequired: 'login_required', | ||
| InvalidRequest: 'invalid_request', | ||
| UserCanceledRequest: 'user_canceled_request', | ||
| } as const |
There was a problem hiding this comment.
Is there a benefit to using this enum object here? We don't seem to be accessing the value outside of the object definition, could it just be simplified to one object?
There was a problem hiding this comment.
We're aligning with JavaScript's property naming conventions by receiving snake_case values and converting them to the preferred camelCase format
There was a problem hiding this comment.
I think I might not have explained myself well, but what I wanted to say is that since we are not actually accessing the enum properties and checking for individual keys inside the getErrorMessage function or anywhere else, there is no need to align this and we can just type it as:
const OktaErrorMappings = { 'server_error': "...", } as const
function getOktaErrorMessage(status){
return OktaHTTPErrorMappings[status] ?? 'default status'
}This way we can drop the enum and make keep the definition in a single type as opposed to having to define enum + record to make changes in the future.
There was a problem hiding this comment.
I understand your point; the way you listed it is more direct. However, this goes against our ESLint rules and JavaScript's preferred key structure. It will also make more sense if we ever need to access the enum keys, instead of having to refactor it later
There was a problem hiding this comment.
Ugh more silly linting rules, gonna address these kinda rules a bit more during the GV2 work because they're hampering us more than helping us.
There was a problem hiding this comment.
Is there a benefit to having these split out from the component where they're being used? Exporting getErrorMessage by itself may be confusing for others as it's not clear what error messages it's getting, and may be accidentally imported elsewhere.
There was a problem hiding this comment.
The object is big and messy. I think it’s cleaner this way. We don’t have a rigid structure for enum files in the codebase, so I don’t think it’s confusing, especially since it’s a file nested within the banner folder
There was a problem hiding this comment.
Yeah, they tend to get large and a little messy, and that's alright. If the error messages only relates to Okta and this component specifically, it might make sense to just move the code definitions inside the banner so they are more tightly coupled or rename the export to getOktaErrorMessage.
There was a problem hiding this comment.
Cool I'll rename it to make it even more explicit for its use
Description
Surfacing okta errors, a full list of the error codes can be found here: https://developer.okta.com/docs/reference/error-codes/#example-errors-for-openid-connect-and-social-login
issue: codecov/engineering-team#2347
PS. The design was discussed with Kyle and dismiss button visibility in dark mode/light will be handled in a different issue.
Notable Changes
Screenshots
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.