Here's data from a production system that I think would be adversely affected if
cleanup_database.pl --fees 1
was run
Query:
SELECT
a.*,
credit.amount AS 'credit amount',
credit.amountoutstanding 'credit outstanding',
credit.description 'credit description',
credit.accounttype AS 'credit type',
debit.amount AS 'debit amount',
debit.amountoutstanding 'debit outstanding',
debit.description 'debit description',
debit.accounttype AS 'debit type'
FROM
account_offsets a
INNER JOIN accountlines credit on a.credit_id = credit.accountlines_id
INNER JOIN accountlines debit on a.debit_id = debit.accountlines_id
WHERE
credit.amountoutstanding = 0
and debit.amountoutstanding > 0
ORDER BY
a.id desc limit 1
*************************** 1. row ***************************
id: 4214002
credit_id: 1708799
debit_id: 1708796
type: Payment
amount: 0.000000
created_on: 2018-03-02 11:51:05
credit amount: -5.000000
credit outstanding: 0.000000
credit description:
credit type: Pay
debit amount: 0.500000
debit outstanding: 0.500000
debit description: I survived the eruption of Mount St. Helens, 1980 / 02/28/2018 23:59
debit type: F
In this case, credit outstanding, i.e. accountlines.amountoutstanding where accountlines_id = 1708799 is 0, so that row would be deleted by purge_zero_balance_fees, even though it's still linked to by account_offsets, and debit outstanding is not 0.
Not enh as Barton indicates possible unexpected data loss. Created attachment 73350 [details] [review] 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<Warning:> 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 Hi Kyle, this checks out well for me and I am signing off to move this along. Just one question: Would it be safe to remove this warning now from the docs of cleanup_database? WARNING: Fees and payments may not be deleted together. This will not affect the account balance but may be confusing to staff. Created attachment 73516 [details] [review] 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<Warning:> 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 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 73599 [details] [review] 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<Warning:> 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 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73603 [details] [review] 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<Warning:> 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 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> (In reply to Katrin Fischer from comment #4) > Hi Kyle, this checks out well for me and I am signing off to move this > along. Just one question: > > Would it be safe to remove this warning now from the docs of > cleanup_database? > > WARNING: Fees and payments may not be deleted together. This will not affect > the account balance but may be confusing to staff. Kyle, ping Created attachment 73655 [details] [review] Bug 20325: Move tests to a subtest Created attachment 73656 [details] [review] Bug 20325: Do not remove accountlines between tests This is part of the tests, to make sure they have not been deleted (In reply to Jonathan Druart from comment #8) > (In reply to Katrin Fischer from comment #4) > > Hi Kyle, this checks out well for me and I am signing off to move this > > along. Just one question: > > > > Would it be safe to remove this warning now from the docs of > > cleanup_database? > > > > WARNING: Fees and payments may not be deleted together. This will not affect > > the account balance but may be confusing to staff. > > Kyle, ping Absolutely! I'll submit a followup right now! Created attachment 73678 [details] [review] Bug 20325: Remove warning, it is no longer true Pushed to master for 18.05, thanks to everybody involved! Pushed to stable for 17.11.06 Awesome work all! Can you confirm this depends on Bug 14826 ? (In reply to Fridolin SOMERS from comment #15) > Can you confirm this depends on Bug 14826 ? Yes, it's a problem since account_offsets was reintroduced. (In reply to Katrin Fischer from comment #16) > (In reply to Fridolin SOMERS from comment #15) > > Can you confirm this depends on Bug 14826 ? > > Yes, it's a problem since account_offsets was reintroduced. OK thanks a lot. Not pushed to 17.05.x then |
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<Warning:> 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.