Bug 20325 - C4::Accounts::purge_zero_balance_fees does not check account_offsets
Summary: C4::Accounts::purge_zero_balance_fees does not check account_offsets
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-02 18:12 UTC by Barton Chittenden
Modified: 2018-05-22 10:54 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20325: C4::Accounts::purge_zero_balance_fees does not check account_offsets (5.50 KB, patch)
2018-03-27 19:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20325: C4::Accounts::purge_zero_balance_fees does not check account_offsets (5.47 KB, patch)
2018-04-01 10:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20325: C4::Accounts::purge_zero_balance_fees does not check account_offsets (5.53 KB, patch)
2018-04-04 09:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20325: C4::Accounts::purge_zero_balance_fees does not check account_offsets (5.62 KB, patch)
2018-04-04 10:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20325: Move tests to a subtest (5.69 KB, patch)
2018-04-04 18:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20325: Do not remove accountlines between tests (2.95 KB, patch)
2018-04-04 18:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20325: Remove warning, it is no longer true (1.18 KB, patch)
2018-04-05 10:12 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2018-03-02 18:12:26 UTC
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.
Comment 1 Barton Chittenden 2018-03-02 18:23:42 UTC
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.
Comment 2 Katrin Fischer 2018-03-04 11:32:46 UTC
Not enh as Barton indicates possible unexpected data loss.
Comment 3 Kyle M Hall 2018-03-27 19:17:31 UTC
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
Comment 4 Katrin Fischer 2018-04-01 10:36:56 UTC
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.
Comment 5 Katrin Fischer 2018-04-01 10:37:49 UTC
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>
Comment 6 Josef Moravec 2018-04-04 09:41:55 UTC
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>
Comment 7 Kyle M Hall 2018-04-04 10:32:15 UTC
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>
Comment 8 Jonathan Druart 2018-04-04 18:28:34 UTC
(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
Comment 9 Jonathan Druart 2018-04-04 18:29:07 UTC
Created attachment 73655 [details] [review]
Bug 20325: Move tests to a subtest
Comment 10 Jonathan Druart 2018-04-04 18:37:21 UTC
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
Comment 11 Kyle M Hall 2018-04-05 10:10:43 UTC
(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!
Comment 12 Kyle M Hall 2018-04-05 10:12:59 UTC
Created attachment 73678 [details] [review]
Bug 20325: Remove warning, it is no longer true
Comment 13 Jonathan Druart 2018-04-05 17:54:46 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 14 Nick Clemens 2018-05-14 21:13:49 UTC
Pushed to stable for 17.11.06

Awesome work all!
Comment 15 Fridolin SOMERS 2018-05-22 10:54:58 UTC
Can you confirm this depends on Bug 14826 ?