Bug 20946: (QA follow-up) make outstanding_debits return the account lines only

This patch was discussed with Jonathan on a QA conversation. It is
better to keep this simpler and more reusable. And is the right approach
in this case.

This patch makes Koha::Account::outstanding_debits return the account
lines, and a method is added to Koha::Account::Lines so the outstanding
amount is calculated on the resultset. This is done the dame way it was
done before, and the tests got adjusted.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

diff --git a/Koha/Account.pm b/Koha/Account.pm
index 4b48d69..8dee72b 100644 (file)
--- a/Koha/Account.pm
+++ b/Koha/Account.pm
@@ -22,7 +22,6 @@ use Modern::Perl;
 use Carp;
 use Data::Dumper;
 use List::MoreUtils qw( uniq );
-use List::Util qw( sum0 );
 
 use C4::Log qw( logaction );
 use C4::Stats qw( UpdateStats );
@@ -291,7 +290,7 @@ sub balance {
 
 =head3 outstanding_debits
 
-my ( $total, $lines ) = Koha::Account->new({ patron_id => $patron_id })->outstanding_debits;
+my $lines = Koha::Account->new({ patron_id => $patron_id })->outstanding_debits;
 
 =cut
 
@@ -305,9 +304,7 @@ sub outstanding_debits {
         }
     );
 
-    my $total = sum0( $lines->get_column('amountoutstanding') );
-
-    return ( $total, $lines );
+    return $lines;
 }
 
 =head3 non_issues_charges

diff --git a/Koha/Account/Lines.pm b/Koha/Account/Lines.pm
index 0e0677c..a36e182 100644 (file)
--- a/Koha/Account/Lines.pm
+++ b/Koha/Account/Lines.pm
@@ -18,9 +18,9 @@ package Koha::Account::Lines;
 use Modern::Perl;
 
 use Carp;
+use List::Util qw( sum0 );
 
 use Koha::Database;
-
 use Koha::Account::Line;
 
 use base qw(Koha::Objects);
@@ -34,9 +34,27 @@ Koha::Account::Lines - Koha Account Line Object set class
 
 =head2 Class Methods
 
+=head3 total_outstanding
+
+    my $lines = Koha::Account::Lines->search({ ...  });
+    my $total = $lines->total_outstanding;
+
+Returns the sum of the outstanding amounts of the resultset. If the resultset is
+empty it returns 0.
+
 =cut
 
-=head3 type
+sub total_outstanding {
+    my ( $self ) = @_;
+
+    my $total = sum0( $self->get_column('amountoutstanding') );
+
+    return $total;
+}
+
+=head2 Internal methods
+
+=head3 _type
 
 =cut
 

diff --git a/members/pay.pl b/members/pay.pl
index 8245f38..1d4c5b0 100755 (executable)
--- a/members/pay.pl
+++ b/members/pay.pl
@@ -139,7 +139,8 @@ output_html_with_http_headers $input, $cookie, $template->output;
 sub add_accounts_to_template {
 
     my $patron = Koha::Patrons->find( $borrowernumber );
-    my ( $total, $account_lines ) = Koha::Account->new( { patron_id => $borrowernumber } )->outstanding_debits;
+    my $account_lines = $patron->account->outstanding_debits;
+    my $total = $account_lines->total_outstanding;
     my @accounts;
     while ( my $account_line = $account_lines->next ) {
         $account_line = $account_line->unblessed;

diff --git a/members/paycollect.pl b/members/paycollect.pl
index 1676177..f1f9f16 100755 (executable)
--- a/members/paycollect.pl
+++ b/members/paycollect.pl
@@ -58,8 +58,9 @@ my $borrower       = $patron->unblessed;
 my $category       = $patron->category;
 my $user           = $input->remote_user;
 
-my $branch         = C4::Context->userenv->{'branch'};
-my ( $total_due ) = Koha::Account->new( { patron_id => $borrowernumber } )->outstanding_debits;
+my $branch    = C4::Context->userenv->{'branch'};
+my $total_due = $patron->account->outstanding_debits->total_outstanding;
+
 my $total_paid = $input->param('paid');
 
 my $individual   = $input->param('pay_individual');

diff --git a/t/db_dependent/Koha/Account.t b/t/db_dependent/Koha/Account.t
index c0cce13..6ef46af 100755 (executable)
--- a/t/db_dependent/Koha/Account.t
+++ b/t/db_dependent/Koha/Account.t
@@ -43,10 +43,10 @@ subtest 'outstanding_debits() tests' => sub {
     push @generated_lines, Koha::Account::Line->new({ borrowernumber => $patron->id, amountoutstanding => 3 })->store;
     push @generated_lines, Koha::Account::Line->new({ borrowernumber => $patron->id, amountoutstanding => 4 })->store;
 
-    my $account = Koha::Account->new({ patron_id => $patron->id });
-    my ( $total, $lines ) = $account->outstanding_debits();
+    my $account = $patron->account;
+    my $lines   = $account->outstanding_debits();
 
-    is( $total, 10, 'Outstandig debits total is correctly calculated' );
+    is( $lines->total_outstanding, 10, 'Outstandig debits total is correctly calculated' );
 
     my $i = 0;
     foreach my $line ( @{ $lines->as_list } ) {
@@ -55,16 +55,12 @@ subtest 'outstanding_debits() tests' => sub {
         $i++;
     }
 
-    ( $total, $lines ) =  Koha::Account->new({ patron_id => 'InvalidBorrowernumber' })->outstanding_debits();
-    is( $total, 0, "Total if no outstanding debits is 0" );
-    is( $lines->count, 0, "With no outstanding debits, we get back a Lines object with 0 lines" );
-
     my $patron_2 = $builder->build_object({ class => 'Koha::Patrons' });
     Koha::Account::Line->new({ borrowernumber => $patron_2->id, amountoutstanding => -2 })->store;
     my $just_one = Koha::Account::Line->new({ borrowernumber => $patron_2->id, amountoutstanding =>  3 })->store;
     Koha::Account::Line->new({ borrowernumber => $patron_2->id, amountoutstanding => -6 })->store;
-    ( $total, $lines ) =  Koha::Account->new({ patron_id => $patron_2->id })->outstanding_debits();
-    is( $total, 3, "Total if some outstanding debits and some credits is only debits" );
+    $lines = $patron_2->account->outstanding_debits();
+    is( $lines->total_outstanding, 3, "Total if some outstanding debits and some credits is only debits" );
     is( $lines->count, 1, "With 1 outstanding debits, we get back a Lines object with 1 lines" );
     my $the_line = Koha::Account::Lines->find( $just_one->id );
     is_deeply( $the_line->unblessed, $lines->next->unblessed, "We get back the one correct line");
@@ -73,9 +69,14 @@ subtest 'outstanding_debits() tests' => sub {
     Koha::Account::Line->new({ borrowernumber => $patron_2->id, amountoutstanding => -2 })->store;
     Koha::Account::Line->new({ borrowernumber => $patron_2->id, amountoutstanding => -20 })->store;
     Koha::Account::Line->new({ borrowernumber => $patron_2->id, amountoutstanding => -200 })->store;
-    ( $total, $lines ) =  Koha::Account->new({ patron_id => $patron_3->id })->outstanding_debits();
-    is( $total, 0, "Total if no outstanding debits total is 0" );
+    $lines = $patron_3->account->outstanding_debits();
+    is( $lines->total_outstanding, 0, "Total if no outstanding debits total is 0" );
     is( $lines->count, 0, "With 0 outstanding debits, we get back a Lines object with 0 lines" );
 
+    my $patron_4 = $builder->build_object({ class => 'Koha::Patrons' });
+    $lines = $patron_4->account->outstanding_debits();
+    is( $lines->total_outstanding, 0, "Total if no outstanding debits is 0" );
+    is( $lines->count, 0, "With no outstanding debits, we get back a Lines object with 0 lines" );
+
     $schema->storage->txn_rollback;
 };

diff --git a/t/db_dependent/Koha/Account/Lines.t b/t/db_dependent/Koha/Account/Lines.t
index 0c3287d..11c8df1 100755 (executable)
--- a/t/db_dependent/Koha/Account/Lines.t
+++ b/t/db_dependent/Koha/Account/Lines.t
@@ -19,7 +19,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 1;
+use Test::More tests => 2;
 
 use Koha::Account::Lines;
 use Koha::Items;
@@ -27,39 +27,104 @@ use Koha::Items;
 use t::lib::TestBuilder;
 
 my $schema = Koha::Database->new->schema;
-$schema->storage->txn_begin;
-
 my $builder = t::lib::TestBuilder->new;
 
 subtest 'item' => sub {
-        plan tests => 2;
-
-        my $library = $builder->build( { source => 'Branch' } );
-        my $biblioitem = $builder->build( { source => 'Biblioitem' } );
-        my $patron = $builder->build( { source => 'Borrower' } );
-        my $item = Koha::Item->new(
-        {
-            biblionumber     => $biblioitem->{biblionumber},
-            biblioitemnumber => $biblioitem->{biblioitemnumber},
-            homebranch       => $library->{branchcode},
-            holdingbranch    => $library->{branchcode},
-            barcode          => 'some_barcode_12',
-            itype            => 'BK',
-        })->store;
-
-        my $line = Koha::Account::Line->new(
-        {
-            borrowernumber => $patron->{borrowernumber},
-            itemnumber     => $item->itemnumber,
-            accounttype    => "F",
-            amount         => 10,
-        })->store;
-
-        my $account_line_item = $line->item;
-        is( ref( $account_line_item ), 'Koha::Item', 'Koha::Account::Line->item should return a Koha::Item' );
-        is( $line->itemnumber, $account_line_item->itemnumber, 'Koha::Account::Line->item should return the correct item' );
+
+    plan tests => 2;
+
+    $schema->storage->txn_begin;
+
+    my $library = $builder->build( { source => 'Branch' } );
+    my $biblioitem = $builder->build( { source => 'Biblioitem' } );
+    my $patron = $builder->build( { source => 'Borrower' } );
+    my $item = Koha::Item->new(
+    {
+        biblionumber     => $biblioitem->{biblionumber},
+        biblioitemnumber => $biblioitem->{biblioitemnumber},
+        homebranch       => $library->{branchcode},
+        holdingbranch    => $library->{branchcode},
+        barcode          => 'some_barcode_12',
+        itype            => 'BK',
+    })->store;
+
+    my $line = Koha::Account::Line->new(
+    {
+        borrowernumber => $patron->{borrowernumber},
+        itemnumber     => $item->itemnumber,
+        accounttype    => "F",
+        amount         => 10,
+    })->store;
+
+    my $account_line_item = $line->item;
+    is( ref( $account_line_item ), 'Koha::Item', 'Koha::Account::Line->item should return a Koha::Item' );
+    is( $line->itemnumber, $account_line_item->itemnumber, 'Koha::Account::Line->item should return the correct item' );
+
+    $schema->storage->txn_rollback;
 };
 
-$schema->storage->txn_rollback;
+subtest 'total_outstanding' => sub {
+
+    plan tests => 5;
+
+    $schema->storage->txn_begin;
+
+    my $patron  = $builder->build_object({ class => 'Koha::Patrons' });
+
+    my $lines = Koha::Account::Lines->search({ borrowernumber => $patron->id });
+    is( $lines->total_outstanding, 0, 'total_outstanding returns 0 if no lines (undef case)' );
 
-1;
+    my $debit_1 = Koha::Account::Line->new(
+        {   borrowernumber    => $patron->id,
+            accounttype       => "F",
+            amount            => 10,
+            amountoutstanding => 10
+        }
+    )->store;
+
+    my $debit_2 = Koha::Account::Line->new(
+        {   borrowernumber    => $patron->id,
+            accounttype       => "F",
+            amount            => 10,
+            amountoutstanding => 10
+        }
+    )->store;
+
+    $lines = Koha::Account::Lines->search({ borrowernumber => $patron->id });
+    is( $lines->total_outstanding, 20, 'total_outstanding sums correctly' );
+
+    my $credit_1 = Koha::Account::Line->new(
+        {   borrowernumber    => $patron->id,
+            accounttype       => "F",
+            amount            => -10,
+            amountoutstanding => -10
+        }
+    )->store;
+
+    $lines = Koha::Account::Lines->search({ borrowernumber => $patron->id });
+    is( $lines->total_outstanding, 10, 'total_outstanding sums correctly' );
+
+    my $credit_2 = Koha::Account::Line->new(
+        {   borrowernumber    => $patron->id,
+            accounttype       => "F",
+            amount            => -10,
+            amountoutstanding => -10
+        }
+    )->store;
+
+    $lines = Koha::Account::Lines->search({ borrowernumber => $patron->id });
+    is( $lines->total_outstanding, 0, 'total_outstanding sums correctly' );
+
+    my $credit_3 = Koha::Account::Line->new(
+        {   borrowernumber    => $patron->id,
+            accounttype       => "F",
+            amount            => -100,
+            amountoutstanding => -100
+        }
+    )->store;
+
+    $lines = Koha::Account::Lines->search({ borrowernumber => $patron->id });
+    is( $lines->total_outstanding, -100, 'total_outstanding sums correctly' );
+
+    $schema->storage->txn_rollback;
+};