Bugzilla – Attachment 95560 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: Squash of sign off follow-ups
Bug-23051-Squash-of-sign-off-follow-ups.patch (text/plain), 42.35 KB, created by
Andrew Isherwood
on 2019-11-19 14:48:46 UTC
(
hide
)
Description:
Bug 23051: Squash of sign off follow-ups
Filename:
MIME Type:
Creator:
Andrew Isherwood
Created:
2019-11-19 14:48:46 UTC
Size:
42.35 KB
patch
obsolete
>From 31f1170d59a3852c3b6c8f7d0bf62c773951abba Mon Sep 17 00:00:00 2001 >From: Andrew Isherwood <andrew.isherwood@ptfs-europe.com> >Date: Wed, 9 Oct 2019 15:13:54 +0100 >Subject: [PATCH] Bug 23051: Squash of sign off follow-ups > >Rebasing was a nightmare, so I'm squashing the sign off follow-ups to >ease the pain with any future rebases > >Includes: > >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. > >Bug 23051: (follow-up) Provide feedback >For renewals that fail when a fine is being paid off, this patch causes >any errors to be passed back to the template for display. >Addresses the second point in Nick's comment #20 > >Bug 23051: (follow-up) Fix unit tests >As raised by Nick in comment #35 > >Bug 23051: (follow-up) Fix/improve feedback >This follow up patch addresses the following parts of Nick's feedback in >comment #35: >- it would be nice to get feedback on what was successfully renewed as well >- In general I think I would prefer to see 'ok' and 'not_ok' returned as >a single 'renewal_results' array >- There is no listing of errors if I use the 'pay' button on an >individual fine > >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 > >Bug 23051: (follow-up) Add SIP summary >This follow up patch addresses the following parts of Nick's feedback in >comment #35: >- Ideally SIP would get feedback in a screen message > >Bug 23051: (follow-up) Renewing in OPAC >This follow up patch addresses the following parts of Nick's feedback in >comment #35: >- I am also not sure about the code path if a patron paid fines on the >opac (via paypal etc.) but renewals are not allowed on the opac. > >We've introduced the syspref RenewAccruingItemInOpac (default is off) >which, when enabled, will cause items attached to fines that are paid >off in the OPAC (via payment plugins), to be automatically renewed. >--- > C4/SIP/ILS.pm | 8 +- > C4/SIP/ILS/Transaction/FeePayment.pm | 20 ++- > C4/SIP/Sip/MsgType.pm | 42 +++++- > Koha/Account.pm | 70 +++------- > Koha/Account/Line.pm | 142 ++++++++++++++------- > Koha/REST/V1/Patrons/Account.pm | 2 +- > ..._23051_add_RenewAccruingItemInOpac_syspref.perl | 8 ++ > ...3051_add_RenewAccruingItemWhenPaid_syspref.perl | 2 +- > installer/data/mysql/sysprefs.sql | 3 +- > .../prog/en/includes/renew_results.inc | 12 ++ > .../prog/en/includes/renew_strings.inc | 32 +++++ > .../en/modules/admin/preferences/circulation.pref | 9 +- > .../prog/en/modules/members/boraccount.tt | 1 + > .../intranet-tmpl/prog/en/modules/members/pay.tt | 1 + > members/boraccount.pl | 20 +++ > members/pay.pl | 19 +++ > members/paycollect.pl | 36 +++++- > t/db_dependent/Accounts.t | 16 +-- > t/db_dependent/Koha/Account.t | 6 +- > t/db_dependent/Koha/Account/Lines.t | 81 +++++++++++- > 20 files changed, 397 insertions(+), 133 deletions(-) > create mode 100644 installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemInOpac_syspref.perl > create mode 100644 koha-tmpl/intranet-tmpl/prog/en/includes/renew_results.inc > create mode 100644 koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc > >diff --git a/C4/SIP/ILS.pm b/C4/SIP/ILS.pm >index b543539dd0..e938f24d4d 100644 >--- a/C4/SIP/ILS.pm >+++ b/C4/SIP/ILS.pm >@@ -271,10 +271,14 @@ sub pay_fee { > $trans->screen_msg('Invalid patron barcode.'); > return $trans; > } >- my $ok = $trans->pay( $patron->{borrowernumber}, $fee_amt, $pay_type, $fee_id, $is_writeoff, $disallow_overpayment ); >+ my $trans_result = $trans->pay( $patron->{borrowernumber}, $fee_amt, $pay_type, $fee_id, $is_writeoff, $disallow_overpayment ); >+ my $ok = $trans_result->{ok}; > $trans->ok($ok); > >- return $trans; >+ return { >+ status => $trans, >+ pay_response => $trans_result->{pay_response} >+ }; > } > > sub add_hold { >diff --git a/C4/SIP/ILS/Transaction/FeePayment.pm b/C4/SIP/ILS/Transaction/FeePayment.pm >index 7e19cc26df..152c17610e 100644 >--- a/C4/SIP/ILS/Transaction/FeePayment.pm >+++ b/C4/SIP/ILS/Transaction/FeePayment.pm >@@ -57,13 +57,13 @@ sub pay { > my $account = Koha::Account->new( { patron_id => $borrowernumber } ); > > if ($disallow_overpayment) { >- return 0 if $account->balance < $amt; >+ return { ok => 0 } if $account->balance < $amt; > } > > if ($fee_id) { > my $fee = Koha::Account::Lines->find($fee_id); > if ( $fee ) { >- $account->pay( >+ my $pay_response = $account->pay( > { > amount => $amt, > type => $type, >@@ -72,14 +72,19 @@ sub pay { > interface => C4::Context->interface > } > ); >- return 1; >+ return { >+ ok => 1, >+ pay_response => $pay_response >+ }; > } > else { >- return 0; >+ return { >+ ok => 0 >+ }; > } > } > else { >- $account->pay( >+ my $pay_response = $account->pay( > { > amount => $amt, > type => $type, >@@ -87,7 +92,10 @@ sub pay { > interface => C4::Context->interface > } > ); >- return 1; >+ return { >+ ok => 1, >+ pay_response => $pay_response >+ }; > } > } > >diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm >index 425ff76552..c5123081f1 100644 >--- a/C4/SIP/Sip/MsgType.pm >+++ b/C4/SIP/Sip/MsgType.pm >@@ -20,6 +20,7 @@ use CGI qw ( -utf8 ); > use C4::Auth qw(&check_api_auth); > > use Koha::Patron::Attributes; >+use Koha::Items; > > use UNIVERSAL::can; > >@@ -1103,7 +1104,46 @@ sub handle_fee_paid { > > $ils->check_inst_id( $inst_id, "handle_fee_paid" ); > >- $status = $ils->pay_fee( $patron_id, $patron_pwd, $fee_amt, $fee_type, $pay_type, $fee_id, $trans_id, $currency, $is_writeoff, $disallow_overpayment ); >+ my $pay_result = $ils->pay_fee( $patron_id, $patron_pwd, $fee_amt, $fee_type, $pay_type, $fee_id, $trans_id, $currency, $is_writeoff, $disallow_overpayment ); >+ $status = $pay_result->{status}; >+ my $pay_response = $pay_result->{pay_response}; >+ >+ my $failmap = { >+ "no_item" => "No matching item could be found", >+ "no_checkout" => "Item is not checked out", >+ "too_soon" => "Cannot yet be renewed", >+ "too_many" => "Renewed the maximum number of times", >+ "auto_too_soon" => "Scheduled for automatic renewal and cannot yet be renewed", >+ "auto_too_late" => "Scheduled for automatic renewal and cannot yet be any more", >+ "auto_account_expired" => "Scheduled for automatic renewal and cannot be renewed because the patron's account has expired", >+ "auto_renew" => "Scheduled for automatic renewal", >+ "auto_too_much_oweing" => "Scheduled for automatic renewal", >+ "on_reserve" => "On hold for another patron", >+ "patron_restricted" => "Patron is currently restricted", >+ "item_denied_renewal" => "Item is not allowed renewal", >+ "onsite_checkout" => "Item is an onsite checkout" >+ }; >+ my @success = (); >+ my @fail = (); >+ foreach my $result( @{$pay_response->{renew_result}} ) { >+ my $item = Koha::Items->find({ itemnumber => $result->{itemnumber} }); >+ if ($result->{success}) { >+ push @success, '"' . $item->biblio->title . '"'; >+ } else { >+ push @fail, '"' . $item->biblio->title . '" : ' . $failmap->{$result->{error}}; >+ } >+ } >+ >+ my $msg = ""; >+ if (scalar @success > 0) { >+ $msg.="The following items were renewed: " . join(", ", @success) . ". "; >+ } >+ if (scalar @fail > 0) { >+ $msg.="The following items were not renewed: " . join(", ", @fail) . "."; >+ } >+ if (length $msg > 0) { >+ $status->screen_msg($status->screen_msg . " $msg"); >+ } > > $resp .= ( $status->ok ? 'Y' : 'N' ) . timestamp; > $resp .= add_field( FID_INST_ID, $inst_id, $server ); >diff --git a/Koha/Account.pm b/Koha/Account.pm >index 9df029531f..33b04f6fb4 100644 >--- a/Koha/Account.pm >+++ b/Koha/Account.pm >@@ -96,10 +96,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; >@@ -118,16 +118,14 @@ 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 ( >- $new_amountoutstanding == 0 && >- $fine->accounttype && >- $fine->accounttype eq 'OVERDUE' && >- $fine->status && >- $fine->status eq 'UNRETURNED' >- ) { >- $overdue_unreturned->{$fine->itemnumber} = $fine; >+ # Attempt to renew the item associated with this debit if >+ # appropriate >+ if ($fine->renewable) { >+ # We're ignoring the definition of $interface above, by all >+ # accounts we can't rely on C4::Context::interface, so here >+ # we're only using what we've been explicitly passed >+ my $outcome = $fine->renew_item($params->{interface}); >+ push @{$renew_outcomes}, $outcome if $outcome; > } > > # Same logic exists in Koha::Account::Line::apply >@@ -192,14 +190,10 @@ 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' >- ) { >- $overdue_unreturned->{$fine->itemnumber} = $fine; >+ my $amt = $old_amountoutstanding - $amount_to_pay; >+ if ($fine->renewable) { >+ my $outcome = $fine->renew_item; >+ push @{$renew_outcomes}, $outcome; > } > > if ( $fine->amountoutstanding == 0 >@@ -282,36 +276,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 >- # 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 >- ); >- } >- } >- } >- > if ( C4::Context->preference("FinesLog") ) { > logaction( > "FINES", 'CREATE', >@@ -359,7 +323,7 @@ sub pay { > } > } > >- return $payment->id; >+ return { payment_id => $payment->id, renew_result => $renew_outcomes }; > } > > =head3 add_credit >diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm >index 118916dd1a..78657f558c 100644 >--- a/Koha/Account/Line.pm >+++ b/Koha/Account/Line.pm >@@ -218,11 +218,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} ) { > >@@ -255,17 +250,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. >- # 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; >+ # Attempt to renew the item associated with this debit if >+ # appropriate >+ if ($debit->renewable) { >+ $debit->renew_item($params->{interface}); > } > > # Same logic exists in Koha::Account::pay >@@ -280,36 +268,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 >- # 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 >- ); >- } >- } >- } >- > return $available_credit; > } > >@@ -481,6 +439,98 @@ sub to_api_mapping { > manager_id => 'user_id', > note => 'internal_note', > }; >+ >+=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, $params) = @_; >+ >+ my $outcome = {}; >+ >+ # We want to reject the call to renew if any of these apply: >+ # - The RenewAccruingItemWhenPaid syspref is off >+ # - The line item doesn't have an item attached to it >+ # - The line item doesn't have a patron attached to it >+ # >+ # - The RenewAccruingItemInOpac syspref is off >+ # AND >+ # - There is an interface param passed and it's value is 'opac' >+ >+ if ( >+ !C4::Context->preference('RenewAccruingItemWhenPaid') || >+ !$self->item || >+ !$self->patron || >+ ( >+ !C4::Context->preference('RenewAccruingItemInOpac') && >+ $params->{interface} && >+ $params->{interface} eq 'opac' >+ ) >+ ) { >+ return; >+ } >+ >+ 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 >diff --git a/Koha/REST/V1/Patrons/Account.pm b/Koha/REST/V1/Patrons/Account.pm >index 63888cd082..4ca546287e 100644 >--- a/Koha/REST/V1/Patrons/Account.pm >+++ b/Koha/REST/V1/Patrons/Account.pm >@@ -130,7 +130,7 @@ sub add_credit { > my $outstanding_credit = $credit->amountoutstanding; > if ($debits) { > # pay them! >- $outstanding_credit = $credit->apply({ debits => [ $debits->as_list ], offset_type => 'payment' }); >+ $outstanding_credit = $credit->apply({ debits => [ $debits->as_list ], offset_type => 'payment' })->{outstanding_amount}; > } > > if ($outstanding_credit) { >diff --git a/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemInOpac_syspref.perl b/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemInOpac_syspref.perl >new file mode 100644 >index 0000000000..c118180474 >--- /dev/null >+++ b/installer/data/mysql/atomicupdate/bug_23051_add_RenewAccruingItemInOpac_syspref.perl >@@ -0,0 +1,8 @@ >+$DBversion = 'XXX'; # will be replaced by the RM >+if( CheckVersion( $DBversion ) ) { >+ >+ $dbh->do( q| INSERT IGNORE INTO systempreferences (variable, value, explanation, options, type) VALUES ('RenewAccruingItemInOpac', '0', 'If enabled, when the fines on an item accruing is paid off in the OPAC via a payment plugin, 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 RenewAccruingItemInOpac syspref)\n"; >+} >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 b5e8348846..0e3cd73cf7 100644 >--- a/installer/data/mysql/sysprefs.sql >+++ b/installer/data/mysql/sysprefs.sql >@@ -513,7 +513,8 @@ 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'), >+('RenewAccruingItemInOpac','0','','If enabled, when the fines on an item accruing is paid off in the OPAC via a payment plugin, 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_results.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_results.inc >new file mode 100644 >index 0000000000..4c4ab3c056 >--- /dev/null >+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_results.inc >@@ -0,0 +1,12 @@ >+[% IF renew_results && renew_results.size > 0 %] >+ <div class="alert"> >+ The fines on the following items were paid off, renewal results are displayed below: >+ [% FOREACH result IN renew_results %] >+ [% IF result.success %] >+ <p>[% INCLUDE 'biblio-title.inc' biblio=result.item.biblio %] ( [% result.item.barcode | html %] ): Renewed - due [% result.info | html %]</p> >+ [% ELSE %] >+ <p>[% INCLUDE 'biblio-title.inc' biblio=result.item.biblio %] ( [% result.item.barcode | html %] ): Not renewed - [% INCLUDE 'renew_strings.inc' error=result.info %]</p> >+ [% END %] >+ [% END %] >+ </div> >+[% END %] >diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc >new file mode 100644 >index 0000000000..45274872ed >--- /dev/null >+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc >@@ -0,0 +1,32 @@ >+[% SWITCH error %] >+[% CASE 'no_item' %] >+ No matching item could be found >+[% CASE 'no_checkout' %] >+ Item is not checked out >+[% CASE 'too_soon' %] >+ Cannot yet be renewed >+[% CASE 'too_many' %] >+ Renewed the maximum number of times >+[% CASE 'auto_too_soon' %] >+ Scheduled for automatic renewal and cannot yet be renewed >+[% CASE 'auto_too_late' %] >+ Scheduled for automatic renewal and cannot yet be any more >+[% CASE 'auto_account_expired' %] >+ Scheduled for automatic renewal and cannot be renewed because the patron's account has expired >+[% CASE 'auto_renew' %] >+ Scheduled for automatic renewal >+[% CASE 'auto_too_much_oweing' %] >+ Scheduled for automatic renewal >+[% CASE 'on_reserve' %] >+ On hold for another patron >+[% CASE 'patron_restricted' %] >+ Patron is currently restricted >+[% CASE 'item_denied_renewal' %] >+ 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 9e98ce14b9..d66fcaeba4 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 >@@ -491,7 +491,14 @@ 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. >+ - >+ - If a patron pays off all fines on an overdue item that is accruing fines in the OPAC via a payment plugin >+ - pref: RenewAccruingItemInOpac >+ choices: >+ yes: Renew >+ no: "Don't renew" >+ - 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/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt >index 2ad20bb340..329840d683 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt >@@ -39,6 +39,7 @@ > <li><a href="/cgi-bin/koha/members/mancredit.pl?borrowernumber=[% patron.borrowernumber | uri %]" >Create manual credit</a></li> > </ul> > <div class="tabs-container"> >+[% INCLUDE 'renew_results.inc' renew_results=renew_results %] > <!-- The table with the account items --> > <table id="table_account_fines"> > <thead> >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt >index e2883b371a..6c8388fc95 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt >@@ -40,6 +40,7 @@ > <form action="/cgi-bin/koha/members/pay.pl" method="post" id="pay-fines-form"> > <input type="hidden" name="borrowernumber" id="borrowernumber" value="[% patron.borrowernumber | html %]" /> > <p><span class="checkall"><a id="CheckAll" href="#"><i class="fa fa-check"></i> Select all</a></span> | <span class="clearall"><a id="CheckNone" href="#"><i class="fa fa-remove"></i> Clear all</a></span></p> >+[% INCLUDE 'renew_results.inc' renew_results=renew_results %] > <table id="finest"> > <thead> > <tr> >diff --git a/members/boraccount.pl b/members/boraccount.pl >index 251a3aa565..85f765218c 100755 >--- a/members/boraccount.pl >+++ b/members/boraccount.pl >@@ -23,6 +23,7 @@ > # along with Koha; if not, see <http://www.gnu.org/licenses>. > > use Modern::Perl; >+use URI::Escape; > > use C4::Auth; > use C4::Output; >@@ -31,6 +32,7 @@ use C4::Members; > use C4::Accounts; > use Koha::Patrons; > use Koha::Patron::Categories; >+use Koha::Items; > > my $input=new CGI; > >@@ -51,6 +53,7 @@ my $borrowernumber = $input->param('borrowernumber'); > my $payment_id = $input->param('payment_id'); > my $change_given = $input->param('change_given'); > my $action = $input->param('action') || ''; >+my @renew_results = $input->param('renew_result'); > > my $logged_in_user = Koha::Patrons->find( $loggedinuser ) or die "Not logged in"; > my $patron = Koha::Patrons->find( $borrowernumber ); >@@ -80,6 +83,22 @@ if($total <= 0){ > $totalcredit = 1; > } > >+# Populate an arrayref with everything we need to display any >+# renew errors that occurred based on what we were passed >+my $renew_results_display = []; >+foreach my $renew_result(@renew_results) { >+ my ($itemnumber, $success, $info) = split(/,/, $renew_result); >+ my $item = Koha::Items->find($itemnumber); >+ if ($success) { >+ $info = uri_unescape($info); >+ } >+ push @{$renew_results_display}, { >+ item => $item, >+ success => $success, >+ info => $info >+ }; >+} >+ > $template->param( > patron => $patron, > finesview => 1, >@@ -88,6 +107,7 @@ $template->param( > accounts => \@accountlines, > payment_id => $payment_id, > change_given => $change_given, >+ renew_results => $renew_results_display, > ); > > output_html_with_http_headers $input, $cookie, $template->output; >diff --git a/members/pay.pl b/members/pay.pl >index 5094a5f581..94cccf60cc 100755 >--- a/members/pay.pl >+++ b/members/pay.pl >@@ -39,6 +39,7 @@ use C4::Stats; > use C4::Koha; > use C4::Overdues; > use Koha::Patrons; >+use Koha::Items; > > use Koha::Patron::Categories; > use URI::Escape; >@@ -65,6 +66,7 @@ if ( !$borrowernumber ) { > > my $payment_id = $input->param('payment_id'); > our $change_given = $input->param('change_given'); >+our @renew_results = $input->multi_param('renew_result'); > > # get borrower details > my $logged_in_user = Koha::Patrons->find( $loggedinuser ) or die "Not logged in"; >@@ -135,10 +137,27 @@ for (@names) { > } > } > >+# Populate an arrayref with everything we need to display any >+# renew results that occurred based on what we were passed >+my $renew_results_display = []; >+foreach my $renew_result(@renew_results) { >+ my ($itemnumber, $success, $info) = split(/,/, $renew_result); >+ my $item = Koha::Items->find($itemnumber); >+ if ($success) { >+ $info = uri_unescape($info); >+ } >+ push @{$renew_results_display}, { >+ item => $item, >+ success => $success, >+ info => $info >+ }; >+} >+ > $template->param( > finesview => 1, > payment_id => $payment_id, > change_given => $change_given, >+ renew_results => $renew_results_display > ); > > add_accounts_to_template(); >diff --git a/members/paycollect.pl b/members/paycollect.pl >index 1a39f17b00..f0b003c53c 100755 >--- a/members/paycollect.pl >+++ b/members/paycollect.pl >@@ -34,6 +34,7 @@ use Koha::Patron::Categories; > use Koha::AuthorisedValues; > use Koha::Account; > use Koha::Token; >+use Koha::DateUtils; > > my $input = CGI->new(); > >@@ -171,9 +172,11 @@ if ( $total_paid and $total_paid ne '0.00' ) { > token => scalar $input->param('csrf_token'), > }); > >+ my $url; >+ my $pay_result; > if ($pay_individual) { > my $line = Koha::Account::Lines->find($accountlines_id); >- $payment_id = $account->pay( >+ $pay_result = $account->pay( > { > lines => [$line], > amount => $total_paid, >@@ -184,8 +187,9 @@ if ( $total_paid and $total_paid ne '0.00' ) { > cash_register => $registerid > } > ); >- print $input->redirect( >- "/cgi-bin/koha/members/pay.pl?borrowernumber=$borrowernumber&payment_id=$payment_id&change_given=$change_given"); >+ $payment_id = $pay_result->{payment_id}; >+ >+ $url = "/cgi-bin/koha/members/pay.pl"; > } else { > if ($selected_accts) { > if ( $total_paid > $total_due ) { >@@ -196,7 +200,7 @@ if ( $total_paid and $total_paid ne '0.00' ) { > } else { > my $note = $input->param('selected_accts_notes'); > >- $payment_id = $account->pay( >+ $pay_result = $account->pay( > { > type => $type, > amount => $total_paid, >@@ -207,12 +211,13 @@ if ( $total_paid and $total_paid ne '0.00' ) { > payment_type => $payment_type, > cash_register => $registerid > } >- ); >+ ); > } >+ $payment_id = $pay_result->{payment_id}; > } > else { > my $note = $input->param('selected_accts_notes'); >- $payment_id = $account->pay( >+ $pay_result = $payment_id = $account->pay( > { > amount => $total_paid, > library_id => $library_id, >@@ -224,9 +229,26 @@ if ( $total_paid and $total_paid ne '0.00' ) { > } > ); > } >+ $payment_id = $pay_result->{payment_id}; > >- print $input->redirect("/cgi-bin/koha/members/boraccount.pl?borrowernumber=$borrowernumber&payment_id=$payment_id&change_given=$change_given"); >+ $url = "/cgi-bin/koha/members/boraccount.pl"; >+ } >+ # It's possible renewals took place, parse any renew results >+ # and pass on >+ my @renew_result = (); >+ foreach my $ren( @{$pay_result->{renew_result}} ) { >+ my $str = "renew_result=$ren->{itemnumber},$ren->{success},"; >+ my $app = $ren->{success} ? >+ uri_escape( >+ output_pref({ dt => $ren->{due_date}, as_due_date => 1 }) >+ ) : $ren->{error}; >+ push @renew_result, "${str}${app}"; > } >+ my $append = scalar @renew_result ? '&' . join('&', @renew_result) : ''; >+ >+ $url .= "?borrowernumber=$borrowernumber&payment_id=$payment_id&change_given=${change_given}${append}"; >+ >+ print $input->redirect($url); > } > } else { > $total_paid = '0.00'; #TODO not right with pay_individual >diff --git a/t/db_dependent/Accounts.t b/t/db_dependent/Accounts.t >index 9236edfde8..aaaf0ebeb6 100644 >--- a/t/db_dependent/Accounts.t >+++ b/t/db_dependent/Accounts.t >@@ -214,7 +214,7 @@ subtest "Koha::Account::pay tests" => sub { > my $borrowernumber = $borrower->borrowernumber; > my $data = '20.00'; > my $payment_note = '$20.00 payment note'; >- my $id = $account->pay( { amount => $data, note => $payment_note, payment_type => "TEST_TYPE" } ); >+ my $id = $account->pay( { amount => $data, note => $payment_note, payment_type => "TEST_TYPE" } )->{payment_id}; > > my $accountline = Koha::Account::Lines->find( $id ); > is( $accountline->payment_type, "TEST_TYPE", "Payment type passed into pay is set in account line correctly" ); >@@ -294,7 +294,7 @@ subtest "Koha::Account::pay tests" => sub { > is($note,'$200.00 payment note', '$200.00 payment note is registered'); > > my $line3 = $account->add_debit({ type => 'ACCOUNT', amount => 42, interface => 'commandline' }); >- my $payment_id = $account->pay( { lines => [$line3], amount => 42 } ); >+ my $payment_id = $account->pay( { lines => [$line3], amount => 42 } )->{payment_id}; > my $payment = Koha::Account::Lines->find( $payment_id ); > is( $payment->amount(), '-42.000000', "Payment paid the specified fine" ); > $line3 = Koha::Account::Lines->find( $line3->id ); >@@ -376,7 +376,7 @@ subtest "Koha::Account::pay writeoff tests" => sub { > amount => 42, > type => 'WRITEOFF', > } >- ); >+ )->{payment_id}; > > $line->_result->discard_changes(); > >@@ -1111,7 +1111,7 @@ subtest "Koha::Account::Offset credit & debit tests" => sub { > lines => [$line1, $line2], > amount => 30, > } >- ); >+ )->{payment_id}; > > # Test debit and credit methods for Koha::Account::Offset > my $account_offset = Koha::Account::Offsets->find( { credit_id => $id, debit_id => $line1->id } ); >@@ -1177,15 +1177,15 @@ subtest "Payment notice tests" => sub { > $letter->store(); > > t::lib::Mocks::mock_preference('UseEmailReceipts', '0'); >- my $id = $account->pay( { amount => 1 } ); >+ my $id = $account->pay( { amount => 1 } )->{payment_id}; > is( Koha::Notice::Messages->search()->count(), 0, 'Notice for payment not sent if UseEmailReceipts is disabled' ); > >- $id = $account->pay( { amount => 1, type => 'WRITEOFF' } ); >+ $id = $account->pay( { amount => 1, type => 'WRITEOFF' } )->{payment_id}; > is( Koha::Notice::Messages->search()->count(), 0, 'Notice for writeoff not sent if UseEmailReceipts is disabled' ); > > t::lib::Mocks::mock_preference('UseEmailReceipts', '1'); > >- $id = $account->pay( { amount => 12, library_id => $branchcode } ); >+ $id = $account->pay( { amount => 12, library_id => $branchcode } )->{payment_id}; > my $notice = Koha::Notice::Messages->search()->next(); > is( $notice->subject, 'Account payment', 'Notice subject is correct for payment' ); > is( $notice->letter_code, 'ACCOUNT_PAYMENT', 'Notice letter code is correct for payment' ); >@@ -1196,7 +1196,7 @@ subtest "Payment notice tests" => sub { > $letter->content('[%- USE Price -%]A writeoff of [% credit.amount * -1 | $Price %] has been applied to your account. Your [% branch.branchcode %]'); > $letter->store(); > >- $id = $account->pay( { amount => 13, type => 'WRITEOFF', library_id => $branchcode } ); >+ $id = $account->pay( { amount => 13, type => 'WRITEOFF', library_id => $branchcode } )->{payment_id}; > $notice = Koha::Notice::Messages->search()->next(); > is( $notice->subject, 'Account writeoff', 'Notice subject is correct for payment' ); > is( $notice->letter_code, 'ACCOUNT_WRITEOFF', 'Notice letter code is correct for writeoff' ); >diff --git a/t/db_dependent/Koha/Account.t b/t/db_dependent/Koha/Account.t >index 0444b1bc69..c4d80b9d65 100755 >--- a/t/db_dependent/Koha/Account.t >+++ b/t/db_dependent/Koha/Account.t >@@ -19,7 +19,7 @@ > > use Modern::Perl; > >-use Test::More tests => 11; >+use Test::More tests => 12; > use Test::MockModule; > use Test::Exception; > >@@ -685,12 +685,12 @@ subtest 'pay() tests' => sub { > my $context = Test::MockModule->new('C4::Context'); > $context->mock( 'userenv', { branch => $library->id } ); > >- my $credit_1_id = $account->pay({ amount => 200 }); >+ my $credit_1_id = $account->pay({ amount => 200 })->{payment_id}; > my $credit_1 = Koha::Account::Lines->find( $credit_1_id ); > > is( $credit_1->branchcode, undef, 'No branchcode is set if library_id was not passed' ); > >- my $credit_2_id = $account->pay({ amount => 150, library_id => $library->id }); >+ my $credit_2_id = $account->pay({ amount => 150, library_id => $library->id })->{payment_id}; > my $credit_2 = Koha::Account::Lines->find( $credit_2_id ); > > is( $credit_2->branchcode, $library->id, 'branchcode set because library_id was passed' ); >diff --git a/t/db_dependent/Koha/Account/Lines.t b/t/db_dependent/Koha/Account/Lines.t >index df32b4d6e4..94ad5f8394 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; > >@@ -365,7 +365,7 @@ subtest 'apply() tests' => sub { > my $module = new Test::MockModule('C4::Circulation'); > $module->mock('AddRenewal', sub { $called = 1; }); > my $credit_renew = $account->add_credit({ amount => 100, user_id => $patron->id, interface => 'commandline' }); >- my $debits_renew = Koha::Account::Lines->search({ accountlines_id => $accountline->id }); >+ my $debits_renew = Koha::Account::Lines->search({ accountlines_id => $accountline->id })->as_list; > $credit_renew->apply( { debits => $debits_renew, offset_type => 'Manual Credit' } ); > > is( $called, 1, 'RenewAccruingItemWhenPaid causes C4::Circulation::AddRenew to be called when appropriate' ); >@@ -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; >@@ -617,7 +692,7 @@ subtest "void() tests" => sub { > lines => [$line1, $line2], > amount => 30, > } >- ); >+ )->{payment_id}; > > my $account_payment = Koha::Account::Lines->find( $id ); > >-- >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=95560">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