From d2e2b3d2b1cdddfb71ea6b52335902f33672bffe Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 14 Apr 2018 10:51:29 -0500 Subject: [PATCH 1/3] pagination: add paginator tests. --- test/unit/paginator_test.rb | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 test/unit/paginator_test.rb diff --git a/test/unit/paginator_test.rb b/test/unit/paginator_test.rb new file mode 100644 index 000000000..2f1eb9767 --- /dev/null +++ b/test/unit/paginator_test.rb @@ -0,0 +1,34 @@ +require 'test_helper' + +class PaginatorTest < ActiveSupport::TestCase + setup do + @posts = FactoryBot.create_list(:post, 5) + end + + context "sequential pagination (before)" do + should "return the correct set of records" do + expected_posts = Post.limit(3).order(id: :desc) + posts = Post.paginate("b9999999", limit: 3) + + assert_equal(expected_posts.map(&:id), posts.map(&:id)) + end + end + + context "sequential pagination (after)" do + should "return the correct set of records" do + expected_posts = Post.limit(3).order(id: :asc).reverse + posts = Post.paginate("a0", limit: 3) + + assert_equal(expected_posts.map(&:id), posts.map(&:id)) + end + end + + context "numbered pagination" do + should "return the correct set of records" do + expected_posts = Post.limit(3).order(id: :desc) + posts = Post.order(id: :desc).paginate("1", limit: 3) + + assert_equal(expected_posts.map(&:id), posts.map(&:id)) + end + end +end From 074b66bf274e0fdc5ce1e2d9f43693cb03dc0e72 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 14 Apr 2018 10:53:23 -0500 Subject: [PATCH 2/3] pagination: prefer `extending` over `extend. The `extending` method is the preferred way to add methods to an ActiveRecord collection. * http://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-extending * https://ruby-doc.org/core-2.5.0/Object.html#method-i-extend --- .../paginator/active_record_extension.rb | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/app/logical/danbooru/paginator/active_record_extension.rb b/app/logical/danbooru/paginator/active_record_extension.rb index 667c8dc73..1e9488495 100644 --- a/app/logical/danbooru/paginator/active_record_extension.rb +++ b/app/logical/danbooru/paginator/active_record_extension.rb @@ -37,17 +37,17 @@ module Danbooru c = c.where("#{table_name}.id < ?", before_id.to_i) end - c.reorder("#{table_name}.id desc").tap do |obj| - obj.extend(SequentialCollectionExtension) - obj.sequential_paginator_mode = :before - end + c = c.reorder("#{table_name}.id desc") + c = c.extending(SequentialCollectionExtension) + c.sequential_paginator_mode = :before + c end def paginate_sequential_after(after_id) - limit(records_per_page + 1).where("#{table_name}.id > ?", after_id.to_i).reorder("#{table_name}.id asc").tap do |obj| - obj.extend(SequentialCollectionExtension) - obj.sequential_paginator_mode = :after - end + c = limit(records_per_page + 1).where("#{table_name}.id > ?", after_id.to_i).reorder("#{table_name}.id asc") + c = c.extending(SequentialCollectionExtension) + c.sequential_paginator_mode = :after + c end def paginate_numbered(page) @@ -57,8 +57,7 @@ module Danbooru raise ::Danbooru::Paginator::PaginationError.new("You cannot go beyond page #{Danbooru.config.max_numbered_pages}. Please narrow your search terms.") end - limit(records_per_page).offset((page - 1) * records_per_page).tap do |obj| - obj.extend(NumberedCollectionExtension) + extending(NumberedCollectionExtension).limit(records_per_page).offset((page - 1) * records_per_page).tap do |obj| if records_per_page > 0 obj.total_pages = (obj.total_count.to_f / records_per_page).ceil else From d42ef7d7dc156c7fb44dd30f12319389612cc2db Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 14 Apr 2018 10:18:20 -0500 Subject: [PATCH 3/3] Fix #3642: Issues with sequential pagination. As of Rails 5, overriding `to_a` on an ActiveRecord collection does nothing. We need to override `records` instead. ref: https://github.com/rails/rails/commit/cdd45fa09d76f7c16ccbb6682e672a44f82174a0 --- .../danbooru/paginator/sequential_collection_extension.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/logical/danbooru/paginator/sequential_collection_extension.rb b/app/logical/danbooru/paginator/sequential_collection_extension.rb index 9cee8fc4a..c78da7846 100644 --- a/app/logical/danbooru/paginator/sequential_collection_extension.rb +++ b/app/logical/danbooru/paginator/sequential_collection_extension.rb @@ -19,7 +19,10 @@ module Danbooru end end - def to_a + # XXX Hack: in sequential pagination we fetch one more record than we need + # so that we can tell when we're on the first or last page. Here we override + # a rails internal method to discard that extra record. See #2044, #3642. + def records if sequential_paginator_mode == :before super.first(records_per_page) else