From a66628a150ab30f509f485857c42ce7b1d359559 Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
Date: Fri, 3 Dec 2021 16:45:21 -0300
Subject: [PATCH] Bug 29349: Do not assume holding branch is a valid pickup
 location

The original code for pickup locations when placing item-level holds
picked the currently logged-in library.

We made things more robust, as the logged-in library might not be a
valid pickup location for the patron and item. But it was wrongly chosen
to use the holding branch as the default.

A more robust approach is needed, and this precedence is picked this
time (it could be configuration-driven in the future):

    - Logged-in library
    - Empty

To test:
1. Pick a biblio with various valid pickup locations, some not including
   the logged-in library.
2. Pick a patron for placing the hold
=> FAIL: Notice that (when valid pickup location) the holding branch is
always chosen
3. Apply this patch
4. Repeat 2
=> SUCCESS: If valid pickup location, the logged-in branch is picked as
default for item-type level. When it is not, an empty dropdown is used
as a fallback.
5. Sign off :-D

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
---
 reserve/request.pl | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/reserve/request.pl b/reserve/request.pl
index 10fa575f11..5f52f47728 100755
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -586,13 +586,20 @@ foreach my $biblionumber (@biblionumbers) {
                   )
                 {
                     # Send the pickup locations count to the UI, the pickup locations will be pulled using the API
-                    my $pickup_locations = $item_object->pickup_locations({ patron => $patron });
-                    $item->{pickup_locations_count} = $pickup_locations->count;
-                    if ( $item->{pickup_locations_count} > 0 ) {
+                    my @pickup_locations = $item_object->pickup_locations({ patron => $patron })->as_list;
+                    $item->{pickup_locations_count} = scalar @pickup_locations;
+
+                    if ( @pickup_locations ) {
                         $num_available++;
                         $item->{available} = 1;
-                        # pass the holding branch for use as default
-                        my $default_pickup_location = $pickup_locations->search({ branchcode => $item->{holdingbranch} })->next;
+
+                        my $default_pickup_location;
+
+                        # Default to logged-in, if valid
+                        if ( C4::Context->userenv->{branch} ) {
+                            ($default_pickup_location) = grep { $_->branchcode eq C4::Context->userenv->{branch} } @pickup_locations;
+                        }
+
                         $item->{default_pickup_location} = $default_pickup_location;
                     }
                     else {
-- 
2.20.1