-
Notifications
You must be signed in to change notification settings - Fork 130
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
[MM-933] Updated code to display proper errors on UI #986
Conversation
1. Updated the backend code to return proper errors instead of empty response for the "get-search-autocomplete-fields" and "get-search-users" API. 2. Updated UI code to display a generic error in case of "get-search-autocomplete-fields" and "get-search-users" API calls.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #986 +/- ##
==========================================
+ Coverage 29.65% 29.66% +0.01%
==========================================
Files 52 52
Lines 7794 7790 -4
==========================================
Hits 2311 2311
+ Misses 5287 5283 -4
Partials 196 196
☔ View full report in Codecov by Sentry. |
webapp/src/components/data_selectors/jira_autocomplete_selector/jira_autocomplete_selector.tsx
Show resolved
Hide resolved
}).catch((e) => { | ||
throw new Error('Error fetching data'); |
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 are sort of "swallowing" this error here. Is the error present in e
helpful to the user at all?
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.
@mickmister I don't think the errors are helpful for the user. You can check the errors returned in this PR only.
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 guess I'm wondering why we're returning an error from the API if we're not using it on the frontend though
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.
@mickmister We want to show the error to the user in case we are unable to fetch the data. If we don't send the error from the API we won't be able to display the generic error. Please correct me if I am wrong
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.
Alright that makes sense
} | ||
|
||
w.Header().Set("Content-Type", "application/json") | ||
_, err = w.Write(bb) | ||
if err != nil { | ||
return http.StatusInternalServerError, | ||
errors.WithMessage(err, "failed to write response") | ||
return http.StatusInternalServerError, errors.WithMessage(err, "failed to write response") |
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 notice we're not calling respondErr
here. I think that's intentional since this error is coming from a failure to write a response
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.
@mickmister We were not calling respondErr
anywhere in the function. I think we should as the user will not see any data in the dropdown in this case as well. Please let me know your thoughts on this.
Summary
Screenshot
Ticket Link
Fixes #933