From 1bfe7ea16ef1346585211345ba89d18e303a0c32 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 Signed-off-by: Victor Grousset/tuxayo Signed-off-by: Kyle M Hall Signed-off-by: Jonathan Druart --- Koha/REST/V1/Holds.pm | 20 ++++- t/db_dependent/api/v1/holds.t | 149 +++++++++++++++++++++++++++------- 2 files changed, 139 insertions(+), 30 deletions(-) diff --git a/Koha/REST/V1/Holds.pm b/Koha/REST/V1/Holds.pm index 30579caa40..0a9cfb93dc 100644 --- a/Koha/REST/V1/Holds.pm +++ b/Koha/REST/V1/Holds.pm @@ -231,7 +231,25 @@ sub edit { my $body = $c->req->json; - my $pickup_library_id = $body->{pickup_library_id} // $hold->branchcode; + my $pickup_library_id = $body->{pickup_library_id}; + + unless ( + !defined $pickup_library_id + or $hold->is_pickup_location_valid( + { library_id => $pickup_library_id } + ) + ) + { + return $c->render( + status => 400, + openapi => { + error => 'The supplied pickup location is not valid' + } + ); + } + + $pickup_library_id = $hold->branchcode + unless defined $pickup_library_id; 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; diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t index 585e19a8a5..6b977ced64 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,121 @@ subtest 'pickup_locations() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'edit() tests' => sub { + + plan tests => 14; + + $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) + ->json_is({ error => 'The supplied pickup location is not valid' }); + + $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) + ->json_is({ error => 'The supplied pickup location is not valid' }); + + $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.39.5