From 0a32e2114b4265742a98cacb95d8d377bed167a4 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 Signed-off-by: Josef Moravec --- 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 | 74 +++++++++------------- 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, 197 insertions(+), 265 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 4b39a2d..bd01966 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2696,7 +2696,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 90ebb54..727a7af 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 6cd3300..17034c0 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 ed1fd3c..ba79f66 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 f7578e7..2065de7 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 7414378..93ed09c 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -555,7 +555,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->is_child) { 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 158470f..d7684a6 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 32e7533..c344cc8 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 af35fe8..22fc699 100755 --- a/members/boraccount.pl +++ b/members/boraccount.pl @@ -71,14 +71,23 @@ if ( $patron->is_child ) { } #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; } if (C4::Context->preference('ExtendedPatronAttributes')) { @@ -110,7 +125,7 @@ $template->param( total => sprintf("%.2f",$total), totalcredit => $totalcredit, 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 2130228..0f7eddd 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); @@ -214,18 +215,12 @@ else { my $library = Koha::Libraries->find( $data->{branchcode})->unblessed; @{$data}{keys %$library} = values %$library; # merge in all branch columns # FIXME This is really ugly, we should pass the library instead -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 ); } @@ -339,6 +334,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 14329ef..05b73e5 100755 --- a/members/pay.pl +++ b/members/pay.pl @@ -124,7 +124,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 32f0370..7c16a0a 100755 --- a/members/paycollect.pl +++ b/members/paycollect.pl @@ -58,7 +58,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 53e4445..c73f8f5 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; @@ -64,59 +65,46 @@ if ( $patron->is_child ) { } #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( patron => $patron, finesview => 1, total => sprintf("%.2f",$total), totalcredit => $totalcredit, - 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 a3ad69b..a75b0a6 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; @@ -59,65 +60,44 @@ if ( $patron->is_child ) { } #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( patron => $patron, finesview => 1, total => sprintf( "%.2f", $total ), totalcredit => $totalcredit, - 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 01dcf22..03e9d89 100755 --- a/members/summary-print.pl +++ b/members/summary-print.pl @@ -47,14 +47,11 @@ my $logged_in_user = Koha::Patrons->find( $loggedinuser ) or die "Not logged in" my $patron = Koha::Patrons->find( $borrowernumber ); output_and_exit_if_error( $input, $cookie, $template, { module => 'members', logged_in_user => $logged_in_user, current_patron => $patron } ); -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 c93b5b7..9611554 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 b007b81..723dced 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 2e4532e..2be6991 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 55c04a3..ac91d4f 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 6716131..d94a8ab 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -179,12 +179,13 @@ if ($borrowernumber_hold && !$action) { $diffbranch = 1; } + my $amount_outstanding = $patron->account->balance; $template->param( expiry => $expiry, diffbranch => $diffbranch, messages => $messages, warnings => $warnings, - 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 b5a36a3..7ad7d05 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -396,14 +396,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); @@ -411,16 +412,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 ac928b2..aaf1262 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.1.4