From 47db834c8118a7bf436777b40ebcffa8a31c9c55 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Thu, 13 Jun 2013 09:51:51 +0000 Subject: [PATCH] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReservedFromId and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Signed-off-by: Benjamin Rokseth Rebased. Hopefully it applies now. Fixed test t/db_dependant/Reserves.t and updated return reference in CancelReserveFromId. Tested via ILS-DI : placing hold (HoldItem) checking patron info (GetPatronInfo) cancelling hold (CancelHold) Verified that CancelHold parameter item_id takes reserve_id May need additional testing and sign-off Signed-off-by: Kyle M Hall --- C4/ILSDI/Services.pm | 28 ++++++---------------- C4/Reserves.pm | 55 +++++++++++++++++++++++++++++++++++++++++++++ t/db_dependent/Reserves.t | 21 ++++++++++++++++- 3 files changed, 83 insertions(+), 21 deletions(-) mode change 100755 => 100644 t/db_dependent/Reserves.t diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index bd201d3..f2a5111 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -26,7 +26,7 @@ use C4::Circulation; use C4::Branch; use C4::Accounts; use C4::Biblio; -use C4::Reserves qw(AddReserve CancelReserve GetReservesFromBiblionumber GetReservesFromBorrowernumber CanBookBeReserved CanItemBeReserved); +use C4::Reserves qw(AddReserve GetReservesFromBiblionumber GetReservesFromBorrowernumber CanBookBeReserved CanItemBeReserved); use C4::Context; use C4::AuthoritiesMarc; use XML::Simple; @@ -724,7 +724,7 @@ Parameters: - patron_id (Required) a borrowernumber - item_id (Required) - an itemnumber + a reserve_id =cut @@ -736,25 +736,13 @@ sub CancelHold { my $borrower = GetMemberDetails( $borrowernumber ); return { code => 'PatronNotFound' } unless $$borrower{borrowernumber}; - # Get the item or return an error code - my $itemnumber = $cgi->param('item_id'); - my $item = GetItem( $itemnumber ); - return { code => 'RecordNotFound' } unless $$item{itemnumber}; - - # Get borrower's reserves - my @reserves = GetReservesFromBorrowernumber( $borrowernumber, undef ); - my @reserveditems; - - # ...and loop over it to build an array of reserved itemnumbers - foreach my $reserve (@reserves) { - push @reserveditems, $reserve->{'itemnumber'}; - } - - # if the item was not reserved by the borrower, returns an error code - return { code => 'NotCanceled' } unless any { $itemnumber eq $_ } @reserveditems; + # 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); - # Cancel the reserve - CancelReserve({ itemnumber => $itemnumber, borrowernumber => $borrowernumber }); + C4::Reserves::CancelReserveFromId($reserve_id); return { code => 'Canceled' }; } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index eb1ae2b..fe9f5f9 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -119,6 +119,7 @@ BEGIN { &CanBookBeReserved &CanItemBeReserved &CancelReserve + &CancelReserveFromId &CancelExpiredReserves &AutoUnsuspendReserves @@ -1062,6 +1063,60 @@ sub CancelReserve { _FixPriority({ biblionumber => $reserve->{biblionumber} }); } +=head2 CancelReserveFromId + + C4::Reserves::CancelReserveFromId($reserve_id); + +Cancel a reserve, given its reserve_id + +=cut + +sub CancelReserveFromId { + my ($reserve_id) = @_; + + my $reserve = GetReserve($reserve_id); + if ($reserve) { + my $dbh = C4::Context->dbh; + + # update the database, removing the record... + my $query = qq{ + UPDATE reserves + SET cancellationdate = NOW(), + found = NULL, + priority = 0 + WHERE reserve_id = ? + }; + my $sth = $dbh->prepare($query); + $sth->execute($reserve_id); + $sth->finish; + + $query = qq{ + INSERT INTO old_reserves + SELECT * FROM reserves + WHERE reserve_id = ? + }; + $sth = $dbh->prepare($query); + my $rv = $sth->execute($reserve_id); + + if ($rv) { + $query = qq{ + DELETE FROM reserves + WHERE reserve_id = ? + }; + $sth = $dbh->prepare($query); + $sth->execute($reserve_id); + } else { + warn "ERROR: Failed to move reserve $reserve_id to table old_reserves."; + return; + } + + # now fix the priority on the others... + _FixPriority({ biblionumber => $reserve->{biblionumber} }); + } + + return $reserve; +} + =head2 ModReserve ModReserve({ rank => $rank, diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t old mode 100755 new mode 100644 index 4b0058c..8401c21 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -2,7 +2,7 @@ use Modern::Perl; -use Test::More tests => 31; +use Test::More tests => 39; use MARC::Record; use DateTime::Duration; @@ -229,6 +229,25 @@ is( $messages->{ResFound}->{borrowernumber}, $requesters{'RPL'}, 'for generous library, its items fill first hold request in line (bug 10272)'); +my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber}); +isa_ok($reserves, 'ARRAY'); +is(scalar @$reserves, 1, "Only one reserves for this biblio"); +my $reserve_id = $reserves->[0]->{reserve_id}; + +$reserve = GetReserve($reserve_id); +isa_ok($reserve, 'HASH', "GetReserve return"); +is($reserve->{biblionumber}, $biblionumber); + +$reserve = CancelReserveFromId($reserve_id); +isa_ok($reserve, 'HASH', "CancelReserveFromId return"); +is($reserve->{biblionumber}, $biblionumber); + +$reserve = GetReserve($reserve_id); +is($reserve, undef, "GetReserve returns undef after deletion"); + +$reserve = CancelReserveFromId($reserve_id); +is($reserve, undef, "CancelReserveFromId return undef if reserve does not exist"); + # 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) # Test 9761a: Add a reserve without date, CheckReserve should return it -- 1.7.2.5