Bug 19057: Move C4::Reserve::GetReserve to Koha::Holds
authorJonathan Druart <jonathan.druart@bugs.koha-community.org>
Wed, 2 Aug 2017 15:21:20 +0000 (12:21 -0300)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 1 Sep 2017 20:05:17 +0000 (17:05 -0300)
This GetReserve subroutine can be replaced with Koha::Holds->find

Test plan:
-  git grep GetReserve
must not return results where GetReserve is called
- Cancel a reserve

Signed-off-by: Marc VĂ©ron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

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

C4/ILSDI/Services.pm
C4/Reserves.pm
Koha/REST/V1/Hold.pm
t/db_dependent/Circulation/issue.t
t/db_dependent/Holds.t
t/db_dependent/Items/MoveItemFromBiblio.t
t/db_dependent/Reserves.t

index 816f1ed..f42a86b 100644 (file)
@@ -771,9 +771,9 @@ sub CancelHold {
 
     # Get the reserve or return an error code
     my $reserve_id = $cgi->param('item_id');
-    my $reserve = C4::Reserves::GetReserve($reserve_id);
-    return { code => 'RecordNotFound' } unless $reserve;
-    return { code => 'RecordNotFound' } unless ($reserve->{borrowernumber} == $borrowernumber);
+    my $hold = Koha::Holds->find( $reserve_id );
+    return { code => 'RecordNotFound' } unless $hold;
+    return { code => 'RecordNotFound' } unless ($hold->borrowernumber == $borrowernumber);
 
     C4::Reserves::CancelReserve({reserve_id => $reserve_id});
 
index 924ce5f..e1b3227 100644 (file)
@@ -465,10 +465,10 @@ sub CanReserveBeCanceledFromOpac {
     my ($reserve_id, $borrowernumber) = @_;
 
     return unless $reserve_id and $borrowernumber;
-    my $reserve = GetReserve($reserve_id);
+    my $reserve = Koha::Holds->find($reserve_id);
 
-    return 0 unless $reserve->{borrowernumber} == $borrowernumber;
-    return 0 if ( $reserve->{found} eq 'W' ) or ( $reserve->{found} eq 'T' );
+    return 0 unless $reserve->borrowernumber == $borrowernumber;
+    return 0 if ( $reserve->found eq 'W' ) or ( $reserve->found eq 'T' );
 
     return 1;
 
@@ -874,48 +874,46 @@ sub CancelReserve {
 
     my $dbh = C4::Context->dbh;
 
-    my $reserve = GetReserve( $reserve_id );
-    if ($reserve) {
+    my $hold = Koha::Holds->find( $reserve_id );
+    return unless $hold;
 
-        my $hold = Koha::Holds->find( $reserve_id );
-        logaction( 'HOLDS', 'CANCEL', $hold->reserve_id, Dumper($hold->unblessed) )
-            if C4::Context->preference('HoldsLog');
+    logaction( 'HOLDS', 'CANCEL', $hold->reserve_id, Dumper($hold->unblessed) )
+        if C4::Context->preference('HoldsLog');
 
-        my $query = "
-            UPDATE reserves
-            SET    cancellationdate = now(),
-                   priority         = 0
-            WHERE  reserve_id = ?
-        ";
-        my $sth = $dbh->prepare($query);
-        $sth->execute( $reserve_id );
+    my $query = "
+        UPDATE reserves
+        SET    cancellationdate = now(),
+               priority         = 0
+        WHERE  reserve_id = ?
+    ";
+    my $sth = $dbh->prepare($query);
+    $sth->execute( $reserve_id );
 
-        $query = "
-            INSERT INTO old_reserves
-            SELECT * FROM reserves
-            WHERE  reserve_id = ?
-        ";
-        $sth = $dbh->prepare($query);
-        $sth->execute( $reserve_id );
+    $query = "
+        INSERT INTO old_reserves
+        SELECT * FROM reserves
+        WHERE  reserve_id = ?
+    ";
+    $sth = $dbh->prepare($query);
+    $sth->execute( $reserve_id );
 
-        $query = "
-            DELETE FROM reserves
-            WHERE  reserve_id = ?
-        ";
-        $sth = $dbh->prepare($query);
-        $sth->execute( $reserve_id );
+    $query = "
+        DELETE FROM reserves
+        WHERE  reserve_id = ?
+    ";
+    $sth = $dbh->prepare($query);
+    $sth->execute( $reserve_id );
 
-        # now fix the priority on the others....
-        _FixPriority({ biblionumber => $reserve->{biblionumber} });
+    # now fix the priority on the others....
+    _FixPriority({ biblionumber => $hold->biblionumber });
 
-        # and, if desired, charge a cancel fee
-        my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
-        if ( $charge && $params->{'charge_cancel_fee'} ) {
-            manualinvoice($reserve->{'borrowernumber'}, $reserve->{'itemnumber'}, '', 'HE', $charge);
-        }
+    # and, if desired, charge a cancel fee
+    my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
+    if ( $charge && $params->{'charge_cancel_fee'} ) {
+        manualinvoice($hold->borrowernumber, $hold->itemnumber, '', 'HE', $charge);
     }
 
-    return $reserve;
+    return $hold->unblessed;
 }
 
 =head2 ModReserve
@@ -1311,16 +1309,17 @@ Input: $where is 'up', 'down', 'top' or 'bottom'. Biblionumber, Date reserve was
 sub AlterPriority {
     my ( $where, $reserve_id ) = @_;
 
-    my $reserve = GetReserve( $reserve_id );
+    my $hold = Koha::Holds->find( $reserve_id );
+    return unless $hold;
 
-    if ( $reserve->{cancellationdate} ) {
-        warn "I cannot alter the priority for reserve_id $reserve_id, the reserve has been cancelled (".$reserve->{cancellationdate}.')';
+    if ( $hold->cancellationdate ) {
+        warn "I cannot alter the priority for reserve_id $reserve_id, the reserve has been cancelled (" . $hold->cancellationdate . ')';
         return;
     }
 
     if ( $where eq 'up' || $where eq 'down' ) {
 
-      my $priority = $reserve->{'priority'};
+      my $priority = $hold->priority;
       $priority = $where eq 'up' ? $priority - 1 : $priority + 1;
       _FixPriority({ reserve_id => $reserve_id, rank => $priority })
 
@@ -1333,6 +1332,7 @@ sub AlterPriority {
       _FixPriority({ reserve_id => $reserve_id, rank => '999999' });
 
     }
+    # FIXME Should return the new priority
 }
 
 =head2 ToggleLowestPriority
@@ -1468,8 +1468,8 @@ sub _FixPriority {
     my $dbh = C4::Context->dbh;
 
     unless ( $biblionumber ) {
-        my $res = GetReserve( $reserve_id );
-        $biblionumber = $res->{biblionumber};
+        my $hold = Koha::Holds->find( $reserve_id );
+        $biblionumber = $hold->biblionumber;
     }
 
     if ( $rank eq "del" ) {
index 9128c24..a31a9e6 100644 (file)
@@ -118,11 +118,10 @@ sub edit {
     my ($c, $args, $cb) = @_;
 
     my $reserve_id = $args->{reserve_id};
-    my $reserve = C4::Reserves::GetReserve($reserve_id);
+    my $hold = Koha::Holds->find( $reserve_id );
 
-    unless ($reserve) {
-        return $c->$cb({error => "Reserve not found"}, 404);
-    }
+    return $c->$cb({error => "Reserve not found"}, 404)
+        unless $hold;
 
     my $body = $c->req->json;
 
@@ -142,20 +141,19 @@ sub edit {
     };
 
     C4::Reserves::ModReserve($params);
-    $reserve = Koha::Holds->find($reserve_id);
+    $hold = Koha::Holds->find($reserve_id);
 
-    return $c->$cb($reserve, 200);
+    return $c->$cb($hold, 200);
 }
 
 sub delete {
     my ($c, $args, $cb) = @_;
 
     my $reserve_id = $args->{reserve_id};
-    my $reserve = C4::Reserves::GetReserve($reserve_id);
+    my $hold = Koha::Holds->find( $reserve_id );
 
-    unless ($reserve) {
-        return $c->$cb({error => "Reserve not found"}, 404);
-    }
+    return $c->$cb({error => "Reserve not found"}, 404)
+        unless $hold;
 
     C4::Reserves::CancelReserve({ reserve_id => $reserve_id });
 
index fde7613..2f12201 100644 (file)
@@ -31,6 +31,7 @@ use C4::Members;
 use C4::Reserves;
 use Koha::Database;
 use Koha::DateUtils;
+use Koha::Holds;
 use Koha::Library;
 use Koha::Patrons;
 
@@ -372,8 +373,8 @@ my $reserve_id = AddReserve($branchcode_1, $borrower_id1, $biblionumber,
     undef,  1, undef, undef, "a note", "a title", undef, '');
 ok( $reserve_id, 'The reserve should have been inserted' );
 AddIssue( $borrower_2, $barcode_1, dt_from_string, 'cancel' );
-my $reserve = GetReserve( $reserve_id );
-is( $reserve, undef, 'The reserve should have been correctly cancelled' );
+my $hold = Koha::Holds->find( $reserve_id );
+is( $hold, undef, 'The reserve should have been correctly cancelled' );
 
 #End transaction
 $schema->storage->txn_rollback;
index 871d687..3d41670 100755 (executable)
@@ -145,22 +145,22 @@ ModReserve({
     suspend_until => output_pref( { dt => dt_from_string( "2013-01-01", "iso" ), dateonly => 1 } ),
 });
 
-my $reserve = GetReserve( $reserve_id );
-ok( $reserve->{'priority'} eq '4', "Test GetReserve(), priority changed correctly" );
-ok( $reserve->{'suspend'}, "Test GetReserve(), suspend hold" );
-is( $reserve->{'suspend_until'}, '2013-01-01 00:00:00', "Test GetReserve(), suspend until date" );
+$hold = Koha::Holds->find( $reserve_id );
+ok( $hold->priority eq '4', "Test ModReserve, priority changed correctly" );
+ok( $hold->suspend, "Test ModReserve, suspend hold" );
+is( $hold->suspend_until, '2013-01-01 00:00:00', "Test ModReserve, suspend until date" );
 
 ToggleSuspend( $reserve_id );
-$reserve = GetReserve( $reserve_id );
-ok( !$reserve->{'suspend'}, "Test ToggleSuspend(), no date" );
+$hold = Koha::Holds->find( $reserve_id );
+ok( ! $hold->suspend, "Test ToggleSuspend(), no date" );
 
 ToggleSuspend( $reserve_id, '2012-01-01' );
-$reserve = GetReserve( $reserve_id );
-is( $reserve->{'suspend_until'}, '2012-01-01 00:00:00', "Test ToggleSuspend(), with date" );
+$hold = Koha::Holds->find( $reserve_id );
+is( $hold->suspend_until, '2012-01-01 00:00:00', "Test ToggleSuspend(), with date" );
 
 AutoUnsuspendReserves();
-$reserve = GetReserve( $reserve_id );
-ok( !$reserve->{'suspend'}, "Test AutoUnsuspendReserves()" );
+$hold = Koha::Holds->find( $reserve_id );
+ok( ! $hold->suspend, "Test AutoUnsuspendReserves()" );
 
 SuspendAll(
     borrowernumber => $borrowernumber,
@@ -168,18 +168,18 @@ SuspendAll(
     suspend => 1,
     suspend_until => '2012-01-01',
 );
-$reserve = GetReserve( $reserve_id );
-is( $reserve->{'suspend'}, 1, "Test SuspendAll()" );
-is( $reserve->{'suspend_until'}, '2012-01-01 00:00:00', "Test SuspendAll(), with date" );
+$hold = Koha::Holds->find( $reserve_id );
+is( $hold->suspend, 1, "Test SuspendAll()" );
+is( $hold->suspend_until, '2012-01-01 00:00:00', "Test SuspendAll(), with date" );
 
 SuspendAll(
     borrowernumber => $borrowernumber,
     biblionumber   => $biblionumber,
     suspend => 0,
 );
-$reserve = GetReserve( $reserve_id );
-is( $reserve->{'suspend'}, 0, "Test resuming with SuspendAll()" );
-is( $reserve->{'suspend_until'}, undef, "Test resuming with SuspendAll(), should have no suspend until date" );
+$hold = Koha::Holds->find( $reserve_id );
+is( $hold->suspend, 0, "Test resuming with SuspendAll()" );
+is( $hold->suspend_until, undef, "Test resuming with SuspendAll(), should have no suspend until date" );
 
 # Add a new hold for the borrower whose hold we canceled earlier, this time at the bib level
 AddReserve(
@@ -204,27 +204,27 @@ my $reserveid = C4::Reserves::GetReserveId(
     }
 );
 is( $reserveid, $holds->next->reserve_id, "Test GetReserveId" );
-ModReserveMinusPriority( $itemnumber, $reserve->{'reserve_id'} );
+ModReserveMinusPriority( $itemnumber, $reserveid );
 $holds = $patron->holds;
 is( $holds->next->itemnumber, $itemnumber, "Test ModReserveMinusPriority()" );
 
 $holds = $biblio->holds;
 $hold = $holds->next;
 AlterPriority( 'top', $hold->reserve_id );
-$reserve = GetReserve( $reserve->{'reserve_id'} );
-is( $reserve->{'priority'}, '1', "Test AlterPriority(), move to top" );
+$hold = Koha::Holds->find( $reserveid );
+is( $hold->priority, '1', "Test AlterPriority(), move to top" );
 
-AlterPriority( 'down', $reserve->{'reserve_id'} );
-$reserve = GetReserve( $reserve->{'reserve_id'} );
-is( $reserve->{'priority'}, '2', "Test AlterPriority(), move down" );
+AlterPriority( 'down', $hold->reserve_id );
+$hold = Koha::Holds->find( $reserveid );
+is( $hold->priority, '2', "Test AlterPriority(), move down" );
 
-AlterPriority( 'up', $reserve->{'reserve_id'} );
-$reserve = GetReserve( $reserve->{'reserve_id'} );
-is( $reserve->{'priority'}, '1', "Test AlterPriority(), move up" );
+AlterPriority( 'up', $hold->reserve_id );
+$hold = Koha::Holds->find( $reserveid );
+is( $hold->priority, '1', "Test AlterPriority(), move up" );
 
-AlterPriority( 'bottom', $reserve->{'reserve_id'} );
-$reserve = GetReserve( $reserve->{'reserve_id'} );
-is( $reserve->{'priority'}, '5', "Test AlterPriority(), move to bottom" );
+AlterPriority( 'bottom', $hold->reserve_id );
+$hold = Koha::Holds->find( $reserveid );
+is( $hold->priority, '5', "Test AlterPriority(), move to bottom" );
 
 # Regression test for bug 2394
 #
@@ -315,8 +315,8 @@ my $reserveid2 = C4::Reserves::GetReserveId(
 
 CancelReserve({ reserve_id => $reserveid1 });
 
-my $reserve2 = GetReserve( $reserveid2 );
-is( $reserve2->{priority}, 1, "After cancelreserve, the 2nd reserve becomes the first on the waiting list" );
+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(
@@ -333,12 +333,12 @@ my $reserveid3 = C4::Reserves::GetReserveId(
     }
 );
 
-my $reserve3 = GetReserve( $reserveid3 );
-is( $reserve3->{priority}, 2, "New reserve for patron 0, the reserve has a priority = 2" );
+my $hold3 = Koha::Holds->find( $reserveid3 );
+is( $hold3->priority, 2, "New reserve for patron 0, the reserve has a priority = 2" );
 
 ModReserve({ reserve_id => $reserveid2, rank => 'del' });
-$reserve3 = GetReserve( $reserveid3 );
-is( $reserve3->{priority}, 1, "After ModReserve, the 3rd reserve becomes the first on the waiting list" );
+$hold3 = Koha::Holds->find( $reserveid3 );
+is( $hold3->priority, 1, "After ModReserve, the 3rd reserve becomes the first on the waiting list" );
 
 ModItem({ damaged => 1 }, $item_bibnum, $itemnumber);
 t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 1 );
index d1a9e82..21855d9 100644 (file)
@@ -21,6 +21,7 @@ use Test::More tests => 8;
 use C4::Items;
 use C4::Reserves;
 use Koha::Database;
+use Koha::Holds;
 
 use t::lib::TestBuilder;
 use Data::Dumper qw|Dumper|;
@@ -85,13 +86,13 @@ is( $get_item2->{biblionumber}, $to_biblio->{biblionumber}, 'The item2 should ha
 my $get_item3 = C4::Items::GetItem( $item3->{itemnumber} );
 is( $get_item3->{biblionumber}, $to_biblio->{biblionumber}, 'The item3 should not have been moved' );
 
-my $get_bib_level_hold    = C4::Reserves::GetReserve( $bib_level_hold_not_to_move->{reserve_id} );
-my $get_item_level_hold_1 = C4::Reserves::GetReserve( $item_level_hold_not_to_move->{reserve_id} );
-my $get_item_level_hold_2 = C4::Reserves::GetReserve( $item_level_hold_to_move->{reserve_id} );
+my $get_bib_level_hold    = Koha::Holds->find( $bib_level_hold_not_to_move->{reserve_id} );
+my $get_item_level_hold_1 = Koha::Holds->find( $item_level_hold_not_to_move->{reserve_id} );
+my $get_item_level_hold_2 = Koha::Holds->find( $item_level_hold_to_move->{reserve_id} );
 
-is( $get_bib_level_hold->{biblionumber},    $from_biblio->{biblionumber}, 'MoveItemFromBiblio should not have moved the biblio-level hold' );
-is( $get_item_level_hold_1->{biblionumber}, $from_biblio->{biblionumber}, 'MoveItemFromBiblio should not have moved the item-level hold placed on item 1' );
-is( $get_item_level_hold_2->{biblionumber}, $to_biblio->{biblionumber},   'MoveItemFromBiblio should have moved the item-level hold placed on item 2' );
+is( $get_bib_level_hold->biblionumber,    $from_biblio->{biblionumber}, 'MoveItemFromBiblio should not have moved the biblio-level hold' );
+is( $get_item_level_hold_1->biblionumber, $from_biblio->{biblionumber}, 'MoveItemFromBiblio should not have moved the item-level hold placed on item 1' );
+is( $get_item_level_hold_2->biblionumber, $to_biblio->{biblionumber},   'MoveItemFromBiblio should have moved the item-level hold placed on item 2' );
 
 $schema->storage->txn_rollback;
 
index 99f42b2..fc55a18 100755 (executable)
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 72;
+use Test::More tests => 70;
 use Test::MockModule;
 use Test::Warn;
 
@@ -320,16 +320,12 @@ $holds = $biblio->holds;
 is($holds->count, 1, "Only one reserves for this biblio");
 my $reserve_id = $holds->next->reserve_id;
 
-$reserve = GetReserve($reserve_id);
-isa_ok($reserve, 'HASH', "GetReserve return");
-is($reserve->{biblionumber}, $biblionumber);
-
 $reserve = CancelReserve({reserve_id => $reserve_id});
 isa_ok($reserve, 'HASH', "CancelReserve return");
 is($reserve->{biblionumber}, $biblionumber);
 
-$reserve = GetReserve($reserve_id);
-is($reserve, undef, "GetReserve returns undef after deletion");
+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");