From a6c1ebb0d896245ba71184dc325067532a1fa129 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 11 Dec 2020 17:55:40 -0300 Subject: [PATCH] Bug 27205: Check valid pickup location on PUT /holds/:hold_id This patch adds a test for valid pickup locations when updating a hold through the API. Tests are adjusted to reflect this change. 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: Tomas Cohen Arazi --- Koha/REST/V1/Holds.pm | 21 +++++ t/db_dependent/api/v1/holds.t | 147 +++++++++++++++++++++++++++------- 2 files changed, 139 insertions(+), 29 deletions(-) diff --git a/Koha/REST/V1/Holds.pm b/Koha/REST/V1/Holds.pm index 30579caa403..e7c31d2a8df 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 @@ -232,6 +233,19 @@ sub edit { my $body = $c->req->json; my $pickup_library_id = $body->{pickup_library_id} // $hold->branchcode; + + my @pickup_locations; + if ( $hold->itemnumber ) { # item-level + @pickup_locations = $hold->item->pickup_locations({ patron => $hold->patron }); + } + else { # biblio-level + @pickup_locations = $hold->biblio->pickup_locations({ patron => $hold->patron }); + } + + unless ( any { $_->branchcode eq $pickup_library_id } @pickup_locations ) { + Koha::Exceptions::Hold::InvalidPickupLocation->throw; + } + my $priority = $body->{priority} // $hold->priority; # suspended_until can also be set to undef my $suspended_until = exists $body->{suspended_until} ? $body->{suspended_until} : $hold->suspend_until; @@ -253,6 +267,13 @@ sub edit { ); } catch { + if ( blessed $_ and $_->isa('Koha::Exceptions::Hold::InvalidPickupLocation')) { + return $c->render( + status => 400, + openapi => { error => "$_" } + ); + } + $c->unhandled_exception($_); }; } diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t index 585e19a8a50..d8fa6851f05 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 => 10; +use Test::More tests => 11; use Test::MockModule; use Test::Mojo; use t::lib::TestBuilder; @@ -200,7 +200,7 @@ subtest "Test endpoints without permission" => sub { subtest "Test endpoints with permission" => sub { - plan tests => 62; + plan tests => 44; $t->get_ok( "//$userid_1:$password@/api/v1/holds" ) ->status_is(200) @@ -213,33 +213,6 @@ subtest "Test endpoints with permission" => sub { ->json_is('/0/patron_id', $patron_2->borrowernumber) ->json_hasnt('/1'); - # While suspended_until is date-time, it's always set to midnight. - my $expected_suspended_until = $suspended_until->strftime('%FT00:00:00%z'); - substr($expected_suspended_until, -2, 0, ':'); - - $t->put_ok( "//$userid_1:$password@/api/v1/holds/$reserve_id" => json => $put_data ) - ->status_is(200) - ->json_is( '/hold_id', $reserve_id ) - ->json_is( '/suspended_until', $expected_suspended_until ) - ->json_is( '/priority', 2 ) - ->json_is( '/pickup_library_id', $branchcode ); - - # Change only pickup library, everything else should remain - $t->put_ok( "//$userid_1:$password@/api/v1/holds/$reserve_id" => json => { pickup_library_id => $branchcode2 } ) - ->status_is(200) - ->json_is( '/hold_id', $reserve_id ) - ->json_is( '/suspended_until', $expected_suspended_until ) - ->json_is( '/priority', 2 ) - ->json_is( '/pickup_library_id', $branchcode2 ); - - # Reset suspended_until, everything else should remain - $t->put_ok( "//$userid_1:$password@/api/v1/holds/$reserve_id" => json => { suspended_until => undef } ) - ->status_is(200) - ->json_is( '/hold_id', $reserve_id ) - ->json_is( '/suspended_until', undef ) - ->json_is( '/priority', 2 ) - ->json_is( '/pickup_library_id', $branchcode2 ); - $t->delete_ok( "//$userid_3:$password@/api/v1/holds/$reserve_id" ) ->status_is(204, 'SWAGGER3.2.4') ->content_is('', 'SWAGGER3.3.4'); @@ -777,3 +750,119 @@ subtest 'pickup_locations() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'edit() tests' => sub { + + plan tests => 12; + + $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_data->{pickup_library_id} = $library_1->branchcode; + + $t->put_ok( "//$userid:$password@/api/v1/holds/" + . $biblio_hold->id + => json => $biblio_hold_data ) + ->status_is(400); + + $biblio_hold->discard_changes; + is( $biblio_hold->branchcode, $library_3->branchcode, 'branchcode remains untouched' ); + + $biblio_hold_data->{pickup_library_id} = $library_2->branchcode; + $t->put_ok( "//$userid:$password@/api/v1/holds/" + . $biblio_hold->id + => json => $biblio_hold_data ) + ->status_is(200); + + $biblio_hold->discard_changes; + is( $biblio_hold->branchcode, $library_2->id, 'Pickup location changed correctly' ); + + # 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_data->{pickup_library_id} = $library_1->branchcode; + + $t->put_ok( "//$userid:$password@/api/v1/holds/" + . $item_hold->id + => json => $item_hold_data ) + ->status_is(400); + + $item_hold->discard_changes; + is( $item_hold->branchcode, $library_3->branchcode, 'branchcode remains untouched' ); + + $item_hold_data->{pickup_library_id} = $library_2->branchcode; + $t->put_ok( "//$userid:$password@/api/v1/holds/" + . $item_hold->id + => json => $item_hold_data ) + ->status_is(200); + + $item_hold->discard_changes; + is( $item_hold->branchcode, $library_2->id, 'Pickup location changed correctly' ); + + $schema->storage->txn_rollback; +}; -- 2.29.2