From 267ba800f8adc55eed1f6043bf6183ec3aba20c2 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 2 Aug 2017 14:00:12 -0300 Subject: [PATCH] [SIGNED-OFF] Bug 19059: Move C4::Reserves::CancelReserve to Koha::Hold->cancel This patch adds a new Koha::Hold->cancel method and replaces the calls to C4::Reserves::CancelReserve with it. Test plan: - Add and cancel holds - Change priority of holds Signed-off-by: Owen Leonard --- C4/Biblio.pm | 3 +- C4/ILSDI/Services.pm | 2 +- C4/Reserves.pm | 51 +++++++++++++++--------- C4/SIP/ILS/Transaction/Hold.pm | 14 +++++-- Koha/Hold.pm | 58 +++++++++++++++++++++++++++- Koha/Patron/Discharge.pm | 3 +- Koha/REST/V1/Hold.pm | 2 +- circ/branchtransfers.pl | 23 +++++++---- circ/returns.pl | 6 ++- circ/waitingreserves.pl | 2 +- opac/opac-modrequest.pl | 6 ++- reserve/request.pl | 3 +- t/db_dependent/Circulation.t | 4 +- t/db_dependent/Holds.t | 14 +++---- t/db_dependent/Holds/HoldFulfillmentPolicy.t | 19 ++++----- t/db_dependent/Holds/HoldItemtypeLimit.t | 8 ++-- t/db_dependent/HoldsQueue.t | 25 ++++++------ t/db_dependent/Reserves.t | 18 +++------ t/db_dependent/UsageStats.t | 2 +- tools/batch_delete_records.pl | 2 +- 20 files changed, 176 insertions(+), 89 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 0ee689d..cee8fe8 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -406,9 +406,8 @@ sub DelBiblio { # We delete any existing holds my $biblio = Koha::Biblios->find( $biblionumber ); my $holds = $biblio->holds; - require C4::Reserves; while ( my $hold = $holds->next ) { - C4::Reserves::CancelReserve({ reserve_id => $hold->reserve_id }); # TODO Replace with $hold->cancel + $hold->cancel; } # Delete in Zebra. Be careful NOT to move this line after _koha_delete_biblio diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index dd52619..54ca13d 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -773,7 +773,7 @@ sub CancelHold { return { code => 'RecordNotFound' } unless $hold; return { code => 'RecordNotFound' } unless ($hold->borrowernumber == $borrowernumber); - C4::Reserves::CancelReserve({reserve_id => $reserve_id}); + $hold->cancel; return { code => 'Canceled' }; } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 3be9df9..20ad806 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -121,7 +121,6 @@ BEGIN { &CanBookBeReserved &CanItemBeReserved &CanReserveBeCanceledFromOpac - &CancelReserve &CancelExpiredReserves &AutoUnsuspendReserves @@ -798,23 +797,27 @@ sub CancelExpiredReserves { my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare( " - SELECT * FROM reserves WHERE DATE(expirationdate) < DATE( CURDATE() ) - AND expirationdate IS NOT NULL - " ); - $sth->execute(); - while ( my $res = $sth->fetchrow_hashref() ) { - my $calendar = Koha::Calendar->new( branchcode => $res->{'branchcode'} ); - my $cancel_params = { reserve_id => $res->{'reserve_id'} }; + my $dtf = Koha::Database->new->schema->storage->datetime_parser; + my $today = dt_from_string; + # FIXME To move to Koha::Holds->search_expired (?) + my $holds = Koha::Holds->search( + { + expirationdate => { '<', $dtf->format_date($today) } + } + ); + + while ( my $hold = $holds->next ) { + my $calendar = Koha::Calendar->new( branchcode => $hold->branchcode ); next if !$cancel_on_holidays && $calendar->is_holiday( $today ); - if ( $res->{found} eq 'W' ) { + my $cancel_params = {}; + if ( $holds->found eq 'W' ) { $cancel_params->{charge_cancel_fee} = 1; } + $hold->cancel( $cancel_params ); - CancelReserve($cancel_params); } } @@ -954,11 +957,12 @@ sub ModReserve { $reserve_id = $hold->reserve_id; } + $hold ||= Koha::Holds->find($reserve_id); + if ( $rank eq "del" ) { - CancelReserve({ reserve_id => $reserve_id }); + $hold->cancel; } elsif ($rank =~ /^\d+/ and $rank > 0) { - $hold ||= Koha::Holds->find($reserve_id); logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, Dumper($hold->unblessed) ) if C4::Context->preference('HoldsLog'); @@ -1016,6 +1020,7 @@ sub ModReserveFill { } ); + # FIXME Must call Koha::Hold->cancel ? Koha::Old::Hold->new( $hold->unblessed() )->store(); $hold->delete(); @@ -1128,7 +1133,9 @@ sub ModReserveCancelAll { my ( $itemnumber, $borrowernumber ) = @_; #step 1 : cancel the reservation - my $CancelReserve = CancelReserve({ itemnumber => $itemnumber, borrowernumber => $borrowernumber }); + my $holds = Koha::Holds->search({ itemnumber => $itemnumber, borrowernumber => $borrowernumber }); + return unless $holds->count; + $holds->next->cancel; #step 2 launch the subroutine of the others reserves ( $messages, $nextreservinfo ) = GetOtherReserves($itemnumber); @@ -1452,13 +1459,18 @@ sub _FixPriority { my $dbh = C4::Context->dbh; - unless ( $biblionumber ) { - my $hold = Koha::Holds->find( $reserve_id ); + my $hold; + if ( $reserve_id ) { + $hold = Koha::Holds->find( $reserve_id ); + return unless $hold; + } + + unless ( $biblionumber ) { # FIXME This is a very weird API $biblionumber = $hold->biblionumber; } - if ( $rank eq "del" ) { - CancelReserve({ reserve_id => $reserve_id }); + if ( $rank eq "del" ) { # FIXME will crash if called without $hold + $hold->cancel; } elsif ( $rank eq "W" || $rank eq "0" ) { @@ -1894,7 +1906,8 @@ sub MoveReserve { RevertWaitingStatus({ itemnumber => $itemnumber }); } elsif ( $cancelreserve eq 'cancel' || $cancelreserve ) { # cancel reserves on this item - CancelReserve( { reserve_id => $res->{'reserve_id'} } ); + my $hold = Koha::Holds->find( $res->{reserve_id} ); + $hold->cancel; } } } diff --git a/C4/SIP/ILS/Transaction/Hold.pm b/C4/SIP/ILS/Transaction/Hold.pm index 3d38bc4..d834516 100644 --- a/C4/SIP/ILS/Transaction/Hold.pm +++ b/C4/SIP/ILS/Transaction/Hold.pm @@ -8,6 +8,7 @@ use Modern::Perl; use C4::SIP::ILS::Transaction; use C4::Reserves; # AddReserve +use Koha::Holds; use Koha::Patrons; use parent qw(C4::SIP::ILS::Transaction); @@ -81,11 +82,16 @@ sub drop_hold { } my $item = Koha::Items->find({ barcode => $self->{item}->id }); - CancelReserve({ + my $holds = Koha::Holds->search( + { biblionumber => $item->biblionumber, - itemnumber => $self->{item}->id, - borrowernumber => $patron->borrowernumber - }); + itemnumber => $self->{item}->id, + borrowernumber => $patron->borrowernumber + } + ); + return $self unless $holds->count; + + $holds->next->cancel; $self->ok(1); return $self; diff --git a/Koha/Hold.pm b/Koha/Hold.pm index d708054..5c6d000 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -1,6 +1,7 @@ package Koha::Hold; # Copyright ByWater Solutions 2014 +# Copyright 2017 Koha Development team # # This file is part of Koha. # @@ -30,6 +31,7 @@ use Koha::Patrons; use Koha::Biblios; use Koha::Items; use Koha::Libraries; +use Koha::Old::Holds; use base qw(Koha::Object); @@ -294,6 +296,58 @@ sub is_suspended { return $self->suspend(); } + +=head3 cancel + +my $cancel_hold = $hold->cancel(); + +Cancel a hold: +- The hold will be moved to the old_reserves table with a priority=0 +- The priority of other holds will be updated +- The patron will be charge (see ExpireReservesMaxPickUpDelayCharge) if the charge_cancel_fee parameter is set +- a CANCEL HOLDS log will be done if the pref HoldsLog is on + +=cut + +sub cancel { + my ( $self, $params ) = @_; + $self->_result->result_source->schema->txn_do( + sub { + $self->cancellationdate(dt_from_string); + $self->priority(0); + $self->_move_to_old; + $self->delete; + + # now fix the priority on the others.... + C4::Reserves::_FixPriority({ biblionumber => $self->biblionumber }); + + # and, if desired, charge a cancel fee + my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); + if ( $charge && $params->{'charge_cancel_fee'} ) { + C4::Accounts::manualinvoice($self->borrowernumber, $self->itemnumber, '', 'HE', $charge); + } + + C4::Log::logaction( 'HOLDS', 'CANCEL', $self->reserve_id, Dumper($self->unblessed) ) + if C4::Context->preference('HoldsLog'); + } + ); + return $self; +} + +=head3 _move_to_old + +my $is_moved = $hold->_move_to_old; + +Move a hold to the old_reserve table following the same pattern as Koha::Patron->move_to_deleted + +=cut + +sub _move_to_old { + my ($self) = @_; + my $hold_infos = $self->unblessed; + return Koha::Old::Hold->new( $hold_infos )->store; +} + =head3 type =cut @@ -302,10 +356,12 @@ sub _type { return 'Reserve'; } -=head1 AUTHOR +=head1 AUTHORS Kyle M Hall +Jonathan Druart + =cut 1; diff --git a/Koha/Patron/Discharge.pm b/Koha/Patron/Discharge.pm index 06e3101..a9a6b8d 100644 --- a/Koha/Patron/Discharge.pm +++ b/Koha/Patron/Discharge.pm @@ -7,7 +7,6 @@ use Carp; use C4::Templates qw ( gettemplate ); use C4::Members qw( GetPendingIssues ); -use C4::Reserves qw( CancelReserve ); use Koha::Database; use Koha::DateUtils qw( dt_from_string output_pref ); @@ -82,7 +81,7 @@ sub discharge { my $patron = Koha::Patrons->find( $borrowernumber ); my $holds = $patron->holds; while ( my $hold = $holds->next ) { - CancelReserve( { reserve_id => $hold->reserve_id } ); + $hold->cancel; } # Debar the member diff --git a/Koha/REST/V1/Hold.pm b/Koha/REST/V1/Hold.pm index a31a9e6..ed4f1dd 100644 --- a/Koha/REST/V1/Hold.pm +++ b/Koha/REST/V1/Hold.pm @@ -155,7 +155,7 @@ sub delete { return $c->$cb({error => "Reserve not found"}, 404) unless $hold; - C4::Reserves::CancelReserve({ reserve_id => $reserve_id }); + $hold->cancel; return $c->$cb({}, 200); } diff --git a/circ/branchtransfers.pl b/circ/branchtransfers.pl index 32e77f2..bdb007f 100755 --- a/circ/branchtransfers.pl +++ b/circ/branchtransfers.pl @@ -33,6 +33,7 @@ use C4::Koha; use C4::Members; use Koha::BiblioFrameworks; use Koha::AuthorisedValues; +use Koha::Holds; use Koha::Items; use Koha::Patrons; @@ -81,12 +82,15 @@ my $ignoreRs = 0; # Deal with the requests.... if ( $request eq "KillWaiting" ) { my $item = $query->param('itemnumber'); - CancelReserve({ + my $holds = Koha::Holds->search( itemnumber => $item, borrowernumber => $borrowernumber }); - $cancelled = 1; - $reqmessage = 1; + if ( $holds->count ) { + $holds->next->cancel; + $cancelled = 1; + $reqmessage = 1; + } # FIXME else? } elsif ( $request eq "SetWaiting" ) { my $item = $query->param('itemnumber'); @@ -96,13 +100,16 @@ elsif ( $request eq "SetWaiting" ) { $reqmessage = 1; } elsif ( $request eq 'KillReserved' ) { - my $biblio = $query->param('biblionumber'); - CancelReserve({ - biblionumber => $biblio, + my $biblionumber = $query->param('biblionumber'); + my $holds = Koha::Holds->search( + biblionumber => $biblionumber, borrowernumber => $borrowernumber }); - $cancelled = 1; - $reqmessage = 1; + if ( $holds->count ) { + $holds->next->cancel; + $cancelled = 1; + $reqmessage = 1; + } # FIXME else? } # collect the stack of books already transfered so they can printed... diff --git a/circ/returns.pl b/circ/returns.pl index 5fe3c70..c42e750 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -54,6 +54,7 @@ use Koha::DateUtils; use Koha::Calendar; use Koha::BiblioFrameworks; use Koha::Checkouts; +use Koha::Holds; use Koha::Items; use Koha::Patrons; @@ -159,7 +160,10 @@ if ( $query->param('reserve_id') ) { my $biblio = $item->biblio; if ( $cancel_reserve ) { - CancelReserve({ reserve_id => $reserve_id, charge_cancel_fee => !$forgivemanualholdsexpire }); + my $hold = Koha::Holds->find( $reserve_id ); + if ( $hold ) { + $hold->cancel( { charge_cancel_fee => !$forgivemanualholdsexpire } ); + } # FIXME else? } else { my $diffBranchSend = ($userenv_branch ne $diffBranchReturned) ? $diffBranchReturned : undef; # diffBranchSend tells ModReserveAffect whether document is expected in this library or not, diff --git a/circ/waitingreserves.pl b/circ/waitingreserves.pl index 555d04e..bab2387 100755 --- a/circ/waitingreserves.pl +++ b/circ/waitingreserves.pl @@ -68,7 +68,7 @@ my $transfer_when_cancel_all = C4::Context->preference('TransferWhenCancelAllWai $template->param( TransferWhenCancelAllWaitingHolds => 1 ) if $transfer_when_cancel_all; my @cancel_result; -# if we have a return from the form we launch the subroutine CancelReserve +# if we have a return from the form we cancel the holds if ($item) { my $res = cancel( $item, $borrowernumber, $fbr, $tbr ); push @cancel_result, $res if $res; diff --git a/opac/opac-modrequest.pl b/opac/opac-modrequest.pl index 8a2e041..1c7a235 100755 --- a/opac/opac-modrequest.pl +++ b/opac/opac-modrequest.pl @@ -29,6 +29,7 @@ use CGI qw ( -utf8 ); use C4::Output; use C4::Reserves; use C4::Auth; +use Koha::Holds; my $query = new CGI; my ( $template, $borrowernumber, $cookie ) = get_template_and_user( @@ -44,7 +45,10 @@ my ( $template, $borrowernumber, $cookie ) = get_template_and_user( my $reserve_id = $query->param('reserve_id'); if ($reserve_id && $borrowernumber) { - CancelReserve({ reserve_id => $reserve_id }) if CanReserveBeCanceledFromOpac($reserve_id, $borrowernumber); + if ( CanReserveBeCanceledFromOpac($reserve_id, $borrowernumber) ) { + my $hold = Koha::Holds->find( $reserve_id ); + $hold->cancel if $hold; + } } print $query->redirect("/cgi-bin/koha/opac-user.pl#opac-user-holds"); diff --git a/reserve/request.pl b/reserve/request.pl index 2728170..3930e27 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -91,7 +91,8 @@ if ( $action eq 'move' ) { AlterPriority( $where, $reserve_id ); } elsif ( $action eq 'cancel' ) { my $reserve_id = $input->param('reserve_id'); - CancelReserve({ reserve_id => $reserve_id }); + my $hold = Koha::Holds->find( $reserve_id ); + $hold->cancel if $hold; } elsif ( $action eq 'setLowestPriority' ) { my $reserve_id = $input->param('reserve_id'); ToggleLowestPriority( $reserve_id ); diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 781f496..040fe16 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -503,8 +503,8 @@ C4::Context->dbh->do("DELETE FROM accountlines"); is( $renewokay, 0, '(Bug 8236), Cannot renew, this item is overdue'); - $reserveid = Koha::Holds->search({ biblionumber => $biblionumber, borrowernumber => $reserving_borrowernumber })->next->reserve_id; - CancelReserve({ reserve_id => $reserveid }); + my $hold = Koha::Holds->search({ biblionumber => $biblionumber, borrowernumber => $reserving_borrowernumber })->next; + $hold->cancel; # Bug 14101 # Test automatic renewal before value for "norenewalbefore" in policy is set diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index f9083d4..241d024 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -127,9 +127,10 @@ $holds = $patron->holds; is( $holds->next->borrowernumber, $borrowernumbers[0], "Test Koha::Patron->holds"); -CancelReserve({ 'reserve_id' => $reserve_id }); +Koha::Holds->find( $reserve_id )->cancel; + $holds = $biblio->holds; -is( $holds->count, $borrowers_count - 1, "Test CancelReserve()" ); +is( $holds->count, $borrowers_count - 1, "Koha::Hold->cancel" ); $holds = $item->current_holds; $first_hold = $holds->next; @@ -288,29 +289,28 @@ AddReserve( my $reserveid1 = Koha::Holds->search({ biblionumber => $bibnum, borrowernumber => $borrowernumbers[0] })->next->reserve_id; ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1 } , $bibnum); -AddReserve( +my $reserveid2 = AddReserve( $branch_1, $borrowernumbers[1], $bibnum, '', 2, ); -my $reserveid2 = Koha::Holds->search({ biblionumber => $bibnum, borrowernumber => $borrowernumbers[1] })->next->reserve_id; -CancelReserve({ reserve_id => $reserveid1 }); +my $hold1 = Koha::Holds->find( $reserveid1 ); +$hold1->cancel; my $hold2 = Koha::Holds->find( $reserveid2 ); is( $hold2->priority, 1, "After cancelreserve, the 2nd reserve becomes the first on the waiting list" ); ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1 } , $bibnum); -AddReserve( +my $reserveid3 = AddReserve( $branch_1, $borrowernumbers[0], $bibnum, '', 2, ); -my $reserveid3 = Koha::Holds->search({ biblionumber => $bibnum, borrowernumber => $borrowernumbers[0] })->next->reserve_id; my $hold3 = Koha::Holds->find( $reserveid3 ); is( $hold3->priority, 2, "New reserve for patron 0, the reserve has a priority = 2" ); diff --git a/t/db_dependent/Holds/HoldFulfillmentPolicy.t b/t/db_dependent/Holds/HoldFulfillmentPolicy.t index 33c0b1f..34bb3ed 100755 --- a/t/db_dependent/Holds/HoldFulfillmentPolicy.t +++ b/t/db_dependent/Holds/HoldFulfillmentPolicy.t @@ -7,6 +7,7 @@ use C4::Context; use Test::More tests => 10; use t::lib::TestBuilder; +use Koha::Holds; BEGIN { use_ok('C4::Reserves'); @@ -79,19 +80,19 @@ $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy my $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); my ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup branch matches home branch targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch $reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); ( $status ) = CheckReserves($itemnumber); is($status, q{}, "Hold where pickup ne home, pickup eq home not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch $reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); ( $status ) = CheckReserves($itemnumber); is( $status, q{}, "Hold where pickup ne home, pickup ne holding not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # With hold_fulfillment_policy = holdingbranch, hold should only be picked up if pickup branch = holdingbranch $dbh->do("DELETE FROM default_circ_rules"); @@ -101,19 +102,19 @@ $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); ( $status ) = CheckReserves($itemnumber); is( $status, q{}, "Hold where pickup eq home, pickup ne holding not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch $reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup ne home, pickup eq holding targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch $reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); ( $status ) = CheckReserves($itemnumber); is( $status, q{}, "Hold where pickup ne home, pickup ne holding not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # With hold_fulfillment_policy = any, hold should be pikcup up reguardless of matching home or holding branch $dbh->do("DELETE FROM default_circ_rules"); @@ -123,16 +124,16 @@ $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup eq home, pickup ne holding targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch $reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup ne home, pickup eq holding targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch $reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where pickup ne home, pickup ne holding targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; diff --git a/t/db_dependent/Holds/HoldItemtypeLimit.t b/t/db_dependent/Holds/HoldItemtypeLimit.t index 8fc8b3e..ba3e9c7 100644 --- a/t/db_dependent/Holds/HoldItemtypeLimit.t +++ b/t/db_dependent/Holds/HoldItemtypeLimit.t @@ -8,6 +8,8 @@ use Test::More tests => 4; use t::lib::TestBuilder; +use Koha::Holds; + BEGIN { use FindBin; use lib $FindBin::Bin; @@ -94,19 +96,19 @@ $dbh->do("INSERT INTO default_circ_rules ( holdallowed, hold_fulfillment_policy my $reserve_id = AddReserve( $branchcode, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, $right_itemtype ); my ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Hold where itemtype matches item's itemtype targed" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Itemtypes don't match $reserve_id = AddReserve( $branchcode, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, $wrong_itemtype ); ( $status ) = CheckReserves($itemnumber); is($status, q{}, "Hold where itemtype does not match item's itemtype not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # No itemtype set $reserve_id = AddReserve( $branchcode, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, undef ); ( $status ) = CheckReserves($itemnumber); is( $status, 'Reserved', "Item targeted with no hold itemtype set" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Cleanup $schema->storage->txn_rollback; diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t index 288432e..e2f749a 100755 --- a/t/db_dependent/HoldsQueue.t +++ b/t/db_dependent/HoldsQueue.t @@ -17,6 +17,7 @@ use C4::Members; use Koha::Database; use Koha::DateUtils; use Koha::Items; +use Koha::Holds; use t::lib::TestBuilder; use t::lib::Mocks; @@ -561,21 +562,21 @@ $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup branch matches home branch targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch $reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Hold where pickup ne home, pickup eq home not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch $reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Hold where pickup ne home, pickup ne holding not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # With hold_fulfillment_policy = holdingbranch, hold should only be picked up if pickup branch = holdingbranch $dbh->do("DELETE FROM default_circ_rules"); @@ -586,21 +587,21 @@ $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Hold where pickup eq home, pickup ne holding not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch $reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup ne home, pickup eq holding targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch $reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Hold where pickup ne home, pickup ne holding not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # With hold_fulfillment_policy = any, hold should be pikcup up reguardless of matching home or holding branch $dbh->do("DELETE FROM default_circ_rules"); @@ -611,21 +612,21 @@ $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup eq home, pickup ne holding targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch $reserve_id = AddReserve( $library_B, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup ne home, pickup eq holding targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch $reserve_id = AddReserve( $library_C, $borrowernumber, $biblionumber, '', 1 ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Hold where pickup ne home, pickup ne holding targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # End testing hold_fulfillment_policy @@ -673,21 +674,21 @@ $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1, und C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 0, "Item with incorrect itemtype not targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Holding branch matches pickup branch $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, $right_itemtype ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Item with matching itemtype is targeted" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # Neither branch matches pickup branch $reserve_id = AddReserve( $library_A, $borrowernumber, $biblionumber, '', 1, undef, undef, undef, undef, undef, undef, undef ); C4::HoldsQueue::CreateQueue(); $holds_queue = $dbh->selectall_arrayref( "SELECT * FROM tmp_holdsqueue", { Slice => {} } ); is( @$holds_queue, 1, "Item targeted when hold itemtype is not set" ); -CancelReserve( { reserve_id => $reserve_id } ); +Koha::Holds->find( $reserve_id )->cancel; # End testing hold itemtype limit diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 94c3c94..5b6656d 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 70; +use Test::More tests => 67; use Test::MockModule; use Test::Warn; @@ -321,16 +321,10 @@ $holds = $biblio->holds; is($holds->count, 1, "Only one reserves for this biblio"); my $reserve_id = $holds->next->reserve_id; -$reserve = CancelReserve({reserve_id => $reserve_id}); -isa_ok($reserve, 'HASH', "CancelReserve return"); -is($reserve->{biblionumber}, $biblionumber); +Koha::Holds->find( $reserve_id )->cancel; my $hold = Koha::Holds->find( $reserve_id ); -is($hold, undef, "CancelReserve should have cancel the reserve"); - -$reserve = CancelReserve({reserve_id => $reserve_id}); -is($reserve, undef, "CancelReserve return undef if reserve does not exist"); - +is($hold, undef, "Koha::Holds->cancel should have cancel the reserve"); # Tests for bug 9761 (ConfirmFutureHolds): new CheckReserves lookahead parameter, and corresponding change in AddReturn # Note that CheckReserve uses its lookahead parameter and does not check ConfirmFutureHolds pref (it should be passed if needed like AddReturn does) @@ -602,7 +596,7 @@ my $bz14464_reserve = AddReserve( ok( $bz14464_reserve, 'Bug 14464 - 1st reserve correctly created' ); -CancelReserve({ reserve_id => $bz14464_reserve, charge_cancel_fee => 1 }); +Koha::Holds->find( $bz14464_reserve )->cancel( { charge_cancel_fee => 1 } ); my $old_reserve = Koha::Database->new()->schema()->resultset('OldReserve')->find( $bz14464_reserve ); is($old_reserve->get_column('found'), 'W', 'Bug 14968 - Keep found column from reserve'); @@ -629,7 +623,7 @@ $bz14464_reserve = AddReserve( ok( $bz14464_reserve, 'Bug 14464 - 2nd reserve correctly created' ); -CancelReserve({ reserve_id => $bz14464_reserve }); +Koha::Holds->find( $bz14464_reserve )->cancel(); $bz14464_fines = $patron->account->balance; is( !$bz14464_fines || $bz14464_fines==0, 1, 'Bug 14464 - No fines after cancelling reserve with no charge desired' ); @@ -651,7 +645,7 @@ $bz14464_reserve = AddReserve( ok( $bz14464_reserve, 'Bug 14464 - 1st reserve correctly created' ); -CancelReserve({ reserve_id => $bz14464_reserve, charge_cancel_fee => 1 }); +Koha::Holds->find( $bz14464_reserve )->cancel( { charge_cancel_fee => 1 } ); $bz14464_fines = $patron->account->balance; is( int( $bz14464_fines ), 42, 'Bug 14464 - Fine applied after cancelling reserve with charge desired and configured' ); diff --git a/t/db_dependent/UsageStats.t b/t/db_dependent/UsageStats.t index 5f4dfad..f6b5c5a 100644 --- a/t/db_dependent/UsageStats.t +++ b/t/db_dependent/UsageStats.t @@ -304,7 +304,7 @@ sub construct_objects_needed { AddReserve( $branchcode, $borrowernumber1, $biblionumber1, '', 1, undef, undef, '', 'Title', undef, undef ); my $biblio = Koha::Biblios->find( $biblionumber1 ); my $holds = $biblio->holds; - CancelReserve( { reserve_id => $holds->next->reserve_id } ); + $holds->next->cancel if $holds->count; # ---------- Add 1 aqbudgets $query = ' diff --git a/tools/batch_delete_records.pl b/tools/batch_delete_records.pl index 4be2427..715347c 100755 --- a/tools/batch_delete_records.pl +++ b/tools/batch_delete_records.pl @@ -147,7 +147,7 @@ if ( $op eq 'form' ) { my $holds = $biblio->holds; while ( my $hold = $holds->next ) { eval{ - C4::Reserves::CancelReserve( { reserve_id => $hold->reserve_id } ); + $hold->cancel; }; if ( $@ ) { push @messages, { -- 2.1.4