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 bug last_page? wrong when current_page is 1 and total_pages is 0 (version 1.0.1) #877

Closed
wants to merge 2 commits into from

Conversation

HoangQuan
Copy link

@HoangQuan HoangQuan commented Mar 15, 2017

I found a bug when when current_page is 1 and total_pages is 0 (There last page is also last page in this case)
When I tried to query like this: User.where(id: 'not_exist').page(1).per(10)
Then last_page? is false But it's should be true.
Please review and comfirm if I am correct. thanks

@HoangQuan HoangQuan changed the title fix bug last_page? wrong when current_page is 1 and total_pages is 0 fix bug last_page? wrong when current_page is 1 and total_pages is 0 (kaminari (1.0.1)) Mar 15, 2017
@HoangQuan HoangQuan changed the title fix bug last_page? wrong when current_page is 1 and total_pages is 0 (kaminari (1.0.1)) fix bug last_page? wrong when current_page is 1 and total_pages is 0 (version 1.0.1) Mar 15, 2017
@yuki24
Copy link
Member

yuki24 commented Mar 20, 2017

Could you elaborate? Theoretically User.where(id: 'not_exist').page(1).per(10) (is this just an example?) would always return 0 and doesn't make sense to use it in production. There is of course a case where there is no record in the table you are trying to load data from, but it's an edge case and I'm typically not concerned about such a case.

@HoangQuan
Copy link
Author

@yuki24 , Thank you very much for your replied
In my case, I write an API that check and update the page and display_next_button using last_page? like that:

result[:display_next_button] = false
unless users.last_page?
  result[:page] += 1 
  result[:display_next_button] = true
end

There are 2 scenario below:
1, page = 1 and users has no record. Then total_pages is 0 (as I commented above)
-> In this case my result[:page] is increase and the display_next_button is displayed but It's wrong
2, Suppose that the total_pages is 10 but someone request to server with params[:page] = 11
-> Then the page is increase too, and the display_next_button is still displaying.

How do you think about those two cases?
I don't think it's an edge case.

@sanemat
Copy link
Contributor

sanemat commented Mar 21, 2017

This is my case.

#controller
@entries = Entry.where()... #=> Entry.none
#view
json = {}
json[:_links] = {}
json[:_links][:self] = { href: entries_url(page: @entries.current_page, limit: @entries.limit_value) }
json[:_links][:prev] = { href: entries_url(page: @entries.prev_page, limit: @entries.limit_value) } unless @entries.first_page?
json[:_links][:next] = { href: entries_url(page: @entries.next_page, limit: @entries.limit_value) } unless @entries.last_page?

I should use #exists??

json[:_links][:next] = { href: entries_url(page: @entries.next_page, limit: @entries.limit_value) } if @entries.exists? && !@entries.last_page?

@HoangQuan
Copy link
Author

HoangQuan commented Mar 21, 2017

@sanemat In your case, I think we only need next_page

json[:_links][:next] = { href: entries_url(page: @entries.next_page, limit: @entries.limit_value) } if @entries.next_page

because @entries.next_page will return number when next_page is exist and return nil if it's not.
Can you try it and comfirm?
anw, I want to use @entries.last_page? but there are some problem as I commented above.

@yuki24
Copy link
Member

yuki24 commented Mar 21, 2017

It seems that you could just the out_of_range? method. Keep in mind that we may deprecate the out_of_range? method since it behaves exactly like the empty? method.

if !users.last_page? || !users.out_of_range?
  result[:page] += 1
  result[:display_next_button] = true
end

Please also have a look at the paginator template for how you could show/hide pagination links. I would also recommend implementing RFC5988 rather than passing flags and page numbers around. I'm not sure about your codebase but I have a hunch that there's a better way of showing a link to the next page.

@@ -337,7 +337,7 @@ def shutdown
end

test 'out of range' do
assert_false model_class.page(11).per(10).last_page?
assert_true model_class.page(11).per(10).last_page?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should change this since this is mathematically incorrect.

@HoangQuan
Copy link
Author

@yuki24 Thank you for corrected me.

@yuki24
Copy link
Member

yuki24 commented Apr 5, 2017

I'm closing this issue since kaminari used to behave exactly like the proposed behavior before. Thanks!

@yuki24 yuki24 closed this Apr 5, 2017
@To1ne
Copy link

To1ne commented Aug 17, 2017

Apparently this is the intended behavior, see #583.

@HoangQuan
Copy link
Author

thank you for the information (y)

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.

4 participants