Bug 19058: Move C4::Reserves::GetReserveId to the Koha namespace

GetReserveId can easily be replaced with a call to
Koha::Holds->search->next->reserve_id

It will ease next changes to use Koha::Hold objects

Test plan:
Cancel a reserve and print a slip reserve

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 575e486..3be9df9 100644 (file)
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -846,15 +846,13 @@ sub CancelReserve {
     my ( $params ) = @_;
 
     my $reserve_id = $params->{'reserve_id'};
-    # Filter out only the desired keys; this will insert undefined values for elements missing in
-    # \%params, but GetReserveId filters them out anyway.
-    $reserve_id = GetReserveId( { biblionumber => $params->{'biblionumber'}, borrowernumber => $params->{'borrowernumber'}, itemnumber => $params->{'itemnumber'} } ) unless ( $reserve_id );
-
-    return unless ( $reserve_id );
-
-    my $dbh = C4::Context->dbh;
+    my $hold;
+    if ( $reserve_id ) {
+        $hold = Koha::Holds->find( $reserve_id );
+    } else {
+        $hold = Koha::Holds->search( $params ); # biblionumber, borrowernumber, itemnumber
+    }
 
-    my $hold = Koha::Holds->find( $reserve_id );
     return unless $hold;
 
     logaction( 'HOLDS', 'CANCEL', $hold->reserve_id, Dumper($hold->unblessed) )
@@ -866,6 +864,7 @@ sub CancelReserve {
                priority         = 0
         WHERE  reserve_id = ?
     ";
+    my $dbh = C4::Context->dbh;
     my $sth = $dbh->prepare($query);
     $sth->execute( $reserve_id );
 
@@ -947,13 +946,19 @@ sub ModReserve {
     return if $rank eq "n";
 
     return unless ( $reserve_id || ( $borrowernumber && ( $biblionumber || $itemnumber ) ) );
-    $reserve_id = GetReserveId({ biblionumber => $biblionumber, borrowernumber => $borrowernumber, itemnumber => $itemnumber }) unless ( $reserve_id );
+
+    my $hold;
+    unless ( $reserve_id ) {
+        $hold = Koha::Holds->search({ biblionumber => $biblionumber, borrowernumber => $borrowernumber, itemnumber => $itemnumber });
+        return unless $hold; # FIXME Should raise an exception
+        $reserve_id = $hold->reserve_id;
+    }
 
     if ( $rank eq "del" ) {
         CancelReserve({ reserve_id => $reserve_id });
     }
     elsif ($rank =~ /^\d+/ and $rank > 0) {
-        my $hold = Koha::Holds->find($reserve_id);
+        $hold ||= Koha::Holds->find($reserve_id);
         logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, Dumper($hold->unblessed) )
             if C4::Context->preference('HoldsLog');
 
@@ -1999,30 +2004,6 @@ sub RevertWaitingStatus {
     _FixPriority( { biblionumber => $reserve->{biblionumber} } );
 }
 
-=head2 GetReserveId
-
-  $reserve_id = GetReserveId({ biblionumber => $biblionumber, borrowernumber => $borrowernumber [, itemnumber => $itemnumber ] });
-
-  Returnes the first reserve id that matches the given criteria
-
-=cut
-
-sub GetReserveId {
-    my ( $params ) = @_;
-
-    return unless ( ( $params->{'biblionumber'} || $params->{'itemnumber'} ) && $params->{'borrowernumber'} );
-
-    foreach my $key ( keys %$params ) {
-        delete $params->{$key} unless defined( $params->{$key} );
-    }
-
-    my $hold = Koha::Holds->search( $params )->next();
-
-    return unless $hold;
-
-    return $hold->id();
-}
-
 =head2 ReserveSlip
 
   ReserveSlip($branchcode, $borrowernumber, $biblionumber)
@@ -2047,12 +2028,9 @@ sub ReserveSlip {
 #   return unless ( C4::Context->boolean_preference('printreserveslips') );
     my $patron = Koha::Patrons->find( $borrowernumber );
 
-    my $reserve_id = GetReserveId({
-        biblionumber => $biblionumber,
-        borrowernumber => $borrowernumber
-    }) or return;
-    my $reserve = Koha::Holds->find($reserve_id) or return;
-    $reserve = $reserve->unblessed;
+    my $hold = Koha::Holds->search({biblionumber => $biblionumber, borrowernumber => $borrowernumber })->next;
+    return unless $hold;
+    my $reserve = $hold->unblessed;
 
     return  C4::Letters::GetPreparedLetter (
         module => 'circulation',

diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
index 7f85ad6..3e81576 100755 (executable)
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@@ -377,7 +377,7 @@ C4::Context->dbh->do("DELETE FROM accountlines");
     is( $renewokay, 0, '(Bug 10663) Cannot renew, reserved');
     is( $error, 'on_reserve', '(Bug 10663) Cannot renew, reserved (returned error is on_reserve)');
 
-    my $reserveid = C4::Reserves::GetReserveId({ biblionumber => $biblionumber, borrowernumber => $reserving_borrowernumber});
+    my $reserveid = Koha::Holds->search({ biblionumber => $biblionumber, borrowernumber => $reserving_borrowernumber })->next->reserve_id;
     my $reserving_borrower = Koha::Patrons->find( $reserving_borrowernumber )->unblessed;
     AddIssue($reserving_borrower, $barcode3);
     my $reserve = $dbh->selectrow_hashref(
@@ -516,7 +516,7 @@ C4::Context->dbh->do("DELETE FROM accountlines");
     is( $renewokay, 0, '(Bug 8236), Cannot renew, this item is overdue');
 
 
-    $reserveid = C4::Reserves::GetReserveId({ biblionumber => $biblionumber, itemnumber => $itemnumber, borrowernumber => $reserving_borrowernumber});
+    $reserveid = Koha::Holds->search({ biblionumber => $biblionumber, borrowernumber => $reserving_borrowernumber })->next->reserve_id;
     CancelReserve({ reserve_id => $reserveid });
 
     # Bug 14101

diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t
index 3d41670..f9083d4 100755 (executable)
--- a/t/db_dependent/Holds.t
+++ b/t/db_dependent/Holds.t
@@ -7,7 +7,7 @@ use t::lib::TestBuilder;
 
 use C4::Context;
 
-use Test::More tests => 56;
+use Test::More tests => 55;
 use MARC::Record;
 use C4::Biblio;
 use C4::Items;
@@ -197,13 +197,7 @@ AddReserve(
 );
 $patron = Koha::Patrons->find( $borrowernumber );
 $holds = $patron->holds;
-my $reserveid = C4::Reserves::GetReserveId(
-    {
-        biblionumber => $biblionumber,
-        borrowernumber => $borrowernumber
-    }
-);
-is( $reserveid, $holds->next->reserve_id, "Test GetReserveId" );
+my $reserveid = Koha::Holds->search({ biblionumber => $bibnum, borrowernumber => $borrowernumbers[0] })->next->reserve_id;
 ModReserveMinusPriority( $itemnumber, $reserveid );
 $holds = $patron->holds;
 is( $holds->next->itemnumber, $itemnumber, "Test ModReserveMinusPriority()" );
@@ -291,12 +285,7 @@ AddReserve(
     1,
 );
 
-my $reserveid1 = C4::Reserves::GetReserveId(
-    {
-        biblionumber => $bibnum,
-        borrowernumber => $borrowernumbers[0]
-    }
-);
+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(
@@ -306,12 +295,7 @@ AddReserve(
     '',
     2,
 );
-my $reserveid2 = C4::Reserves::GetReserveId(
-    {
-        biblionumber => $bibnum,
-        borrowernumber => $borrowernumbers[1]
-    }
-);
+my $reserveid2 = Koha::Holds->search({ biblionumber => $bibnum, borrowernumber => $borrowernumbers[1] })->next->reserve_id;
 
 CancelReserve({ reserve_id => $reserveid1 });
 
@@ -326,12 +310,7 @@ AddReserve(
     '',
     2,
 );
-my $reserveid3 = C4::Reserves::GetReserveId(
-    {
-        biblionumber => $bibnum,
-        borrowernumber => $borrowernumbers[0]
-    }
-);
+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" );