The current code, given an 'item' or a 'biblio' param, makes 'pickup_locations' set @pickup_locations an empty list. After that, there's a check for emptiness, in which case it populates the @libraries variable with ALL PICKUP LOCATIONS, which is totally wrong! This is a bug introduced by bug 24350.
*** Bug 27970 has been marked as a duplicate of this bug. ***
Created attachment 129056 [details] [review] Bug 29807: Regression tests
Created attachment 129057 [details] [review] Bug 29807: Make Branches plugin handle empty pickup locations list This patch makes the plugin handle empty Koha::Biblio->pickup_locations and Koha::Item->pickup_locations correctly. It does so by explicitly calling ->as_list. It also restores the logic that was changed by 6cd1ffab4e491349c99769018a7df33dc8a8aabf so, now that ->empty is handled correctly, it doesn't return ALL the pickup locations when the item/biblio doesn't have valid pickup locations. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Branches.t => FAIL: Awful error 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D
Created attachment 129065 [details] [review] Bug 29807: Regression tests Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 129066 [details] [review] Bug 29807: Make Branches plugin handle empty pickup locations list This patch makes the plugin handle empty Koha::Biblio->pickup_locations and Koha::Item->pickup_locations correctly. It does so by explicitly calling ->as_list. It also restores the logic that was changed by 6cd1ffab4e491349c99769018a7df33dc8a8aabf so, now that ->empty is handled correctly, it doesn't return ALL the pickup locations when the item/biblio doesn't have valid pickup locations. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Branches.t => FAIL: Awful error 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 129080 [details] [review] Bug 29807: Regression tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 129081 [details] [review] Bug 29807: Make Branches plugin handle empty pickup locations list This patch makes the plugin handle empty Koha::Biblio->pickup_locations and Koha::Item->pickup_locations correctly. It does so by explicitly calling ->as_list. It also restores the logic that was changed by 6cd1ffab4e491349c99769018a7df33dc8a8aabf so, now that ->empty is handled correctly, it doesn't return ALL the pickup locations when the item/biblio doesn't have valid pickup locations. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/Template/Plugin/Branches.t => FAIL: Awful error 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Clear correction of code, covered by tests.. no regressions, tests passing, qa script happy Passing QA
+ } else { + @libraries = Koha::Libraries->search( { pickup_location => 1 }, { order_by => ['branchname'] } ) + unless @libraries; Do we still need "unless @libraries" ?
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
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.