@@ -, +, @@ Koha::Biblio --- 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(-) --- a/Koha/Biblio.pm +++ a/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 }); --- a/Koha/Hold.pm +++ a/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 --- a/Koha/REST/V1/Holds.pm +++ a/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( --- a/t/db_dependent/Koha/Biblio.t +++ a/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') { --- a/t/db_dependent/api/v1/holds.t +++ a/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 ) --