Bugzilla – Attachment 71800 Details for
Bug 12001
GetMemberAccountRecords slows down display of patron details and checkout pages
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges
Bug-12001-Move-GetMemberAccountBalance-to-KohaAcco.patch (text/plain), 12.91 KB, created by
Jonathan Druart
on 2018-02-16 14:39:26 UTC
(
hide
)
Description:
Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2018-02-16 14:39:26 UTC
Size:
12.91 KB
patch
obsolete
>From 90a721ae9ca1b7ce12278cd1df963e4e5e49f9a3 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Fri, 5 Jan 2018 17:25:41 -0300 >Subject: [PATCH] Bug 12001: Move GetMemberAccountBalance to > Koha::Account->non_issues_charges > >As said previously, GetMemberAccountBalance returns ( the balance, the >non issues charges, the other charges) > >The other charges are the balance - the non issues charges. > >In this patch we remove the subroutine from C4::Members and use the >new Koha::Account->non_issues_charges subroutine instead >--- > C4/Circulation.pm | 33 ++++++++++++------------- > C4/Members.pm | 50 ++++---------------------------------- > Koha/Account.pm | 53 ++++++++++++++++++++++++++++++++++++++++- > t/db_dependent/ILSDI_Services.t | 13 ++++++++-- > t/db_dependent/Members.t | 39 +----------------------------- > 5 files changed, 86 insertions(+), 102 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 43585168c0..2b656f8276 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -753,8 +753,10 @@ sub CanBookBeIssued { > # > > # DEBTS >- my ($balance, $non_issue_charges, $other_charges) = >- C4::Members::GetMemberAccountBalance( $patron->borrowernumber ); >+ my $account = $patron->account; >+ my $balance = $account->balance; >+ my $non_issues_charges = $account->non_issues_charges; >+ my $other_charges = $balance - $non_issues_charges; > > my $amountlimit = C4::Context->preference("noissuescharge"); > my $allowfineoverride = C4::Context->preference("AllowFineOverride"); >@@ -767,8 +769,7 @@ sub CanBookBeIssued { > my @guarantees = $patron->guarantees(); > my $guarantees_non_issues_charges; > foreach my $g ( @guarantees ) { >- my ( $b, $n, $o ) = C4::Members::GetMemberAccountBalance( $g->id ); >- $guarantees_non_issues_charges += $n; >+ $guarantees_non_issues_charges += $g->account->non_issues_charges; > } > > if ( $guarantees_non_issues_charges > $no_issues_charge_guarantees && !$inprocess && !$allowfineoverride) { >@@ -781,21 +782,21 @@ sub CanBookBeIssued { > } > > if ( C4::Context->preference("IssuingInProcess") ) { >- if ( $non_issue_charges > $amountlimit && !$inprocess && !$allowfineoverride) { >- $issuingimpossible{DEBT} = sprintf( "%.2f", $non_issue_charges ); >- } elsif ( $non_issue_charges > $amountlimit && !$inprocess && $allowfineoverride) { >- $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issue_charges ); >- } elsif ( $allfinesneedoverride && $non_issue_charges > 0 && $non_issue_charges <= $amountlimit && !$inprocess ) { >- $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issue_charges ); >+ if ( $non_issues_charges > $amountlimit && !$inprocess && !$allowfineoverride) { >+ $issuingimpossible{DEBT} = sprintf( "%.2f", $non_issues_charges ); >+ } elsif ( $non_issues_charges > $amountlimit && !$inprocess && $allowfineoverride) { >+ $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issues_charges ); >+ } elsif ( $allfinesneedoverride && $non_issues_charges > 0 && $non_issues_charges <= $amountlimit && !$inprocess ) { >+ $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issues_charges ); > } > } > else { >- if ( $non_issue_charges > $amountlimit && $allowfineoverride ) { >- $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issue_charges ); >- } elsif ( $non_issue_charges > $amountlimit && !$allowfineoverride) { >- $issuingimpossible{DEBT} = sprintf( "%.2f", $non_issue_charges ); >- } elsif ( $non_issue_charges > 0 && $allfinesneedoverride ) { >- $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issue_charges ); >+ if ( $non_issues_charges > $amountlimit && $allowfineoverride ) { >+ $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issues_charges ); >+ } elsif ( $non_issues_charges > $amountlimit && !$allowfineoverride) { >+ $issuingimpossible{DEBT} = sprintf( "%.2f", $non_issues_charges ); >+ } elsif ( $non_issues_charges > 0 && $allfinesneedoverride ) { >+ $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issues_charges ); > } > } > >diff --git a/C4/Members.pm b/C4/Members.pm >index 727a7af6de..58f6e8a3de 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -176,7 +176,9 @@ sub patronflags { > my %flags; > my ( $patroninformation) = @_; > my $dbh=C4::Context->dbh; >- my ($balance, $owing) = GetMemberAccountBalance( $patroninformation->{'borrowernumber'}); >+ my $patron = Koha::Patrons->find( $patroninformation->{borrowernumber} ); >+ my $account = $patron->account; >+ my $owing = $account->non_issues_charges; > if ( $owing > 0 ) { > my %flaginfo; > my $noissuescharge = C4::Context->preference("noissuescharge") || 5; >@@ -187,7 +189,7 @@ sub patronflags { > } > $flags{'CHARGES'} = \%flaginfo; > } >- elsif ( $balance < 0 ) { >+ elsif ( ( my $balance = $account->balance ) < 0 ) { > my %flaginfo; > $flaginfo{'message'} = sprintf 'Patron has credit of %.02f', -$balance; > $flaginfo{'amount'} = sprintf "%.02f", $balance; >@@ -202,8 +204,7 @@ sub patronflags { > my @guarantees = $p->guarantees(); > my $guarantees_non_issues_charges; > foreach my $g ( @guarantees ) { >- my ( $b, $n, $o ) = C4::Members::GetMemberAccountBalance( $g->id ); >- $guarantees_non_issues_charges += $n; >+ $guarantees_non_issues_charges += $g->account->non_issues_charges; > } > > if ( $guarantees_non_issues_charges > $no_issues_charge_guarantees ) { >@@ -260,7 +261,6 @@ sub patronflags { > $flags{'ODUES'} = \%flaginfo; > } > >- my $patron = Koha::Patrons->find( $patroninformation->{borrowernumber} ); > my $waiting_holds = $patron->holds->search({ found => 'W' }); > my $nowaiting = $waiting_holds->count; > if ( $nowaiting > 0 ) { >@@ -734,46 +734,6 @@ sub GetAllIssues { > return $sth->fetchall_arrayref( {} ); > } > >- >-=head2 GetMemberAccountBalance >- >- ($total_balance, $non_issue_balance, $other_charges) = &GetMemberAccountBalance($borrowernumber); >- >-Calculates amount immediately owing by the patron - non-issue charges. >-Based on GetMemberAccountRecords. >-Charges exempt from non-issue are: >-* Res (reserves) >-* Rent (rental) if RentalsInNoissuesCharge syspref is set to false >-* Manual invoices if ManInvInNoissuesCharge syspref is set to false >- >-=cut >- >-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; >- push @not_fines, 'Res' unless C4::Context->preference('HoldsInNoissuesCharge'); >- push @not_fines, 'Rent' unless C4::Context->preference('RentalsInNoissuesCharge'); >- unless ( C4::Context->preference('ManInvInNoissuesCharge') ) { >- my $dbh = C4::Context->dbh; >- push @not_fines, @{ $dbh->selectcol_arrayref(qq{SELECT authorised_value FROM authorised_values WHERE category = 'MANUAL_INV'}) }; >- } >- @not_fines = map { substr($_, 0, $ACCOUNT_TYPE_LENGTH) } uniq (@not_fines); >- >- 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); >-} >- > sub checkcardnumber { > my ( $cardnumber, $borrowernumber ) = @_; > >diff --git a/Koha/Account.pm b/Koha/Account.pm >index ba79f6600d..3e3d76adeb 100644 >--- a/Koha/Account.pm >+++ b/Koha/Account.pm >@@ -21,6 +21,7 @@ use Modern::Perl; > > use Carp; > use Data::Dumper; >+use List::MoreUtils qw( uniq ); > > use C4::Log qw( logaction ); > use C4::Stats qw( UpdateStats ); >@@ -281,7 +282,57 @@ sub balance { > ); > > my $total = $fines->count >- ? $fines->next->get_column('total_amountoutstanding') >+ ? $fines->next->get_column('total_amountoutstanding') + 0 >+ : 0; >+} >+ >+=head3 non_issues_charges >+ >+my $non_issues_charges = $self->non_issues_charges >+ >+Calculates amount immediately owing by the patron - non-issue charges. >+ >+Charges exempt from non-issue are: >+* Res (holds) if HoldsInNoissuesCharge syspref is set to false >+* Rent (rental) if RentalsInNoissuesCharge syspref is set to false >+* Manual invoices if ManInvInNoissuesCharge syspref is set to false >+ >+=cut >+ >+sub non_issues_charges { >+ my ($self) = @_; >+ >+ # 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; >+ push @not_fines, 'Res' >+ unless C4::Context->preference('HoldsInNoissuesCharge'); >+ push @not_fines, 'Rent' >+ unless C4::Context->preference('RentalsInNoissuesCharge'); >+ unless ( C4::Context->preference('ManInvInNoissuesCharge') ) { >+ my $dbh = C4::Context->dbh; >+ push @not_fines, >+ @{ >+ $dbh->selectcol_arrayref(q| >+ SELECT authorised_value FROM authorised_values WHERE category = 'MANUAL_INV' >+ |) >+ }; >+ } >+ @not_fines = map { substr( $_, 0, $ACCOUNT_TYPE_LENGTH ) } uniq(@not_fines); >+ >+ my $non_issues_charges = Koha::Account::Lines->search( >+ { >+ borrowernumber => $self->{patron_id}, >+ accounttype => { -not_in => \@not_fines } >+ }, >+ { >+ select => [ { sum => 'amountoutstanding' } ], >+ as => ['non_issues_charges'], >+ } >+ ); >+ return $non_issues_charges->count >+ ? $non_issues_charges->next->get_column('non_issues_charges') + 0 > : 0; > } > >diff --git a/t/db_dependent/ILSDI_Services.t b/t/db_dependent/ILSDI_Services.t >index a433551947..d918e43a73 100644 >--- a/t/db_dependent/ILSDI_Services.t >+++ b/t/db_dependent/ILSDI_Services.t >@@ -166,8 +166,17 @@ subtest 'GetPatronInfo/GetBorrowerAttributes test for extended patron attributes > } > } ); > >- my $members = Test::MockModule->new('C4::Members'); >- $members->mock( 'GetMemberAccountBalance', sub { return ( 10, 10, 0 ); } ); >+ $builder->build( >+ { >+ source => 'Accountline', >+ value => { >+ borrowernumber => $brwr->{borrowernumber}, >+ accountno => 1, >+ accounttype => 'xxx', >+ amountoutstanding => 10 >+ } >+ } >+ ); > > # Prepare and send web request for IL-SDI server: > my $query = new CGI; >diff --git a/t/db_dependent/Members.t b/t/db_dependent/Members.t >index aaf1262139..c7bc6b30ee 100755 >--- a/t/db_dependent/Members.t >+++ b/t/db_dependent/Members.t >@@ -17,7 +17,7 @@ > > use Modern::Perl; > >-use Test::More tests => 61; >+use Test::More tests => 60; > use Test::MockModule; > use Test::Exception; > >@@ -373,43 +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 'GetMemberAccountBalance' => sub { >- >- plan tests => 6; >- >- my $members_mock = new Test::MockModule('C4::Members'); >- $members_mock->mock( 'GetMemberAccountRecords', sub { >- my ($borrowernumber) = @_; >- if ($borrowernumber) { >- my @accountlines = ( >- { amountoutstanding => '7', accounttype => 'Rent' }, >- { amountoutstanding => '5', accounttype => 'Res' }, >- { amountoutstanding => '3', accounttype => 'Pay' } ); >- return ( 15, \@accountlines ); >- } >- else { >- my @accountlines; >- return ( 0, \@accountlines ); >- } >- }); >- >- # do not count holds charges >- t::lib::Mocks::mock_preference( 'HoldsInNoissuesCharge', '1' ); >- t::lib::Mocks::mock_preference( 'ManInvInNoissuesCharge', '0' ); >- my ($total, $total_minus_charges, >- $other_charges) = C4::Members::GetMemberAccountBalance(123); >- is( $total, 15 , "Total calculated correctly"); >- is( $total_minus_charges, 15, "Holds charges are not count if HoldsInNoissuesCharge=1"); >- is( $other_charges, 0, "Holds charges are not considered if HoldsInNoissuesCharge=1"); >- >- t::lib::Mocks::mock_preference( 'HoldsInNoissuesCharge', '0' ); >- ($total, $total_minus_charges, >- $other_charges) = C4::Members::GetMemberAccountBalance(123); >- is( $total, 15 , "Total calculated correctly"); >- is( $total_minus_charges, 10, "Holds charges are count if HoldsInNoissuesCharge=0"); >- is( $other_charges, 5, "Holds charges are considered if HoldsInNoissuesCharge=1"); >-}; >- > subtest 'purgeSelfRegistration' => sub { > plan tests => 2; > >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 12001
:
70314
|
70342
|
70343
|
70344
|
70345
|
70346
|
71798
|
71799
|
71800
|
71801
|
71802
|
71852
|
71853
|
71854
|
71855
|
71856
|
71857
|
71979
|
71980
|
71981
|
71982
|
71983
|
71984
|
71985
|
71986
|
71987
|
71988
|
77066
|
77136
|
77278
|
77279