-
Notifications
You must be signed in to change notification settings - Fork 80
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
Support defining paginator on resource #92
base: master
Are you sure you want to change the base?
Conversation
@@ -273,7 +273,7 @@ def result_options(records, options) | |||
end | |||
|
|||
if JSONAPI.configuration.top_level_meta_include_page_count | |||
data[:page_count] = page_count_for(data[:record_count]) | |||
data[:page_count] = apply_pagination?(options) ? page_count_for(data[:record_count]) : 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If JSONAPI.configuration.top_level_meta_include_page_count
is true but apply_pagination?
is false, then page count should always equal 1
@@ -62,16 +62,21 @@ def record_count_for(records, options) | |||
# | |||
# @api private | |||
def paginator | |||
@paginator ||= paginator_klass.new(page_params) | |||
@paginator ||= begin | |||
paginator_klass = "#{resource_paginator_name}_paginator".classify.constantize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this from being its own method to living inside paginator
because a) it's not used anywhere else and b) I thought it might confusing to have paginator
, paginator_klass
, and resource_paginator_name
methods.
@@ -101,7 +101,7 @@ | |||
context 'with "page"' do | |||
context 'when using "paged" paginator' do | |||
before(:all) do | |||
JSONAPI.configuration.default_paginator = :paged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to change the resource paginator instead of the default config because of the way @request.resource_klass._paginator
works in Pagination. If @paginator
has not been set on the resource, then it is set to the default config paginator. That's all good for the first set of paginator tests, but when it moves on to the next ones and updates the default paginator setting, @paginator
for UserResource is still set to what it was previously.
So, in the course of running the "paged" tests, UserResource._paginator
gets set to :paged
, even if you never updated the UserResource directly & changed the default paginator setting. When it gets to the "offset" tests and you update the default paginator to "offset", then you'll get an error because UserResource._paginator
is still set as :paged
.
Basically, this change is solving an issue with the tests themselves, not an issue being raised by the code.
@@ -77,7 +77,7 @@ | |||
end | |||
end | |||
|
|||
describe '#count_pages_for' do | |||
describe '#page_count_for' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to match the corresponding method name
@phantomphildius Would be great if you could take a look as well! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is very useful
Updates Pagination/Formatters to check the resource paginator instead of the default paginator. Currently, if you've set a paginator directly on your resource, it's ignored in favor of the default.
@tiagopog Could you please take a look at this for me? Let me know if you have any questions or if there's anything you'd like me to add / change.
Thanks!