Description
Tomás Cohen Arazi (tcohen)
2017-08-29 13:31:28 UTC
Created attachment 66599 [details] [review] Bug 19196: Unit tests This patch adds unit tests for the new pagination Mojo plugin. Sponsored-by: ByWater Solutions Sponsored-by: Camden County Created attachment 66600 [details] [review] Bug 19196: Add Koha::REST::Plugin::Pagination This patch introduces a Mojolicious plugin to be used on the REST api. It adds two helper methods: total_count_header ================== When used, it adds an _X-Total-Count_ header with the value it got passed like this: my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->total_count_header({ total_count => $count }); => Response contains X-Total-Count: $count links_header ============ When used, it adds a _Link_ header to the reponse with the calculated values for pagination, like this: my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->links_header({ total => $count, page => 2, per_page => 2 }); To test: - Run: $ sudo koha-shell kohadev k$ cd kohaclone k$ prove t/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: ByWater solutions Sponsored-by: Camden County Created attachment 66601 [details] [review] Bug 19196: Make plugin available to endpoints This patch just initializes the plugin on the main controller class so it is available for all endpoints to use. As it is not used, in order to test just restart plack and make sure the endpoints work (run the t/db_dependent/api/v1 tests). Created attachment 66609 [details] [review] Bug 19196: Unit tests This patch adds unit tests for the new pagination Mojo plugin. Sponsored-by: ByWater Solutions Sponsored-by: Camden County Created attachment 66610 [details] [review] Bug 19196: Add Koha::REST::Plugin::Pagination This patch introduces a Mojolicious plugin to be used on the REST api. It adds two helper methods: total_count_header ================== When used, it adds an _X-Total-Count_ header with the value it got passed like this: my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->total_count_header({ total => $count }); => Response contains X-Total-Count: $count links_header ============ When used, it adds a _Link_ header to the reponse with the calculated values for pagination, like this: my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->links_header({ total => $count, page => 2, per_page => 2 }); To test: - Run: $ sudo koha-shell kohadev k$ cd kohaclone k$ prove t/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: ByWater solutions Sponsored-by: Camden County Created attachment 66611 [details] [review] Bug 19196: Make plugin available to endpoints This patch just initializes the plugin on the main controller class so it is available for all endpoints to use. As it is not used, in order to test just restart plack and make sure the endpoints work (run the t/db_dependent/api/v1 tests). Created attachment 66656 [details] [review] Bug 19196: Unit tests This patch adds unit tests for the new pagination Mojo plugin. Sponsored-by: ByWater Solutions Sponsored-by: Camden County Created attachment 66657 [details] [review] Bug 19196: Add Koha::REST::Plugin::Pagination This patch introduces a Mojolicious plugin to be used on the REST api. It adds a helper method: add_pagination_headers ====================== When used, it adds a _Link_ header to the reponse with the calculated values for pagination, and X-Total-Count containing the total results like this: my $params = $c->validation->output; my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->add_pagination_headers({ total => $count, page => $params->{page}, per_page => $params->{per_page} }); To test: - Run: $ sudo koha-shell kohadev k$ cd kohaclone k$ prove t/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: ByWater solutions Sponsored-by: Camden County lib Created attachment 66658 [details] [review] Bug 19196: Make plugin available to endpoints This patch just initializes the plugin on the main controller class so it is available for all endpoints to use. As it is not used, in order to test just restart plack and make sure the endpoints work (run the t/db_dependent/api/v1 tests). Created attachment 66820 [details] [review] Bug 19196: Unit tests This patch adds unit tests for the new pagination Mojo plugin. Sponsored-by: ByWater Solutions Sponsored-by: Camden County Created attachment 66821 [details] [review] Bug 19196: Add Koha::REST::Plugin::Pagination This patch introduces a Mojolicious plugin to be used on the REST api. It adds a helper method: add_pagination_headers ====================== When used, it adds a _Link_ header to the reponse with the calculated values for pagination, and X-Total-Count containing the total results like this: my $params = $c->validation->output; my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->add_pagination_headers({ total => $count, params => $params )}; To test: - Run: $ sudo koha-shell kohadev k$ cd kohaclone k$ prove t/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: ByWater solutions Sponsored-by: Camden County Created attachment 66822 [details] [review] Bug 19196: Make plugin available to endpoints This patch just initializes the plugin on the main controller class so it is available for all endpoints to use. As it is not used, in order to test just restart plack and make sure the endpoints work (run the t/db_dependent/api/v1 tests). Patches reworked so the resulting pagination links preserve the query params :-D Great work Tomas.
please fix typo in your example ;)
> $c->add_pagination_headers({
> total => $count,
> params => $params )};
and add some sensible defaults (10/10?) to page and per_page so Pagination.pm doesnt die if none is given as params.
Created attachment 66988 [details] [review] Bug 19196: Unit tests This patch adds unit tests for the new pagination Mojo plugin. Sponsored-by: ByWater Solutions Sponsored-by: Camden County Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 66989 [details] [review] Bug 19196: Add Koha::REST::Plugin::Pagination This patch introduces a Mojolicious plugin to be used on the REST api. It adds a helper method: add_pagination_headers ====================== When used, it adds a _Link_ header to the reponse with the calculated values for pagination, and X-Total-Count containing the total results like this: my $params = $c->validation->output; my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->add_pagination_headers({ total => $count, params => $params )}; To test: - Run: $ sudo koha-shell kohadev k$ cd kohaclone k$ prove t/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: ByWater solutions Sponsored-by: Camden County Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 66990 [details] [review] Bug 19196: Make plugin available to endpoints This patch just initializes the plugin on the main controller class so it is available for all endpoints to use. As it is not used, in order to test just restart plack and make sure the endpoints work (run the t/db_dependent/api/v1 tests). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I really like this.. only minor query would be can we document the decision as to why we've chosen to do page based instead of cursor based result 'paging'. I'm personally happy to go with either.. but to prevent FUD in future developers I think it's worth documenting the decision process so we can point to that when someone asks down the line ;) Created attachment 67338 [details] [review] Bug 19196: Unit tests This patch adds unit tests for the new pagination Mojo plugin. Sponsored-by: ByWater Solutions Sponsored-by: Camden County Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 67339 [details] [review] Bug 19196: Add Koha::REST::Plugin::Pagination This patch introduces a Mojolicious plugin to be used on the REST api. It adds a helper method: add_pagination_headers ====================== When used, it adds a _Link_ header to the reponse with the calculated values for pagination, and X-Total-Count containing the total results like this: my $params = $c->validation->output; my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->add_pagination_headers({ total => $count, params => $params )}; To test: - Run: $ sudo koha-shell kohadev k$ cd kohaclone k$ prove t/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: ByWater solutions Sponsored-by: Camden County Edit: I fixed a mistake on the POD (tcohen) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 67340 [details] [review] Bug 19196: Make plugin available to endpoints This patch just initializes the plugin on the main controller class so it is available for all endpoints to use. As it is not used, in order to test just restart plack and make sure the endpoints work (run the t/db_dependent/api/v1 tests). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> (In reply to Martin Renvoize from comment #18) > I really like this.. only minor query would be can we document the decision > as to why we've chosen to do page based instead of cursor based result > 'paging'. I'm personally happy to go with either.. but to prevent FUD in > future developers I think it's worth documenting the decision process so we > can point to that when someone asks down the line ;) I think we could implement both pagination methods on a as-needed basis and there should be no conflict. A new set of helper can be added based on this work, with proper tests c&p-ed from the ones on this bug. Regarding the specifics of why I chose to start with page-based results paging, I'd say: - It looked pretty *natural* when you look at SQL::Abstract syntax for it. - It is what Github uses (https://developer.github.com/v3/#pagination) and I really like its simplicity. - Cursor based pagination is really harder to implement, and the use cases I'm tackling wouldn't benefit much from it (i.e. it'ss just vendor orders, not stuffs that change very often and pagination would become really difficult, in which case other artifacts would get involved like websockets/server side events to notify changes). Created attachment 67346 [details] [review] Bug 19196: Rename page and per_page This patch just renames 'page' and 'per_page' params, for '_page' and '_per_page' respectively. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 67366 [details] [review] Bug 19196: Unit tests This patch adds unit tests for the new pagination Mojo plugin. Sponsored-by: ByWater Solutions Sponsored-by: Camden County Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> Created attachment 67367 [details] [review] Bug 19196: Add Koha::REST::Plugin::Pagination This patch introduces a Mojolicious plugin to be used on the REST api. It adds a helper method: add_pagination_headers ====================== When used, it adds a _Link_ header to the reponse with the calculated values for pagination, and X-Total-Count containing the total results like this: my $params = $c->validation->output; my $patrons = Koha::Patrons->search; my $count = $patrons->count; $c->add_pagination_headers({ total => $count, params => $params )}; To test: - Run: $ sudo koha-shell kohadev k$ cd kohaclone k$ prove t/Koha/REST/Plugin/Pagination.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: ByWater solutions Sponsored-by: Camden County Edit: I fixed a mistake on the POD (tcohen) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> Created attachment 67368 [details] [review] Bug 19196: Make plugin available to endpoints This patch just initializes the plugin on the main controller class so it is available for all endpoints to use. As it is not used, in order to test just restart plack and make sure the endpoints work (run the t/db_dependent/api/v1 tests). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> Created attachment 67369 [details] [review] Bug 19196: Rename page and per_page This patch just renames 'page' and 'per_page' params, for '_page' and '_per_page' respectively. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Lari Taskula <lari.taskula@jns.fi> =head3 register =cut sub register { my ( $self, $app ) = @_; =head2 Helper methods This sub contains POD. Please adjust. (In reply to Marcel de Rooy from comment #28) > =head3 register > > =cut > > sub register { > my ( $self, $app ) = @_; > > =head2 Helper methods > > This sub contains POD. Please adjust. What do you suggest? I thought doing it 'inline' was better. Keep in mind that the 'register' sub is the only sub, and it contains the 'helpers' definitiions. The 'helpers' are the only important thing here. If you prefer, I can move them POD into a separate section like here: http://cpansearch.perl.org/src/JBERGER/Mojo-IOLoop-ForkCall-0.16/lib/Mojolicious/Plugin/ForkCall.pm but I still prefer this way. (In reply to Tomás Cohen Arazi from comment #29) > What do you suggest? I thought doing it 'inline' was better. Keep in mind > that the 'register' sub is the only sub, and it contains the 'helpers' > definitiions. The 'helpers' are the only important thing here. > If you prefer, I can move them POD into a separate section like here: > > http://cpansearch.perl.org/src/JBERGER/Mojo-IOLoop-ForkCall-0.16/lib/ > Mojolicious/Plugin/ForkCall.pm > > but I still prefer this way. OK. You have a reason. Personally I would not prefer it, but it should not be a blocker. Passing it back to RM for final decision. Pushed to master for 17.11, thanks to everybody involved! Error while loading /etc/koha/plack.psgi: Can't load application from file "api/v1/app.pl": Can't locate Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at (eval 1295) line 1. (In reply to Marcel de Rooy from comment #32) > Error while loading /etc/koha/plack.psgi: Can't load application from file > "api/v1/app.pl": Can't locate > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at > (eval 1295) line 1. Mojolicious version? (In reply to Tomás Cohen Arazi from comment #33) > (In reply to Marcel de Rooy from comment #32) > > Error while loading /etc/koha/plack.psgi: Can't load application from file > > "api/v1/app.pl": Can't locate > > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: > > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at > > (eval 1295) line 1. > > Mojolicious version? 7.21 Since commenting this line solves the problem: # $self->plugin( 'Koha::REST::Plugin::Pagination' ); I still hope to see something moving here ;) Anything with setting path for Mojo plugins or so? Mojo experts around? (In reply to Marcel de Rooy from comment #35) > Since commenting this line solves the problem: > # $self->plugin( 'Koha::REST::Plugin::Pagination' ); > I still hope to see something moving here ;) > Anything with setting path for Mojo plugins or so? Mojo experts around? For the record, I cannot reproduce. (In reply to Marcel de Rooy from comment #32) > Error while loading /etc/koha/plack.psgi: Can't load application from file > "api/v1/app.pl": Can't locate > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at > (eval 1295) line 1. Same issue here. Also 7.21 of Mojolicious. Commenting the line in Koha/REST/V1.pm works as a temporary fix here as well. (In reply to Stefan Berndtsson from comment #37) > (In reply to Marcel de Rooy from comment #32) > > Error while loading /etc/koha/plack.psgi: Can't load application from file > > "api/v1/app.pl": Can't locate > > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: > > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at > > (eval 1295) line 1. > > Same issue here. Also 7.21 of Mojolicious. Commenting the line in > Koha/REST/V1.pm works as a temporary fix here as well. Thanks for your confirmation of this issue. We might convince Tomas ;) (In reply to Marcel de Rooy from comment #38) > (In reply to Stefan Berndtsson from comment #37) > > (In reply to Marcel de Rooy from comment #32) > > > Error while loading /etc/koha/plack.psgi: Can't load application from file > > > "api/v1/app.pl": Can't locate > > > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: > > > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at > > > (eval 1295) line 1. > > > > Same issue here. Also 7.21 of Mojolicious. Commenting the line in > > Koha/REST/V1.pm works as a temporary fix here as well. > > Thanks for your confirmation of this issue. We might convince Tomas ;) I belive you! I just can't reproduce it (In reply to Marcel de Rooy from comment #32) > Error while loading /etc/koha/plack.psgi: Can't load application from file > "api/v1/app.pl": Can't locate > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at > (eval 1295) line 1. What happened? No idea, but (suddenly) I cannot reproduce this error any more. Glad to see that, but it is still intriguing. Hard to say what resolved the problem after a few weeks of merging master and commenting the plugin line in V1. If it comes up again, I will certainly tell you.. (In reply to Marcel de Rooy from comment #40) > (In reply to Marcel de Rooy from comment #32) > > Error while loading /etc/koha/plack.psgi: Can't load application from file > > "api/v1/app.pl": Can't locate > > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: > > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at > > (eval 1295) line 1. > > What happened? No idea, but (suddenly) I cannot reproduce this error any > more. Glad to see that, but it is still intriguing. Hard to say what > resolved the problem after a few weeks of merging master and commenting the > plugin line in V1. If it comes up again, I will certainly tell you.. This was dealth with on bug 19546 |