Bug 30526 - The pagination in the REST API does not work if $c->objects->search is called multiple times.
Summary: The pagination in the REST API does not work if $c->objects->search is called...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-13 14:06 UTC by Andreas Jonsson
Modified: 2023-01-12 00:08 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 30526: Copy parameters when building links in rest api. (833 bytes, patch)
2022-04-13 14:09 UTC, Andreas Jonsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Jonsson 2022-04-13 14:06:19 UTC
The REST-plugin Koha::REST::Plugin::Pagination defines two methods:
dbic_merge_pagination and add_pagination_headers.  The method
add_pagination_headers use the helper subroutine _build_link.  The last
call in add_pagination_headers looks like this:

            push @links,
                _build_link( $c,
                { page => $pages, per_page => $per_page, rel => 'last', params => $args->{params} } );

Here is _build_link:

sub _build_link {
    my ( $c, $args ) = @_;

    my $params = $args->{params};

    $params->{_page}     = $args->{page};
    $params->{_per_page} = $args->{per_page};


Note that the original parameters _page and _per_page is overwritten
here.  This means that _page will be $pages.  Subsequent calls to
$c->objects->search will, thus, always return the last page.

At least REST::V1::Holds::pickup_locations is affected by this
(because a redundant call to $c->objects->search is made if
AllowHoldPolicyOverride is activated).  This causes the dropdown menu with pickup locations in the holds view to only contain the last page of libraries if there are more than 20 pickup libraries and AllowHoldPolicyOverride is activated.
Comment 1 Andreas Jonsson 2022-04-13 14:09:26 UTC
Created attachment 133262 [details] [review]
Bug 30526: Copy parameters when building links in rest api.
Comment 2 Andreas Jonsson 2022-04-13 14:23:11 UTC
Test plan:

* Make sure you have at least 3 libraries that may be pickup locations.
* Activate the system preference AllowHoldPolicyOverride
* Change the system preference  RESTdefaultPageSize to 1
* Go to a record and make a place a hold.
* In the Holds view for the record, verify that all libraries appear in the pickup library drop down menu on the hold.
Comment 3 Tomás Cohen Arazi 2022-04-13 14:58:11 UTC
(In reply to Andreas Jonsson from comment #2)
> Test plan:
> 
> * Make sure you have at least 3 libraries that may be pickup locations.
> * Activate the system preference AllowHoldPolicyOverride
> * Change the system preference  RESTdefaultPageSize to 1
> * Go to a record and make a place a hold.
> * In the Holds view for the record, verify that all libraries appear in the
> pickup library drop down menu on the hold.

Hi, nice catch and really happy to see your patch.
I suggest you take a look at this files:

- t/Koha/REST/Plugin/Pagination.t
- t/db_dependent/api/v1/pagination.t

in those files, you should add regression tests at different levels.

In case you need it, ping me on IRC (tcohen) or just drop an email to koha-devel.
Comment 4 David Nind 2022-07-03 18:35:37 UTC
Patch still applies.

Changed status to Failed QA - see comments from Tomás in comment #3

Following the test plan, I noticed no difference in either of the dropdown lists (hold details area and place a hold on a specific item) when placing a hold - both before and after the patch is applied. I even added some additional libraries so that there were 22 where holds could be palced.
Comment 5 Andreas Jonsson 2022-07-04 14:37:42 UTC
It seems that the issue at hand have been accidentally fixed.

Let me propose a simpler test plan:

* Open up Koha/REST/Plugin/Query.pm and look at the subroutine _build_link
* Observe that the subroutine overwrites $args->{params}->{_page} and $args->{params}->{_per_page}
* Ponder for a second over how one can reasonable expect that these parameters remain unchanged for the duration of the request.
* Ponder for a second over how this unexpected modification of the parameters might have caused the issue at hand, how it might be causing other undiscovered issues and how it may cause issues with code yet to be written.
* Ponder for a second over how no sane programmer could have written any code which, outside of the subroutine _build_link, relies on this unexpected update of the parameters.
* Ponder for a second over how the proposed patch unburdens you from these concerns.