From 45f474abdf935c928b4cd20e9823c96ca82af398 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Wed, 3 Oct 2012 13:23:04 +0200 Subject: [PATCH] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. - Added subroutine C4::Reserves::GetReserve - C4::ILSDI::Services::GetRecords now returns the reserve_id - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReserved and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Signed-off-by: Leila and Sonia Signed-off-by: Benjamin Rokseth Signed-off-by: Kyle M Hall Signed-off-by: Chris Cormack Signing off, while noting a style issue in the patch review Signed-off-by: Katrin Fischer Passes tests and QA script. Placed and cancelled a hold using ILS-DI successfully. Adding a follow-up to also update the ils-di documentation page in the bootstrap theme. Signed-off-by: Tomas Cohen Arazi EDIT: I removed the changes it did to the prog theme. --- C4/ILSDI/Services.pm | 30 +++++++---------------- C4/Reserves.pm | 51 +++++++++++++++++++++------------------ t/db_dependent/Reserves.t | 23 ++++++++++++++++-- 3 files changed, 57 insertions(+), 47 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index bd201d3d1a..0b02bc70d6 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; @@ -722,9 +722,9 @@ Cancels an active reserve request for the borrower. Parameters: - patron_id (Required) - a borrowernumber + 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::CancelReserve({reserve_id => $reserve_id}); return { code => 'Canceled' }; } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 8ecd5caebe..baa99ef460 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1063,34 +1063,37 @@ sub CancelReserve { my $dbh = C4::Context->dbh; my $reserve = GetReserve( $reserve_id ); + if ($reserve) { + my $query = " + UPDATE reserves + SET cancellationdate = now(), + found = Null, + priority = 0 + WHERE reserve_id = ? + "; + my $sth = $dbh->prepare($query); + $sth->execute( $reserve_id ); - my $query = " - UPDATE reserves - SET cancellationdate = now(), - found = Null, - 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 => $reserve->{biblionumber} }); + return $reserve; } =head2 ModReserve diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 0698534607..8b5a12ad86 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -2,8 +2,7 @@ use Modern::Perl; -use Test::More tests => 35; - +use Test::More tests => 43; use MARC::Record; use DateTime::Duration; @@ -257,6 +256,26 @@ 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 = 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"); + +$reserve = CancelReserve({reserve_id => $reserve_id}); +is($reserve, undef, "CancelReserve 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 -- 2.39.5