Bug 29807 - Branches template plugin doesn't handle empty lists correctly
Summary: Branches template plugin doesn't handle empty lists correctly
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
: 27970 (view as bug list)
Depends on:
Blocks: 29806
  Show dependency treegraph
 
Reported: 2022-01-06 11:49 UTC by Tomás Cohen Arazi
Modified: 2022-02-21 08:52 UTC (History)
6 users (show)

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


Attachments
Bug 29807: Regression tests (1.97 KB, patch)
2022-01-06 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29807: Make Branches plugin handle empty pickup locations list (2.85 KB, patch)
2022-01-06 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29807: Regression tests (2.02 KB, patch)
2022-01-06 13:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29807: Make Branches plugin handle empty pickup locations list (2.90 KB, patch)
2022-01-06 13:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29807: Regression tests (2.09 KB, patch)
2022-01-06 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29807: Make Branches plugin handle empty pickup locations list (2.96 KB, patch)
2022-01-06 13:36 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 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.