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

Jump to bottom

Enrich 4xx error message with error text from response.body #647

Merged
merged 1 commit into from
Dec 5, 2019
Merged

Enrich 4xx error message with error text from response.body #647

merged 1 commit into from
Dec 5, 2019

Conversation

vbyno
Copy link
Contributor

@vbyno vbyno commented Dec 5, 2019

This PR makes 4xx error messages more informative.

Instead of having this:

ActiveResource::ResourceInvalid, "Failed.  Response code = 422.  Response message = Unprocessable Entity.

the message will contain the error from response.body['error']

ActiveResource::ResourceInvalid, "Failed.  Response code = 422.  Response message = Unprocessable Entity (Cannot cancel a paid and fulfilled order).

so that it would be easier to quickly identify what went wrong

@vbyno vbyno requested a review from a team as a code owner December 5, 2019 13:16
@ghost ghost added the cla-needed label Dec 5, 2019
@vbyno vbyno force-pushed the enrich_4xx_error_messages branch from 18a7bab to 62fd848 Compare December 5, 2019 13:24
@ghost ghost removed the cla-needed label Dec 5, 2019
tanema
tanema approved these changes Dec 5, 2019
View reviewed changes
Copy link
Contributor

@tanema tanema left a comment

Choose a reason for hiding this comment

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

This is an awesome change, thank you!

@tanema
Copy link
Contributor

tanema commented Dec 5, 2019

One thing! Can you add a line to the changelog about your change.

@vbyno
Copy link
Contributor Author

vbyno commented Dec 5, 2019 •
edited
Loading

@tanema , of course! I'm not sure if adding it to 8.0.0 section in Changelog is fine, since it is already released. Shall I create a new section (8.0.1 ?) and bump the version, or just put the text without specifying the version?

UPD: added a line

@vbyno vbyno force-pushed the enrich_4xx_error_messages branch from 62fd848 to e2fc188 Compare December 5, 2019 15:51
@vbyno vbyno force-pushed the enrich_4xx_error_messages branch from e2fc188 to d2bd6d2 Compare December 5, 2019 15:52
@tanema
Copy link
Contributor

tanema commented Dec 5, 2019

Yup, just the one line is good and we will add the version and the changelog title when we release the next version

@tanema tanema merged commit b458fb9 into Shopify:master Dec 5, 2019
@ignacio-chiazzo ignacio-chiazzo mentioned this pull request Dec 23, 2019
Merged
@ignacio-chiazzo ignacio-chiazzo temporarily deployed to rubygems December 23, 2019 18:09 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanema tanema tanema approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vbyno @tanema @ignacio-chiazzo