From 1697c8adcf942ca5a9fdf69fc41fd16814648d86 Mon Sep 17 00:00:00 2001
From: Thibaud Guillot <thibaud.guillot@biblibre.com>
Date: Mon, 5 Feb 2024 12:59:06 +0100
Subject: [PATCH] Bug 35573: Revert "Bug 30687: Allow pickup location to be
 forced when override is allowed"

This reverts commit ab93008da7eb61b697e4586c679b88c2eebaacd0.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
(cherry picked from commit 33da81f9feb145e357647d456ae3bad0644f6963)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
(cherry picked from commit aa149ff575e43c2662d21268d7119906a3e2a938)
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
---
 .../prog/en/modules/reserve/request.tt        |  4 +-
 reserve/request.pl                            | 52 ++++++++++++-------
 2 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
index f45868f7fd..67f854104d 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ -789,7 +789,7 @@
                                                     [% END # /IF force_hold_level %]
                                                 </td>
                                                 <td>
-                                                [% IF (itemloo.pickup_locations_count > 0) || itemloo.override %]
+                                                [% IF (itemloo.pickup_locations_count > 0) %]
                                                     <select name="item_pickup_[% itemloo.itemnumber | html %]" class="pickup_locations" style="width:100%;"
                                                             data-item-id="[% itemloo.itemnumber | html %]"
                                                             data-patron-id="[% patron.borrowernumber | html %]"
@@ -1507,7 +1507,7 @@
                     msg = (_("- Please select an item to place a hold") + "\n");
                 }
             } else {
-                if( $("#pickup").length < 1 || $("#pickup").val() == "" || $('#pickup').val() === null ){
+                if( $("#pickup").length < 1 || $("#pickup").val() == "" ){
                     msg = _("- Please select a pickup location for this hold" + "\n");
                 }
             }
diff --git a/reserve/request.pl b/reserve/request.pl
index f9f18aa62f..f68f46762f 100755
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -500,19 +500,13 @@ if (   ( $findborrower && $borrowernumber_hold || $findclub && $club_hold )
                         $default_pickup_branch = C4::Context->userenv->{branch};
                     }
 
-                    if ( $can_item_be_reserved eq 'itemAlreadyOnHold' ) {
-                        # itemAlreadyOnHold cannot be overridden
-                        $num_alreadyheld++
-                    }
-                    elsif (
-                        (
-                               !$item->{cantreserve}
-                            && !$exceeded_maxreserves
-                            && $can_item_be_reserved eq 'OK'
-                            && IsAvailableForItemLevelRequest($item_object, $patron, undef)
-                        ) || C4::Context->preference('AllowHoldPolicyOverride')
-                             # If AllowHoldPolicyOverride is set, it overrides EVERY restriction
-                             # not just branch item rules
+                    if (
+                           !$item->{cantreserve}
+                        && !$exceeded_maxreserves
+                        && $can_item_be_reserved eq 'OK'
+                        # items_any_available defined outside of the current loop,
+                        # so we avoiding loop inside IsAvailableForItemLevelRequest:
+                        && IsAvailableForItemLevelRequest($item_object, $patron, undef)
                       )
                     {
                         # Send the pickup locations count to the UI, the pickup locations will be pulled using the API
@@ -525,17 +519,37 @@ if (   ( $findborrower && $borrowernumber_hold || $findclub && $club_hold )
                             my $default_pickup_location = $pickup_locations->search({ branchcode => $default_pickup_branch })->next;
                             $item->{default_pickup_location} = $default_pickup_location;
                         }
-                        elsif ( C4::Context->preference('AllowHoldPolicyOverride') ){
-                             $num_items_available++;
-                             $item->{override} = 1;
-                            my $default_pickup_location = $pickup_locations->search({ branchcode => $default_pickup_branch })->next;
-                            $item->{default_pickup_location} = $default_pickup_location;
-                        }
                         else {
                             $item->{available} = 0;
                             $item->{not_holdable} = "no_valid_pickup_location";
                         }
 
+                        push( @available_itemtypes, $item->{itype} );
+                    }
+                    elsif ( C4::Context->preference('AllowHoldPolicyOverride') ) {
+                        # If AllowHoldPolicyOverride is set, it should override EVERY restriction, not just branch item rules
+                        # with the exception of itemAlreadyOnHold because, you know, the item is already on hold
+                        if ( $can_item_be_reserved ne 'itemAlreadyOnHold' ) {
+                            # 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 })->as_list;
+                            $item->{pickup_locations_count} = scalar @pickup_locations;
+
+                            if ( @pickup_locations ) {
+                                $num_items_available++;
+                                $item->{override} = 1;
+
+                                my $default_pickup_location;
+
+                                ($default_pickup_location) = grep { $_->branchcode eq $default_pickup_branch } @pickup_locations;
+
+                                $item->{default_pickup_location} = $default_pickup_location;
+                            }
+                            else {
+                                $item->{available} = 0;
+                                $item->{not_holdable} = "no_valid_pickup_location";
+                            }
+                        } else { $num_alreadyheld++ }
+
                         push( @available_itemtypes, $item->{itype} );
                     } else {
                         # If none of the conditions hold true, then neither override nor available is set and the item cannot be checked
-- 
2.39.5