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::Pagination concern #28826

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

mjankowski
Copy link
Contributor

Several related changes:

  • Add an API::Pagination concern, and put the existing pagination methods from the api base controller into it
  • Add an rspec matcher which wraps up the logic of comparing expected url values to the prev/next header link values, and replace previous specs with this approach -- in some cases this served to remove what was basically a double request call when the checks were in different examples
  • Basically every API conrtoller that supports pagination had the same exact method definition of the insert_pagination_headers before_action. Move this to the newly created concern.

I think there is room for more future improvement in this area, but didn't want to go much further than this in first pass.

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.05%. Comparing base (86500e3) to head (4b039ef).
Report is 256 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #28826      +/-   ##
==========================================
+ Coverage   85.01%   85.05%   +0.04%     
==========================================
  Files        1059     1039      -20     
  Lines       28277    28092     -185     
  Branches     4538     4532       -6     
==========================================
- Hits        24040    23894     -146     
+ Misses       3074     3038      -36     
+ Partials     1163     1160       -3     

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

@mjankowski
Copy link
Contributor Author

The two linked PRs here show some possible future additions to this concern.

@renchap renchap added refactoring Improving code quality ruby Pull requests that update Ruby code labels Feb 16, 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 the max/since one merged. This is probably ready to go as well, but let me know if this would be easier to review in smaller chunks. I could probably pull out the new matcher and spec changes into separate PR, and possibly do the concern changes in 2-3 PRs as well.

Separately, the linked pagination params PR - #28845 - should either be merged first and then incorporated into this change; or if this is merged first, it should be updated to move things to the concern instead of the base class.

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.

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 #29606 merged - this is now even smaller/cleaner, just moving the pagination-related methods already moved to base class, out to concern.

It would probably be best to review #28845 first, and then rebase this after that one.

Copy link
Contributor

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

@mjankowski mjankowski requested a review from a team April 16, 2024 15:14
@ClearlyClaire ClearlyClaire added this pull request to the merge queue Apr 17, 2024
Merged via the queue into mastodon:main with commit 1d3ecd3 Apr 17, 2024
32 checks passed
@mjankowski mjankowski deleted the api-concern-pagination branch April 17, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Improving code quality ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants