Bug 8868: ILS-DI: CancelHold needs to take a reserve_id
authorJulian Maurice <julian.maurice@biblibre.com>
Wed, 3 Oct 2012 11:23:04 +0000 (13:23 +0200)
committerTomas Cohen Arazi <tomascohen@gmail.com>
Thu, 18 Sep 2014 12:48:41 +0000 (09:48 -0300)
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 <koha.aixmarseille@gmail.com>
Signed-off-by: Benjamin Rokseth <bensinober@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signing off, while noting a style issue in the patch review

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
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 <tomascohen@gmail.com>

EDIT: I removed the changes it did to the prog theme.

C4/ILSDI/Services.pm
C4/Reserves.pm
t/db_dependent/Reserves.t

index bd201d3..0b02bc7 100644 (file)
@@ -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' };
 }
index 8ecd5ca..baa99ef 100644 (file)
@@ -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
index 0698534..8b5a12a 100755 (executable)
@@ -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