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.

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

feat(ui) Add standardized GQL error handling function to FE #9470

Conversation

chriscollins3456
Copy link
Collaborator

This PR adds a new helper function that we can use throughout our FE app to standardize our error messages and make it easier to provide more helpful messages.

Right now this handles either a permissions error message or a default error message. I implemented it only for deleting a query for now as an example of how we can use this throughout the app.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Dec 15, 2023
defaultMessage: string;
}

export default function handleGraphQLError({ error, permissionMessage, defaultMessage }: Props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice I really appreciate this.

We can probably also have defaults for permission message / default message in caseyn start to add more specific error codes if we want...

Currently we have

400 -> Malformed query
401 / 403 -> Auth
500 -> Server error

but ideally we can continue to add to this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes definitely! that was my thought as well - this can just handle all of the errors we care about in a more standard way - making it easier to provide specific messages for your operations.

i'll add the 400 and 500 situations as well and also handle 401 as well as 403

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so yeah we actually do handle 401 (not logged in) in App.tsx and there we redirect you to the login page so that should be good to go - i can create a const for not logged in in the new enum i have and use that as well

Comment on lines +35 to +37
defaultMessage: 'Failed to delete Query! An unexpected error occurred',
permissionMessage: 'Unauthorized to delete Query. Please contact your DataHub administrator.',
});
Copy link
Collaborator

@samblackk samblackk Jan 19, 2024

Choose a reason for hiding this comment

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

I vote to make the permission message generic with the option to override. Unauthorized. Please contact your Datahub admin would suffice. Would mean it wouldn't need to be set for every instance of use, but will still provide sufficient feedback to the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice I like this

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@chriscollins3456 chriscollins3456 merged commit 0c940c7 into datahub-project:master Jan 19, 2024
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants