Bugzilla – Attachment 114447 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 POST /holds
Bug-27205-Check-valid-pickup-location-on-POST-hold.patch (text/plain), 10.14 KB, created by
Kyle M Hall (khall)
on 2020-12-16 15:15:00 UTC
(
hide
)
Description:
Bug 27205: Check valid pickup location on POST /holds
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2020-12-16 15:15:00 UTC
Size:
10.14 KB
patch
obsolete
>From 56d30b1b1a6f6f786695147e0f0044dd46e542bc Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <victor@tuxayo.net> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > 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.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