From 130c35b03667f843b858f5a76ff53ce1d2f3c99a Mon Sep 17 00:00:00 2001 From: Nick Clemens 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 --- 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