Bug 29804

Summary: Koha::Hold->is_pickup_location_valid explodes if empty list of pickup locations
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrewfh, jonathan.druart+koha, kyle, lucas, martin.renvoize, nick, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.03,21.05.11
Bug Depends on:    
Bug Blocks: 29806    
Attachments: Bug 29804: Regression tests
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding
Bug 29804: Regression tests
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding
Bug 29804: Regression tests
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding

Description Tomás Cohen Arazi 2022-01-06 10:09:34 UTC
Due to bug 28871, there's a problem in production when checking if a pickup location is valid using Koha::Hold->is_pickup_location_valid.

I file this bug because the other one is blocked and is not clear what path it will take, but this is affecting users.
Comment 1 Tomás Cohen Arazi 2022-01-06 10:19:11 UTC
Created attachment 129048 [details] [review]
Bug 29804: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2022-01-06 10:19:14 UTC
Created attachment 129049 [details] [review]
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding

This trivial patch acknowledges the fact bug 28871 is probably not going
to be pushed, and changes the method so  it, internally, uses
Koha::Item->pickup_locations and Koha::Biblio->pickup_locations in
scalar context.

This is probably the correct solution as the discussion on bug 28883
points towards the future removal of the use of wantarray altogether,
eventually.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> FAIL: It explodes as we see in production, with:
The method Koha::Libraries->branchcode is not covered by tests!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Nind 2022-01-06 11:51:44 UTC
Created attachment 129054 [details] [review]
Bug 29804: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2022-01-06 11:51:49 UTC
Created attachment 129055 [details] [review]
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding

This trivial patch acknowledges the fact bug 28871 is probably not going
to be pushed, and changes the method so  it, internally, uses
Koha::Item->pickup_locations and Koha::Biblio->pickup_locations in
scalar context.

This is probably the correct solution as the discussion on bug 28883
points towards the future removal of the use of wantarray altogether,
eventually.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> FAIL: It explodes as we see in production, with:
The method Koha::Libraries->branchcode is not covered by tests!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Martin Renvoize 2022-01-06 13:33:21 UTC
Created attachment 129078 [details] [review]
Bug 29804: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-01-06 13:33:25 UTC
Created attachment 129079 [details] [review]
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding

This trivial patch acknowledges the fact bug 28871 is probably not going
to be pushed, and changes the method so  it, internally, uses
Koha::Item->pickup_locations and Koha::Biblio->pickup_locations in
scalar context.

This is probably the correct solution as the discussion on bug 28883
points towards the future removal of the use of wantarray altogether,
eventually.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> FAIL: It explodes as we see in production, with:
The method Koha::Libraries->branchcode is not covered by tests!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2022-01-06 13:34:01 UTC
Nice work... no regressions, bug is fixed.. QA script happy..

PQA
Comment 8 Fridolin Somers 2022-01-18 00:56:41 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 9 Kyle M Hall 2022-02-04 18:59:59 UTC
Pushed to 21.11.x for 21.11.03
Comment 10 Andrew Fuerste-Henry 2022-02-11 12:31:29 UTC
Pushed to 21.05.x for 21.05.11
Comment 11 Victor Grousset/tuxayo 2022-02-19 03:08:09 UTC
What would the scenario to trigger this bug?

The fix rely on bug 27131 which isn't in 20.11.x so that make things hard. (can't backport 27131) And raise the question of the 20.11.x being affected or not.

So, can't backport to 20.11.x.
If there is an interest in having this backported, please confirm validity or help to check for it as a first step.