Attachment #153473 for bug #33471

View | Details | Raw Unified | Return to bug 33471
Collapse All | Expand All




}


=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 });

Lines 298-309 sub is_pickup_location_valid { Link Here

(-)a/Koha/Hold.pm (-1 / +3 lines)
298		298
299	if ( $self->itemnumber ) { # item-level	299	if ( $self->itemnumber ) { # item-level
300	$pickup_locations = $self->item->pickup_locations({ patron => $self->patron });	300	$pickup_locations = $self->item->pickup_locations({ patron => $self->patron });
		301	return any { $_->branchcode eq $params->{library_id} } $pickup_locations->as_list;
301	}	302	}
302	else { # biblio-level	303	else { # biblio-level
303	$pickup_locations = $self->biblio->pickup_locations({ patron => $self->patron });	304	$pickup_locations = $self->biblio->pickup_locations({ patron => $self->patron });
		305	return $self->biblio->validate_pickup_location(
		306	{ patron => $self->patron, pickup_location_id => $params->{library_id} } );
304	}	307	}
305		308
306	return any { $_->branchcode eq $params->{library_id} } $pickup_locations->as_list;
307	}	309	}
308		310
309	=head3 set_pickup_location	311	=head3 set_pickup_location

Lines 155-163 sub add { Link Here

(-)a/Koha/REST/V1/Holds.pm (-3 / +1 lines)
155	}	155	}
156	else {	156	else {
157	$valid_pickup_location =	157	$valid_pickup_location =
158	any { $_->branchcode eq $pickup_library_id }	158	$biblio->validate_pickup_location({ patron => $patron, pickup_location_id => $pickup_library_id });
159	$biblio->pickup_locations(
160	{ patron => $patron } )->as_list;
161	}	159	}
162		160
163	return $c->render(	161	return $c->render(





subtest 'pickup_locations() tests' => sub {

    plan tests => 75;

    $schema->storage->txn_begin;


              . ' 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') {




      ->status_is(201);

    # empty cases
    $mock_biblio->mock( 'validate_pickup_location', sub {
        return;
    });

    $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $biblio_hold_data )

      ->json_is({ error => 'The supplied pickup location is not valid' });

    # empty cases
    $mock_item->mock( 'validate_pickup_location', sub {
        return;
    });

    $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $item_hold_data )
- 

Return to bug 33471

Lines 217-223 subtest 'is_serial() tests' => sub { Link Here

(-)a/t/db_dependent/Koha/Biblio.t (-1 / +10 lines)
217		217
218	subtest 'pickup_locations() tests' => sub {	218	subtest 'pickup_locations() tests' => sub {
219		219
220	plan tests => 11;	220	plan tests => 75;
221		221
222	$schema->storage->txn_begin;	222	$schema->storage->txn_begin;
223		223
Lines 442-447 subtest 'pickup_locations() tests' => sub { Link Here
442	. ' but returns '	442	. ' but returns '
443	. scalar(@pl)	443	. scalar(@pl)
444	);	444	);
		445
		446	foreach my $branchcode (@branchcodes) {
		447	my $valid;
		448	$valid = 1 if grep { $branchcode eq $_ } @pl;
		449	is(
		450	$valid, $biblio->validate_pickup_location( { patron => $patron, pickup_location_id => $branchcode } ),
		451	"Validate pickup location returns the expected result"
		452	);
		453	}
445	}	454	}
446		455
447	foreach my $cbranch ('ItemHomeLibrary','PatronLibrary') {	456	foreach my $cbranch ('ItemHomeLibrary','PatronLibrary') {

Lines 1248-1255 subtest 'add() tests' => sub { Link Here

(-)a/t/db_dependent/api/v1/holds.t (-5 / +4 lines)
1248	->status_is(201);	1248	->status_is(201);
1249		1249
1250	# empty cases	1250	# empty cases
1251	$mock_biblio->mock( 'pickup_locations', sub {	1251	$mock_biblio->mock( 'validate_pickup_location', sub {
1252	return Koha::Libraries->new->empty;	1252	return;
1253	});	1253	});
1254		1254
1255	$t->post_ok( "//$userid:$password@/api/v1/holds" => json => $biblio_hold_data )	1255	$t->post_ok( "//$userid:$password@/api/v1/holds" => json => $biblio_hold_data )
Lines 1257-1264 subtest 'add() tests' => sub { Link Here
1257	->json_is({ error => 'The supplied pickup location is not valid' });	1257	->json_is({ error => 'The supplied pickup location is not valid' });
1258		1258
1259	# empty cases	1259	# empty cases
1260	$mock_item->mock( 'pickup_locations', sub {	1260	$mock_item->mock( 'validate_pickup_location', sub {
1261	return Koha::Libraries->new->empty;	1261	return;
1262	});	1262	});
1263		1263
1264	$t->post_ok( "//$userid:$password@/api/v1/holds" => json => $item_hold_data )	1264	$t->post_ok( "//$userid:$password@/api/v1/holds" => json => $item_hold_data )
1265	-

Lines 285-290 sub can_be_transferred { Link Here

(-)a/Koha/Biblio.pm (+35 lines)
285	}	285	}
286		286
287		287
		288	=head3 validate_pickup_location
		289
		290	$biblio->validate_pickup_location( {patron => $patron, pickup_location_id => $branchcode } );
		291
		292	Returns 1 if the pickup location is valid, nothing if it is not. This function is similar to, but distinct
		293	from pickup_locations in that it does not need to calculate all valid locations, but simply to check until
		294	1 item is found that validates the passed location. Rather than confusing the logic in that sub, some code
		295	is duplicated.
		296
		297	=cut
		298
		299	sub validate_pickup_location {
		300	my ( $self, $params ) = @_;
		301
		302	my $patron = $params->{patron};
		303	my $branchcode = $params->{pickup_location_id};
		304
		305	my %seen;
		306	foreach my $item ( $self->items->as_list ) {
		307
		308	# The 5 variables below determine the valid locations for any item, no need to check twice
		309	my $cache_key = sprintf "Pickup_locations:%s:%s:%s:%s:%s",
		310	$item->itype, $item->homebranch, $item->holdingbranch, $item->ccode \|\| "", $patron->branchcode \|\| "";
		311	next if $seen{$cache_key};
		312
		313	my @item_pickup_locations =
		314	$item->pickup_locations( { patron => $patron } )->_resultset->get_column('branchcode')->all;
		315	$seen{$cache_key} = 1;
		316
		317	return 1 if grep { $branchcode eq $_ } @item_pickup_locations;
		318	}
		319
		320	return;
		321	}
		322
288	=head3 pickup_locations	323	=head3 pickup_locations
289		324
290	my $pickup_locations = $biblio->pickup_locations({ patron => $patron });	325	my $pickup_locations = $biblio->pickup_locations({ patron => $patron });