From c5d1d9b62393d5f6e03ab2fd03bd2b0f8124354c Mon Sep 17 00:00:00 2001 From: Colin Campbell Date: Fri, 29 Jun 2012 18:43:28 +0100 Subject: [PATCH] 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 Signed-off-by: Paul Poulain --- C4/SIP/ILS.pm | 15 ++---- C4/SIP/ILS/Transaction/Renew.pm | 28 +++++----- C4/SIP/ILS/Transaction/RenewAll.pm | 84 ++++++++++++++++-------------- C4/SIP/Sip/MsgType.pm | 8 ++- 4 files changed, 72 insertions(+), 63 deletions(-) diff --git a/C4/SIP/ILS.pm b/C4/SIP/ILS.pm index f0465680ae..5d5985092a 100644 --- 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 57db003a2b..73b811ffee 100644 --- 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 adc467a529..c7be96b594 100644 --- 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 c8b39a3839..43f48dcf78 100644 --- 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); -- 2.39.5