Bugzilla – Attachment 114446 Details for
Bug 27205
Hold routes are not dealing with invalid pickup locations
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 27205: Check valid pickup location on PUT /holds/:hold_id
Bug-27205-Check-valid-pickup-location-on-PUT-holds.patch (text/plain), 8.42 KB, created by
Kyle M Hall (khall)
on 2020-12-16 15:14:49 UTC
(
hide
)
Description:
Bug 27205: Check valid pickup location on PUT /holds/:hold_id
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2020-12-16 15:14:49 UTC
Size:
8.42 KB
patch
obsolete
>From 132f043e186b9fcadfb1fac51861dfcf4e2acc8e Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> >Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > 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.24.1 (Apple Git-126)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 27205
:
114358
|
114375
|
114377
|
114382
|
114384
| 114446 |
114447
|
114512