From 7e5898e9858c2cc83a986d1a8adfc884597ef2e5 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 2 Aug 2017 12:21:20 -0300 Subject: [PATCH] Bug 19057: Move C4::Reserve::GetReserve to Koha::Holds 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 --- C4/ILSDI/Services.pm | 6 +-- C4/Reserves.pm | 86 +++++++++++++++---------------- Koha/REST/V1/Hold.pm | 18 +++---- t/db_dependent/Circulation/issue.t | 5 +- t/db_dependent/Holds.t | 68 ++++++++++++------------ t/db_dependent/Items/MoveItemFromBiblio.t | 13 ++--- t/db_dependent/Reserves.t | 10 ++-- 7 files changed, 101 insertions(+), 105 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 5d35b2b1b4..dd526195a9 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -769,9 +769,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}); diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 924ce5fde6..e1b322718c 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -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" ) { diff --git a/Koha/REST/V1/Hold.pm b/Koha/REST/V1/Hold.pm index 9128c24d11..a31a9e6e8f 100644 --- a/Koha/REST/V1/Hold.pm +++ b/Koha/REST/V1/Hold.pm @@ -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 }); diff --git a/t/db_dependent/Circulation/issue.t b/t/db_dependent/Circulation/issue.t index 5adb123665..ed8b3c86a0 100644 --- a/t/db_dependent/Circulation/issue.t +++ b/t/db_dependent/Circulation/issue.t @@ -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; @@ -373,8 +374,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; diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 871d68704f..3d416700cb 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -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 ); diff --git a/t/db_dependent/Items/MoveItemFromBiblio.t b/t/db_dependent/Items/MoveItemFromBiblio.t index 29078ca793..44bb318fd3 100644 --- a/t/db_dependent/Items/MoveItemFromBiblio.t +++ b/t/db_dependent/Items/MoveItemFromBiblio.t @@ -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; diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 810050efc0..94c3c944c2 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 72; +use Test::More tests => 70; use Test::MockModule; use Test::Warn; @@ -321,16 +321,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"); -- 2.11.0