Added test current_page for array when page 1 per 0

[araslanov-e]

No description provided.

[yuki24]

Could you elaborate on why you are sending this PR?

[araslanov-e]

@yuki24 sorry, tests didn't run locally. And I have a problem with current_page when use limit(0)

[araslanov-e]

@yuki24 In tests I wanted to reproduce the problem

[solutus]

@yuki24, Could you please clarify behaviour of pagination when per = 0.

1. model_class.page(1).per(0) returns 0 items without exception.
2. model_class.page(VERY_BIG_NUM).per(1).current_page returns VERY_BIG_NUM even if there no any items on this page.
3. model_class.page(1).per(0).current_page raises ZeroPerPageOperation.

Why model_class.page(1).per(0).current_page does not return 1 as for VERY_BIG_NUM?

It seems it is possible to avoid such kind of exceptions, without impact. Client could handle result as no items, i.e total_pages = 0.
Please, share you opinion. Thanks for advance.

[yuki24]

Could you also add a test with page number 2?

assert_equal 2, @array.page(2).per(0).current_page

[araslanov-e]

ok

[yuki24]

I don't think this spec makes sense since it is mathematically impossible to define what total_pages is in case of per(0) and it would make the definition of total_pages ambiguous.

[araslanov-e]

You're right

[yuki24]

First of all, we may be deprecating the #current_page method as its behavior is always unpredictable and not intuitive. See #990. We should not have to re-compute the current page number because the params[:page] is already there. As demonstrated in the PR, Kaminari's internal code totally works without it, and I'd imagine that it is also the case in a typical Rails app as well.

To answer your question, model_class.page(1).per(0).current_page == 1 actually makes more sense than raising an exception. But again, you should not rely on this method to get the current page.

Also, what are the reasons why you have to pass in 0 to the per method? I have personally never done that myself before, and I am not sure in what case we actually need it.

[solutus]

Also, what are the reasons you have to pass in 0 to the per method

I agree, it is unusual case for pure pagination logic.
But it is convenient for case, I tried describe below.

We have API with aggregations, similar to elasticsearch aggregations https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-terms-aggregation.html

Example:
GET /api/items?date_gte=2019-07-08&page=2&per=1&with_aggregations=city

with result:

{
  "items": [
    {"city": "Tokyo"}
  ],
  "aggregations": {
    "city": ["Tokyo", "Kyoto"]
  },
  "metadata": {
    "total_pages": 2, 
    "current_page": 1
  }
}

Using such API, client is getting items and list of aggregations data per filter at once.
Aggregations are using to display possible filters on index page.

Use case:

1. User opens /items page
2. Possible filters (cities, Tokyo and Kyoto) are rendered together with items.
3. User filter items for Tokyo.

Sometimes client wants only aggregations, without items.
We want reuse the same api. But total_pages & current_page raises Exception.

[yuki24]

I wanted to check in and see how this is going. I think @solutus's use case is legit and I am open to changing the behavior here. However, the failing builds should be fixe and new tests should be added to demonstrate the point of the change.

[yuki24]

I'd be happy to take another look at this pull request, but there doesn't seem to be a lot of activity here. I'll close this issue in the coming weeks if there are no more updates