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

fix to properly use max per page settings #3279

Merged
merged 5 commits into from
Jan 27, 2023
Merged

Conversation

briri
Copy link
Contributor

@briri briri commented Jan 11, 2023

  • Update API v1 to obey they api_max_page_size configuration param
  • Update paginable.rb to use results_per_page instead of api_max_page_size

Copy link
Contributor

@pengyin-shan pengyin-shan left a comment

Choose a reason for hiding this comment

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

Tested your branch and it works. Thank you!

@johnpinto1
Copy link
Contributor

@briri can I suggest you add logic to the Rest API version of V0 app/controllers/api/v0/base_api_controller.rb
as you have done for app/controllers/api/v1/base_api_controller.rb so that Rails.configuration.x.application.api_max_page_size value is taken into account in V0 too now that it has been removed from app/controllers/concerns/paginable.rb.

@johnpinto1
Copy link
Contributor

johnpinto1 commented Jan 25, 2023

@briri The API V1 works fine and UI pagination.

Found issue with API V0. It is broken. Error:

Processing by Api::V0::PlansController#index as JSON
[ActionMailer URL Options] {:host=>"lvh.me:3000", :protocol=>"http://"}
Completed 500 Internal Server Error in 19ms (ActiveRecord: 0.0ms | Allocations: 10082)

NoMethodError (undefined method `pagination_params' for #Api::V0::PlansController:0x0000000002c308
Did you mean? paginable_params):

Please add pagination_params() to app/controllers/api/v0/base_controller.rb. It works when I added it in.

  # Retrieve the requested pagination params or use defaults
  # only allow 100 per page as the max
  def pagination_params
    max_per_page = Rails.configuration.x.application.api_max_page_size
    @page = params.fetch('page', 1).to_i
    @per_page = params.fetch('per_page', max_per_page).to_i
    @per_page = max_per_page if @per_page > max_per_page
  end

@briri
Copy link
Contributor Author

briri commented Jan 25, 2023

thanks @johnpinto1. I had originally added the function there to mimic v1 but then realized that plans was the only paginated endpoint and it needed @args instead of the individual @per_page and @page.

Copy link
Contributor

@johnpinto1 johnpinto1 left a comment

Choose a reason for hiding this comment

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

Thanks @briri. Ready to merge @benjaminfaure

@benjaminfaure benjaminfaure merged commit 00459b6 into development Jan 27, 2023
@benjaminfaure benjaminfaure deleted the pagination-fix branch January 27, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants