From d89878620678513eafda5df38986a4506debf7ad Mon Sep 17 00:00:00 2001 From: Andrew Isherwood Date: Mon, 21 Oct 2019 12:09:13 +0100 Subject: [PATCH] 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 --- Koha/Account.pm | 19 ++++------ .../prog/en/includes/renew_results.inc | 12 ++++++ .../{renew_error_strings.inc => renew_strings.inc} | 0 .../prog/en/modules/members/boraccount.tt | 11 +----- .../intranet-tmpl/prog/en/modules/members/pay.tt | 9 +---- members/boraccount.pl | 20 +++++++--- members/pay.pl | 21 +++++++---- members/paycollect.pl | 43 ++++++++++++---------- 8 files changed, 75 insertions(+), 60 deletions(-) create mode 100644 koha-tmpl/intranet-tmpl/prog/en/includes/renew_results.inc rename koha-tmpl/intranet-tmpl/prog/en/includes/{renew_error_strings.inc => renew_strings.inc} (100%) diff --git a/Koha/Account.pm b/Koha/Account.pm index 61e3a8ca33..7d64ec7bb9 100644 --- a/Koha/Account.pm +++ b/Koha/Account.pm @@ -758,16 +758,12 @@ and, if the syspref is set, renew them sub _maybe_renew { my ($items, $patron_id) = @_; - my $results = { - ok => [], - not_ok => [] - }; + 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 ); @@ -784,20 +780,21 @@ sub _maybe_renew { undef, 1 ); - push @{$results->{ok}}, { + push @results, { itemnumber => $itemnumber, - due_date => $due_date + due_date => $due_date, + success => 1 }; } else { - push @{$results->{not_ok}}, { + push @results, { itemnumber => $itemnumber, - error => $error + error => $error, + success => 0 }; } } - return $results; } - return $results; + return \@results; } 1; 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 %] +
+ The fines on the following items were paid off, renewal results are displayed below: + [% FOREACH result IN renew_results %] + [% IF result.success %] +

[% INCLUDE 'biblio-title.inc' biblio=result.item.biblio %] ( [% result.item.barcode | html %] ): Renewed - due [% result.info | html %]

+ [% ELSE %] +

[% INCLUDE 'biblio-title.inc' biblio=result.item.biblio %] ( [% result.item.barcode | html %] ): Not renewed - [% INCLUDE 'renew_strings.inc' error=result.info %]

+ [% END %] + [% END %] +
+[% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/renew_error_strings.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc similarity index 100% rename from koha-tmpl/intranet-tmpl/prog/en/includes/renew_error_strings.inc rename to koha-tmpl/intranet-tmpl/prog/en/includes/renew_strings.inc 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 5bf5382dab..100044736b 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt @@ -38,15 +38,8 @@
  • Create manual invoice
  • 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 %] +
    +[% INCLUDE 'renew_results.inc' renew_results=renew_results %] 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 6fd8317dc2..bc4b55a927 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt @@ -40,14 +40,7 @@

    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 %] +[% INCLUDE 'renew_results.inc' renew_results=renew_results %]
    diff --git a/members/boraccount.pl b/members/boraccount.pl index 91994158b5..85f765218c 100755 --- a/members/boraccount.pl +++ b/members/boraccount.pl @@ -23,6 +23,7 @@ # along with Koha; if not, see . use Modern::Perl; +use URI::Escape; use C4::Auth; use C4::Output; @@ -52,7 +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_errors = $input->param('renew_error'); +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 ); @@ -84,11 +85,18 @@ if($total <= 0){ # 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 $renew_results_display = []; +foreach my $renew_result(@renew_results) { + my ($itemnumber, $success, $info) = split(/,/, $renew_result); my $item = Koha::Items->find($itemnumber); - push @{$renew_errors_display}, { item => $item, error => $error }; + if ($success) { + $info = uri_unescape($info); + } + push @{$renew_results_display}, { + item => $item, + success => $success, + info => $info + }; } $template->param( @@ -99,7 +107,7 @@ $template->param( accounts => \@accountlines, payment_id => $payment_id, change_given => $change_given, - renew_errors => $renew_errors_display, + 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 505a6360c1..e3ce844d8e 100755 --- a/members/pay.pl +++ b/members/pay.pl @@ -66,7 +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'); +our @renew_results = $input->multi_param('renew_result'); # get borrower details my $logged_in_user = Koha::Patrons->find( $loggedinuser ) or die "Not logged in"; @@ -138,19 +138,26 @@ 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); +# 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); - push @{$renew_errors_display}, { item => $item, error => $error }; + 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_errors => $renew_errors_display + renew_results => $renew_results_display ); add_accounts_to_template(); diff --git a/members/paycollect.pl b/members/paycollect.pl index db9587a139..6c03024c3c 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(); @@ -150,9 +151,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); - my $pay_result = $account->pay( + $pay_result = $account->pay( { lines => [$line], amount => $total_paid, @@ -164,17 +167,9 @@ if ( $total_paid and $total_paid ne '0.00' ) { } ); $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}${append}"); + + $url = "/cgi-bin/koha/members/pay.pl"; } else { - my $pay_result; if ($select) { if ( $select =~ /^([\d,]*).*/ ) { $select = $1; # ensure passing no junk @@ -220,15 +215,25 @@ if ( $total_paid and $total_paid ne '0.00' ) { ); } $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}"); + + $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 -- 2.11.0