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

Support defining paginator on resource #92

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

coryeb
Copy link

@coryeb coryeb commented Aug 14, 2018

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!

@@ -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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@coryeb
Copy link
Author

coryeb commented Aug 14, 2018

@phantomphildius Would be great if you could take a look as well!

Copy link

@palytoxin palytoxin left a 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

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.

None yet

2 participants