From fa43d67081b3b55ec02bfcb7613c92061067ae23 Mon Sep 17 00:00:00 2001 From: Andrew Isherwood Date: Mon, 21 Oct 2019 16:07:48 +0100 Subject: [PATCH] Bug 23051: (follow-up) Refactor methods This follow up patch addresses the following parts of Nick's feedback in comment #35: - I don't really like that the functions are internal functions and then exported - I think the pref description should highlight that if 'RenewalPeriodBase' is set to due date, there may be doubled charges --- Koha/Account.pm | 102 +++----------------- Koha/Account/Line.pm | 103 ++++++++++++++++----- ...3051_add_RenewAccruingItemWhenPaid_syspref.perl | 2 +- installer/data/mysql/sysprefs.sql | 2 +- .../prog/en/includes/renew_strings.inc | 2 + .../en/modules/admin/preferences/circulation.pref | 2 +- t/db_dependent/Koha/Account/Lines.t | 77 ++++++++++++++- 7 files changed, 175 insertions(+), 115 deletions(-) diff --git a/Koha/Account.pm b/Koha/Account.pm index 7d64ec7bb9..81b92b0ed1 100644 --- a/Koha/Account.pm +++ b/Koha/Account.pm @@ -95,10 +95,10 @@ sub pay { && !defined($cash_register) ); my @fines_paid; # List of account lines paid on with this payment - # Item numbers that have had a fine paid where the line has a accounttype - # of OVERDUE and a status of UNRETURNED. We might want to try and renew - # these items. - my $overdue_unreturned = {}; + + # The outcome of any attempted item renewals as a result of fines being + # paid off + my $renew_outcomes = []; my $balance_remaining = $amount; # Set it now so we can adjust the amount if necessary $balance_remaining ||= 0; @@ -117,10 +117,11 @@ sub pay { $fine->amountoutstanding($new_amountoutstanding)->store(); $balance_remaining = $balance_remaining - $amount_to_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 (_fine_paid_renewable($new_amountoutstanding, $fine)) { - $overdue_unreturned->{$fine->itemnumber} = $fine; + # Attempt to renew the item associated with this debit if + # appropriate + if ($fine->renewable) { + my $outcome = $fine->renew_item; + push @{$renew_outcomes}, $outcome; } # Same logic exists in Koha::Account::Line::apply @@ -186,8 +187,9 @@ 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. my $amt = $old_amountoutstanding - $amount_to_pay; - if (_fine_paid_renewable($amt, $fine)) { - $overdue_unreturned->{$fine->itemnumber} = $fine; + if ($fine->renewable) { + my $outcome = $fine->renew_item; + push @{$renew_outcomes}, $outcome; } if ( $fine->amountoutstanding == 0 @@ -270,11 +272,6 @@ 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 - my $renew_result = _maybe_renew($overdue_unreturned, $self->{patron_id}); - if ( C4::Context->preference("FinesLog") ) { logaction( "FINES", 'CREATE', @@ -322,7 +319,7 @@ sub pay { } } - return { payment_id => $payment->id, renew_result => $renew_result }; + return { payment_id => $payment->id, renew_result => $renew_outcomes }; } =head3 add_credit @@ -724,79 +721,6 @@ 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) = @_; - - my @results = (); - - 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 ( $can_renew, $error ) = - C4::Circulation::CanBookBeRenewed($patron_id, $itemnumber); - if ( $can_renew ) { - my $due_date = C4::Circulation::AddRenewal( - $patron_id, - $itemnumber, - $items->{$itemnumber}->{branchcode}, - undef, - undef, - 1 - ); - push @results, { - itemnumber => $itemnumber, - due_date => $due_date, - success => 1 - }; - } else { - push @results, { - itemnumber => $itemnumber, - error => $error, - success => 0 - }; - } - } - } - return \@results; -} - 1; =head2 Name mappings diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm index 767ef13cb8..a62601f7fb 100644 --- a/Koha/Account/Line.pm +++ b/Koha/Account/Line.pm @@ -23,7 +23,6 @@ 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; @@ -205,11 +204,6 @@ sub apply { my $schema = Koha::Database->new->schema; - # Item numbers that have had a fine paid where the line has a accounttype - # of OVERDUE and a status of UNRETURNED. We might want to try and renew - # these items. - my $overdue_unreturned = {}; - $schema->txn_do( sub { for my $debit ( @{$debits} ) { @@ -242,14 +236,10 @@ sub apply { $self->amountoutstanding( $available_credit * -1 )->store; $debit->amountoutstanding( $owed - $amount_to_cancel )->store; - # If we need to make a note of the item associated with this line, - # in order that we can potentially renew it, do so. - my $renewable = Koha::Account::_fine_paid_renewable( - $debit->amountoutstanding, - $debit - ); - if ($renewable && $debit->itemnumber) { - $overdue_unreturned->{$debit->itemnumber} = $debit; + # Attempt to renew the item associated with this debit if + # appropriate + if ($debit->renewable) { + $debit->renew_item; } # Same logic exists in Koha::Account::pay @@ -264,14 +254,6 @@ 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 - Koha::Account::_maybe_renew( - $overdue_unreturned, - $self->borrowernumber - ); - return $available_credit; } @@ -423,6 +405,83 @@ sub is_debit { return !$self->is_credit; } +=head3 renewable + + my $bool = $line->renewable; + +=cut + +sub renewable { + my ($self) = @_; + + return ( + $self->amountoutstanding == 0 && + $self->accounttype && + $self->accounttype eq 'OVERDUE' && + $self->status && + $self->status eq 'UNRETURNED' + ) ? 1 : 0; +} + +=head3 renew_item + + my $renew_result = $line->renew_item; + +Conditionally attempt to renew an item and return the outcome. This is +as a consequence of the fine on an item being fully paid off + +=cut + +sub renew_item { + my ($self) = @_; + + my $outcome = {}; + + if ( + C4::Context->preference('RenewAccruingItemWhenPaid') && + $self->item && + $self->patron + ) { + my $itemnumber = $self->item->itemnumber; + my $borrowernumber = $self->patron->borrowernumber; + # Only do something if this item has no fines left on it + my $fine = C4::Overdues::GetFine($itemnumber, $borrowernumber); + if ($fine && $fine > 0) { + return { + itemnumber => $itemnumber, + error => 'has_fine', + success => 0 + }; + } + my ( $can_renew, $error ) = C4::Circulation::CanBookBeRenewed( + $borrowernumber, + $itemnumber + ); + if ( $can_renew ) { + my $due_date = C4::Circulation::AddRenewal( + $borrowernumber, + $itemnumber, + $self->{branchcode}, + undef, + undef, + 1 + ); + return { + itemnumber => $itemnumber, + due_date => $due_date, + success => 1 + }; + } else { + return { + itemnumber => $itemnumber, + error => $error, + success => 0 + }; + } + } + +} + =head2 Internal methods =cut diff --git a/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemWhenPaid_syspref.perl b/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemWhenPaid_syspref.perl index 223e244a7b..af10caf68f 100644 --- a/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemWhenPaid_syspref.perl +++ b/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemWhenPaid_syspref.perl @@ -1,7 +1,7 @@ $DBversion = 'XXX'; # will be replaced by the RM if( CheckVersion( $DBversion ) ) { - $dbh->do( q| INSERT IGNORE INTO systempreferences (variable, value, explanation, options, type) VALUES ('RenewAccruingItemWhenPaid', '0', 'If enabled, when the fines on an item accruing is paid off, attempt to renew that item', '', 'YesNo'); | ); + $dbh->do( q| INSERT IGNORE INTO systempreferences (variable, value, explanation, options, type) VALUES ('RenewAccruingItemWhenPaid', '0', 'If enabled, when the fines on an item accruing is paid off, attempt to renew that item. If the syspref "RenewalPeriodBase" is set to "due date", renewed items may still be overdue', '', 'YesNo'); | ); SetVersion( $DBversion ); print "Upgrade to $DBversion done (Bug 23051 - Add RenewAccruingItemWhenPaid syspref)\n"; diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index 71a8db84b9..d7ba79b313 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -504,7 +504,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('RandomizeHoldsQueueWeight','0',NULL,'if ON, the holds queue in circulation will be randomized, either based on all location codes, or by the location codes specified in StaticHoldsQueueWeight','YesNo'), ('RecordLocalUseOnReturn','0',NULL,'If ON, statistically record returns of unissued items as local use, instead of return','YesNo'), ('RefundLostOnReturnControl','CheckinLibrary','CheckinLibrary|ItemHomeBranch|ItemHoldingBranch','If a lost item is returned, choose which branch to pick rules for refunding.','Choice'), -('RenewAccruingItemWhenPaid','0','','If enabled, when the fines on an item accruing is paid off, attempt to renew that item','YesNo'), +('RenewAccruingItemWhenPaid','0','','If enabled, when the fines on an item accruing is paid off, attempt to renew that item. If the syspref "RenewalPeriodBase" is set to "due date", renewed items may still be overdue','YesNo'), ('RenewalLog','0','','If ON, log information about renewals','YesNo'), ('RenewalPeriodBase','date_due','date_due|now','Set whether the renewal date should be counted from the date_due or from the moment the Patron asks for renewal ','Choice'), ('RenewalSendNotice','0','',NULL,'YesNo'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc index daa0fa6ed3..45274872ed 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc @@ -25,6 +25,8 @@ Item is not allowed renewal [% CASE 'onsite_checkout' %] Item is an onsite checkout +[% CASE 'has_fine' %] + Item has an outstanding fine [% CASE %] Unknown error [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 1ccebaac2d..889ca997ce 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -490,7 +490,7 @@ Circulation: choices: yes: Renew no: "Don't renew" - - the item automatically. + - the item automatically. If the syspref "RenewalPeriodBase" is set to "due date", renewed items may still be overdue. - - pref: ItemsDeniedRenewal type: textarea diff --git a/t/db_dependent/Koha/Account/Lines.t b/t/db_dependent/Koha/Account/Lines.t index 70131ef369..a8d91052b8 100755 --- a/t/db_dependent/Koha/Account/Lines.t +++ b/t/db_dependent/Koha/Account/Lines.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 9; +use Test::More tests => 10; use Test::Exception; use Test::MockModule; @@ -415,6 +415,81 @@ subtest 'Keep account info when related patron, staff or item is deleted' => sub $schema->storage->txn_rollback; }; +subtest 'Renewal related tests' => sub { + + plan tests => 7; + + $schema->storage->txn_begin; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $staff = $builder->build_object( { class => 'Koha::Patrons' } ); + my $item = $builder->build_object({ class => 'Koha::Items' }); + my $issue = $builder->build_object( + { + class => 'Koha::Checkouts', + value => { + itemnumber => $item->itemnumber, + onsite_checkout => 0, + renewals => 99, + auto_renew => 0 + } + } + ); + my $line = Koha::Account::Line->new( + { + borrowernumber => $patron->borrowernumber, + manager_id => $staff->borrowernumber, + itemnumber => $item->itemnumber, + accounttype => "OVERDUE", + status => "UNRETURNED", + amountoutstanding => 0, + interface => 'commandline', + })->store; + + is( $line->renewable, 1, "Item is returned as renewable when it meets the conditions" ); + $line->amountoutstanding(5); + is( $line->renewable, 0, "Item is returned as unrenewable when it has outstanding fine" ); + $line->amountoutstanding(0); + $line->accounttype("VOID"); + is( $line->renewable, 0, "Item is returned as unrenewable when it has the wrong account type" ); + $line->accounttype("OVERDUE"); + $line->status("RETURNED"); + is( $line->renewable, 0, "Item is returned as unrenewable when it has the wrong account status" ); + + + t::lib::Mocks::mock_preference( 'RenewAccruingItemWhenPaid', 0 ); + is ($line->renew_item, 0, 'Attempt to renew fails when syspref is not set'); + t::lib::Mocks::mock_preference( 'RenewAccruingItemWhenPaid', 1 ); + is_deeply( + $line->renew_item, + { + itemnumber => $item->itemnumber, + error => 'too_many', + success => 0 + }, + 'Attempt to renew fails when CanBookBeRenewed returns false' + ); + $issue->delete; + $issue = $builder->build_object( + { + class => 'Koha::Checkouts', + value => { + itemnumber => $item->itemnumber, + onsite_checkout => 0, + renewals => 0, + auto_renew => 0 + } + } + ); + my $called = 0; + my $module = new Test::MockModule('C4::Circulation'); + $module->mock('AddRenewal', sub { $called = 1; }); + $line->renew_item; + is( $called, 1, 'Attempt to renew succeeds when conditions are met' ); + + $schema->storage->txn_rollback; +}; + subtest 'adjust() tests' => sub { plan tests => 29; -- 2.11.0