From 90a721ae9ca1b7ce12278cd1df963e4e5e49f9a3 Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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