-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add useDataset hook #1462
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
Add useDataset hook #1462
Conversation
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.
One comment for maintainability otherwise LGTM
client/src/hooks/useDataset.js
Outdated
switch (statusCode) { | ||
case 400: | ||
case 403: | ||
case 404: | ||
case 405: | ||
case 409: | ||
return exceptions[statusCode] | ||
default: | ||
return defaultMessage | ||
} |
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.
switch (statusCode) { | |
case 400: | |
case 403: | |
case 404: | |
case 405: | |
case 409: | |
return exceptions[statusCode] | |
default: | |
return defaultMessage | |
} | |
if (statusCode in exceptions) { | |
return exceptions[statusCode] | |
} | |
return defaultMessage | |
I think something like this would be easier to maintain since you already have defined the supported codes.
I don't think we want to be presenting the HTTP errors to the user. For now this is ok but we should have a TODO to handle recoverable errors for now at least.
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.
Much cleaner using the in
operator! I've also added a TODO
comment for recoverable error handling.
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
client/src/hooks/useDataset.js
Outdated
|
||
const process = async (dataset) => { | ||
if (!token) { | ||
addError('A valid token is required to update the dataset') |
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.
addError('A valid token is required to update the dataset') | |
addError('A valid token is required to process the dataset') |
…set.getErrorMessage and add a TODO comment for recoverable error handling, and fix a typo in useDataset.process
Issue Number
Closes #1407
Purpose/Implementation Notes
I've bootstrap the
useDataset
hook.Changes include:
useDataset
hook with following methods:create
get
process
update
getErrorMessage
: returns a error message that matches the corresponding status code on API request failureTypes of changes
Functional tests
N/A
Checklist
Screenshots
N/A