Bug 8336 Support Sip Renewal Transaction

Renewals were being rejected for incorrect reasons
Checking was being done against the wrong object
Add more informative messages on failure
Correctly set due_date for renewal response
Avoid crashing the SIPServer because it handles RenewAll
incorrectly

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>

diff --git a/C4/SIP/ILS.pm b/C4/SIP/ILS.pm
index f046568..5d59850 100644 (file)
--- a/C4/SIP/ILS.pm
+++ b/C4/SIP/ILS.pm
@@ -428,17 +428,12 @@ sub renew {
     if (!defined($item)) {
                $trans->screen_msg("Item not checked out to " . $patron->name);     # not checked out to $patron_id
         $trans->ok(0);
-    } elsif (!$item->available($patron_id)) {
-               $trans->screen_msg("Item unavailable due to outstanding holds");
-        $trans->ok(0);
     } else {
-               $trans->renewal_ok(1);
-               $trans->desensitize(0); # It's already checked out
-               $trans->do_renew();
-               syslog("LOG_DEBUG", "done renew (ok:%s): %s renews %s", $trans->renewal_ok, $patron_id, $item_id);
-
-#              $item->{due_date} = $nb_due_date if $no_block eq 'Y';
-#              $item->{sip_item_properties} = $item_props if $item_props;
+        $trans->do_renew();
+        if ($trans->renewal_ok()) {
+            $item->{due_date} = $trans->{due};
+            $trans->desensitize(0);
+        }
     }
 
     return $trans;

diff --git a/C4/SIP/ILS/Transaction/Renew.pm b/C4/SIP/ILS/Transaction/Renew.pm
index 57db003..73b811f 100644 (file)
--- a/C4/SIP/ILS/Transaction/Renew.pm
+++ b/C4/SIP/ILS/Transaction/Renew.pm
@@ -8,12 +8,11 @@ use warnings;
 use strict;
 
 use ILS;
-use ILS::Transaction;
 
 use C4::Circulation;
 use C4::Members;
 
-our @ISA = qw(ILS::Transaction);
+use base qw(ILS::Transaction);
 
 my %fields = (
        renewal_ok => 0,
@@ -36,21 +35,26 @@ sub do_renew_for  {
        my $borrower = shift;
        my ($renewokay,$renewerror) = CanBookBeRenewed($borrower->{borrowernumber},$self->{item}->{itemnumber});
        if ($renewokay){
-               $self->{due} = AddIssue( $borrower, $self->{item}->id, undef, 0 );
-               $self->renewal_ok(1);
+        $self->{due} = undef;
+        my $due_date = AddIssue( $borrower, $self->{item}->id, undef, 0 );
+        if ($due_date) {
+            $self->{due} = $due_date;
+        }
+        $self->renewal_ok(1);
        } else {
-               $self->screen_msg(($self->screen_msg || '') . " " . $renewerror);
+        $renewerror=~s/on_reserve/Item unavailable due to outstanding holds/;
+        $renewerror=~s/too_many/Item has reached maximum renewals/;
+               $self->screen_msg($renewerror);
                $self->renewal_ok(0);
        }
-    $! and warn "do_renew_for error: $!";
-       $self->ok(1) unless $!;
-       return $self;
+       $self->ok(1);
+       return;
 }
 
 sub do_renew {
-       my $self = shift;
-       my $borrower = GetMember( 'cardnumber'=>$self->{patron}->id);
-       return $self->do_renew_for($borrower);
-}      
+    my $self = shift;
+    my $borrower = GetMember( cardnumber => $self->{patron}->id );
+    return $self->do_renew_for($borrower);
+}
 
 1;

diff --git a/C4/SIP/ILS/Transaction/RenewAll.pm b/C4/SIP/ILS/Transaction/RenewAll.pm
index adc467a..c7be96b 100644 (file)
--- a/C4/SIP/ILS/Transaction/RenewAll.pm
+++ b/C4/SIP/ILS/Transaction/RenewAll.pm
@@ -1,4 +1,4 @@
-# 
+#
 # RenewAll: class to manage status of "Renew All" transaction
 
 package ILS::Transaction::RenewAll;
@@ -9,57 +9,63 @@ use warnings;
 use Sys::Syslog qw(syslog);
 
 use ILS::Item;
-use ILS::Transaction::Renew;
 
-use C4::Members;       # GetMember
+use C4::Members qw( GetMember );
 
-our @ISA = qw(ILS::Transaction::Renew);
+use base qw(ILS::Transaction::Renew);
 
 my %fields = (
-         renewed => [],
-       unrenewed => [],
+    renewed   => [],
+    unrenewed => [],
 );
 
 sub new {
-       my $class = shift;
-       my $self = $class->SUPER::new();
+    my $class = shift;
+    my $self  = $class->SUPER::new();
 
-    foreach my $element (keys %fields) {
-               $self->{_permitted}->{$element} = $fields{$element};
-       }
+    foreach my $element ( keys %fields ) {
+        $self->{_permitted}->{$element} = $fields{$element};
+    }
 
-       @{$self}{keys %fields} = values %fields;
-       return bless $self, $class;
+    @{$self}{ keys %fields } = values %fields;
+    return bless $self, $class;
 }
 
 sub do_renew_all {
-       my $self = shift;
-       my $patron = $self->{patron};                                                   # SIP's  patron
-       my $borrower = GetMember('cardnumber'=>$patron->id);    # Koha's patron
-       my $all_ok = 1;
-    $self->{renewed} = [];
+    my $self     = shift;
+    my $patron   = $self->{patron};                           # SIP's  patron
+    my $borrower = GetMember( cardnumber => $patron->id );    # Koha's patron
+    my $all_ok   = 1;
+    $self->{renewed}   = [];
     $self->{unrenewed} = [];
-       foreach my $itemx (@{$patron->{items}}) {
-               my $item_id = $itemx->{barcode};
-               my $item = new ILS::Item $item_id;
-               if (!defined($item)) {
-                       syslog("LOG_WARNING",
-                               "renew_all: Invalid item id '%s' associated with patron '%s'",
-                               $item_id, $patron->id);
-                       $all_ok = 0;
-                       next;
-               }
-               $self->{item} = $item;
-               $self->do_renew_for($borrower);
-               if ($self->ok) {
-                   $item->{due_date} = $self->{due}->clone();
-                   push @{$self->renewed  }, $item_id;
-               } else {
-            push @{$self->{unrenewed}}, $item_id;
-               }
-       }
-       $self->ok($all_ok);
-       return $self;
+    foreach my $itemx ( @{ $patron->{items} } ) {
+        my $item_id = $itemx->{barcode};
+        my $item    = ILS::Item->new($item_id);
+        if ( !defined($item) ) {
+            syslog(
+                'LOG_WARNING',
+                q|renew_all: Invalid item id '%s' associated with patron '%s'|,
+                $item_id,
+                $patron->id
+            );
+
+            # $all_ok = 0; Do net set as still ok
+            push @{ $self->unrenewed }, $item_id;
+            next;
+        }
+        $self->{item} = $item;
+        $self->do_renew_for($borrower);
+        if ( $self->renewal_ok ) {
+            $item->{due_date} = $self->{due};
+            push @{ $self->{renewed} }, $item_id;
+        }
+        else {
+            push @{ $self->{unrenewed} }, $item_id;
+        }
+        $self->screen_msg(q{});    # clear indiv message
+    }
+    $self->ok($all_ok);
+    return $self;
 }
 
 1;

diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm
index c8b39a3..43f48dc 100644 (file)
--- a/C4/SIP/Sip/MsgType.pm
+++ b/C4/SIP/Sip/MsgType.pm
@@ -1346,7 +1346,7 @@ sub handle_renew {
     $patron = $status->patron;
     $item   = $status->item;
 
-    if ($status->ok) {
+    if ($status->renewal_ok) {
        $resp .= '1';
        $resp .= $status->renewal_ok ? 'Y' : 'N';
        if ($ils->supports('magnetic media')) {
@@ -1359,7 +1359,11 @@ sub handle_renew {
        $resp .= add_field(FID_PATRON_ID, $patron->id);
        $resp .= add_field(FID_ITEM_ID,  $item->id);
        $resp .= add_field(FID_TITLE_ID, $item->title_id);
-       $resp .= add_field(FID_DUE_DATE, Sip::timestamp($item->due_date));
+    if ($item->due_date) {
+        $resp .= add_field(FID_DUE_DATE, Sip::timestamp($item->due_date));
+    } else {
+        $resp .= add_field(FID_DUE_DATE, q{});
+    }
        if ($ils->supports('security inhibit')) {
            $resp .= add_field(FID_SECURITY_INHIBIT,
                               $status->security_inhibit);