Bugzilla – Attachment 94608 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) Provide feedback
Bug-23051-follow-up-Provide-feedback.patch (text/plain), 18.46 KB, created by
Andrew Isherwood
on 2019-10-23 10:59:45 UTC
(
hide
)
Description:
Bug 23051: (follow-up) Provide feedback
Filename:
MIME Type:
Creator:
Andrew Isherwood
Created:
2019-10-23 10:59:45 UTC
Size:
18.46 KB
patch
obsolete
>From b073cae2272a6d0aad494c05bc1d564eacca2547 Mon Sep 17 00:00:00 2001 >From: Andrew Isherwood <andrew.isherwood@ptfs-europe.com> >Date: Fri, 11 Oct 2019 17:06:27 +0100 >Subject: [PATCH] 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 >--- > Koha/Account.pm | 26 ++++++++++++++----- > Koha/REST/V1/Patrons/Account.pm | 2 +- > .../prog/en/includes/renew_error_strings.inc | 30 ++++++++++++++++++++++ > .../prog/en/modules/members/boraccount.tt | 8 ++++++ > .../intranet-tmpl/prog/en/modules/members/pay.tt | 8 ++++++ > members/boraccount.pl | 12 +++++++++ > members/pay.pl | 12 +++++++++ > members/paycollect.pl | 29 ++++++++++++++++----- > t/db_dependent/Accounts.t | 16 ++++++------ > t/db_dependent/Koha/Account.t | 4 +-- > t/db_dependent/Koha/Account/Lines.t | 2 +- > 11 files changed, 125 insertions(+), 24 deletions(-) > create mode 100644 koha-tmpl/intranet-tmpl/prog/en/includes/renew_error_strings.inc > >diff --git a/Koha/Account.pm b/Koha/Account.pm >index 63e0d67e67..61e3a8ca33 100644 >--- a/Koha/Account.pm >+++ b/Koha/Account.pm >@@ -273,7 +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 >- _maybe_renew($overdue_unreturned, $self->{patron_id}); >+ my $renew_result = _maybe_renew($overdue_unreturned, $self->{patron_id}); > > if ( C4::Context->preference("FinesLog") ) { > logaction( >@@ -322,7 +322,7 @@ sub pay { > } > } > >- return $payment->id; >+ return { payment_id => $payment->id, renew_result => $renew_result }; > } > > =head3 add_credit >@@ -758,18 +758,24 @@ and, if the syspref is set, renew them > sub _maybe_renew { > my ($items, $patron_id) = @_; > >+ my $results = { >+ ok => [], >+ not_ok => [] >+ }; >+ > 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 ) = >+ my ( $can_renew, $error ) = > C4::Circulation::CanBookBeRenewed($patron_id, $itemnumber); >- if ( $renew_ok ) { >+ if ( $can_renew ) { > my $due_date = C4::Circulation::AddRenewal( > $patron_id, > $itemnumber, >@@ -778,12 +784,20 @@ sub _maybe_renew { > undef, > 1 > ); >- return $due_date; >+ push @{$results->{ok}}, { >+ itemnumber => $itemnumber, >+ due_date => $due_date >+ }; > } else { >- return $error; >+ push @{$results->{not_ok}}, { >+ itemnumber => $itemnumber, >+ error => $error >+ }; > } > } >+ return $results; > } >+ return $results; > } > > 1; >diff --git a/Koha/REST/V1/Patrons/Account.pm b/Koha/REST/V1/Patrons/Account.pm >index eb495f924e..eec073ae61 100644 >--- a/Koha/REST/V1/Patrons/Account.pm >+++ b/Koha/REST/V1/Patrons/Account.pm >@@ -132,7 +132,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/koha-tmpl/intranet-tmpl/prog/en/includes/renew_error_strings.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_error_strings.inc >new file mode 100644 >index 0000000000..daa0fa6ed3 >--- /dev/null >+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_error_strings.inc >@@ -0,0 +1,30 @@ >+[% 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 %] >+ Unknown error >+[% END %] >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..5bf5382dab 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,14 @@ > <li><a href="/cgi-bin/koha/members/mancredit.pl?borrowernumber=[% patron.borrowernumber | uri %]" >Create manual credit</a></li> > </ul> > <div class="tabs-container"> >+[% IF renew_errors.size > 0 %] >+ <div class="alert"> >+ The fines on the following items were paid off, but the items could not be renewed: >+ [% FOREACH problem IN renew_errors %] >+ <p>[% INCLUDE 'biblio-title.inc' biblio=problem.item.biblio %] ( [% problem.item.barcode | html %] ): [% INCLUDE 'renew_error_strings.inc' error=problem.error %]</p> >+ [% END %] >+ </div> >+[% END %] > <!-- 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 8dec77b827..6fd8317dc2 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,14 @@ > <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> >+[% IF renew_errors.size > 0 %] >+ <div class="alert"> >+ The fines on the following items were paid off, but the items could not be renewed: >+ [% FOREACH problem IN renew_errors %] >+ <p>[% INCLUDE 'biblio-title.inc' biblio=problem.item.biblio %] ( [% problem.item.barcode | html %] ): [% INCLUDE 'renew_error_strings.inc' error=problem.error %]</p> >+ [% END %] >+ </div> >+[% END %] > <table id="finest"> > <thead> > <tr> >diff --git a/members/boraccount.pl b/members/boraccount.pl >index 251a3aa565..91994158b5 100755 >--- a/members/boraccount.pl >+++ b/members/boraccount.pl >@@ -31,6 +31,7 @@ use C4::Members; > use C4::Accounts; > use Koha::Patrons; > use Koha::Patron::Categories; >+use Koha::Items; > > my $input=new CGI; > >@@ -51,6 +52,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_errors = $input->param('renew_error'); > > my $logged_in_user = Koha::Patrons->find( $loggedinuser ) or die "Not logged in"; > my $patron = Koha::Patrons->find( $borrowernumber ); >@@ -80,6 +82,15 @@ 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_errors_display = []; >+foreach my $renew_error(@renew_errors) { >+ my ($itemnumber, $error) = split(/,/, $renew_error); >+ my $item = Koha::Items->find($itemnumber); >+ push @{$renew_errors_display}, { item => $item, error => $error }; >+} >+ > $template->param( > patron => $patron, > finesview => 1, >@@ -88,6 +99,7 @@ $template->param( > accounts => \@accountlines, > payment_id => $payment_id, > change_given => $change_given, >+ renew_errors => $renew_errors_display, > ); > > output_html_with_http_headers $input, $cookie, $template->output; >diff --git a/members/pay.pl b/members/pay.pl >index ee2d5f23ff..505a6360c1 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_errors = $input->param('renew_error'); > > # get borrower details > my $logged_in_user = Koha::Patrons->find( $loggedinuser ) or die "Not logged in"; >@@ -135,10 +137,20 @@ for (@names) { > } > } > >+# Populate an arrayref with everything we need to display any >+# renew errors that occurred based on what we were passed >+my $renew_errors_display = []; >+foreach my $renew_error(@renew_errors) { >+ my ($itemnumber, $error) = split(/,/, $renew_error); >+ my $item = Koha::Items->find($itemnumber); >+ push @{$renew_errors_display}, { item => $item, error => $error }; >+} >+ > $template->param( > finesview => 1, > payment_id => $payment_id, > change_given => $change_given, >+ renew_errors => $renew_errors_display > ); > > add_accounts_to_template(); >diff --git a/members/paycollect.pl b/members/paycollect.pl >index bbd4b5a322..db9587a139 100755 >--- a/members/paycollect.pl >+++ b/members/paycollect.pl >@@ -152,7 +152,7 @@ if ( $total_paid and $total_paid ne '0.00' ) { > > if ($pay_individual) { > my $line = Koha::Account::Lines->find($accountlines_id); >- $payment_id = $account->pay( >+ my $pay_result = $account->pay( > { > lines => [$line], > amount => $total_paid, >@@ -163,9 +163,18 @@ if ( $total_paid and $total_paid ne '0.00' ) { > cash_register => $registerid > } > ); >+ $payment_id = $pay_result->{payment_id}; >+ # It's possible renewals took place, parse any renew results >+ # and pass errors on >+ my @renew_errors = map { "renew_error=$_->{itemnumber},$_->{error}" } >+ @{$pay_result->{renew_result}->{not_ok}}; >+ my $renew_result = $pay_result->{renew_result}; >+ my $append = scalar @renew_errors ? '&' . join('&', @renew_errors) : ''; >+ > print $input->redirect( >- "/cgi-bin/koha/members/pay.pl?borrowernumber=$borrowernumber&payment_id=$payment_id&change_given=$change_given"); >+ "/cgi-bin/koha/members/pay.pl?borrowernumber=$borrowernumber&payment_id=$payment_id&change_given=${change_given}${append}"); > } else { >+ my $pay_result; > if ($select) { > if ( $select =~ /^([\d,]*).*/ ) { > $select = $1; # ensure passing no junk >@@ -182,7 +191,7 @@ if ( $total_paid and $total_paid ne '0.00' ) { > { order_by => 'date' } > ); > >- $payment_id = $account->pay( >+ $pay_result = $account->pay( > { > type => $type, > amount => $total_paid, >@@ -194,10 +203,11 @@ if ( $total_paid and $total_paid ne '0.00' ) { > 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, >@@ -209,8 +219,15 @@ if ( $total_paid and $total_paid ne '0.00' ) { > } > ); > } >- >- print $input->redirect("/cgi-bin/koha/members/boraccount.pl?borrowernumber=$borrowernumber&payment_id=$payment_id&change_given=$change_given"); >+ $payment_id = $pay_result->{payment_id}; >+ # It's possible renewals took place, parse any renew results >+ # and pass errors on >+ my @renew_errors = map { "renew_error=$_->{itemnumber},$_->{error}" } >+ @{$pay_result->{renew_result}->{not_ok}}; >+ my $renew_result = $pay_result->{renew_result}; >+ my $append = scalar @renew_errors ? '&' . join('&', @renew_errors) : ''; >+ >+ print $input->redirect("/cgi-bin/koha/members/boraccount.pl?borrowernumber=$borrowernumber&payment_id=$payment_id&change_given=${change_given}${append}"); > } > } > } else { >diff --git a/t/db_dependent/Accounts.t b/t/db_dependent/Accounts.t >index 4a450632eb..6799f31fb5 100644 >--- a/t/db_dependent/Accounts.t >+++ b/t/db_dependent/Accounts.t >@@ -203,7 +203,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" ); >@@ -283,7 +283,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 ); >@@ -365,7 +365,7 @@ subtest "Koha::Account::pay writeoff tests" => sub { > amount => 42, > type => 'writeoff', > } >- ); >+ )->{payment_id}; > > $line->_result->discard_changes(); > >@@ -1001,7 +1001,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 } ); >@@ -1060,15 +1060,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 } ); >+ $id = $account->pay( { amount => 12 } )->{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' ); >@@ -1079,7 +1079,7 @@ subtest "Payment notice tests" => sub { > $letter->content('[%- USE Price -%]A writeoff of [% credit.amount * -1 | $Price %] has been applied to your account.'); > $letter->store(); > >- $id = $account->pay( { amount => 13, type => 'writeoff' } ); >+ $id = $account->pay( { amount => 13, type => 'writeoff' } )->{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 29d0d2becb..8e98136e63 100755 >--- a/t/db_dependent/Koha/Account.t >+++ b/t/db_dependent/Koha/Account.t >@@ -597,12 +597,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 9bd3826929..4839017ca7 100755 >--- a/t/db_dependent/Koha/Account/Lines.t >+++ b/t/db_dependent/Koha/Account/Lines.t >@@ -601,7 +601,7 @@ subtest "void() tests" => sub { > lines => [$line1, $line2], > amount => 30, > } >- ); >+ )->{payment_id}; > > my $account_payment = Koha::Account::Lines->find( $id ); > >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
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