From 6cd4252046af0fe30385df3023864c676e365ccf Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Tue, 27 Mar 2018 15:16:16 -0400 Subject: [PATCH] Bug 20325: C4::Accounts::purge_zero_balance_fees does not check account_offsets purge_zero_balance_fees is used in cleanup_database.pl to determine which fees can be cleaned up. It uses a simple SQL query to determine which rows in accountlines need to be removed: 463 my $sth = $dbh->prepare( 464 q{ 465 DELETE FROM accountlines 466 WHERE date < date_sub(curdate(), INTERVAL ? DAY) 467 AND ( amountoutstanding = 0 or amountoutstanding IS NULL ); 468 } The function comes with the following warning: 451 B Because fines and payments are not linked in accountlines, it is 452 possible for a fine to be deleted without the accompanying payment, 453 or vise versa. This won't affect the account balance, but might be 454 confusing to staff. This was a reasonable solution prior to the addition of account_offsets in 17.11. The problem now is that rows in accountlines which are linked as credits in accountlines will *always* have amountoutstanding marked as 0. These are linked to debits via account_offsets. purge_zero_balance_fees will delete credits and leave rows in account_offsets which link to deleted credits. Sites using the --fees option cleanup_database.pl which upgrade to 17.11 may have all of their credits removed without warning. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Accounts.t --- C4/Accounts.pm | 15 ++++++++++++--- t/db_dependent/Accounts.t | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/C4/Accounts.pm b/C4/Accounts.pm index b84627b3e7..4a2b18852c 100644 --- a/C4/Accounts.pm +++ b/C4/Accounts.pm @@ -448,9 +448,18 @@ sub purge_zero_balance_fees { my $dbh = C4::Context->dbh; my $sth = $dbh->prepare( q{ - DELETE FROM accountlines - WHERE date < date_sub(curdate(), INTERVAL ? DAY) - AND ( amountoutstanding = 0 or amountoutstanding IS NULL ); + DELETE a1 FROM accountlines a1 + + LEFT JOIN account_offsets credit_offset ON ( a1.accountlines_id = credit_offset.credit_id ) + LEFT JOIN accountlines a2 ON ( credit_offset.debit_id = a2.accountlines_id ) + + LEFT JOIN account_offsets debit_offset ON ( a1.accountlines_id = debit_offset.debit_id ) + LEFT JOIN accountlines a3 ON ( debit_offset.credit_id = a3.accountlines_id ) + + WHERE a1.date < date_sub(curdate(), INTERVAL ? DAY) + AND ( a1.amountoutstanding = 0 or a1.amountoutstanding IS NULL ) + AND ( a2.amountoutstanding = 0 or a2.amountoutstanding IS NULL ) + AND ( a3.amountoutstanding = 0 or a3.amountoutstanding IS NULL ) } ); $sth->execute($days) or die $dbh->errstr; diff --git a/t/db_dependent/Accounts.t b/t/db_dependent/Accounts.t index affaacd207..426a4a4ecd 100644 --- a/t/db_dependent/Accounts.t +++ b/t/db_dependent/Accounts.t @@ -18,7 +18,7 @@ use Modern::Perl; -use Test::More tests => 24; +use Test::More tests => 30; use Test::MockModule; use Test::Warn; @@ -133,6 +133,37 @@ for my $data (@test_data) { is_delete_correct( $data->{delete}, $data->{description}); } +$dbh->do(q|DELETE FROM accountlines|); +my $debit = Koha::Account::Line->new({ borrowernumber => $borrower->id, date => '1900-01-01', amountoutstanding => 0 })->store(); +my $credit = Koha::Account::Line->new({ borrowernumber => $borrower->id, date => '1900-01-01', amountoutstanding => -5 })->store(); +my $offset = Koha::Account::Offset->new({ credit_id => $credit->id, debit_id => $debit->id, type => 'Payment' })->store(); +purge_zero_balance_fees( 1 ); +my $debit_2 = Koha::Account::Lines->find( $debit->id ); +my $credit_2 = Koha::Account::Lines->find( $credit->id ); +ok( $debit_2, 'Debit was correctly not deleted when credit has balance' ); +ok( $credit_2, 'Credit was correctly not deleted when credit has balance' ); + +$dbh->do(q|DELETE FROM accountlines|); +$debit = Koha::Account::Line->new({ borrowernumber => $borrower->id, date => '1900-01-01', amountoutstanding => 5 })->store(); +$credit = Koha::Account::Line->new({ borrowernumber => $borrower->id, date => '1900-01-01', amountoutstanding => 0 })->store(); +$offset = Koha::Account::Offset->new({ credit_id => $credit->id, debit_id => $debit->id, type => 'Payment' })->store(); +purge_zero_balance_fees( 1 ); +$debit_2 = $credit_2 = undef; +$debit_2 = Koha::Account::Lines->find( $debit->id ); +$credit_2 = Koha::Account::Lines->find( $credit->id ); +ok( $debit_2, 'Debit was correctly not deleted when debit has balance' ); +ok( $credit_2, 'Credit was correctly not deleted when debit has balance' ); + +$dbh->do(q|DELETE FROM accountlines|); +$debit = Koha::Account::Line->new({ borrowernumber => $borrower->id, date => '1900-01-01', amountoutstanding => 0 })->store(); +$credit = Koha::Account::Line->new({ borrowernumber => $borrower->id, date => '1900-01-01', amountoutstanding => 0 })->store(); +$offset = Koha::Account::Offset->new({ credit_id => $credit->id, debit_id => $debit->id, type => 'Payment' })->store(); +purge_zero_balance_fees( 1 ); +$debit_2 = Koha::Account::Lines->find( $debit->id ); +$credit_2 = Koha::Account::Lines->find( $credit->id ); +ok( !$debit_2, 'Debit was correctly deleted' ); +ok( !$credit_2, 'Credit was correctly deleted' ); + $dbh->do(q|DELETE FROM accountlines|); subtest "Koha::Account::pay tests" => sub { -- 2.14.3 (Apple Git-98)