From d5960a55a292225261ed93876746d73dcdec9b99 Mon Sep 17 00:00:00 2001 From: Andrew Isherwood Date: Wed, 9 Oct 2019 15:13:54 +0100 Subject: [PATCH] Bug 23051: (follow-up) Refactor renewal code As per Nick's first point in comment #20, the code that tests for renewability and renews items has been refactored into it's own function. --- Koha/Account.pm | 106 ++++++++++++++++++++++++++++++++------------------- Koha/Account/Line.pm | 46 +++++----------------- 2 files changed, 76 insertions(+), 76 deletions(-) diff --git a/Koha/Account.pm b/Koha/Account.pm index b576f0c5e3..63e0d67e67 100644 --- a/Koha/Account.pm +++ b/Koha/Account.pm @@ -119,13 +119,7 @@ sub pay { # If we need to make a note of the item associated with this line, # in order that we can potentially renew it, do so. - if ( - $new_amountoutstanding == 0 && - $fine->accounttype && - $fine->accounttype eq 'OVERDUE' && - $fine->status && - $fine->status eq 'UNRETURNED' - ) { + if (_fine_paid_renewable($new_amountoutstanding, $fine)) { $overdue_unreturned->{$fine->itemnumber} = $fine; } @@ -191,13 +185,8 @@ sub pay { # If we need to make a note of the item associated with this line, # in order that we can potentially renew it, do so. - if ( - $old_amountoutstanding - $amount_to_pay == 0 && - $fine->accounttype && - $fine->accounttype eq 'OVERDUE' && - $fine->status && - $fine->status eq 'UNRETURNED' - ) { + my $amt = $old_amountoutstanding - $amount_to_pay; + if (_fine_paid_renewable($amt, $fine)) { $overdue_unreturned->{$fine->itemnumber} = $fine; } @@ -284,32 +273,7 @@ sub pay { # If we have overdue unreturned items that have had payments made # against them, check whether the balance on those items is now zero # and, if the syspref is set, renew them - # Same logic exists in Koha::Account::Line::apply - if ( - C4::Context->preference('RenewAccruingItemWhenPaid') && - keys %{$overdue_unreturned} - ) { - foreach my $itemnumber (keys %{$overdue_unreturned}) { - # Only do something if this item has no fines left on it - my $fine = C4::Overdues::GetFine( $itemnumber, $self->{patron_id} ); - next if $fine && $fine > 0; - - my ( $renew_ok, $error ) = - C4::Circulation::CanBookBeRenewed( - $self->{patron_id}, $itemnumber - ); - if ( $renew_ok ) { - C4::Circulation::AddRenewal( - $self->{patron_id}, - $itemnumber, - $library_id, - undef, - undef, - 1 - ); - } - } - } + _maybe_renew($overdue_unreturned, $self->{patron_id}); if ( C4::Context->preference("FinesLog") ) { logaction( @@ -760,6 +724,68 @@ sub reconcile_balance { return $self; } +=head3 _fine_paid_renewable + +my $bool = _fine_paid_renewable($amt, $fine); + +Given an outstanding amount and a fine object, determine if this item +is potentially renewable as a result of the fine being paid off + +=cut + +sub _fine_paid_renewable { + my ($amt, $fine) = @_; + + return ( + $amt == 0 && + $fine->accounttype && + $fine->accounttype eq 'OVERDUE' && + $fine->status && + $fine->status eq 'UNRETURNED' + ) ? 1 : 0; +} + +=head3 _maybe_renew + +my $result = _maybe_renew($overdue_unreturned, $patron_id, $library_id); + +If we have overdue unreturned items that have had payments made +against them, check whether the balance on those items is now zero +and, if the syspref is set, renew them + +=cut + +sub _maybe_renew { + my ($items, $patron_id) = @_; + + if ( + C4::Context->preference('RenewAccruingItemWhenPaid') && + keys %{$items} + ) { + foreach my $itemnumber (keys %{$items}) { + # Only do something if this item has no fines left on it + my $fine = C4::Overdues::GetFine( $itemnumber, $patron_id ); + next if $fine && $fine > 0; + + my ( $renew_ok, $error ) = + C4::Circulation::CanBookBeRenewed($patron_id, $itemnumber); + if ( $renew_ok ) { + my $due_date = C4::Circulation::AddRenewal( + $patron_id, + $itemnumber, + $items->{$itemnumber}->{branchcode}, + undef, + undef, + 1 + ); + return $due_date; + } else { + return $error; + } + } + } +} + 1; =head2 Name mappings diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm index 19351a0c19..3793eba733 100644 --- a/Koha/Account/Line.pm +++ b/Koha/Account/Line.pm @@ -23,6 +23,7 @@ use Data::Dumper; use C4::Log qw(logaction); use C4::Overdues qw(GetFine); +use Koha::Account qw( _fine_paid_renewable _maybe_renew ); use Koha::Account::Offsets; use Koha::Database; use Koha::Exceptions::Account; @@ -243,16 +244,11 @@ sub apply { # If we need to make a note of the item associated with this line, # in order that we can potentially renew it, do so. - # Same logic existing in Koha::Account::pay - if ( - $debit->amountoutstanding == 0 && - $debit->accounttype && - $debit->accounttype eq 'OVERDUE' && - $debit->status && - $debit->status eq 'UNRETURNED' - ) { - $overdue_unreturned->{$debit->itemnumber} = $debit; - } + my $renewable = Koha::Account::_fine_paid_renewable( + $debit->amountoutstanding, + $debit + ); + $overdue_unreturned->{$debit->itemnumber} = $debit if $renewable; # Same logic exists in Koha::Account::pay if ( $debit->amountoutstanding == 0 @@ -269,32 +265,10 @@ sub apply { # If we have overdue unreturned items that have had payments made # against them, check whether the balance on those items is now zero # and, if the syspref is set, renew them - # Same logic existing in Koha::Account::pay - if ( - C4::Context->preference('RenewAccruingItemWhenPaid') && - keys %{$overdue_unreturned} - ) { - foreach my $itemnumber (keys %{$overdue_unreturned}) { - # Only do something if this item has no fines left on it - my $fine = C4::Overdues::GetFine( $itemnumber, $self->borrowernumber ); - next if $fine && $fine > 0; - - my ( $renew_ok, $error ) = - C4::Circulation::CanBookBeRenewed( - $self->borrowernumber, $itemnumber - ); - if ( $renew_ok ) { - C4::Circulation::AddRenewal( - $self->borrowernumber, - $itemnumber, - $overdue_unreturned->{$itemnumber}->{branchcode}, - undef, - undef, - 1 - ); - } - } - } + Koha::Account::_maybe_renew( + $overdue_unreturned, + $self->borrowernumber + ); return $available_credit; } -- 2.11.0