Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges
authorJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 5 Jan 2018 20:25:41 +0000 (17:25 -0300)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Fri, 23 Feb 2018 13:57:30 +0000 (10:57 -0300)
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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

C4/Circulation.pm
C4/Members.pm
Koha/Account.pm
t/db_dependent/ILSDI_Services.t
t/db_dependent/Members.t

index bd01966..9bd4e06 100644 (file)
@@ -746,8 +746,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");
@@ -760,8 +762,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) {
@@ -774,21 +775,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 );
         }
     }
 
index 727a7af..58f6e8a 100644 (file)
@@ -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 ) = @_;
 
index ba79f66..3e3d76a 100644 (file)
@@ -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;
 }
 
index a433551..d918e43 100644 (file)
@@ -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;
index aaf1262..c7bc6b3 100755 (executable)
@@ -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;