Bug 29807

Summary: Branches template plugin doesn't handle empty lists correctly
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: TemplatesAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andreas.jonsson, andrewfh, fridolin.somers, kyle, martin.renvoize, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
The Branches TT plugin had wrong logic in it, that made it crash, or display wrong pickup locations when the item/biblio didn't have any valid pickup location.
Version(s) released in:
22.05.00,21.11.03,21.05.11
Bug Depends on:    
Bug Blocks: 29806    
Attachments: Bug 29807: Regression tests
Bug 29807: Make Branches plugin handle empty pickup locations list
Bug 29807: Regression tests
Bug 29807: Make Branches plugin handle empty pickup locations list
Bug 29807: Regression tests
Bug 29807: Make Branches plugin handle empty pickup locations list

Description Tomás Cohen Arazi 2022-01-06 11:49:14 UTC
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.
Comment 1 Tomás Cohen Arazi 2022-01-06 11:49:27 UTC
*** Bug 27970 has been marked as a duplicate of this bug. ***
Comment 2 Tomás Cohen Arazi 2022-01-06 11:59:45 UTC
Created attachment 129056 [details] [review]
Bug 29807: Regression tests
Comment 3 Tomás Cohen Arazi 2022-01-06 11:59:49 UTC
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
Comment 4 Owen Leonard 2022-01-06 13:24:26 UTC
Created attachment 129065 [details] [review]
Bug 29807: Regression tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Owen Leonard 2022-01-06 13:24:29 UTC
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>
Comment 6 Martin Renvoize 2022-01-06 13:36:26 UTC
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>
Comment 7 Martin Renvoize 2022-01-06 13:36:30 UTC
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>
Comment 8 Martin Renvoize 2022-01-06 13:37:02 UTC
Clear correction of code, covered by tests.. no regressions, tests passing, qa script happy

Passing QA
Comment 9 Fridolin Somers 2022-01-12 06:13:04 UTC
+    } else {
+        @libraries = Koha::Libraries->search( { pickup_location => 1 }, { order_by => ['branchname'] } )
+          unless @libraries;

Do we still need "unless @libraries" ?
Comment 10 Fridolin Somers 2022-01-19 22:16:26 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 11 Kyle M Hall 2022-02-07 11:39:41 UTC
Pushed to 21.11.x for 21.11.03
Comment 12 Andrew Fuerste-Henry 2022-02-16 17:45:42 UTC
Pushed to 21.05.x for 21.05.11
Comment 13 Victor Grousset/tuxayo 2022-02-21 08:52:11 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.