Add a way to disable JsonApi automatic pagination

[groyoh]

Purpose

Provides a way to prevent automatic links rendering when using kaminari or will_paginate. See #1549 and #1549 (comment).

Changes

Added a NonPaginatedCollectionSerializer with tests and documentation for it.

Related GitHub issues

Solves #1549
Should solve #1268 too

[NullVoxPopuli]

where do page and per_page come from?

[groyoh]

Maybe from ApplicationController 😉 I didn't want to write two methods or more code just to specify where the per_page and page come from. It's outside the scope of AMS or this example in my opinion. I could maybe replace it by params[:page] and params[:per_page].

[NullVoxPopuli]

yeah, I think the params would be helpful. Otherwise it might lead the reader to believe that we are monkey patching those methods in

[groyoh]

Fixed this.

[groyoh]

Added this as a shared test for CollectionSerializer and NonPaginatedCollectionSerializer.

[groyoh]

Fix one ruby warning.

[bf4]

fixed in master

[NullVoxPopuli]

I'm ok with this. 👍 anybody else? @rails-api/ams

[bf4]

Setting  `config.collection_serializer = NonPaginatedCollectionSerializer` now prevents the JSON API adapter from automatically rendering pagination Link objects

[bf4]

I'm kind of concerned at how the changeset here is much larger than the feature. If someone wants to disable json api link object generation, I'd rather just either add a condition to or redefine paginated? and we can move on.

[groyoh]

@bf4 would you rather have a boolean config such config.jsonapi_pagination_links_enabled? Not sure I understood your last comment.

[bf4]

@groyoh yeah config.jsonapi_pagination_links_enabled? or config.jsonapi_automatic_pagination_links? would be simplest. And this is kind of handling the case where a popular library isn't following the golden path. If that can affect our code base the least, I think that would be best. My alternative would be to create the NonPaginating serializer in test, and document that as a way to do it, but not have it as an actual feature.

[groyoh]

@bf4 Actually that was my goal at first. Then I probably misinterpreted your comments in #1549 and added these changes. I'll rework the PR then. Which of the two config names do you prefer?

[bf4]

@groyoh true enough. but couldn't the diff just be (pseudocode)

class CollectionSerializer
+  class NonPaginatedCollectionSerializer < CollectionSerializer
+    def paginated; false; end
+  end
end

and

test "pagination links are generated when `paginated?` is true" do
  # blah
end
+ test "pagination links are not generated when `paginated?` is false" do
+    serialization = serialize(resource, serializer: NonPaginatedCollectionSerializer).as_json
+    assert_equal {}, serialization.fetch(:data).fetch(:links)
+  end

and that's the whole PR (except for adding docs)

[groyoh]

That would work. So no specific config then?

BTW, I wanted to ask you. What's the best practice to define test here: def test_something or test "something" do ?

[bf4]

I don't know which form would better serve ams

B mobile phone

On Mar 27, 2016, at 12:21 PM, Yohan Robert [email protected] wrote:

That would work. So no specific config then?

BTW, I wanted to ask you. What's the best practice to define test here:

def test_something
test "something" do ?
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[NullVoxPopuli]

coming from rspec, I like test 'description' do... better

you can have spaces, and punctuation, and I think it makes reading the test's purpose a little easier.

[groyoh]

@NullVoxPopuli I also prefer it this way but never wanted to change the current test structure.

[NullVoxPopuli]

needs rebase

[bf4]

@groyoh Let's take some time to go over this together. It's basically done, just a lot of non-critical things I want to sync up on before mergin.

[groyoh]

@bf4 I'll have time to rework this from tomorrow night until sunday.

Let's take some time to go over this together. It's basically done, just a lot of non-critical things I want to sync up on before mergin.

I'm still not sure what your final thoughts are. Should we just provide a boolean configuration (render urls or not) or change the current PR to match with #1596 (comment)?

[remear]

So... a config option to disable automatic pagination then?

[bf4]

Even could be solved just with docs

[richmolj]

Solved with #1917