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

Add API::Errors concern for API controllers and add i18n values for strings #27925

Closed
wants to merge 6 commits into from

Conversation

mjankowski
Copy link
Contributor

@mjankowski mjankowski commented Nov 17, 2023

Changes to the Api::BaseController error handling:

  • Some of the error classes handled by the controller were not in the spec, added them
  • Added a check to the spec which asserts against the json error value in the response (previously only http response code was checked)
  • Move the strings out of the controller class and into i18n yml
  • Move the API error handling methods to an Api::ErrorHandling concern

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.89%. Comparing base (86500e3) to head (4e63e64).
Report is 278 commits behind head on main.

❗ Current head 4e63e64 differs from pull request most recent head c533a02. Consider uploading reports for the commit c533a02 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #27925      +/-   ##
==========================================
- Coverage   85.01%   84.89%   -0.13%     
==========================================
  Files        1059     1059              
  Lines       28277    28376      +99     
  Branches     4538     4551      +13     
==========================================
+ Hits        24040    24090      +50     
- Misses       3074     3124      +50     
+ Partials     1163     1162       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@renchap renchap added testing Automated lint and test suites ruby Pull requests that update Ruby code labels Nov 17, 2023
@mjankowski mjankowski force-pushed the api-base-i18n-strings branch 2 times, most recently from 5b9518c to f601ff8 Compare November 21, 2023 18:03
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@mjankowski
Copy link
Contributor Author

Rebased after #28864 added the coverage for missing classes.

This one is now basically two things: moving the error handling to concern, adding i18n strings. If we only want one of these, but not the other (or want them in sepsearate PRs), let me know.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@mjankowski mjankowski changed the title Add missing error classes to api/base controller spec, add i18n values for strings Add API::Errors concern for API controllers and add i18n values for strings Feb 6, 2024
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@mjankowski
Copy link
Contributor Author

Rebased this after errors concern merged, so is this now just a) the i18n stuff, b) helper methods for that, c) spec improvement to check response body in addition to code.

I think that spec stuff is helpful, but am less certain about the i18n stuff. Will pull out spec change into separate PR, and then probably close this and drop remainder.

@mjankowski
Copy link
Contributor Author

Closing in favor of linked PR.

@mjankowski mjankowski closed this Mar 14, 2024
@mjankowski mjankowski deleted the api-base-i18n-strings branch March 14, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rebase needed 🚧 ruby Pull requests that update Ruby code testing Automated lint and test suites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants