From 2fa6852de7c8afac7ff1ee7ede2bb02c5e7a7817 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 --- C4/ILSDI/Services.pm | 30 ++++++++----------------- C4/Reserves.pm | 57 ++++++++++++++++++++++++++++++++++++++++++++++- t/db_dependent/Reserves.t | 21 ++++++++++++++++- 3 files changed, 85 insertions(+), 23 deletions(-) mode change 100755 => 100644 t/db_dependent/Reserves.t diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index bd201d3..0df6680 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,26 +736,14 @@ 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; - - # Cancel the reserve - CancelReserve({ itemnumber => $itemnumber, borrowernumber => $borrowernumber }); + # 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); + C4::Reserves::CancelReserveFromId($reserve_id); + return { code => 'Canceled' }; } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 7a914e3..5a21c9a 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -119,6 +119,7 @@ BEGIN { &CanBookBeReserved &CanItemBeReserved &CancelReserve + &CancelReserveFromId &CancelExpiredReserves &AutoUnsuspendReserves @@ -1059,7 +1060,61 @@ sub CancelReserve { $sth->execute( $reserve_id ); # now fix the priority on the others.... - _FixPriority({ biblionumber => $reserve->{biblionumber} }); + _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 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.8.3.2