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 <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

C4/ILSDI/Services.pm

@@ -775,7 +775,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' };
 }

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;
         }
     }
 }

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;

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 <kyle@bywatersolutions.com>
 
+Jonathan Druart <jonathan.druart@bugs.koha-community.org>
+
 =cut
 
 1;

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

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);
 }

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...

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,

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;

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");

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 );

t/db_dependent/Circulation.t

@@ -516,8 +516,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

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" );

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;

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;

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
 

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;
 
@@ -320,16 +320,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)
@@ -601,7 +595,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');
@@ -628,7 +622,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' );
@@ -650,7 +644,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' );

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 = '

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, {