From 9d3ef557fca60f1f975e5082ff433fe7916fda49 Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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. Signed-off-by: Tomas Cohen Arazi --- 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 | 73 +++++++++------------- members/printinvoice.pl | 72 ++++++++------------- members/summary-print.pl | 13 ++-- opac/opac-account.pl | 39 ++++++------ opac/opac-main.pl | 24 +++---- 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, 196 insertions(+), 265 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 2f75d8c5a1..43585168c0 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2703,7 +2703,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 90ebb54afe..727a7af6de 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 @@ -736,49 +735,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 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); @@ -795,6 +751,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; @@ -802,16 +759,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 aa98f9af1f..ee73f6adbe 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -558,7 +558,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 aef11d34b3..dbf0ac0098 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt @@ -78,7 +78,7 @@ [% CASE %][% account.accounttype %] [%- END -%] [%- IF account.description %], [% account.description %][% END %] -  [% IF ( account.itemnumber ) %][% account.title |html %][% END %] +  [% IF ( account.itemnumber ) %][% account.item.biblio.title |html %][% END %] [% account.note | html_line_break %] [% IF ( account.amountcredit ) %][% ELSE %][% END %][% account.amount | $Price %] [% IF ( account.amountoutstandingcredit ) %][% ELSE %][% END %][% account.amountoutstanding | $Price %] 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 35a841056d..2ad6104447 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 @@ -103,11 +103,10 @@ [% FOREACH account IN accounts %] - [% NEXT IF account.amountoutstanding == 0 %] - [% IF ( account.itemnumber ) %][% END %] - [% account.description %] [% IF ( account.printtitle ) %] [% account.title |html %][% END %] + [% IF ( 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 ) %][% END %] [% account.date | $KohaDates %] diff --git a/members/boraccount.pl b/members/boraccount.pl index 7f6bfda11c..8277ab180d 100755 --- a/members/boraccount.pl +++ b/members/boraccount.pl @@ -71,14 +71,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; @@ -94,6 +103,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)$/ ); @@ -117,7 +132,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 29eea1fe4e..d06831ff3a 100755 --- a/members/moremember.pl +++ b/members/moremember.pl @@ -50,6 +50,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); @@ -218,18 +219,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 ); } @@ -348,6 +343,7 @@ my $patron_messages = Koha::Patron::Messages->search( 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 f51d56d551..f2f5a76e93 100755 --- a/members/pay.pl +++ b/members/pay.pl @@ -129,7 +129,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 1eb418437f..f9a2ac9f3d 100755 --- a/members/paycollect.pl +++ b/members/paycollect.pl @@ -61,7 +61,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 748242f530..d39198857e 100755 --- a/members/printfeercpt.pl +++ b/members/printfeercpt.pl @@ -29,6 +29,7 @@ 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; @@ -69,54 +70,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; @@ -142,6 +128,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 66c19c2a0b..6cb3fe8881 100755 --- a/members/printinvoice.pl +++ b/members/printinvoice.pl @@ -29,6 +29,7 @@ use CGI qw ( -utf8 ); use C4::Members; use C4::Accounts; +use Koha::Account::Lines; use Koha::Patrons; use Koha::Patron::Categories; @@ -64,58 +65,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' ); @@ -141,7 +121,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 9c1c6122eb..c4209be39c 100755 --- a/members/summary-print.pl +++ b/members/summary-print.pl @@ -52,14 +52,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 b007b81ab5..723dced66c 100755 --- a/opac/opac-main.pl +++ b/opac/opac-main.pl @@ -67,24 +67,26 @@ 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 -if ( defined $borrowernumber ){ +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 , $accts, $numaccts) = GetMemberAccountRecords( $borrowernumber ); + + my $total = $patron->account->balance; if ( $checkouts > 0 || $overdues_count > 0 || $holds_pending > 0 || $holds_waiting > 0 || $total > 0 ) { - $template->param( dashboard_info => 1 ); + $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, - ); } $template->param( 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 d66be33a1c..18d7b0f509 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 edda6540a6..6d2edfd064 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -400,14 +400,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); @@ -415,16 +416,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.14.1