Attachment 30908 Details for Bug 8868 – [PASSED QA] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id

[patch] [PASSED QA] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id

PASSED-QA-Bug-8868-ILS-DI-CancelHold-needs-to-take.patch (text/plain), 7.98 KB, created by Katrin Fischer on 2014-08-18 09:06:48 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Katrin Fischer

Created: 2014-08-18 09:06:48 UTC

Size: 7.98 KB

patch

obsolete

>From 01d4e737f0b08e1a6e129dcac0d10c8b7e90320d Mon Sep 17 00:00:00 2001
>From: Julian Maurice <julian.maurice@biblibre.com>
>Date: Wed, 3 Oct 2012 13:23:04 +0200
>Subject: [PATCH] [PASSED QA] 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 <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.
>---
> 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 a98eed4..6b303b8 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 @@
>                     <dt><strong>patron_id</strong> (Required)</dt>
>                     <dd>the unique patron identifier in the ILS; the same identifier returned by LookupPatron or AuthenticatePatron</dd>
>                     <dt><strong>item_id</strong> (Required)</dt>
>-                    <dd>system item identifier</dd>
>+                    <dd>system hold identifier (returned by GetRecords and GetPatronInfo into element 'reserve_id')</dd>
>                 </dl>
>                 <h4>Example call</h4>
>                 <a href="ilsdi.pl?service=CancelHold&amp;patron_id=1&amp;item_id=1">
>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.9.1

Actions: View | Diff | Splinter Review

Attachments on bug 8868: 12657 | 16313 | 16527 | 18921 | 21337 | 21338 | 22723 | 22724 | 24879 | 26150 | 26271 | 26333 | 26645 | 27023 | 27024 | 27771 | 27947 | 30881 | 30908 | 30909