Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 20, 2023
Merged

[MM-933] Updated code to display proper errors on UI #986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions server/autocomplete_search.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,29 +22,27 @@ func (p *Plugin) httpGetAutoCompleteFields(w http.ResponseWriter, r *http.Reques

client, _, _, err := p.getClient(types.ID(instanceID), types.ID(mattermostUserID))
if err != nil {
return http.StatusInternalServerError, err
return respondErr(w, http.StatusInternalServerError, err)
}

results, err := client.SearchAutoCompleteFields(params)
if err != nil {
return http.StatusInternalServerError, err
return respondErr(w, http.StatusInternalServerError, err)
}

if results == nil {
return http.StatusInternalServerError, errors.New("failed to return any results")
return respondErr(w, http.StatusInternalServerError, errors.New("failed to return any results"))
}

bb, err := json.Marshal(results)
if err != nil {
return http.StatusInternalServerError,
errors.WithMessage(err, "failed to marshal response")
return respondErr(w, http.StatusInternalServerError, errors.WithMessage(err, "failed to marshal response"))
}

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 respondErr(w, http.StatusInternalServerError, errors.WithMessage(err, "failed to write response"))
}
return http.StatusOK, nil
}
Expand All @@ -57,30 +55,28 @@ func (p *Plugin) httpGetSearchUsers(w http.ResponseWriter, r *http.Request) (int

client, _, _, err := p.getClient(types.ID(instanceID), types.ID(mattermostUserID))
if err != nil {
return http.StatusInternalServerError, err
return respondErr(w, http.StatusInternalServerError, err)
}

// Get list of assignable users
jiraUsers, err := client.SearchUsersAssignableInProject(projectKey, userSearch, 10)
if StatusCode(err) == 401 {
return http.StatusInternalServerError, err
return respondErr(w, http.StatusInternalServerError, err)
}

if jiraUsers == nil {
return http.StatusInternalServerError, errors.New("failed to return any results")
return respondErr(w, http.StatusInternalServerError, errors.New("failed to return any results"))
}

bb, err := json.Marshal(jiraUsers)
if err != nil {
return http.StatusInternalServerError,
errors.WithMessage(err, "failed to marshal response")
return respondErr(w, http.StatusInternalServerError, errors.WithMessage(err, "failed to marshal response"))
}

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")
Copy link
Contributor

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

Copy link
Contributor Author

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.

}
return http.StatusOK, nil
}
2 changes: 2 additions & 0 deletions ...p/src/components/data_selectors/jira_autocomplete_selector/jira_autocomplete_selector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ export default class JiraAutoCompleteSelector extends React.PureComponent<Props>
value: suggestion.value,
label: stripHTML(suggestion.displayName),
}));
}).catch((e) => {
throw new Error('Error fetching data');
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +47 to +48
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright that makes sense

});
};

Expand Down
2 changes: 2 additions & 0 deletions webapp/src/components/data_selectors/jira_user_selector/jira_user_selector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ export default class JiraUserSelector extends React.PureComponent<Props> {
label,
};
});
}).catch((e) => {
throw new Error('Error fetching data');
});
};

Expand Down
Loading