Bugzilla – Attachment 94611 Details for
Bug 23051
Add ability to optionally renew fine accruing items when all fines on item are paid off
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23051: (follow-up) Refactor methods
Bug-23051-follow-up-Refactor-methods.patch (text/plain), 16.74 KB, created by
Andrew Isherwood
on 2019-10-23 10:59:56 UTC
(
hide
)
Description:
Bug 23051: (follow-up) Refactor methods
Filename:
MIME Type:
Creator:
Andrew Isherwood
Created:
2019-10-23 10:59:56 UTC
Size:
16.74 KB
patch
obsolete
>From 9ef3918e24058c2bd41ef70159968ab986e7ed8b Mon Sep 17 00:00:00 2001 >From: Andrew Isherwood <andrew.isherwood@ptfs-europe.com> >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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=94611">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23051
:
90559
|
90560
|
90561
|
91058
|
91059
|
91060
|
91792
|
91793
|
91794
|
91795
|
92684
|
92685
|
92686
|
92687
|
92688
|
92689
|
93922
|
93923
|
93924
|
93925
|
94020
|
94021
|
94022
|
94023
|
94024
|
94402
|
94434
|
94453
|
94525
|
94604
|
94605
|
94606
|
94607
|
94608
|
94609
|
94610
|
94611
|
94629
|
94649
|
95557
|
95558
|
95559
|
95560
|
100127
|
100128
|
100129
|
100130
|
100131
|
100132
|
100133
|
100134
|
100135
|
100138
|
100172
|
100182
|
100183
|
100184
|
100185
|
100186
|
100187
|
100188
|
100271