From ff69d35ddfdbd36d98e66915eb6e9a3a70d2e1bc Mon Sep 17 00:00:00 2001 From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Date: Fri, 5 Jan 2018 16:18:37 -0300 Subject: [PATCH] Bug 12001: Move GetMemberAccountRecords to the Koha namespace The GetMemberAccountRecords may be a perf killer, it retrieves all the account lines of a patron and then the related item and biblio information. Most of the time we only want to know how much the patron owns to the library (sum of amountoutstanding). We already have this information in Koha::Patron->account->balance. This patch replaces the occurrences of this subroutine by fetching only the information we need, either the balance, the detail, or both. It removes the formatting done in the module, to use the TT plugin 'Price' instead. There is a very weird and error-prone behavior/feature in GetMemberAccountBalance (FIXME): as the accountlines.accounttype is a varchar(5), the value of the authorised value used for the ManInvInNoissuesCharge pref (category MANUAL_INV) is truncated to the 5 first characters. That could lead to unexpected behaviors. On the way, this patchset also replace the GetMemberAccountBalance subroutine, which returns the balance, the non issues charges and the other charges. We only need to have the balance and the non issues charges to calcul the third one. Test plan: Add several fees for a patron and play with HoldsInNoissuesCharge, RentalsInNoissuesCharge and ManInvInNoissuesCharge. The information (biblio and item info, as well as the account line) must be correctly displayed on the different screens: 'Fines' module, fine slips, circulation module Note that this patchset could introduce regression on price formatting, but will be easy to fix using the TT plugin. --- C4/Circulation.pm | 2 +- C4/Members.pm | 64 ++++-------------- C4/SIP/ILS/Patron.pm | 2 +- Koha/Account.pm | 3 +- Koha/Account/Line.pm | 15 ++++- circ/circulation.pl | 2 +- .../prog/en/modules/members/boraccount.tt | 2 +- .../prog/en/modules/members/moremember-print.tt | 5 +- members/boraccount.pl | 21 +++++- members/moremember.pl | 16 ++--- members/pay.pl | 3 +- members/paycollect.pl | 2 +- members/printfeercpt.pl | 76 +++++++++------------- members/printinvoice.pl | 72 ++++++++------------ members/summary-print.pl | 13 ++-- opac/opac-account.pl | 39 ++++++----- opac/opac-main.pl | 32 +++++---- opac/opac-reserve.pl | 2 +- opac/opac-user.pl | 47 ++++++++----- reserve/request.pl | 3 +- t/db_dependent/Koha/Patrons.t | 20 +++--- t/db_dependent/Members.t | 31 +-------- 22 files changed, 203 insertions(+), 269 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index a2a9e7bc53..767915af2e 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2727,7 +2727,7 @@ sub CanBookBeRenewed { if ( C4::Context->preference('OPACFineNoRenewalsBlockAutoRenew') ) { my $fine_no_renewals = C4::Context->preference("OPACFineNoRenewals"); - my ( $amountoutstanding ) = C4::Members::GetMemberAccountRecords($patron->borrowernumber); + my $amountoutstanding = $patron->account->balance; if ( $amountoutstanding and $amountoutstanding > $fine_no_renewals ) { return ( 0, "auto_too_much_oweing" ); } diff --git a/C4/Members.pm b/C4/Members.pm index faa497f98c..cc6b6d885f 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -26,6 +26,7 @@ use C4::Context; use String::Random qw( random_string ); use Scalar::Util qw( looks_like_number ); use Date::Calc qw/Today check_date Date_to_Days/; +use List::MoreUtils qw( uniq ); use C4::Log; # logaction use C4::Overdues; use C4::Reserves; @@ -64,8 +65,6 @@ BEGIN { &GetPendingIssues &GetAllIssues - &GetMemberAccountRecords - &GetBorrowersToExpunge &IssueSlip @@ -737,49 +736,6 @@ sub GetAllIssues { } -=head2 GetMemberAccountRecords - - ($total, $acctlines, $count) = &GetMemberAccountRecords($borrowernumber); - -Looks up accounting data for the patron with the given borrowernumber. - -C<&GetMemberAccountRecords> returns a three-element array. C<$acctlines> is a -reference-to-array, where each element is a reference-to-hash; the -keys are the fields of the C<accountlines> table in the Koha database. -C<$count> is the number of elements in C<$acctlines>. C<$total> is the -total amount outstanding for all of the account lines. - -=cut - -sub GetMemberAccountRecords { - my ($borrowernumber) = @_; - my $dbh = C4::Context->dbh; - my @acctlines; - my $numlines = 0; - my $strsth = qq( - SELECT * - FROM accountlines - WHERE borrowernumber=?); - $strsth.=" ORDER BY accountlines_id desc"; - my $sth= $dbh->prepare( $strsth ); - $sth->execute( $borrowernumber ); - - my $total = 0; - while ( my $data = $sth->fetchrow_hashref ) { - if ( $data->{itemnumber} ) { - my $item = Koha::Items->find( $data->{itemnumber} ); - my $biblio = $item->biblio; - $data->{biblionumber} = $biblio->biblionumber; - $data->{title} = $biblio->title; - } - $acctlines[$numlines] = $data; - $numlines++; - $total += sprintf "%.0f", 1000*$data->{amountoutstanding}; # convert float to integer to avoid round-off errors - } - $total /= 1000; - return ( $total, \@acctlines,$numlines); -} - =head2 GetMemberAccountBalance ($total_balance, $non_issue_balance, $other_charges) = &GetMemberAccountBalance($borrowernumber); @@ -796,6 +752,7 @@ Charges exempt from non-issue are: sub GetMemberAccountBalance { my ($borrowernumber) = @_; + # FIXME REMOVE And add a warning in the about page + update DB if length(MANUAL_INV) > 5 my $ACCOUNT_TYPE_LENGTH = 5; # this is plain ridiculous... my @not_fines; @@ -803,16 +760,17 @@ sub GetMemberAccountBalance { push @not_fines, 'Rent' unless C4::Context->preference('RentalsInNoissuesCharge'); unless ( C4::Context->preference('ManInvInNoissuesCharge') ) { my $dbh = C4::Context->dbh; - my $man_inv_types = $dbh->selectcol_arrayref(qq{SELECT authorised_value FROM authorised_values WHERE category = 'MANUAL_INV'}); - push @not_fines, map substr($_, 0, $ACCOUNT_TYPE_LENGTH), @$man_inv_types; + push @not_fines, @{ $dbh->selectcol_arrayref(qq{SELECT authorised_value FROM authorised_values WHERE category = 'MANUAL_INV'}) }; } - my %not_fine = map {$_ => 1} @not_fines; + @not_fines = map { substr($_, 0, $ACCOUNT_TYPE_LENGTH) } uniq (@not_fines); - my ($total, $acctlines) = GetMemberAccountRecords($borrowernumber); - my $other_charges = 0; - foreach (@$acctlines) { - $other_charges += $_->{amountoutstanding} if $not_fine{ substr($_->{accounttype}, 0, $ACCOUNT_TYPE_LENGTH) }; - } + my $patron = Koha::Patrons->find( $borrowernumber ); + my $total = $patron->account->balance; + my $other_charges = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber, accounttype => { -in => \@not_fines } }, { + select => [ { sum => 'amountoutstanding' } ], + as => ['total_other_charges'], + }); + $other_charges = $other_charges->count ? $other_charges->next->get_column('total_other_charges') : 0; return ( $total, $total - $other_charges, $other_charges); } diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm index 6cd3300eee..17034c0d0f 100644 --- a/C4/SIP/ILS/Patron.pm +++ b/C4/SIP/ILS/Patron.pm @@ -88,7 +88,7 @@ sub new { hold_ok => ( !$debarred && !$expired && !$fine_blocked), card_lost => ( $kp->{lost} || $kp->{gonenoaddress} || $flags->{LOST} ), claims_returned => 0, - fines => $fines_amount, # GetMemberAccountRecords($kp->{borrowernumber}) + fines => $fines_amount, fees => 0, # currently not distinct from fines recall_overdue => 0, items_billed => 0, diff --git a/Koha/Account.pm b/Koha/Account.pm index ed1fd3cea7..ba79f6600d 100644 --- a/Koha/Account.pm +++ b/Koha/Account.pm @@ -279,7 +279,8 @@ sub balance { as => ['total_amountoutstanding'], } ); - return $fines->count + + my $total = $fines->count ? $fines->next->get_column('total_amountoutstanding') : 0; } diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm index f7578e74f9..2065de7e2f 100644 --- a/Koha/Account/Line.pm +++ b/Koha/Account/Line.pm @@ -20,6 +20,7 @@ use Modern::Perl; use Carp; use Koha::Database; +use Koha::Items; use base qw(Koha::Object); @@ -33,7 +34,19 @@ Koha::Account::Lines - Koha accountline Object class =cut -=head3 type +=head3 item + +Return the item linked to this account line if exists + +=cut + +sub item { + my ( $self ) = @_; + my $rs = $self->_result->itemnumber; + return Koha::Item->_new_from_dbic( $rs ); +} + +=head3 _type =cut diff --git a/circ/circulation.pl b/circ/circulation.pl index e46ced1638..48a36a90e6 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -543,7 +543,7 @@ foreach my $flag ( sort keys %$flags ) { my $amountold = $flags ? $flags->{'CHARGES'}->{'message'} || 0 : 0; $amountold =~ s/^.*\$//; # remove upto the $, if any -my ( $total, $accts, $numaccts) = GetMemberAccountRecords( $borrowernumber ); +my $total = $patron ? $patron->account->balance : 0; if ( $patron && $patron->category->category_type eq 'C') { my $patron_categories = Koha::Patron::Categories->search_limited({ category_type => 'A' }, {order_by => ['categorycode']}); 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 efcc726ab3..4a132479f9 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt @@ -108,7 +108,7 @@ $(document).ready(function() { [% CASE %][% account.accounttype %] [%- END -%] [%- IF account.description %], [% account.description %][% END %] - [% IF ( account.itemnumber ) %]<a href="/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=[% account.biblionumber %]&itemnumber=[% account.itemnumber %]">[% account.title |html %]</a>[% END %]</td> + [% IF ( account.itemnumber ) %]<a href="/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=[% account.item.biblionumber %]&itemnumber=[% account.itemnumber %]">[% account.item.biblio.title |html %]</a>[% END %]</td> <td>[% account.note | html_line_break %]</td> [% IF ( account.amountcredit ) %]<td class="credit" style="text-align: right;">[% ELSE %]<td class="debit" style="text-align: right;">[% END %][% account.amount | $Price %]</td> [% IF ( account.amountoutstandingcredit ) %]<td class="credit" style="text-align: right;">[% ELSE %]<td class="debit" style="text-align: right;">[% END %][% account.amountoutstanding | $Price %]</td> diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-print.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-print.tt index 567b18dd3e..c5dacd4f36 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-print.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember-print.tt @@ -102,11 +102,10 @@ </tr> [% FOREACH account IN accounts %] - [% NEXT IF account.amountoutstanding == 0 %] <tr> <td> - [% IF ( account.itemnumber ) %]<a href="/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=[% account.biblionumber %]&itemnumber=[% account.itemnumber %]">[% END %] - [% account.description %] [% IF ( account.printtitle ) %] [% account.title |html %][% END %] + [% IF ( account.itemnumber ) %]<a href="/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=[% account.item.biblionumber %]&itemnumber=[% account.itemnumber %]">[% END %] + [% account.description %] [% IF account.item AND account.accounttype != 'F' AND account.accounttype != 'FU' %] [% account.item.biblio.title |html %][% END %] [% IF ( account.itemnumber ) %]</a>[% END %] </td> <td>[% account.date | $KohaDates %]</td> diff --git a/members/boraccount.pl b/members/boraccount.pl index ac7cee248a..77746e5c4d 100755 --- a/members/boraccount.pl +++ b/members/boraccount.pl @@ -70,14 +70,23 @@ if ( $patron->category->category_type eq 'C') { } #get account details -my ($total,$accts,undef)=GetMemberAccountRecords($borrowernumber); +my $total = $patron->account->balance; + +my $accts = Koha::Account::Lines->search( + { borrowernumber => $patron->borrowernumber }, + { order_by => { -desc => 'accountlines_id' } } +); + my $totalcredit; if($total <= 0){ $totalcredit = 1; } my $reverse_col = 0; # Flag whether we need to show the reverse column -foreach my $accountline ( @{$accts}) { +my @accountlines; +while ( my $line = $accts->next ) { + # FIXME We should pass the $accts iterator to the template and do this formatting part there + my $accountline = $line->unblessed; $accountline->{amount} += 0.00; if ($accountline->{amount} <= 0 ) { $accountline->{amountcredit} = 1; @@ -93,6 +102,12 @@ foreach my $accountline ( @{$accts}) { $accountline->{payment} = 1; $reverse_col = 1; } + + if ( $accountline->{itemnumber} ) { + # Because we will not have access to the object from the template + $accountline->{item} = { biblionumber => $line->item->biblionumber, }; + } + push @accountlines, $accountline; } $template->param( adultborrower => 1 ) if ( $patron->category->category_type =~ /^(A|I)$/ ); @@ -116,7 +131,7 @@ $template->param( totalcredit => $totalcredit, is_child => ($patron->category->category_type eq 'C'), reverse_col => $reverse_col, - accounts => $accts, + accounts => \@accountlines, ); output_html_with_http_headers $input, $cookie, $template->output; diff --git a/members/moremember.pl b/members/moremember.pl index 27185efca5..fb91ee0c9e 100755 --- a/members/moremember.pl +++ b/members/moremember.pl @@ -51,6 +51,7 @@ use C4::Biblio; use C4::Form::MessagingPreferences; use List::MoreUtils qw/uniq/; use C4::Members::Attributes qw(GetBorrowerAttributes); +use Koha::Account::Lines; use Koha::AuthorisedValues; use Koha::CsvProfiles; use Koha::Patron::Debarments qw(GetDebarments); @@ -219,18 +220,12 @@ else { my $library = Koha::Libraries->find( $data->{branchcode})->unblessed; @{$data}{keys %$library} = values %$library; # merge in all branch columns -my ( $total, $accts, $numaccts) = GetMemberAccountRecords( $borrowernumber ); - # If printing a page, send the account informations to the template if ($print eq "page") { - foreach my $accountline (@$accts) { - $accountline->{amount} = sprintf '%.2f', $accountline->{amount}; - $accountline->{amountoutstanding} = sprintf '%.2f', $accountline->{amountoutstanding}; - - if ($accountline->{accounttype} ne 'F' && $accountline->{accounttype} ne 'FU'){ - $accountline->{printtitle} = 1; - } - } + my $accts = Koha::Account::Lines->search( + { borrowernumber => $patron->borrowernumber, amountoutstanding => { '>' => 0 } }, + { order_by => { -desc => 'accountlines_id' } } + ); $template->param( accounts => $accts ); } @@ -337,6 +332,7 @@ if ( C4::Context->preference("ExportCircHistory") ) { my ( $subtag, $region ) = split '-', $patron->lang; my $translated_language = C4::Languages::language_get_description( $subtag, $subtag, 'language' ); +my $total = $patron->account->balance; $template->param( patron => $patron, translated_language => $translated_language, diff --git a/members/pay.pl b/members/pay.pl index b1add01018..70e04186f9 100755 --- a/members/pay.pl +++ b/members/pay.pl @@ -131,7 +131,8 @@ output_html_with_http_headers $input, $cookie, $template->output; sub add_accounts_to_template { - my ( $total, undef, undef ) = GetMemberAccountRecords($borrowernumber); + my $patron = Koha::Patrons->find( $borrowernumber ); + my $total = $patron->account->balance; my $account_lines = Koha::Account::Lines->search({ borrowernumber => $borrowernumber, amountoutstanding => { '!=' => 0 } }, { order_by => ['accounttype'] }); my @accounts; while ( my $account_line = $account_lines->next ) { diff --git a/members/paycollect.pl b/members/paycollect.pl index 6bf0fee04e..2b036fdc1c 100755 --- a/members/paycollect.pl +++ b/members/paycollect.pl @@ -63,7 +63,7 @@ my $user = $input->remote_user; my $branch = C4::Context->userenv->{'branch'}; -my ( $total_due, $accts, $numaccts ) = GetMemberAccountRecords($borrowernumber); +my $total_due = $patron->account->balance; my $total_paid = $input->param('paid'); my $individual = $input->param('pay_individual'); diff --git a/members/printfeercpt.pl b/members/printfeercpt.pl index f5933f16ab..adfaafdeb6 100755 --- a/members/printfeercpt.pl +++ b/members/printfeercpt.pl @@ -22,14 +22,14 @@ # You should have received a copy of the GNU General Public License # along with Koha; if not, see <http://www.gnu.org/licenses>. -use strict; -use warnings; +use Modern::Perl; use C4::Auth; use C4::Output; use CGI qw ( -utf8 ); use C4::Members; use C4::Accounts; +use Koha::Account::Lines; use Koha::DateUtils; use Koha::Patrons; use Koha::Patron::Categories; @@ -71,54 +71,39 @@ if ( $data->{'category_type'} eq 'C') { } #get account details -my ($total,$accts,$numaccts)=GetMemberAccountRecords($borrowernumber); +my $total = $patron->account->balance; + +# FIXME This whole stuff is ugly and should be rewritten +# FIXME We should pass the $accts iterator to the template and do this formatting part there +my $accountline = Koha::Account::Lines->find($accountlines_id)->unblessed; my $totalcredit; if($total <= 0){ $totalcredit = 1; } -my @accountrows; # this is for the tmpl-loop - -my $toggle; -for (my $i=0;$i<$numaccts;$i++){ - next if ( $accts->[$i]{'accountlines_id'} ne $accountlines_id ); - if($i%2){ - $toggle = 0; - } else { - $toggle = 1; - } - $accts->[$i]{'toggle'} = $toggle; - $accts->[$i]{'amount'}+=0.00; - if($accts->[$i]{'amount'} <= 0){ - $accts->[$i]{'amountcredit'} = 1; - $accts->[$i]{'amount'}*=-1.00; - } - $accts->[$i]{'amountoutstanding'}+=0.00; - if($accts->[$i]{'amountoutstanding'} <= 0){ - $accts->[$i]{'amountoutstandingcredit'} = 1; - } - - my %row = ( 'date' => dt_from_string( $accts->[$i]{'date'} ), - 'amountcredit' => $accts->[$i]{'amountcredit'}, - 'amountoutstandingcredit' => $accts->[$i]{'amountoutstandingcredit'}, - 'toggle' => $accts->[$i]{'toggle'}, - 'description' => $accts->[$i]{'description'}, - 'itemnumber' => $accts->[$i]{'itemnumber'}, - 'biblionumber' => $accts->[$i]{'biblionumber'}, - 'amount' => sprintf("%.2f",$accts->[$i]{'amount'}), - 'amountoutstanding' => sprintf("%.2f",$accts->[$i]{'amountoutstanding'}), - 'accountno' => $accts->[$i]{'accountno'}, - accounttype => $accts->[$i]{accounttype}, - 'note' => $accts->[$i]{'note'}, - ); - - if ($accts->[$i]{'accounttype'} ne 'F' && $accts->[$i]{'accounttype'} ne 'FU'){ - $row{'printtitle'}=1; - $row{'title'} = $accts->[$i]{'title'}; - } - - push(@accountrows, \%row); + +$accountline->{'amount'} += 0.00; +if ( $accountline->{'amount'} <= 0 ) { + $accountline->{'amountcredit'} = 1; + $accountline->{'amount'} *= -1.00; +} +$accountline->{'amountoutstanding'} += 0.00; +if ( $accountline->{'amountoutstanding'} <= 0 ) { + $accountline->{'amountoutstandingcredit'} = 1; } +my %row = ( + 'date' => dt_from_string( $accountline->{'date'} ), + 'amountcredit' => $accountline->{'amountcredit'}, + 'amountoutstandingcredit' => $accountline->{'amountoutstandingcredit'}, + 'description' => $accountline->{'description'}, + 'amount' => sprintf( "%.2f", $accountline->{'amount'} ), + 'amountoutstanding' => + sprintf( "%.2f", $accountline->{'amountoutstanding'} ), + 'accountno' => $accountline->{'accountno'}, + accounttype => $accountline->{accounttype}, + 'note' => $accountline->{'note'}, +); + $template->param( adultborrower => 1 ) if ( $data->{'category_type'} eq 'A' || $data->{'category_type'} eq 'I' ); $template->param( picture => 1 ) if $patron->image; @@ -144,6 +129,7 @@ $template->param( total => sprintf("%.2f",$total), totalcredit => $totalcredit, is_child => ($data->{'category_type'} eq 'C'), - accounts => \@accountrows ); + accounts => [$accountline], # FIXME There is always only 1 row! +); output_html_with_http_headers $input, $cookie, $template->output; diff --git a/members/printinvoice.pl b/members/printinvoice.pl index ff2c0d1ed6..b55cab9346 100755 --- a/members/printinvoice.pl +++ b/members/printinvoice.pl @@ -30,6 +30,7 @@ use CGI qw ( -utf8 ); use C4::Members; use C4::Accounts; +use Koha::Account::Lines; use Koha::Patrons; use Koha::Patron::Categories; @@ -66,58 +67,37 @@ if ( $data->{'category_type'} eq 'C' ) { } #get account details -my ( $total, $accts, $numaccts ) = GetMemberAccountRecords($borrowernumber); +my $total = $patron->account->balance; +my $accountline = Koha::Account::Lines->find($accountlines_id)->unblessed; + my $totalcredit; if ( $total <= 0 ) { $totalcredit = 1; } -my @accountrows; # this is for the tmpl-loop - -my $toggle; -for ( my $i = 0 ; $i < $numaccts ; $i++ ) { - next if ( $accts->[$i]{'accountlines_id'} ne $accountlines_id ); - - if ( $i % 2 ) { - $toggle = 0; - } else { - $toggle = 1; - } - - $accts->[$i]{'toggle'} = $toggle; - $accts->[$i]{'amount'} += 0.00; - - if ( $accts->[$i]{'amount'} <= 0 ) { - $accts->[$i]{'amountcredit'} = 1; - } - - $accts->[$i]{'amountoutstanding'} += 0.00; - if ( $accts->[$i]{'amountoutstanding'} <= 0 ) { - $accts->[$i]{'amountoutstandingcredit'} = 1; - } - - my %row = ( - 'date' => output_pref({ dt => dt_from_string( $accts->[$i]{'date'} ), dateonly => 1 }), - 'amountcredit' => $accts->[$i]{'amountcredit'}, - 'amountoutstandingcredit' => $accts->[$i]{'amountoutstandingcredit'}, - 'toggle' => $accts->[$i]{'toggle'}, - 'description' => $accts->[$i]{'description'}, - 'itemnumber' => $accts->[$i]{'itemnumber'}, - 'biblionumber' => $accts->[$i]{'biblionumber'}, - 'amount' => sprintf( "%.2f", $accts->[$i]{'amount'} ), - 'amountoutstanding' => sprintf( "%.2f", $accts->[$i]{'amountoutstanding'} ), - 'accountno' => $accts->[$i]{'accountno'}, - accounttype => $accts->[$i]{accounttype}, - 'note' => $accts->[$i]{'note'}, - ); - - if ( $accts->[$i]{'accounttype'} ne 'F' && $accts->[$i]{'accounttype'} ne 'FU' ) { - $row{'printtitle'} = 1; - $row{'title'} = $accts->[$i]{'title'}; - } - push( @accountrows, \%row ); +$accountline->{'amount'} += 0.00; +if ( $accountline->{'amount'} <= 0 ) { + $accountline->{'amountcredit'} = 1; + $accountline->{'amount'} *= -1.00; } +$accountline->{'amountoutstanding'} += 0.00; +if ( $accountline->{'amountoutstanding'} <= 0 ) { + $accountline->{'amountoutstandingcredit'} = 1; +} + +my %row = ( + 'date' => dt_from_string( $accountline->{'date'}, dateonly => 1 ), + 'amountcredit' => $accountline->{'amountcredit'}, + 'amountoutstandingcredit' => $accountline->{'amountoutstandingcredit'}, + 'description' => $accountline->{'description'}, + 'amount' => sprintf( "%.2f", $accountline->{'amount'} ), + 'amountoutstanding' => + sprintf( "%.2f", $accountline->{'amountoutstanding'} ), + 'accountno' => $accountline->{'accountno'}, + accounttype => $accountline->{accounttype}, + 'note' => $accountline->{'note'}, +); $template->param( adultborrower => 1 ) if ( $data->{'category_type'} eq 'A' || $data->{'category_type'} eq 'I' ); @@ -143,7 +123,7 @@ $template->param( total => sprintf( "%.2f", $total ), totalcredit => $totalcredit, is_child => ( $data->{'category_type'} eq 'C' ), - accounts => \@accountrows + accounts => [$accountline], # FIXME There is always only 1 row! ); output_html_with_http_headers $input, $cookie, $template->output; diff --git a/members/summary-print.pl b/members/summary-print.pl index a6a0e36194..a860a06902 100755 --- a/members/summary-print.pl +++ b/members/summary-print.pl @@ -53,14 +53,11 @@ my $data = $patron->unblessed; $data->{description} = $category->description; $data->{category_type} = $category->category_type; -my ( $total, $accts, $numaccts ) = GetMemberAccountRecords($borrowernumber); -foreach my $accountline (@$accts) { - if ( $accountline->{accounttype} ne 'F' - && $accountline->{accounttype} ne 'FU' ) - { - $accountline->{printtitle} = 1; - } -} +my $total = $patron->account->balance; +my $accts = Koha::Account::Lines->search( + { borrowernumber => $patron->borrowernumber }, + { order_by => { -desc => 'accountlines_id' } } +); our $totalprice = 0; diff --git a/opac/opac-account.pl b/opac/opac-account.pl index c93b5b74da..9611554b58 100755 --- a/opac/opac-account.pl +++ b/opac/opac-account.pl @@ -24,6 +24,7 @@ use CGI qw ( -utf8 ); use C4::Members; use C4::Auth; use C4::Output; +use Koha::Account::Lines; use Koha::Patrons; use Koha::Plugins; @@ -45,32 +46,30 @@ $borrower->{description} = $category->description; $borrower->{category_type} = $category->category_type; $template->param( BORROWER_INFO => $borrower ); -#get account details -my ( $total , $accts, $numaccts) = GetMemberAccountRecords( $borrowernumber ); +my $total = $patron->account->balance; +my $accts = Koha::Account::Lines->search( + { borrowernumber => $patron->borrowernumber }, + { order_by => { -desc => 'accountlines_id' } } +); -for ( my $i = 0 ; $i < $numaccts ; $i++ ) { - $accts->[$i]{'amount'} = sprintf( "%.2f", $accts->[$i]{'amount'} || '0.00'); - if ( $accts->[$i]{'amount'} >= 0 ) { - $accts->[$i]{'amountcredit'} = 1; +my @accountlines; +while ( my $line = $accts->next ) { + my $accountline = $line->unblessed; + $accountline->{'amount'} = sprintf( "%.2f", $accountline->{'amount'} || '0.00'); + if ( $accountline->{'amount'} >= 0 ) { + $accountline->{'amountcredit'} = 1; } - $accts->[$i]{'amountoutstanding'} = - sprintf( "%.2f", $accts->[$i]{'amountoutstanding'} || '0.00' ); - if ( $accts->[$i]{'amountoutstanding'} >= 0 ) { - $accts->[$i]{'amountoutstandingcredit'} = 1; + $accountline->{'amountoutstanding'} = + sprintf( "%.2f", $accountline->{'amountoutstanding'} || '0.00' ); + if ( $accountline->{'amountoutstanding'} >= 0 ) { + $accountline->{'amountoutstandingcredit'} = 1; } -} - -# add the row parity -my $num = 0; -foreach my $row (@$accts) { - $row->{'even'} = 1 if $num % 2 == 0; - $row->{'odd'} = 1 if $num % 2 == 1; - $num++; + push @accountlines, $accountline; } $template->param( - ACCOUNT_LINES => $accts, - total => sprintf( "%.2f", $total ), + ACCOUNT_LINES => \@accountlines, + total => sprintf( "%.2f", $total ), # FIXME Use TT plugin Price accountview => 1, message => scalar $query->param('message') || q{}, message_value => scalar $query->param('message_value') || q{}, diff --git a/opac/opac-main.pl b/opac/opac-main.pl index 33e100f361..d7483c3e76 100755 --- a/opac/opac-main.pl +++ b/opac/opac-main.pl @@ -67,22 +67,28 @@ my $koha_news_count = scalar @$all_koha_news; my $quote = GetDailyQuote(); # other options are to pass in an exact quote id or select a random quote each pass... see perldoc C4::Koha # For dashboard -my $checkouts = Koha::Checkouts->search({ borrowernumber => $borrowernumber })->count; -my ( $overdues_count, $overdues ) = checkoverdues($borrowernumber); -my $holds_pending = Koha::Holds->search({ borrowernumber => $borrowernumber, found => undef })->count; -my $holds_waiting = Koha::Holds->search({ borrowernumber => $borrowernumber })->waiting->count; -my ( $total , $accts, $numaccts) = GetMemberAccountRecords( $borrowernumber ); - -if ( $checkouts > 0 || $overdues_count > 0 || $holds_pending > 0 || $holds_waiting > 0 || $total > 0 ) { - $template->param( dashboard_info => 1 ); +my $patron = Koha::Patrons->find( $borrowernumber ); + +if ( $patron ) { + my $checkouts = Koha::Checkouts->search({ borrowernumber => $borrowernumber })->count; + my ( $overdues_count, $overdues ) = checkoverdues($borrowernumber); + my $holds_pending = Koha::Holds->search({ borrowernumber => $borrowernumber, found => undef })->count; + my $holds_waiting = Koha::Holds->search({ borrowernumber => $borrowernumber })->waiting->count; + my $total = $patron->account->balance; + + if ( $checkouts > 0 || $overdues_count > 0 || $holds_pending > 0 || $holds_waiting > 0 || $total > 0 ) { + $template->param( + dashboard_info => 1, + checkouts => $checkouts, + overdues => $overdues_count, + holds_pending => $holds_pending, + holds_waiting => $holds_waiting, + total_owing => $total, + ); + } } $template->param( - checkouts => $checkouts, - overdues => $overdues_count, - holds_pending => $holds_pending, - holds_waiting => $holds_waiting, - total_owing => $total, koha_news => $all_koha_news, koha_news_count => $koha_news_count, branchcode => $homebranch, diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index dcaec358af..3f22eded50 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -330,7 +330,7 @@ if ( $query->param('place_reserve') ) { my $noreserves = 0; my $maxoutstanding = C4::Context->preference("maxoutstanding"); $template->param( noreserve => 1 ) unless $maxoutstanding; -my ( $amountoutstanding ) = GetMemberAccountRecords($borrowernumber); +my $amountoutstanding = $patron->account->balance; if ( $amountoutstanding && ($amountoutstanding > $maxoutstanding) ) { my $amount = sprintf "%.02f", $amountoutstanding; $template->param( message => 1 ); diff --git a/opac/opac-user.pl b/opac/opac-user.pl index 55c04a3a78..ac91d4f06a 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -33,6 +33,7 @@ use C4::Output; use C4::Biblio; use C4::Items; use C4::Letters; +use Koha::Account::Lines; use Koha::Libraries; use Koha::DateUtils; use Koha::Holds; @@ -88,7 +89,8 @@ if (!$borrowernumber) { } # get borrower information .... -my $borr = Koha::Patrons->find( $borrowernumber )->unblessed; +my $patron = Koha::Patrons->find( $borrowernumber ); +my $borr = $patron->unblessed; my ( $today_year, $today_month, $today_day) = Today(); my ($warning_year, $warning_month, $warning_day) = split /-/, $borr->{'dateexpiry'}; @@ -116,7 +118,7 @@ if ( $userdebarred || $borr->{'gonenoaddress'} || $borr->{'lost'} ) { $canrenew = 0; } -my ( $amountoutstanding ) = GetMemberAccountRecords($borrowernumber); +my $amountoutstanding = $patron->account->balance; if ( $amountoutstanding > 5 ) { $borr->{'amountoverfive'} = 1; } @@ -187,23 +189,32 @@ if ($issues){ $issue->{'reserved'} = 1; } - my ( $total , $accts, $numaccts) = GetMemberAccountRecords( $borrowernumber ); - my $charges = 0; - my $rentalfines = 0; - foreach my $ac (@$accts) { - if ( $ac->{'itemnumber'} == $issue->{'itemnumber'} ) { - $charges += $ac->{'amountoutstanding'} - if $ac->{'accounttype'} eq 'F'; - $charges += $ac->{'amountoutstanding'} - if $ac->{'accounttype'} eq 'FU'; - $charges += $ac->{'amountoutstanding'} - if $ac->{'accounttype'} eq 'L'; - $rentalfines += $ac->{'amountoutstanding'} - if $ac->{'accounttype'} eq 'Rent'; + # Must be moved in a module if reused + my $charges = Koha::Account::Lines->search( + { + borrowernumber => $patron->borrowernumber, + amountoutstanding => { '>' => 0 }, + accounttype => [ 'F', 'FU', 'L' ], + itemnumber => $issue->{itemnumber} + }, + { select => [ { sum => 'amountoutstanding' } ], as => ['charges'] } + ); + $issue->{charges} = $charges->count ? $charges->next->get_column('charges') : 0; + + my $rental_fines = Koha::Account::Lines->search( + { + borrowernumber => $patron->borrowernumber, + amountoutstanding => { '>' => 0 }, + accounttype => 'Rent', + itemnumber => $issue->{itemnumber} + }, + { + select => [ { sum => 'amountoutstanding' } ], + as => ['rental_fines'] } - } - $issue->{'charges'} = $charges; - $issue->{'rentalfines'} = $rentalfines; + ); + $issue->{rentalfines} = $charges->count ? $charges->next->get_column('rental_fines') : 0; + my $marcrecord = GetMarcBiblio({ biblionumber => $issue->{'biblionumber'} }); $issue->{'subtitle'} = GetRecordValue('subtitle', $marcrecord, GetFrameworkCode($issue->{'biblionumber'})); # check if item is renewable diff --git a/reserve/request.pl b/reserve/request.pl index 7a60b857bc..1e7765c29b 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -180,6 +180,7 @@ if ($borrowernumber_hold && !$action) { } my $is_debarred = $patron->is_debarred; + my $amount_outstanding = $patron->account->balance; $template->param( borrowernumber => $patron->borrowernumber, borrowersurname => $patron->surname, @@ -198,7 +199,7 @@ if ($borrowernumber_hold && !$action) { messages => $messages, warnings => $warnings, restricted => $is_debarred, - amount_outstanding => GetMemberAccountRecords($patron->borrowernumber), + amount_outstanding => $amount_outstanding, ); } diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index d793996793..d7b6e5062a 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -399,14 +399,15 @@ subtest 'add_enrolment_fee_if_needed' => sub { my $borrowernumber = C4::Members::AddMember(%borrower_data); $borrower_data{borrowernumber} = $borrowernumber; - my ($total) = C4::Members::GetMemberAccountRecords($borrowernumber); - is( $total, $enrolmentfee_K, "New kid pay $enrolmentfee_K" ); + my $patron = Koha::Patrons->find( $borrowernumber ); + my $total = $patron->account->balance; + is( int($total), int($enrolmentfee_K), "New kid pay $enrolmentfee_K" ); t::lib::Mocks::mock_preference( 'FeeOnChangePatronCategory', 0 ); $borrower_data{categorycode} = 'J'; C4::Members::ModMember(%borrower_data); - ($total) = C4::Members::GetMemberAccountRecords($borrowernumber); - is( $total, $enrolmentfee_K, "Kid growing and become a juvenile, but shouldn't pay for the upgrade " ); + $total = $patron->account->balance; + is( int($total), int($enrolmentfee_K), "Kid growing and become a juvenile, but shouldn't pay for the upgrade " ); $borrower_data{categorycode} = 'K'; C4::Members::ModMember(%borrower_data); @@ -414,16 +415,15 @@ subtest 'add_enrolment_fee_if_needed' => sub { $borrower_data{categorycode} = 'J'; C4::Members::ModMember(%borrower_data); - ($total) = C4::Members::GetMemberAccountRecords($borrowernumber); - is( $total, $enrolmentfee_K + $enrolmentfee_J, "Kid growing and become a juvenile, they should pay " . ( $enrolmentfee_K + $enrolmentfee_J ) ); + $total = $patron->account->balance; + is( int($total), int($enrolmentfee_K + $enrolmentfee_J), "Kid growing and become a juvenile, they should pay " . ( $enrolmentfee_K + $enrolmentfee_J ) ); # Check with calling directly Koha::Patron->get_enrolment_fee_if_needed - my $patron = Koha::Patrons->find($borrowernumber); $patron->categorycode('YA')->store; my $fee = $patron->add_enrolment_fee_if_needed; - ($total) = C4::Members::GetMemberAccountRecords($borrowernumber); - is( $total, - $enrolmentfee_K + $enrolmentfee_J + $enrolmentfee_YA, + $total = $patron->account->balance; + is( int($total), + int($enrolmentfee_K + $enrolmentfee_J + $enrolmentfee_YA), "Juvenile growing and become an young adult, they should pay " . ( $enrolmentfee_K + $enrolmentfee_J + $enrolmentfee_YA ) ); diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t index ac928b2352..aaf1262139 100755 --- a/t/db_dependent/Members.t +++ b/t/db_dependent/Members.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 62; +use Test::More tests => 61; use Test::MockModule; use Test::Exception; @@ -373,35 +373,6 @@ ok( $borrower->{userid}, 'A userid should have been generated correctly' ); is( Check_Userid( C4::Context->config('user'), '' ), 0, 'Check_Userid should return 0 for the DB user (Bug 12226)'); -subtest 'GetMemberAccountRecords' => sub { - - plan tests => 2; - - my $borrowernumber = $builder->build({ source => 'Borrower' })->{ borrowernumber }; - my $accountline_1 = $builder->build({ - source => 'Accountline', - value => { - borrowernumber => $borrowernumber, - amountoutstanding => 64.60 - } - }); - - my ($total,undef,undef) = GetMemberAccountRecords( $borrowernumber ); - is( $total , 64.60, "Rounding works correctly in total calculation (single value)" ); - - my $accountline_2 = $builder->build({ - source => 'Accountline', - value => { - borrowernumber => $borrowernumber, - amountoutstanding => 10.65 - } - }); - - ($total,undef,undef) = GetMemberAccountRecords( $borrowernumber ); - is( $total , 75.25, "Rounding works correctly in total calculation (multiple values)" ); - -}; - subtest 'GetMemberAccountBalance' => sub { plan tests => 6; -- 2.11.0