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

Fix total_pages method fail on active_record model after calling merge with unscoped limit #652

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions lib/will_paginate/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

module WillPaginate
# = Paginating finders for ActiveRecord models
#
#
# WillPaginate adds +paginate+, +per_page+ and other methods to
# ActiveRecord::Base class methods and associations.
#
#
# In short, paginating finders are equivalent to ActiveRecord finders; the
# only difference is that we start with "paginate" instead of "find" and
# that <tt>:page</tt> is required parameter:
Expand All @@ -24,7 +24,7 @@ module RelationMethods
attr_writer :total_entries

def per_page(value = nil)
if value.nil? then limit_value
if value.nil? then limit_value || total_entries
else limit(value)
end
end
Expand Down Expand Up @@ -67,7 +67,7 @@ def offset(value = nil)

def total_entries
@total_entries ||= begin
if loaded? and size < limit_value and (current_page == 1 or size > 0)
if loaded? and limit_value and size < limit_value and (current_page == 1 or size > 0)
offset_value + size
else
@total_entries_queried = true
Expand Down Expand Up @@ -183,15 +183,15 @@ module BaseMethods
# +per_page+.
#
# Example:
#
#
# @developers = Developer.paginate_by_sql ['select * from developers where salary > ?', 80000],
# :page => params[:page], :per_page => 3
#
# A query for counting rows will automatically be generated if you don't
# supply <tt>:total_entries</tt>. If you experience problems with this
# generated SQL, you might want to perform the count manually in your
# application.
#
#
def paginate_by_sql(sql, options)
pagenum = options.fetch(:page) { raise ArgumentError, ":page parameter required" } || 1
per_page = options[:per_page] || self.per_page
Expand Down
37 changes: 23 additions & 14 deletions spec/finders/active_record_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,22 @@
ActiverecordTestConnector.setup

RSpec.describe WillPaginate::ActiveRecord do

extend ActiverecordTestConnector::FixtureSetup

fixtures :topics, :replies, :users, :projects, :developers_projects

it "should integrate with ActiveRecord::Base" do
expect(ActiveRecord::Base).to respond_to(:paginate)
end

it "should paginate" do
expect {
users = User.paginate(:page => 1, :per_page => 5).to_a
expect(users.length).to eq(5)
}.to execute(2).queries
end

it "should fail when encountering unknown params" do
expect {
User.paginate :foo => 'bar', :page => 1, :per_page => 4
Expand Down Expand Up @@ -134,7 +134,7 @@
expect(topics).not_to be_empty
}.to execute(1).queries
end

it "support empty? for grouped queries" do
topics = Topic.group(:project_id).paginate :page => 1, :per_page => 3
expect {
Expand Down Expand Up @@ -190,7 +190,7 @@
it "should count with group" do
expect(Developer.group(:salary).page(1).total_entries).to eq(4)
end

it "should count with select" do
expect(Topic.select('title, content').page(1).total_entries).to eq(4)
end
Expand All @@ -203,8 +203,17 @@
it "should not have zero total_pages when the result set is empty" do
expect(Developer.where("1 = 2").page(1).total_pages).to eq(1)
end

it "should calculate total_pages after calling merge with unscoped limit" do
expect {
paginated_projects = Project.all.paginate(:page => 1, :per_page => 3)
unscoped_activerecord_topics = Topic.mentions_activerecord.unscope(:limit)
result = paginated_projects.joins(:topics).merge(unscoped_activerecord_topics)
expect(result.total_pages).to eq(1)
}.not_to raise_error
end
end

it "should not ignore :select parameter when it says DISTINCT" do
users = User.select('DISTINCT salary').paginate :page => 2
expect(users.total_entries).to eq(5)
Expand Down Expand Up @@ -263,11 +272,11 @@
options = { :page => 1 }
options.expects(:delete).never
options_before = options.dup

Topic.paginate(options)
expect(options).to eq(options_before)
end

it "should get first page of Topics with a single query" do
expect {
result = Topic.paginate :page => nil
Expand All @@ -277,15 +286,15 @@
expect(result.size).to eq(4)
}.to execute(1).queries
end

it "should get second (inexistent) page of Topics, requiring 1 query" do
expect {
result = Topic.paginate :page => 2
expect(result.total_pages).to eq(1)
expect(result).to be_empty
}.to execute(1).queries
end

describe "associations" do
it "should paginate" do
dhh = users(:david)
Expand All @@ -309,7 +318,7 @@
expect {
dhh.projects.order('projects.id').limit(4).to_a
}.not_to raise_error

result = dhh.projects.paginate(:page => 1, :per_page => 4).reorder('projects.id')
expect(result).to eq(expected_id_ordered)

Expand All @@ -331,7 +340,7 @@
}.to execute(1).queries
end
end

it "should paginate with joins" do
result = nil
join_sql = 'LEFT JOIN developers_projects ON users.id = developers_projects.developer_id'
Expand Down