From 72058d27415a82799d9f6acac693e94dc7bf9309 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 2 Jul 2018 12:05:55 -0300 Subject: [PATCH] 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 Signed-off-by: Nick Clemens --- Koha/Account.pm | 7 +- Koha/Account/Lines.pm | 22 ++++- members/pay.pl | 3 +- members/paycollect.pl | 5 +- t/db_dependent/Koha/Account.t | 23 ++--- t/db_dependent/Koha/Account/Lines.t | 127 +++++++++++++++++++++------- 6 files changed, 135 insertions(+), 52 deletions(-) diff --git a/Koha/Account.pm b/Koha/Account.pm index 4b48d692ae..8dee72be24 100644 --- 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 0e0677ca85..a36e182339 100644 --- 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 8245f38b2e..1d4c5b0c22 100755 --- 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 167617778f..f1f9f16e97 100755 --- 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 c0cce13609..6ef46afa99 100755 --- 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 0c3287d8c3..11c8df121f 100755 --- 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; +}; -- 2.39.5