From afd4a3829ccecdbfb7741f4b64a6b1f2e10e70b2 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 --- C4/ILSDI/Services.pm | 30 +++++---------- C4/Reserves.pm | 51 ++++++++++++++------------ koha-tmpl/opac-tmpl/prog/en/modules/ilsdi.tt | 2 +- t/db_dependent/Reserves.t | 23 +++++++++++- 4 files changed, 58 insertions(+), 48 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index bd201d3..0b02bc7 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 5b0f008..d47d99c 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1032,34 +1032,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/koha-tmpl/opac-tmpl/prog/en/modules/ilsdi.tt b/koha-tmpl/opac-tmpl/prog/en/modules/ilsdi.tt index 9eb09be..2cfb983 100644 --- a/koha-tmpl/opac-tmpl/prog/en/modules/ilsdi.tt +++ b/koha-tmpl/opac-tmpl/prog/en/modules/ilsdi.tt @@ -672,7 +672,7 @@
patron_id (Required)
the unique patron identifier in the ILS; the same identifier returned by LookupPatron or AuthenticatePatron
item_id (Required)
-
system item identifier
+
system hold identifier (returned by GetRecords and GetPatronInfo into element 'reserve_id')

Example call

diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 0698534..8b5a12a 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 -- 1.7.10.4