Skip to content

fix to properly use max per page settings - #3279

Merged
benjaminfaure merged 5 commits into
developmentfrom
pagination-fix
Jan 27, 2023
Merged

fix to properly use max per page settings#3279
benjaminfaure merged 5 commits into
developmentfrom
pagination-fix

Conversation

@briri

@briri briri commented Jan 11, 2023

Copy link
Copy Markdown
Contributor
  • 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

@pengyin-shan pengyin-shan left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

johnpinto1 commented Jan 25, 2023

Copy link
Copy Markdown
Contributor

@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

briri commented Jan 25, 2023

Copy link
Copy Markdown
Contributor Author

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.

@johnpinto1 johnpinto1 left a comment

Copy link
Copy Markdown
Contributor

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

4 participants