From b073cae2272a6d0aad494c05bc1d564eacca2547 Mon Sep 17 00:00:00 2001 From: Andrew Isherwood 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 @@
  • Create manual credit
  • +[% IF renew_errors.size > 0 %] +
    + The fines on the following items were paid off, but the items could not be renewed: + [% FOREACH problem IN renew_errors %] +

    [% INCLUDE 'biblio-title.inc' biblio=problem.item.biblio %] ( [% problem.item.barcode | html %] ): [% INCLUDE 'renew_error_strings.inc' error=problem.error %]

    + [% END %] +
    +[% END %] 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 @@

    Select all | Clear all

    +[% IF renew_errors.size > 0 %] +
    + The fines on the following items were paid off, but the items could not be renewed: + [% FOREACH problem IN renew_errors %] +

    [% INCLUDE 'biblio-title.inc' biblio=problem.item.biblio %] ( [% problem.item.barcode | html %] ): [% INCLUDE 'renew_error_strings.inc' error=problem.error %]

    + [% END %] +
    +[% END %]
    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