Adding max_pages for pagination response

[jeremyf]

Kaminari is expecting that max_pages exist

[cbeer]

I don't understand the semantics of Kaminari's max_pages, but I don't think it's the total number of pages (see #total_pages / #num_pages).

The only relevant rubydoc I can find says:

This model's max_pages value returns max_pages value unless explicitly overridden via max_pages_per.

In the code, it seems like this is a ceiling for the number of pages for any result set:

if max_pages.present? && max_pages < total_pages_count
        max_pages
      else

With that logic, maybe we should just return nil?

[jeremyf]

nil would be fine by me; I'll amend the commit

[cbeer]

I'm also trying to figure out what version that logic was introduced. It might be time to pin blacklight to that version (or, at least, pin our testing to something recent?)

[jeremyf]

@cbeer pushed the update to set as nil

[jcoyne]

Was added here: amatsuda/kaminari@c180703
So 0.14.1 (released a year ago)

[cbeer]

Ah, so it isn't in a Kaminari release yet?

[jcoyne]

Cbeer: That code was released a year ago.

[cbeer]

Is it? I don't see it in:

https://github.com/amatsuda/kaminari/blob/v0.14.1/lib/kaminari/models/configuration_methods.rb

[jcoyne]

You're right. Looks like it's just on master.

-Justin

On Mon, Sep 23, 2013 at 11:39 AM, Chris Beer notifications@github.hscsec.cnwrote:

Is it? I don't see it in:

https://github.com/amatsuda/kaminari/blob/v0.14.1/lib/kaminari/models/configuration_methods.rb

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/614#issuecomment-24933858
.

[cbeer]

So.. should we merge this in anticipation of the kaminari release that will require this method? Or wait for them to settle on an API (and maybe provide this behavior for free..)

[jeremyf]

The reason for this pull request is that I'm having to peg to a specific version of blacklight in my repo.

We are referencing gem 'kaminari', github: 'harai/kaminari', branch: 'route_prefix_prototype' for curate/sufia, which must've incorporated that method.

[jcoyne]

@jeremyf Sufia is using that branch of kaminari too, but hasn't had any trouble with "max_pages". How are you triggering that error?

Also, I'm not sure the test you added belongs in the grouping response spec. Shouldn't it be in the most generic kind of response spec?

[MrDys]

So, you're referencing a non-master branch of fork of kaminari? Seems like
something that should not be directly dealt with in blacklight until it
shows up in a main kaminari release.

On Mon, Sep 23, 2013 at 3:13 PM, Jeremy Friesen notifications@github.hscsec.cnwrote:

The reason for this pull request is that I'm having to peg to a specific
version of blacklight in my repo.

We are referencing gem 'kaminari', github: 'harai/kaminari', branch:
'route_prefix_prototype' for curate/sufia, which must've incorporated
that method.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/614#issuecomment-24945835
.

[jeremyf]

@jcoyne - Yes it should belong in the generic response spec…but I didn't see one so I went hunting.

Regarding sufia, I'm not entirely certain why it isn't triggering.

@MrDys - Without diving deeper into implementation details, it looks like master has some knowledge of max_pages within the mixin, but in that module does not explicitly define the max_pages method. What I suspect, based on a 1 minute sweep of the code is that this is now a config option that gets mixed in.

I am looking at as a reference point: https://github.com/amatsuda/kaminari/blob/master/lib/kaminari/models/page_scope_methods.rb#L27

[jcoyne]

@MrDys The fork that is being referenced has a critical fix for those of us
working with mountable engines (that use Blacklight). The maintainer of
kaminari is just ignoring that pull request which has been open for 9
months.

On Mon, Sep 23, 2013 at 2:45 PM, Jeremy Friesen notifications@github.hscsec.cnwrote:

@jcoyne https://github.com/jcoyne - Yes it should belong in the generic
response spec…but I didn't see one so I went hunting.

Regarding sufia, I'm not entirely certain why it isn't triggering.

@MrDys https://github.com/MrDys - Without diving deeper into
implementation details, it looks like master has some knowledge of
max_pages within the mixin, but in that module does not explicitly define
the max_pages method. What I suspect, based on a 1 minute sweep of the code
is that this is now a config option that gets mixed in.

I am looking at as a reference point:
https://github.com/amatsuda/kaminari/blob/master/lib/kaminari/models/page_scope_methods.rb#L27

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/614#issuecomment-24948336
.

[jcoyne]

@jeremyf suddenly this error has manifest in sufia: https://travis-ci.org/projecthydra/sufia/jobs/11710850

[jcoyne]

👍

[cbeer]

In 59a15be, I figured out where max_pages was provided (and used by other kaminari-internal integrations) and included it in our own mixin.