Bug 14402 - Add --fees option to cleanup_database.pl to purge all 0 balance fees and payments older than the given number of days
Summary: Add --fees option to cleanup_database.pl to purge all 0 balance fees and paym...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Barton Chittenden
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 4076 (view as bug list)
Depends on:
Blocks: 16390
  Show dependency treegraph
 
Reported: 2015-06-17 12:09 UTC by Barton Chittenden
Modified: 2016-12-05 21:31 UTC (History)
10 users (show)

See Also:
Change sponsored?: 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
Add --fees option to cleanup_database.pl (3.36 KB, patch)
2015-07-16 19:29 UTC, Barton Chittenden
Details | Diff | Splinter Review
Add function purge_zero_balance_fees to C4/Accounts.pm (1.56 KB, patch)
2015-07-18 20:36 UTC, Barton Chittenden
Details | Diff | Splinter Review
Add unit tests for purge_zero_balance_fees() (5.34 KB, patch)
2015-07-18 20:36 UTC, Barton Chittenden
Details | Diff | Splinter Review
Add option --fees to /misc/cronjobs/cleanup_database.pl (3.43 KB, patch)
2015-07-18 20:37 UTC, Barton Chittenden
Details | Diff | Splinter Review
Make purge_zero_balance_fees() delete fees with NULL balance. (6.72 KB, patch)
2015-09-02 19:44 UTC, Barton Chittenden
Details | Diff | Splinter Review
[SIGNED-OFF] Add function purge_zero_balance_fees to C4/Accounts.pm (1.63 KB, patch)
2015-09-02 20:47 UTC, Barton Chittenden
Details | Diff | Splinter Review
[SIGNED-OFF] Add unit tests for purge_zero_balance_fees() (5.41 KB, patch)
2015-09-02 20:47 UTC, Barton Chittenden
Details | Diff | Splinter Review
[SIGNED-OFF] Add option --fees to /misc/cronjobs/cleanup_database.pl (3.50 KB, patch)
2015-09-02 20:47 UTC, Barton Chittenden
Details | Diff | Splinter Review
[SIGNED-OFF] Make purge_zero_balance_fees() delete fees with NULL balance. (6.79 KB, patch)
2015-09-02 20:47 UTC, Barton Chittenden
Details | Diff | Splinter Review
Add function purge_zero_balance_fees to C4/Accounts.pm (1.61 KB, patch)
2015-09-10 22:54 UTC, Barton Chittenden
Details | Diff | Splinter Review
Add unit tests for purge_zero_balance_fees() (5.23 KB, patch)
2015-09-10 22:54 UTC, Barton Chittenden
Details | Diff | Splinter Review
Add option --fees to /misc/cronjobs/cleanup_database.pl (3.55 KB, patch)
2015-09-10 22:55 UTC, Barton Chittenden
Details | Diff | Splinter Review
[SIGNED-OFF] bug 14402: Make purge_zero_balance_fees() delete fees with NULL balance. (6.74 KB, patch)
2015-09-10 22:55 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14402 - QA Followup (4.45 KB, patch)
2015-09-15 12:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14402 - QA Followup (4.68 KB, patch)
2015-09-15 12:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14402: Add function purge_zero_balance_fees to C4/Accounts.pm (1.62 KB, patch)
2015-09-23 14:47 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14402: Add function purge_zero_balance_fees to C4/Accounts.pm (1.62 KB, patch)
2015-09-23 14:49 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14402: Add unit tests for purge_zero_balance_fees() (5.24 KB, patch)
2015-09-23 14:50 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14402: Add option --fees to /misc/cronjobs/cleanup_database.pl (3.56 KB, patch)
2015-09-23 14:50 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14402: Make purge_zero_balance_fees() delete fees with NULL balance. (6.73 KB, patch)
2015-09-23 14:50 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14402: - QA Followup (5.37 KB, patch)
2015-09-23 14:50 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14402: Add function purge_zero_balance_fees to C4/Accounts.pm (1.69 KB, patch)
2015-10-01 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14402: Add unit tests for purge_zero_balance_fees() (5.31 KB, patch)
2015-10-01 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14402: Add option --fees to /misc/cronjobs/cleanup_database.pl (3.63 KB, patch)
2015-10-01 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14402: Make purge_zero_balance_fees() delete fees with NULL balance. (6.79 KB, patch)
2015-10-01 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14402: - QA Followup (5.44 KB, patch)
2015-10-01 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14402: - QA Followup (5.22 KB, patch)
2015-11-09 17:53 UTC, Barton Chittenden
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2015-06-17 12:09:49 UTC
Work to be done:
Goal: Add ability to purge paid fees and payments that are older than a given number of days.
Course of action: Add new option --fees to cleanup_database.pl to purge all 0 balance fees and payments older than the given number of days
Comment 1 Katrin Fischer 2015-06-21 09:53:05 UTC
Hm, this would need good testing. I think you also need to make sure there is no problem with the way credits work. But it would be good to have!
Comment 2 Barton Chittenden 2015-07-16 19:29:26 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2015-07-17 16:49:36 UTC
(In reply to Katrin Fischer from comment #1)
> Hm, this would need good testing. I think you also need to make sure there
> is no problem with the way credits work. But it would be good to have!

Credits have a negative amount and amountoutstanding, so the query would definitely not affect credits.
Comment 4 Barton Chittenden 2015-07-18 20:36:48 UTC Comment hidden (obsolete)
Comment 5 Barton Chittenden 2015-07-18 20:36:59 UTC Comment hidden (obsolete)
Comment 6 Barton Chittenden 2015-07-18 20:37:03 UTC Comment hidden (obsolete)
Comment 7 Barton Chittenden 2015-09-02 15:47:03 UTC
(In reply to Kyle M Hall from comment #3)
> (In reply to Katrin Fischer from comment #1)
> > Hm, this would need good testing. I think you also need to make sure there
> > is no problem with the way credits work. But it would be good to have!
> 
> Credits have a negative amount and amountoutstanding, so the query would
> definitely not affect credits.

Payments and credits have a negative amount and a *0* amountoutstanding.

As I understand accountlines, the account balance is calculated by summing the amountoutstanding... so removing the records for credits and payments won't affect the balance, but I'm concerned about partial payments: consider the case of partial payments:

+-----------------+----------------+-----------+-------------------+------------+------------------------+
| accountlines_id | borrowernumber | amount    | amountoutstanding | date       | description            |
+-----------------+----------------+-----------+-------------------+------------+------------------------+
|              10 |              1 |  6.000000 |          3.000000 | 2015-08-03 | test 5                 |
|              11 |              1 |  3.000000 |          0.000000 | 2015-08-03 |  partial payment       |
+-----------------+----------------+-----------+-------------------+------------+------------------------+

If we run cleanup_database.pl with a --fees 20, accountlines_id 11 will be removed (0.000000 amountoutstanding, date < 2015-08-13). but accountlines_id 10 won't be removed, because 3.000000 amountoutstanding > 0. The borrower's balance will remain at 3.00, (rather than the original 6.00), but the payment record won't be there.

There's no way around this -- accountlines doesn't provide a way to determine what a credit or debit was applied to.
Comment 8 Barton Chittenden 2015-09-02 19:44:22 UTC Comment hidden (obsolete)
Comment 9 Barton Chittenden 2015-09-02 20:47:24 UTC Comment hidden (obsolete)
Comment 10 Barton Chittenden 2015-09-02 20:47:50 UTC Comment hidden (obsolete)
Comment 11 Barton Chittenden 2015-09-02 20:47:55 UTC Comment hidden (obsolete)
Comment 12 Barton Chittenden 2015-09-02 20:47:57 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-09-03 10:08:59 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Accounts.t
Comment 14 Barton Chittenden 2015-09-10 22:54:21 UTC Comment hidden (obsolete)
Comment 15 Barton Chittenden 2015-09-10 22:54:42 UTC Comment hidden (obsolete)
Comment 16 Barton Chittenden 2015-09-10 22:55:03 UTC Comment hidden (obsolete)
Comment 17 Barton Chittenden 2015-09-10 22:55:25 UTC Comment hidden (obsolete)
Comment 18 Barton Chittenden 2015-09-10 22:57:25 UTC
(In reply to Jonathan Druart from comment #13)
> CONFLICT (content): Merge conflict in t/db_dependent/Account

Fixed conflicts. Please try again.
Comment 19 Jonathan Druart 2015-09-15 09:14:15 UTC
 FAIL   misc/cronjobs/cleanup_database.pl
   FAIL   valid
                Global symbol "$zebraqueue_days" requires explicit package name 
                Global symbol "$allDebarments" requires explicit package name 
                Global symbol "$verbose" requires explicit package name 
                Global symbol "$pDebarments" requires explicit package name 
                Global symbol "$pZ3950" requires explicit package name 
                Global symbol "$pUnvSelfReg" requires explicit package name 
                Global symbol "$fees_days" requires explicit package name 
                Global symbol "$sess_days" requires explicit package name 
                    $help"
                Global symbol "$pExpSelfReg" requires explicit package name 
                Global symbol "$pListShareInvites" requires explicit package name 
                Global symbol "$purge_merged" requires explicit package name 
                Global symbol "$sessions" requires explicit package name 
                Global symbol "$help" requires explicit package name 
                syntax error 
                Scalar found where operator expected 
                Global symbol "$pLogs" requires explicit package name 
                Global symbol "$pSearchhistory" requires explicit package name 
                        (Missing operator before $help?)
                Global symbol "$mail" requires explicit package name 
                misc/cronjobs/cleanup_database.pl had compilation errors.
                Global symbol "$pImport" requires explicit package name 

 FAIL   t/db_dependent/Accounts.t
   FAIL   spelling
                 postive  ==> positive
   FAIL   valid
                Global symbol "$sth" requires explicit package name 
                t/db_dependent/Accounts.t had compilation errors.
                Global symbol "$borrower" requires explicit package name 

And add the "Bug XXXXX" in the commit messages.
Comment 20 Kyle M Hall 2015-09-15 12:46:35 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2015-09-15 12:50:10 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2015-09-16 09:21:05 UTC
The script does not accept --fees 0, is it expected?
Comment 23 Barton Chittenden 2015-09-23 14:47:06 UTC Comment hidden (obsolete)
Comment 24 Barton Chittenden 2015-09-23 14:49:47 UTC Comment hidden (obsolete)
Comment 25 Barton Chittenden 2015-09-23 14:50:07 UTC Comment hidden (obsolete)
Comment 26 Barton Chittenden 2015-09-23 14:50:19 UTC Comment hidden (obsolete)
Comment 27 Barton Chittenden 2015-09-23 14:50:31 UTC Comment hidden (obsolete)
Comment 28 Barton Chittenden 2015-09-23 14:50:41 UTC Comment hidden (obsolete)
Comment 29 NancyK. 2015-09-30 17:51:20 UTC
Can I get a status on Q and A?

Nancy K
Comment 30 Jonathan Druart 2015-10-01 09:24:57 UTC
Created attachment 42993 [details] [review]
Bug 14402: Add function purge_zero_balance_fees to C4/Accounts.pm

http://bugs.koha-community.org/show_bug.cgi?id=14402

Signed-off-by: Nancy Keener <nkeener@washoecounty.us>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2015-10-01 09:25:03 UTC
Created attachment 42994 [details] [review]
Bug 14402: Add unit tests for purge_zero_balance_fees()

The function C4::Accounts::purge_zero_balance_fees() should delete rows
in accountlines where amountoutstanding is 0 and accountlines.date is
less than the current date minus '$days', i.e a number of days passed
to the function. Tests were added to prove the following:

* accountlines.amountoutstanding is 0, and date is set to CURRENT_DATE.
  The accountlines row should not be deleted. This is merely a sanity check,
  because difference between today's date and the fee date cannot be
  greater than $days.
* 'Before', 'On' and 'After' tests for accountlines.amountoutstanding = 0
    * accountlines.amountoutstanding is 0, and date is set to $days - 1
      days ago. The accountlines row should not be deleted.
    * accountlines.amountoutstanding is 0, and date is set to $days
      days ago.  the accountlines row should not be deleted, because
      the difference must be *greater* than $days.
    * accountlines.amountoutstanding is 0, and date is set to $days + 1
      days ago. The accountlines row *will* be deleted in this case.
* 'Before', 'On' and 'After' tests for accountlines.amountoutstanding > 0.
  (3 tests). The accountlines row will never be deleted, because
  amountoutstanding is not 0.
* 'Before', 'On' and 'After' tests for accountlines.amountoutstanding < 0.
  (3 testes) This tests credits. Again, the accountlines row will never
  be deleted, because amountoutstanding is not 0.

http://bugs.koha-community.org/show_bug.cgi?id=14402

Signed-off-by: Nancy Keener <nkeener@washoecounty.us>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2015-10-01 09:25:06 UTC
Created attachment 42995 [details] [review]
Bug 14402: Add option --fees to /misc/cronjobs/cleanup_database.pl

This will call C4::Accounts::purge_zero_balance_fees to remove all rows
from accountlines which have 0 balance, and are more than DAYS old.

Test plan:

  Run: t/db_dependent/Accounts.t

If all unit tests pass, the --fees option to /misc/cronjobs/cleanup_database.pl
should act as documented, because it is a straight pass-through to
C4::Accounts::purge_zero_balance_fees().

http://bugs.koha-community.org/show_bug.cgi?id=14402

Signed-off-by: Nancy Keener <nkeener@washoecounty.us>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2015-10-01 09:25:10 UTC
Created attachment 42996 [details] [review]
Bug 14402: Make purge_zero_balance_fees() delete fees with NULL balance.

Also, add notes to perldocs and usage that payments and
credits are not linked to fines and may be deleted
independently of the associated fine.

Signed-off-by: Nancy Keener <nkeener@washoecounty.us>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 34 Jonathan Druart 2015-10-01 09:25:14 UTC Comment hidden (obsolete)
Comment 35 Barton Chittenden 2015-11-09 17:53:56 UTC
Created attachment 44664 [details] [review]
Bug 14402: - QA Followup

Added notes to usage in misc/cronjobs/cleanup_database.pl that
--fees DAYS requires DAYS to be greater than or equal to 1.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 36 Tomás Cohen Arazi 2015-11-09 18:05:28 UTC
Patches pushed to master.

Thanks Barton!
Comment 37 Katrin Fischer 2016-06-19 22:02:06 UTC
*** Bug 4076 has been marked as a duplicate of this bug. ***
Comment 38 vimal kumar 2016-06-20 06:42:45 UTC
(In reply to Katrin Fischer from comment #37)
> *** Bug 4076 has been marked as a duplicate of this bug. ***

Thank you Katrin.