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.
Created attachment 129048 [details] [review] Bug 29804: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
Nice work... no regressions, bug is fixed.. QA script happy.. PQA
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.03
Pushed to 21.05.x for 21.05.11
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.