Description
Barton Chittenden
2015-06-17 12:09:49 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! Created attachment 41043 [details] [review] Add --fees option to cleanup_database.pl (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. Created attachment 41086 [details] [review] Add function purge_zero_balance_fees to C4/Accounts.pm Created attachment 41087 [details] [review] 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. Created attachment 41088 [details] [review] 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(). (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. Created attachment 42242 [details] [review] 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. Created attachment 42243 [details] [review] [SIGNED-OFF] 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> Created attachment 42244 [details] [review] [SIGNED-OFF] 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> Created attachment 42245 [details] [review] [SIGNED-OFF] 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> Created attachment 42246 [details] [review] [SIGNED-OFF] 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. http://bugs.koha-community.org/show_bug.cgi?id=14402 Signed-off-by: Nancy Keener <nkeener@washoecounty.us> CONFLICT (content): Merge conflict in t/db_dependent/Accounts.t Created attachment 42477 [details] [review] 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> Created attachment 42478 [details] [review] 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> Created attachment 42479 [details] [review] 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> Created attachment 42480 [details] [review] [SIGNED-OFF] 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> (In reply to Jonathan Druart from comment #13) > CONFLICT (content): Merge conflict in t/db_dependent/Account Fixed conflicts. Please try again. 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. Created attachment 42556 [details] [review] Bug 14402 - QA Followup Created attachment 42557 [details] [review] Bug 14402 - QA Followup The script does not accept --fees 0, is it expected? Created attachment 42815 [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> Created attachment 42816 [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> Created attachment 42817 [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> Created attachment 42818 [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> Created attachment 42819 [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> Created attachment 42820 [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. Can I get a status on Q and A? Nancy K 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> 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> 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> 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> Created attachment 42997 [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> 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> Patches pushed to master. Thanks Barton! *** Bug 4076 has been marked as a duplicate of this bug. *** (In reply to Katrin Fischer from comment #37) > *** Bug 4076 has been marked as a duplicate of this bug. *** Thank you Katrin. |