From 13e7cf6585094cec1591d2b948108c164a825eba 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 | 32 ++++++++++++++++++++++++++++++++ Koha/Hold.pm | 3 ++- Koha/REST/V1/Holds.pm | 4 +--- t/db_dependent/Koha/Biblio.t | 9 ++++++++- t/db_dependent/api/v1/holds.t | 8 ++++---- 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm index a94665ddc8..19b11bdb91 100644 --- a/Koha/Biblio.pm +++ b/Koha/Biblio.pm @@ -252,6 +252,38 @@ sub can_be_transferred { return 0; } +=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 diff --git a/Koha/Hold.pm b/Koha/Hold.pm index a5a9d03003..162635f39e 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -298,12 +298,13 @@ 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 e3957a7a67..4156c877d3 100755 --- a/t/db_dependent/Koha/Biblio.t +++ b/t/db_dependent/Koha/Biblio.t @@ -215,7 +215,7 @@ subtest 'is_serial() tests' => sub { }; subtest 'pickup_locations' => sub { - plan tests => 9; + plan tests => 73; $schema->storage->txn_begin; @@ -433,6 +433,13 @@ subtest 'pickup_locations' => 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.34.1