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

Pagination fix #820

Closed
wants to merge 9 commits into from
Closed

Conversation

davidw
Copy link

@davidw davidw commented Sep 21, 2016

This is based on the work here:

https://github.com/amatsuda/kaminari/pull/573

But includes a fix for this change:

rails/rails#18379

@yuki24
Copy link
Member

yuki24 commented Sep 21, 2016

Many specs including the newly added specs are failing. This code would also introduce a dependency that is a private Rails API and I'm not sure if we should go with this.

@davidw
Copy link
Author

davidw commented Sep 21, 2016

I'll look into the specs.

I don't see any perfect solutions to the problem this addresses:

  • Ignore it. This lets bugs slip into people's code, which is why it came to my attention in the first place - one of our API's was returning way fewer pages than it should have because a GROUP BY got added at a certain point.
  • Decide that it just can't do that, and throw an exception if you try and use something with a GROUP BY. That'd at least make things very clear.
  • Something like this solution.
  • Other ideas?

Thank you!

@davidw
Copy link
Author

davidw commented Sep 21, 2016

Locally, the activerecord 4* tests pass, but 32 does not work at all:

home/davidw/.rvm/gems/ruby-2.2.2/gems/activesupport-3.2.22.5/lib/active_support/dependencies.rb:251:in require': cannot load such file -- test/unit/assertions (LoadError)`

@yuki24
Copy link
Member

yuki24 commented Nov 16, 2016

I'm closing this issue in favor of #573.

@yuki24 yuki24 closed this Nov 16, 2016
@yuki24
Copy link
Member

yuki24 commented Nov 16, 2016

But thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants