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

Support pagination link for Kaminari when no data is returned #1700

Merged
merged 1 commit into from
Apr 22, 2016

Conversation

iamnader
Copy link
Contributor

Purpose

This PR fixes a bug where the pagination links were showing up (with incorrect data) when a query returned no results and using kaminari.

The issue was in that case kaminari says total_pages is 0, whereas will_paginate says total_pages is 1.

Changes

This updated code supports both cases now and treats them as it had been working for will_paginate, namely no links show up.

Additional helpful information

I added tests for both kaminari and will_paginate. The will_paginate test passed before my change, and the kaminari one failed. Now they both pass.

@NullVoxPopuli
Copy link
Contributor

Do both of those tests fail pre fix?

@NullVoxPopuli
Copy link
Contributor

I checked out your code, and and changed the <= back to a == and the kaminari test failed (good), but the will_paginate test did not fail.

Can you take a second look at your will_paginate test? or, if this is expected, explain why? :-)

@bf4
Copy link
Member

bf4 commented Apr 22, 2016

@NullVoxPopuli in pr description

I added tests for both kaminari and will_paginate. The will_paginate test passed before my change, and the kaminari one failed. Now they both pass.

@bf4
Copy link
Member

bf4 commented Apr 22, 2016

Just needs a changelog entry

@NullVoxPopuli
Copy link
Contributor

Oops. My bad. :-)

@iamnader
Copy link
Contributor Author

Updated change log. Thanks for the quick response team!

@@ -8,6 +8,7 @@ Breaking changes:
- [#1574](https://github.com/rails-api/active_model_serializers/pull/1574) Default key case for the JsonApi adapter changed to dashed. (@remear)

Features:
- [#1700](https://github.com/rails-api/active_model_serializers/pull/1700) Support pagination link for Kaminari when no data is returned
Copy link
Member

@bf4 bf4 Apr 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, super sorry, there should be headers above this in the changelog for 'master'

### [master (unreleased)](https://github.com/rails-api/active_model_serializers/compare/v0.10.0.rc5...master)

Breaking changes:

Features:
- [#1700](https://github.com/rails-api/active_model_serializers/pull/1700) Support pagination link for Kaminari when no data is returned


Fixes:

Misc:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, moved this to the Fixes section. Thanks!

@@ -8,6 +8,7 @@ Breaking changes:
- [#1574](https://github.com/rails-api/active_model_serializers/pull/1574) Default key case for the JsonApi adapter changed to dashed. (@remear)

Features:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@NullVoxPopuli
Copy link
Contributor

LGTM! nice work!

@NullVoxPopuli NullVoxPopuli merged commit 8c8edd6 into rails-api:master Apr 22, 2016
@bf4
Copy link
Member

bf4 commented Apr 22, 2016

@NullVoxPopuli gosh durn it, the changelog was in the wrong spot so I was merging it on the command line. I fix in mater in a moment

@NullVoxPopuli
Copy link
Contributor

Was it? It's under Fixes. :-\

@NullVoxPopuli
Copy link
Contributor

DOH! it was under rc5 :-(

Gotta get used to that.

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

Successfully merging this pull request may close these issues.

3 participants