Bug 29806

Summary: ->pickup_locations should always be called in scalar context
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P5 - low CC: andrewfh, jonathan.druart, katrin.fischer, kyle, martin.renvoize, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
The Koha::Biblio->pickup_locations and Koha::Item->pickup_location methods don't always honour list context. Because of this, when used, they should assume scalar context. If list context was required, the developer needs to explicitly chain a ->as_list call. This patch tracks the uses of this methods and adjusts accordingly.
Version(s) released in:
22.05.00,21.11.03,21.05.11
Bug Depends on: 29804, 29807    
Bug Blocks: 29844    
Attachments: Bug 29806: Call ->as_list for ->pickup_locations on request.pl
Bug 29806: Regression tests
Bug 29806: Fix POST /holds use of pickup_locations
Bug 29806: Regression tests
Bug 29806: Call ->as_list for ->pickup_locations on request.pl
Bug 29806: Fix POST /holds use of pickup_locations
Bug 29806: Regression tests
Bug 29806: Call ->as_list for ->pickup_locations on request.pl
Bug 29806: Fix POST /holds use of pickup_locations
Bug 29806: (follow-up) Call ->as_list for ->pickup_locations on request.pl

Description Tomás Cohen Arazi 2022-01-06 10:45:35 UTC
The Koha::{Biblio|Item}->pickup_locations methods internally use Koha::Libraries->empty, which doesn't honour list context (bug 28871). When called in list context, if the Koha::Libraries resultset was built using ->empty, then things will explode, as exposed by bug 29804.
Comment 1 Tomás Cohen Arazi 2022-01-06 10:54:42 UTC
I did a:

$ git grep '\->pickup_locations'

and my (manual) findings are:
Koha/Biblio.pm: ok
Koha/Biblios.pm: ok
Koha/Item.pm: ok
Koha/REST/V1/Biblios.pm: ok
Koha/REST/V1/Holds.pm: boo
Koha/Template/Plugin/Branches.pm: boo
reserve/request.pl: boo
t/db_dependent/Koha/Biblio.t: ok
t/db_dependent/Koha/Biblios.t: ok
t/db_dependent/Koha/Hold.t: ok
t/db_dependent/Koha/Item.t: ok
t/db_dependent/Template/Plugin/Branches.t: ok
t/db_dependent/api/v1/holds.t: ok

Working on fixes...
Comment 2 Tomás Cohen Arazi 2022-01-06 12:03:45 UTC
Koha/Template/Plugin/Branches.pm solved on bug 29807.
Comment 3 Tomás Cohen Arazi 2022-01-06 12:32:29 UTC
Created attachment 129058 [details] [review]
Bug 29806: Call ->as_list for ->pickup_locations on request.pl
Comment 4 Tomás Cohen Arazi 2022-01-06 12:32:33 UTC
Created attachment 129059 [details] [review]
Bug 29806: Regression tests
Comment 5 Tomás Cohen Arazi 2022-01-06 12:32:37 UTC
Created attachment 129060 [details] [review]
Bug 29806: Fix POST /holds use of pickup_locations

This patch makes Koha::Item->pickup_locations and
Koha::Biblio->pickup_locations explicitly call ->as_list in list
context. This way we workaround bug 28871.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail, the route gives a 500 (unhandled exception)
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass, the route correctly returns a 400 with a
message about the pickup location being invalid
5. Sign off :-D
Comment 6 David Nind 2022-01-07 07:48:59 UTC
Created attachment 129130 [details] [review]
Bug 29806: Regression tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-01-07 07:49:04 UTC
Created attachment 129131 [details] [review]
Bug 29806: Call ->as_list for ->pickup_locations on request.pl

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-01-07 07:49:10 UTC
Created attachment 129132 [details] [review]
Bug 29806: Fix POST /holds use of pickup_locations

This patch makes Koha::Item->pickup_locations and
Koha::Biblio->pickup_locations explicitly call ->as_list in list
context. This way we workaround bug 28871.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail, the route gives a 500 (unhandled exception)
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass, the route correctly returns a 400 with a
message about the pickup location being invalid
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Jonathan Druart 2022-01-07 14:54:42 UTC
Created attachment 129183 [details] [review]
Bug 29806: Regression tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2022-01-07 14:54:46 UTC
Created attachment 129184 [details] [review]
Bug 29806: Call ->as_list for ->pickup_locations on request.pl

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2022-01-07 14:54:50 UTC
Created attachment 129185 [details] [review]
Bug 29806: Fix POST /holds use of pickup_locations

This patch makes Koha::Item->pickup_locations and
Koha::Biblio->pickup_locations explicitly call ->as_list in list
context. This way we workaround bug 28871.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail, the route gives a 500 (unhandled exception)
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass, the route correctly returns a 400 with a
message about the pickup location being invalid
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2022-01-07 14:54:55 UTC
Created attachment 129186 [details] [review]
Bug 29806: (follow-up) Call ->as_list for ->pickup_locations on request.pl

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Tomás Cohen Arazi 2022-01-07 18:20:10 UTC
Hi! Thanks for the follow-up. Strictly speaking, Koha::Biblios->pickup_locations IS list context aware so this wouldn't be needed. I skipped it because of that reason, but that was before I wrote to koha-devel with my suggestion that we call ->as_list explicitly all the time.

So, this patch is welcome and good to be included!

(In reply to Jonathan Druart from comment #12)
> Created attachment 129186 [details] [review] [review]
> Bug 29806: (follow-up) Call ->as_list for ->pickup_locations on request.pl
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Fridolin Somers 2022-01-19 22:16:21 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 15 Kyle M Hall 2022-02-07 11:46:08 UTC
Pushed to 21.11.x for 21.11.03
Comment 16 Andrew Fuerste-Henry 2022-02-16 20:39:46 UTC
Pushed to 21.05.x for 21.05.11
Comment 17 Victor Grousset/tuxayo 2022-02-21 08:56:06 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.