From 7387266c37557db1224b032c2d7cbbc55b364dca Mon Sep 17 00:00:00 2001 From: mecampbellsoup Date: Mon, 5 Jun 2017 17:23:34 -0400 Subject: [PATCH 1/8] Always display self, first, last pagination links --- .../adapter/json_api/pagination_links.rb | 22 ++++++++++--------- .../adapter/json_api/pagination_links_test.rb | 11 +++++++++- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/lib/active_model_serializers/adapter/json_api/pagination_links.rb b/lib/active_model_serializers/adapter/json_api/pagination_links.rb index b15f5ba68..11ea472ea 100644 --- a/lib/active_model_serializers/adapter/json_api/pagination_links.rb +++ b/lib/active_model_serializers/adapter/json_api/pagination_links.rb @@ -35,19 +35,21 @@ def as_json private def pages_from - return {} if collection.total_pages <= FIRST_PAGE - {}.tap do |pages| - pages[:self] = collection.current_page + pages[:self] = collection.current_page + pages[:first] = FIRST_PAGE + pages[:last] = collection.total_pages - unless collection.current_page == FIRST_PAGE - pages[:first] = FIRST_PAGE - pages[:prev] = collection.current_page - FIRST_PAGE - end + if collection.total_pages > 0 + unless collection.current_page == FIRST_PAGE + pages[:prev] = collection.current_page - FIRST_PAGE + end - unless collection.current_page == collection.total_pages - pages[:next] = collection.current_page + FIRST_PAGE - pages[:last] = collection.total_pages + unless collection.current_page == collection.total_pages + pages[:next] = collection.current_page + FIRST_PAGE + end + else + pages[:last] = FIRST_PAGE end end end diff --git a/test/adapter/json_api/pagination_links_test.rb b/test/adapter/json_api/pagination_links_test.rb index 736ea2fe2..49d766033 100644 --- a/test/adapter/json_api/pagination_links_test.rb +++ b/test/adapter/json_api/pagination_links_test.rb @@ -54,6 +54,14 @@ def data } end + def empty_collection_links + { + self: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", + first: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", + last: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2" + } + end + def links { links: { @@ -71,6 +79,7 @@ def last_page_links links: { self: "#{URI}?page%5Bnumber%5D=3&page%5Bsize%5D=2", first: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", + last: "#{URI}?page%5Bnumber%5D=3&page%5Bsize%5D=2", prev: "#{URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2" } } @@ -111,7 +120,7 @@ def expected_response_with_last_page_pagination_links def expected_response_with_no_data_pagination_links {}.tap do |hash| hash[:data] = [] - hash[:links] = {} + hash.merge! links: empty_collection_links end end From d8e983604b1e24d80940af6408f923ed65316455 Mon Sep 17 00:00:00 2001 From: mecampbellsoup Date: Mon, 5 Jun 2017 17:51:38 -0400 Subject: [PATCH 2/8] Clarify naming of expected response w/ pagination but empty data --- test/adapter/json_api/pagination_links_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/adapter/json_api/pagination_links_test.rb b/test/adapter/json_api/pagination_links_test.rb index 49d766033..f29661468 100644 --- a/test/adapter/json_api/pagination_links_test.rb +++ b/test/adapter/json_api/pagination_links_test.rb @@ -117,7 +117,7 @@ def expected_response_with_last_page_pagination_links end end - def expected_response_with_no_data_pagination_links + def expected_response_with_empty_collection_pagination_links {}.tap do |hash| hash[:data] = [] hash.merge! links: empty_collection_links @@ -148,7 +148,7 @@ def test_pagination_links_when_zero_results_kaminari adapter = load_adapter(using_kaminari(1), mock_request) - assert_equal expected_response_with_no_data_pagination_links, adapter.serializable_hash + assert_equal expected_response_with_empty_collection_pagination_links, adapter.serializable_hash end def test_pagination_links_when_zero_results_will_paginate @@ -156,7 +156,7 @@ def test_pagination_links_when_zero_results_will_paginate adapter = load_adapter(using_will_paginate(1), mock_request) - assert_equal expected_response_with_no_data_pagination_links, adapter.serializable_hash + assert_equal expected_response_with_empty_collection_pagination_links, adapter.serializable_hash end def test_last_page_pagination_links_using_kaminari From ff71ef26eb4e945e0fd01bf2b02ea2d6a68d49ec Mon Sep 17 00:00:00 2001 From: mecampbellsoup Date: Mon, 5 Jun 2017 18:41:15 -0400 Subject: [PATCH 3/8] Amend pagination controller specs --- .../json_api/pagination_test.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/action_controller/json_api/pagination_test.rb b/test/action_controller/json_api/pagination_test.rb index 0af086b7c..b5a8f3d7a 100644 --- a/test/action_controller/json_api/pagination_test.rb +++ b/test/action_controller/json_api/pagination_test.rb @@ -58,8 +58,9 @@ def test_render_pagination_links_with_will_paginate assert_equal expected_links, response['links'] end - def test_render_only_last_and_next_pagination_links + def test_render_only_first_last_and_next_pagination_links expected_links = { 'self' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", + 'first' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", 'next' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2", 'last' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2" } get :render_pagination_using_will_paginate, params: { page: { number: 1, size: 2 } } @@ -78,17 +79,19 @@ def test_render_pagination_links_with_kaminari assert_equal expected_links, response['links'] end - def test_render_only_prev_and_first_pagination_links + def test_render_only_prev_first_and_last_pagination_links expected_links = { 'self' => "#{KAMINARI_URI}?page%5Bnumber%5D=3&page%5Bsize%5D=1", 'first' => "#{KAMINARI_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=1", - 'prev' => "#{KAMINARI_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=1" } + 'prev' => "#{KAMINARI_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=1", + 'last' => "#{KAMINARI_URI}?page%5Bnumber%5D=3&page%5Bsize%5D=1" } get :render_pagination_using_kaminari, params: { page: { number: 3, size: 1 } } response = JSON.parse(@response.body) assert_equal expected_links, response['links'] end - def test_render_only_last_and_next_pagination_links_with_additional_params + def test_render_only_first_last_and_next_pagination_links_with_additional_params expected_links = { 'self' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2&teste=additional", + 'first' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2&teste=additional", 'next' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2&teste=additional", 'last' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2&teste=additional" } get :render_pagination_using_will_paginate, params: { page: { number: 1, size: 2 }, teste: 'additional' } @@ -96,10 +99,11 @@ def test_render_only_last_and_next_pagination_links_with_additional_params assert_equal expected_links, response['links'] end - def test_render_only_prev_and_first_pagination_links_with_additional_params + def test_render_only_prev_first_and_last_pagination_links_with_additional_params expected_links = { 'self' => "#{KAMINARI_URI}?page%5Bnumber%5D=3&page%5Bsize%5D=1&teste=additional", 'first' => "#{KAMINARI_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=1&teste=additional", - 'prev' => "#{KAMINARI_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=1&teste=additional" } + 'prev' => "#{KAMINARI_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=1&teste=additional", + 'last' => "#{KAMINARI_URI}?page%5Bnumber%5D=3&page%5Bsize%5D=1&teste=additional" } get :render_pagination_using_kaminari, params: { page: { number: 3, size: 1 }, teste: 'additional' } response = JSON.parse(@response.body) assert_equal expected_links, response['links'] From 16e5204eaba9d4bb898dcb4eb5eeb7266b3d4185 Mon Sep 17 00:00:00 2001 From: mecampbellsoup Date: Tue, 13 Jun 2017 12:09:00 -0400 Subject: [PATCH 4/8] Always include pagination keys but set null values when not needed --- .../adapter/json_api/pagination_links.rb | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/lib/active_model_serializers/adapter/json_api/pagination_links.rb b/lib/active_model_serializers/adapter/json_api/pagination_links.rb index 11ea472ea..209819b69 100644 --- a/lib/active_model_serializers/adapter/json_api/pagination_links.rb +++ b/lib/active_model_serializers/adapter/json_api/pagination_links.rb @@ -3,7 +3,6 @@ module Adapter class JsonApi < Base class PaginationLinks MissingSerializationContextError = Class.new(KeyError) - FIRST_PAGE = 1 attr_reader :collection, :context @@ -20,12 +19,13 @@ def initialize(collection, adapter_options) end def as_json - per_page = collection.try(:per_page) || collection.try(:limit_value) || collection.size - pages_from.each_with_object({}) do |(key, value), hash| - params = query_parameters.merge(page: { number: value, size: per_page }).to_query - - hash[key] = "#{url(adapter_options)}?#{params}" - end + { + "self": location_url, + "first": first_page_url, + "prev": prev_page_url, + "next": next_page_url, + "last": last_page_url + } end protected @@ -34,37 +34,37 @@ def as_json private - def pages_from - {}.tap do |pages| - pages[:self] = collection.current_page - pages[:first] = FIRST_PAGE - pages[:last] = collection.total_pages + def location_url + url_for_page(collection.current_page) + end - if collection.total_pages > 0 - unless collection.current_page == FIRST_PAGE - pages[:prev] = collection.current_page - FIRST_PAGE - end + def first_page_url + url_for_page(1) + end - unless collection.current_page == collection.total_pages - pages[:next] = collection.current_page + FIRST_PAGE - end - else - pages[:last] = FIRST_PAGE - end - end + def prev_page_url + return nil if collection.first_page? + url_for_page(collection.prev_page) end - def url(options) - @url ||= options.fetch(:links, {}).fetch(:self, nil) || request_url + def next_page_url + return nil if collection.last_page? || collection.out_of_range? + url_for_page(collection.next_page) end - def request_url - @request_url ||= context.request_url + def url_for_page(number) + params = query_parameters.dup + params[:page] = { page: per_page, number: number } + context.url_for(action: :index, params: params) end def query_parameters @query_parameters ||= context.query_parameters end + + def per_page + @per_page ||= collection.try(:per_page) || collection.try(:limit_value) || collection.size + end end end end From 92e9a66e97657cbe3ee2ab536e05fa9e276263f9 Mon Sep 17 00:00:00 2001 From: mecampbellsoup Date: Tue, 13 Jun 2017 12:35:55 -0400 Subject: [PATCH 5/8] Amend tests to always include all pagination keys --- .../adapter/json_api/pagination_links.rb | 27 +++++++++++++++---- .../json_api/pagination_test.rb | 4 +++ .../adapter/json_api/pagination_links_test.rb | 9 ++++--- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/lib/active_model_serializers/adapter/json_api/pagination_links.rb b/lib/active_model_serializers/adapter/json_api/pagination_links.rb index 209819b69..b63cee7a5 100644 --- a/lib/active_model_serializers/adapter/json_api/pagination_links.rb +++ b/lib/active_model_serializers/adapter/json_api/pagination_links.rb @@ -3,6 +3,7 @@ module Adapter class JsonApi < Base class PaginationLinks MissingSerializationContextError = Class.new(KeyError) + FIRST_PAGE = 1 attr_reader :collection, :context @@ -42,20 +43,36 @@ def first_page_url url_for_page(1) end + def last_page_url + if collection.total_pages == 0 + url_for_page(FIRST_PAGE) + else + url_for_page(collection.total_pages) + end + end + def prev_page_url - return nil if collection.first_page? - url_for_page(collection.prev_page) + return nil if collection.current_page == FIRST_PAGE + url_for_page(collection.current_page - FIRST_PAGE) end def next_page_url - return nil if collection.last_page? || collection.out_of_range? + return nil if (collection.total_pages == 0 || collection.current_page == collection.total_pages) url_for_page(collection.next_page) end def url_for_page(number) params = query_parameters.dup - params[:page] = { page: per_page, number: number } - context.url_for(action: :index, params: params) + params[:page] = { size: per_page, number: number } + "#{url(adapter_options)}?#{params.to_query}" + end + + def url(options = {}) + @url ||= options.fetch(:links, {}).fetch(:self, nil) || request_url + end + + def request_url + @request_url ||= context.request_url end def query_parameters diff --git a/test/action_controller/json_api/pagination_test.rb b/test/action_controller/json_api/pagination_test.rb index b5a8f3d7a..98355a055 100644 --- a/test/action_controller/json_api/pagination_test.rb +++ b/test/action_controller/json_api/pagination_test.rb @@ -61,6 +61,7 @@ def test_render_pagination_links_with_will_paginate def test_render_only_first_last_and_next_pagination_links expected_links = { 'self' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", 'first' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", + 'prev' => nil, 'next' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2", 'last' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2" } get :render_pagination_using_will_paginate, params: { page: { number: 1, size: 2 } } @@ -83,6 +84,7 @@ def test_render_only_prev_first_and_last_pagination_links expected_links = { 'self' => "#{KAMINARI_URI}?page%5Bnumber%5D=3&page%5Bsize%5D=1", 'first' => "#{KAMINARI_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=1", 'prev' => "#{KAMINARI_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=1", + 'next' => nil, 'last' => "#{KAMINARI_URI}?page%5Bnumber%5D=3&page%5Bsize%5D=1" } get :render_pagination_using_kaminari, params: { page: { number: 3, size: 1 } } response = JSON.parse(@response.body) @@ -92,6 +94,7 @@ def test_render_only_prev_first_and_last_pagination_links def test_render_only_first_last_and_next_pagination_links_with_additional_params expected_links = { 'self' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2&teste=additional", 'first' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2&teste=additional", + 'prev' => nil, 'next' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2&teste=additional", 'last' => "#{WILL_PAGINATE_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2&teste=additional" } get :render_pagination_using_will_paginate, params: { page: { number: 1, size: 2 }, teste: 'additional' } @@ -103,6 +106,7 @@ def test_render_only_prev_first_and_last_pagination_links_with_additional_params expected_links = { 'self' => "#{KAMINARI_URI}?page%5Bnumber%5D=3&page%5Bsize%5D=1&teste=additional", 'first' => "#{KAMINARI_URI}?page%5Bnumber%5D=1&page%5Bsize%5D=1&teste=additional", 'prev' => "#{KAMINARI_URI}?page%5Bnumber%5D=2&page%5Bsize%5D=1&teste=additional", + 'next' => nil, 'last' => "#{KAMINARI_URI}?page%5Bnumber%5D=3&page%5Bsize%5D=1&teste=additional" } get :render_pagination_using_kaminari, params: { page: { number: 3, size: 1 }, teste: 'additional' } response = JSON.parse(@response.body) diff --git a/test/adapter/json_api/pagination_links_test.rb b/test/adapter/json_api/pagination_links_test.rb index f29661468..ff08df4b0 100644 --- a/test/adapter/json_api/pagination_links_test.rb +++ b/test/adapter/json_api/pagination_links_test.rb @@ -58,7 +58,9 @@ def empty_collection_links { self: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", first: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", - last: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2" + prev: nil, + next: nil, + last: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", } end @@ -79,8 +81,9 @@ def last_page_links links: { self: "#{URI}?page%5Bnumber%5D=3&page%5Bsize%5D=2", first: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", - last: "#{URI}?page%5Bnumber%5D=3&page%5Bsize%5D=2", - prev: "#{URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2" + prev: "#{URI}?page%5Bnumber%5D=2&page%5Bsize%5D=2", + next: nil, + last: "#{URI}?page%5Bnumber%5D=3&page%5Bsize%5D=2" } } end From b7442e741ce119c02a1873a9ad5c504bc576d28b Mon Sep 17 00:00:00 2001 From: mecampbellsoup Date: Wed, 14 Jun 2017 12:52:29 -0400 Subject: [PATCH 6/8] Use symbolized keys for json hash They will be converted to strings when rendered as JSON. --- .../adapter/json_api/pagination_links.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/active_model_serializers/adapter/json_api/pagination_links.rb b/lib/active_model_serializers/adapter/json_api/pagination_links.rb index b63cee7a5..58394b91c 100644 --- a/lib/active_model_serializers/adapter/json_api/pagination_links.rb +++ b/lib/active_model_serializers/adapter/json_api/pagination_links.rb @@ -21,11 +21,11 @@ def initialize(collection, adapter_options) def as_json { - "self": location_url, - "first": first_page_url, - "prev": prev_page_url, - "next": next_page_url, - "last": last_page_url + self: location_url, + first: first_page_url, + prev: prev_page_url, + next: next_page_url, + last: last_page_url } end From e4b3224c6406c587d2441feede5ce793073ef996 Mon Sep 17 00:00:00 2001 From: mecampbellsoup Date: Wed, 14 Jun 2017 12:53:38 -0400 Subject: [PATCH 7/8] Fix indentation mismatch --- .../adapter/json_api/pagination_links.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_model_serializers/adapter/json_api/pagination_links.rb b/lib/active_model_serializers/adapter/json_api/pagination_links.rb index 58394b91c..0327e1660 100644 --- a/lib/active_model_serializers/adapter/json_api/pagination_links.rb +++ b/lib/active_model_serializers/adapter/json_api/pagination_links.rb @@ -15,7 +15,7 @@ def initialize(collection, adapter_options) JsonApi::PaginationLinks requires a ActiveModelSerializers::SerializationContext. Please pass a ':serialization_context' option or override CollectionSerializer#paginated? to return 'false'. - EOF + EOF end end From 402883d84fe2f4ba2ee9ed8722d7ee2ca438b40b Mon Sep 17 00:00:00 2001 From: mecampbellsoup Date: Wed, 14 Jun 2017 13:35:00 -0400 Subject: [PATCH 8/8] Fix Travis lint offenses --- .../adapter/json_api/pagination_links.rb | 2 +- test/adapter/json_api/pagination_links_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/active_model_serializers/adapter/json_api/pagination_links.rb b/lib/active_model_serializers/adapter/json_api/pagination_links.rb index 0327e1660..859368b0c 100644 --- a/lib/active_model_serializers/adapter/json_api/pagination_links.rb +++ b/lib/active_model_serializers/adapter/json_api/pagination_links.rb @@ -57,7 +57,7 @@ def prev_page_url end def next_page_url - return nil if (collection.total_pages == 0 || collection.current_page == collection.total_pages) + return nil if collection.total_pages == 0 || collection.current_page == collection.total_pages url_for_page(collection.next_page) end diff --git a/test/adapter/json_api/pagination_links_test.rb b/test/adapter/json_api/pagination_links_test.rb index ff08df4b0..3cdbab0ef 100644 --- a/test/adapter/json_api/pagination_links_test.rb +++ b/test/adapter/json_api/pagination_links_test.rb @@ -60,7 +60,7 @@ def empty_collection_links first: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", prev: nil, next: nil, - last: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2", + last: "#{URI}?page%5Bnumber%5D=1&page%5Bsize%5D=2" } end