| Summary: | The pagination in the REST API does not work if $c->objects->search is called multiple times. | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Andreas Jonsson <andreas.jonsson> |
| Component: | REST API | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Status: | Failed QA --- | QA Contact: | |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | david, tomascohen |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: | Bug 30526: Copy parameters when building links in rest api. | ||
Created attachment 133262 [details] [review] Bug 30526: Copy parameters when building links in rest api. 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. (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. 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. 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.
|
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.