From 130c35b03667f843b858f5a76ff53ce1d2f3c99a Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Sun, 9 Apr 2023 20:06:05 +0000
Subject: [PATCH] Bug 33471: Add validate_pickup_location routine to
 Koha::Biblio

This patch adds a new routine, similar to pickup_locations except that
it short circuits once a location has been found as acceptable.

To test:
1 - Attempt placing a hold for an acceptable pickup location via the API
2 - Attempt to change pickup location to an invalid location, you are blocked
3 - Attempt to place a another hold via API for an invalid locatoin, you are blocked
4 - Apply patch
5 - Repeat
6 - Results should be the same

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
---
 Koha/Biblio.pm                | 35 +++++++++++++++++++++++++++++++++++
 Koha/Hold.pm                  |  4 +++-
 Koha/REST/V1/Holds.pm         |  4 +---
 t/db_dependent/Koha/Biblio.t  | 11 ++++++++++-
 t/db_dependent/api/v1/holds.t |  8 ++++----
 5 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm
index 26dc83f2d4..82f586c8d6 100644
--- a/Koha/Biblio.pm
+++ b/Koha/Biblio.pm
@@ -285,6 +285,41 @@ sub can_be_transferred {
 }
 
 
+=head3 validate_pickup_location
+
+    $biblio->validate_pickup_location( {patron => $patron, pickup_location_id => $branchcode } );
+
+Returns 1 if the pickup location is valid, nothing if it is not. This function is similar to, but distinct
+from pickup_locations in that it does not need to calculate all valid locations, but simply to check until
+1 item is found that validates the passed location. Rather than confusing the logic in that sub, some code
+is duplicated.
+
+=cut
+
+sub validate_pickup_location {
+    my ( $self, $params ) = @_;
+
+    my $patron     = $params->{patron};
+    my $branchcode = $params->{pickup_location_id};
+
+    my %seen;
+    foreach my $item ( $self->items->as_list ) {
+
+        # The 5 variables below determine the valid locations for any item, no need to check twice
+        my $cache_key = sprintf "Pickup_locations:%s:%s:%s:%s:%s",
+            $item->itype, $item->homebranch, $item->holdingbranch, $item->ccode || "", $patron->branchcode || "";
+        next if $seen{$cache_key};
+
+        my @item_pickup_locations =
+            $item->pickup_locations( { patron => $patron } )->_resultset->get_column('branchcode')->all;
+        $seen{$cache_key} = 1;
+
+        return 1 if grep { $branchcode eq $_ } @item_pickup_locations;
+    }
+
+    return;
+}
+
 =head3 pickup_locations
 
     my $pickup_locations = $biblio->pickup_locations({ patron => $patron });
diff --git a/Koha/Hold.pm b/Koha/Hold.pm
index f275530d97..bd614035fe 100644
--- a/Koha/Hold.pm
+++ b/Koha/Hold.pm
@@ -298,12 +298,14 @@ sub is_pickup_location_valid {
 
     if ( $self->itemnumber ) { # item-level
         $pickup_locations = $self->item->pickup_locations({ patron => $self->patron });
+        return any { $_->branchcode eq $params->{library_id} } $pickup_locations->as_list;
     }
     else { # biblio-level
         $pickup_locations = $self->biblio->pickup_locations({ patron => $self->patron });
+        return $self->biblio->validate_pickup_location(
+            { patron => $self->patron, pickup_location_id => $params->{library_id} } );
     }
 
-    return any { $_->branchcode eq $params->{library_id} } $pickup_locations->as_list;
 }
 
 =head3 set_pickup_location
diff --git a/Koha/REST/V1/Holds.pm b/Koha/REST/V1/Holds.pm
index a8a998ef12..7c55b1e635 100644
--- a/Koha/REST/V1/Holds.pm
+++ b/Koha/REST/V1/Holds.pm
@@ -155,9 +155,7 @@ sub add {
             }
             else {
                 $valid_pickup_location =
-                  any { $_->branchcode eq $pickup_library_id }
-                $biblio->pickup_locations(
-                    { patron => $patron } )->as_list;
+                  $biblio->validate_pickup_location({ patron => $patron, pickup_location_id  => $pickup_library_id });
             }
 
             return $c->render(
diff --git a/t/db_dependent/Koha/Biblio.t b/t/db_dependent/Koha/Biblio.t
index 4bc53195ec..235c63dff9 100755
--- a/t/db_dependent/Koha/Biblio.t
+++ b/t/db_dependent/Koha/Biblio.t
@@ -217,7 +217,7 @@ subtest 'is_serial() tests' => sub {
 
 subtest 'pickup_locations() tests' => sub {
 
-    plan tests => 11;
+    plan tests => 75;
 
     $schema->storage->txn_begin;
 
@@ -442,6 +442,15 @@ subtest 'pickup_locations() tests' => sub {
               . ' but returns '
               . scalar(@pl)
         );
+
+        foreach my $branchcode (@branchcodes) {
+            my $valid;
+            $valid = 1 if grep { $branchcode eq $_ } @pl;
+            is(
+                $valid, $biblio->validate_pickup_location( { patron => $patron, pickup_location_id => $branchcode } ),
+                "Validate pickup location returns the expected result"
+            );
+        }
     }
 
     foreach my $cbranch ('ItemHomeLibrary','PatronLibrary') {
diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t
index adc5429660..0e8d2c946b 100755
--- a/t/db_dependent/api/v1/holds.t
+++ b/t/db_dependent/api/v1/holds.t
@@ -1248,8 +1248,8 @@ subtest 'add() tests' => sub {
       ->status_is(201);
 
     # empty cases
-    $mock_biblio->mock( 'pickup_locations', sub {
-        return Koha::Libraries->new->empty;
+    $mock_biblio->mock( 'validate_pickup_location', sub {
+        return;
     });
 
     $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $biblio_hold_data )
@@ -1257,8 +1257,8 @@ subtest 'add() tests' => sub {
       ->json_is({ error => 'The supplied pickup location is not valid' });
 
     # empty cases
-    $mock_item->mock( 'pickup_locations', sub {
-        return Koha::Libraries->new->empty;
+    $mock_item->mock( 'validate_pickup_location', sub {
+        return;
     });
 
     $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $item_hold_data )
-- 
2.30.2