From 95bdbb1340d5fb3135696ca9b7f460adf03ac7e3 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 14 Dec 2020 10:19:05 -0300 Subject: [PATCH] Bug 27205: Check valid pickup location on POST /holds This patch adds a check for valid pickup location to the POST /holds route. A 400 code is returned if the supplied pickup library is not valid. To test: 1. Apply this patch 2. Run; $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Victor Grousset/tuxayo --- Koha/REST/V1/Holds.pm | 26 ++++++- t/db_dependent/api/v1/holds.t | 137 +++++++++++++++++++++++++++++++++- 2 files changed, 160 insertions(+), 3 deletions(-) diff --git a/Koha/REST/V1/Holds.pm b/Koha/REST/V1/Holds.pm index 0a9cfb93dc..27f20996ce 100644 --- a/Koha/REST/V1/Holds.pm +++ b/Koha/REST/V1/Holds.pm @@ -27,6 +27,7 @@ use Koha::Patrons; use Koha::Holds; use Koha::DateUtils; +use List::MoreUtils qw(any); use Try::Tiny; =head1 API @@ -65,6 +66,7 @@ sub add { my $body = $c->validation->param('body'); my $biblio; + my $item; my $biblio_id = $body->{biblio_id}; my $pickup_library_id = $body->{pickup_library_id}; @@ -99,7 +101,7 @@ sub add { } } elsif ($item_id) { - my $item = Koha::Items->find($item_id); + $item = Koha::Items->find($item_id); unless ($item) { return $c->render( @@ -136,6 +138,28 @@ sub add { ); } + # Validate pickup location + my $valid_pickup_location; + if ($item) { # item-level hold + $valid_pickup_location = + any { $_->branchcode eq $pickup_library_id } + $item->pickup_locations( + { patron => $patron } ); + } + else { + $valid_pickup_location = + any { $_->branchcode eq $pickup_library_id } + $biblio->pickup_locations( + { patron => $patron } ); + } + + return $c->render( + status => 400, + openapi => { + error => 'The supplied pickup location is not valid' + } + ) unless $valid_pickup_location; + my $can_place_hold = $item_id ? C4::Reserves::CanItemBeReserved( $patron_id, $item_id ) diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t index 6b977ced64..6d692d576d 100755 --- a/t/db_dependent/api/v1/holds.t +++ b/t/db_dependent/api/v1/holds.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 11; +use Test::More tests => 12; use Test::MockModule; use Test::Mojo; use t::lib::TestBuilder; @@ -238,9 +238,16 @@ subtest "Test endpoints with permission" => sub { ->status_is(204, 'SWAGGER3.2.4') ->content_is('', 'SWAGGER3.3.4'); + # Make sure pickup location checks doesn't get in the middle + my $mock_biblio = Test::MockModule->new('Koha::Biblio'); + $mock_biblio->mock( 'pickup_locations', sub { return Koha::Libraries->search; }); + my $mock_item = Test::MockModule->new('Koha::Item'); + $mock_item->mock( 'pickup_locations', sub { return Koha::Libraries->search }); + $t->post_ok( "//$userid_3:$password@/api/v1/holds" => json => $post_data ) ->status_is(201) ->json_has('/hold_id'); + # Get id from response $reserve_id = $t->tx->res->json->{hold_id}; @@ -290,6 +297,12 @@ subtest 'Reserves with itemtype' => sub { ->status_is(204, 'SWAGGER3.2.4') ->content_is('', 'SWAGGER3.3.4'); + # Make sure pickup location checks doesn't get in the middle + my $mock_biblio = Test::MockModule->new('Koha::Biblio'); + $mock_biblio->mock( 'pickup_locations', sub { return Koha::Libraries->search; }); + my $mock_item = Test::MockModule->new('Koha::Item'); + $mock_item->mock( 'pickup_locations', sub { return Koha::Libraries->search }); + $t->post_ok( "//$userid_3:$password@/api/v1/holds" => json => $post_data ) ->status_is(201) ->json_has('/hold_id'); @@ -329,6 +342,12 @@ subtest 'test AllowHoldDateInFuture' => sub { t::lib::Mocks::mock_preference( 'AllowHoldDateInFuture', 1 ); + # Make sure pickup location checks doesn't get in the middle + my $mock_biblio = Test::MockModule->new('Koha::Biblio'); + $mock_biblio->mock( 'pickup_locations', sub { return Koha::Libraries->search; }); + my $mock_item = Test::MockModule->new('Koha::Item'); + $mock_item->mock( 'pickup_locations', sub { return Koha::Libraries->search }); + $t->post_ok( "//$userid_3:$password@/api/v1/holds" => json => $post_data ) ->status_is(201) ->json_is('/hold_date', output_pref({ dt => $future_hold_date, dateformat => 'rfc3339', dateonly => 1 })); @@ -345,13 +364,19 @@ subtest 'test AllowHoldPolicyOverride' => sub { itemtype => undef, branchcode => undef, rules => { - holdallowed => 1 + holdallowed => 1 } } ); t::lib::Mocks::mock_preference( 'AllowHoldPolicyOverride', 0 ); + # Make sure pickup location checks doesn't get in the middle + my $mock_biblio = Test::MockModule->new('Koha::Biblio'); + $mock_biblio->mock( 'pickup_locations', sub { return Koha::Libraries->search; }); + my $mock_item = Test::MockModule->new('Koha::Item'); + $mock_item->mock( 'pickup_locations', sub { return Koha::Libraries->search }); + $t->post_ok( "//$userid_3:$password@/api/v1/holds" => json => $post_data ) ->status_is(403) ->json_has('/error'); @@ -606,6 +631,12 @@ subtest 'add() tests (maxreserves behaviour)' => sub { my $biblio_3 = $builder->build_sample_biblio; my $item_3 = $builder->build_sample_item({ biblionumber => $biblio_3->biblionumber }); + # Make sure pickup location checks doesn't get in the middle + my $mock_biblio = Test::MockModule->new('Koha::Biblio'); + $mock_biblio->mock( 'pickup_locations', sub { return Koha::Libraries->search; }); + my $mock_item = Test::MockModule->new('Koha::Item'); + $mock_item->mock( 'pickup_locations', sub { return Koha::Libraries->search }); + # Disable logging t::lib::Mocks::mock_preference( 'HoldsLog', 0 ); t::lib::Mocks::mock_preference( 'RESTBasicAuth', 1 ); @@ -868,3 +899,105 @@ subtest 'edit() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'add() tests' => sub { + + plan tests => 10; + + $schema->storage->txn_begin; + + my $password = 'AbcdEFG123'; + + my $library = $builder->build_object({ class => 'Koha::Libraries' }); + my $patron = $builder->build_object( + { class => 'Koha::Patrons', value => { flags => 1 } } ); + $patron->set_password( { password => $password, skip_validation => 1 } ); + my $userid = $patron->userid; + $builder->build( + { + source => 'UserPermission', + value => { + borrowernumber => $patron->borrowernumber, + module_bit => 6, + code => 'modify_holds_priority', + }, + } + ); + + # Disable logging + t::lib::Mocks::mock_preference( 'HoldsLog', 0 ); + t::lib::Mocks::mock_preference( 'RESTBasicAuth', 1 ); + + my $mock_biblio = Test::MockModule->new('Koha::Biblio'); + my $mock_item = Test::MockModule->new('Koha::Item'); + + my $library_1 = $builder->build_object({ class => 'Koha::Libraries' }); + my $library_2 = $builder->build_object({ class => 'Koha::Libraries' }); + my $library_3 = $builder->build_object({ class => 'Koha::Libraries' }); + + # let's control what Koha::Biblio->pickup_locations returns, for testing + $mock_biblio->mock( 'pickup_locations', sub { + return Koha::Libraries->search( { branchcode => [ $library_2->branchcode, $library_3->branchcode ] } ); + }); + # let's mock what Koha::Item->pickup_locations returns, for testing + $mock_item->mock( 'pickup_locations', sub { + return Koha::Libraries->search( { branchcode => [ $library_2->branchcode, $library_3->branchcode ] } ); + }); + + my $biblio = $builder->build_sample_biblio; + my $item = $builder->build_sample_item({ biblionumber => $biblio->biblionumber }); + + # Test biblio-level holds + my $biblio_hold = $builder->build_object( + { + class => "Koha::Holds", + value => { + biblionumber => $biblio->biblionumber, + branchcode => $library_3->branchcode, + itemnumber => undef, + priority => 1, + } + } + ); + + my $biblio_hold_data = $biblio_hold->to_api; + $biblio_hold->delete; + $biblio_hold_data->{pickup_library_id} = $library_1->branchcode; + delete $biblio_hold_data->{hold_id}; + + $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $biblio_hold_data ) + ->status_is(400) + ->json_is({ error => 'The supplied pickup location is not valid' }); + + $biblio_hold_data->{pickup_library_id} = $library_2->branchcode; + $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $biblio_hold_data ) + ->status_is(201); + + # Test item-level holds + my $item_hold = $builder->build_object( + { + class => "Koha::Holds", + value => { + biblionumber => $biblio->biblionumber, + branchcode => $library_3->branchcode, + itemnumber => $item->itemnumber, + priority => 1, + } + } + ); + + my $item_hold_data = $item_hold->to_api; + $item_hold->delete; + $item_hold_data->{pickup_library_id} = $library_1->branchcode; + delete $item_hold->{hold_id}; + + $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $item_hold_data ) + ->status_is(400) + ->json_is({ error => 'The supplied pickup location is not valid' }); + + $item_hold_data->{pickup_library_id} = $library_2->branchcode; + $t->post_ok( "//$userid:$password@/api/v1/holds" => json => $item_hold_data ) + ->status_is(201); + + $schema->storage->txn_rollback; +}; -- 2.29.2