From 63bd50e7214c0fa30bb44ba76c436999c93f6737 Mon Sep 17 00:00:00 2001 From: Kyle Hall Date: Thu, 23 Sep 2021 08:57:30 -0400 Subject: [PATCH] Bug 29094: Adding hold via SIP should check if patron can hold item first When placing holds via SIP2, there is no holdability check. This seems very incorrect. Test Plan: 1) Apply this patch 2) prove -r t/db_dependent/SIP Signed-off-by: David Nind Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi (cherry picked from commit 7b21fb020b868ebaad97fc8a8970dc9570b76e56) Signed-off-by: Lucas Gass --- C4/SIP/ILS/Transaction/Hold.pm | 31 ++++++++------- t/db_dependent/SIP/ILS.t | 9 ++++- t/db_dependent/SIP/Transaction.t | 66 +++++++++++++++++++++++++++++++- 3 files changed, 90 insertions(+), 16 deletions(-) diff --git a/C4/SIP/ILS/Transaction/Hold.pm b/C4/SIP/ILS/Transaction/Hold.pm index 82fddf1e97..b3dae13860 100644 --- a/C4/SIP/ILS/Transaction/Hold.pm +++ b/C4/SIP/ILS/Transaction/Hold.pm @@ -7,7 +7,7 @@ use Modern::Perl; use C4::SIP::ILS::Transaction; -use C4::Reserves qw( CalculatePriority AddReserve ModReserve ); +use C4::Reserves qw( CalculatePriority AddReserve ModReserve CanItemBeReserved ); use Koha::Holds; use Koha::Patrons; use Koha::Items; @@ -61,18 +61,23 @@ sub do_hold { return $self; } - my $priority = C4::Reserves::CalculatePriority($item->biblionumber); - AddReserve( - { - priority => $priority, - branchcode => $branch, - borrowernumber => $patron->borrowernumber, - biblionumber => $item->biblionumber - } - ); - - # unfortunately no meaningful return value - $self->ok(1); + my $canReserve = CanItemBeReserved($patron, $item, $branch); + if ($canReserve->{status} eq 'OK') { + my $priority = C4::Reserves::CalculatePriority($item->biblionumber); + AddReserve( + { + priority => $priority, + branchcode => $branch, + borrowernumber => $patron->borrowernumber, + biblionumber => $item->biblionumber + } + ); + + $self->ok(1); + } else { + $self->ok(0); + } + return $self; } diff --git a/t/db_dependent/SIP/ILS.t b/t/db_dependent/SIP/ILS.t index c117e53763..11d52e0ce6 100755 --- a/t/db_dependent/SIP/ILS.t +++ b/t/db_dependent/SIP/ILS.t @@ -74,7 +74,14 @@ is( $ils->test_cardnumber_compare( 'A1234', 'b1234' ), subtest add_hold => sub { plan tests => 4; - my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library = $builder->build_object( + { + class => 'Koha::Libraries', + value => { + pickup_location => 1 + } + } + ); my $patron = $builder->build_object( { class => 'Koha::Patrons', diff --git a/t/db_dependent/SIP/Transaction.t b/t/db_dependent/SIP/Transaction.t index f11efa8038..b17b33e635 100755 --- a/t/db_dependent/SIP/Transaction.t +++ b/t/db_dependent/SIP/Transaction.t @@ -4,7 +4,7 @@ # Current state is very rudimentary. Please help to extend it! use Modern::Perl; -use Test::More tests => 14; +use Test::More tests => 15; use Koha::Database; use t::lib::TestBuilder; @@ -212,7 +212,14 @@ subtest cancel_hold => sub { subtest do_hold => sub { plan tests => 8; - my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library = $builder->build_object( + { + class => 'Koha::Libraries', + value => { + pickup_location => 1 + } + } + ); my $patron_1 = $builder->build_object( { class => 'Koha::Patrons', @@ -271,6 +278,61 @@ subtest do_hold => sub { is( $THE_hold->branchcode, $patron_2->branchcode, 'Hold placed from SIP should have the branchcode set' ); }; +subtest "Placing holds via SIP check CanItemBeReserved" => sub { + plan tests => 4; + + my $library = $builder->build_object( + { + class => 'Koha::Libraries', + value => { + pickup_location => 0 + } + } + ); + my $patron_1 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + branchcode => $library->branchcode, + } + } + ); + my $patron_2 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + branchcode => $library->branchcode, + categorycode => $patron_1->categorycode, + } + } + ); + + t::lib::Mocks::mock_userenv( + { branchcode => $library->branchcode, flags => 1 } ); + + my $item = $builder->build_sample_item( + { + library => $library->branchcode, + } + ); + + my $sip_patron = C4::SIP::ILS::Patron->new( $patron_2->cardnumber ); + my $sip_item = C4::SIP::ILS::Item->new( $item->barcode ); + my $transaction = C4::SIP::ILS::Transaction::Hold->new(); + is( + ref $transaction, + "C4::SIP::ILS::Transaction::Hold", + "New transaction created" + ); + is( $transaction->patron($sip_patron), + $sip_patron, "Patron assigned to transaction" ); + is( $transaction->item($sip_item), + $sip_item, "Item assigned to transaction" ); + my $hold = $transaction->do_hold(); + + is( $transaction->ok, 0, "Hold was not allowed" ); +}; + subtest do_checkin => sub { plan tests => 12; -- 2.39.5