Accept arguments for `#count` #303

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

DanielVartanov commented Apr 3, 2013

Sometimes it is needed to call relation.count(distinct: true). I am not sure what is the best way to test it within will_paginate's testing conventions. Can you guys give me some advice?

Thanks in advance.

Owner

mislav commented Apr 3, 2013

Why do you need to count distinct entries on a paginated collection?

will_paginate's testing conventions aren't anything special. You set up some data in the database (I use fixtures), then call the method and assert that you got the value you expected.

I'm not happy with the current hack I did for count, but it was needed to work around some Active Record bugs. Since count accepts arguments and I seem to have broken that, your fix is in order.

Contributor

DanielVartanov commented Apr 21, 2013

Why do you need to count distinct entries on a paginated collection?

I came up with a request which I have to denote disctinct: true for in order to get a correct result.

Honestly, I believe that it is a bug of ActiveRecord (relation.uniq.count.to_sql returns SELECT COUNT(id) instead of SELECT COUNT(DISTINCT id)). I will try to fix it in AR, but anyway it is useful for will_paginate to reflect actual AR syntax.

So, for now I should write tests and it will be enough, correct?

Owner

mislav commented Sep 16, 2013

If AR Relation accepts arguments to count, I definitely want to forward them. It doesn't matter so much what they are. Please add tests for this

Contributor

DanielVartanov commented Sep 16, 2013

Ok! I'll be back with tests soon.

+1

Want to be able to pass count(:all) due to rails/rails#10710

Contributor

nathany commented May 1, 2014

Need to same feature for the same reason as @cyrusstoller.

Thanks @DanielVartanov.

Contributor

nathany commented May 29, 2014

@DanielVartanov Are you adding tests for this and rebasing to master?

Contributor

DanielVartanov commented May 29, 2014

@nathany procrastinating, unfortunately. I publicly announce, that it will be done by June 2nd, 00:00 UTC.

I offer to put me in shame if I don't! But I will :-)

Contributor

nathany commented May 29, 2014

😄

Contributor

DanielVartanov commented May 29, 2014

@mislav Please review the updated commit (sorry for a force-push).

Contributor

nathany commented May 30, 2014

Thanks @DanielVartanov. There is still another issue with count internally from will_paginate if there is a select scope.

It can be fixed with count(:all) but I'm not sure how far back Rails supports that syntax and how far back will_paginate intends to support with this version.

nathany referenced this pull request May 30, 2014

Closed

Test against Rails 4.1 #380

@mislav mislav commented on the diff Jun 1, 2014

spec/finders/active_record_spec.rb
@@ -205,6 +205,11 @@
$query_sql.last.should_not =~ /\ORDER\b/
end
+ it "accepts arguments for #count" do
+ Project.page(1).count(distinct: true)
@mislav

mislav Jun 1, 2014

Owner

I wish this test was more solid. It should assert the result of count. The data and the query should be set up in a way that makes count() yield different results depending on whether it was with or without arguments. This way we're sure that the arguments really got forwarded and that the correct SQL query was executed. Then there will be no need for inspecting the query directly on the next line.

Also, if such test passes on CI, we are confident that it works in all combinations of Active Record + db adapter versions.

@mislav

mislav Jun 18, 2014

Owner

In the end I removed this test before merging, since it didn't yield the same results on all Rails versions that we test with (~ half of them failed)

mislav closed this in 0c6d08e Jun 18, 2014

@mislav mislav added a commit that referenced this pull request Jun 28, 2014

@mislav mislav Forward `count` arguments on Relation for Rails 4.1 compat
Fixes #382, closes #303
b6a726c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment