Bug 29804 - Koha::Hold->is_pickup_location_valid explodes if empty list of pickup locations
Summary: Koha::Hold->is_pickup_location_valid explodes if empty list of pickup locations
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 29806
  Show dependency treegraph
 
Reported: 2022-01-06 10:09 UTC by Tomás Cohen Arazi
Modified: 2022-02-19 03:08 UTC (History)
7 users (show)

See Also:
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


Attachments
Bug 29804: Regression tests (2.62 KB, patch)
2022-01-06 10:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding (2.00 KB, patch)
2022-01-06 10:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29804: Regression tests (2.67 KB, patch)
2022-01-06 11:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding (2.04 KB, patch)
2022-01-06 11:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 29804: Regression tests (2.73 KB, patch)
2022-01-06 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29804: Fix Koha::Hold->is_pickup_location_valid exploding (2.11 KB, patch)
2022-01-06 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.