Created attachment 23346 [details] Warning 1 Tested in versions 3.12.6, 3.14 and 3.15 (Bywater demo) When deleting by dateexpiry in batch deletion tool. The warning states a low number, and then when it comes back with a total of deletion, it deletes thousands more than it give a warning for. For instance it warned it would delete 102, the deleted 3661 in one system. We can't get the accurate count it is deleting patron's on either way. When queried with the same parameters (dateexpiry) the number is different. To recreate. Go to Batch Patron/Anonymzation Tool Select a dateexpiry date Warning should come up with count to delete Select to Permanently delete (not put in trash) Count of deleted items will show differently than warning-by thousands.
Ouch sounds nasty, So the deleted count is actually correct? IE if you look in the db, it did actually delete that many? And just the warning count is incorrect? Is it deleting ones it shouldn't?
Yes, it is deleting ones it shouldn't and we couldn't get any of the numbers to show up: warning, count, or on the database. We even queried with the deletion parameters before deletion and could not get numbers to match up.
To add insult to injury, this tool in some instances will completely delete the patrons and not move them to deleted_borrowers table. It just nukes the borrowers completely. (deleted permanently is the preselected option) (In reply to Danielle Elder from comment #2) > Yes, it is deleting ones it shouldn't and we couldn't get any of the numbers > to show up: warning, count, or on the database. We even queried with the > deletion parameters before deletion and could not get numbers to match up.
(In reply to Joy Nelson from comment #3) > To add insult to injury, this tool in some instances will completely delete > the patrons and not move them to deleted_borrowers table. It just nukes the > borrowers completely. (deleted permanently is the preselected option) Remedied by bug 11642
(In reply to Chris Cormack from comment #1) > Is it deleting ones it shouldn't? Yes; I can replicate this as well. It will delete (or move to trash) ones it shouldn't. Evidently there is a problem with how given parameter borrower_dateexpiry is (not) being properly converted when passed from step2 to step3 in tools/cleanborrowers.pl - somehow this parameter value gets mangled in the process. In step2 this script does: $filterdate1 = format_date_in_iso( $params->{'filterdate1'} ); $filterdate2 = format_date_in_iso( $params->{'filterdate2'} ); $borrower_dateexpiry = format_date_in_iso( $params->{'borrower_dateexpiry'} ); ... $template->param( filterdate1 => format_date($filterdate1), filterdate2 => format_date($filterdate2), borrower_dateexpiry => $borrower_dateexpiry, Note there is no format_date() call on $borrower_dateexpiry in line #100.. Changing line 100 to borrower_dateexpiry => format_date($borrower_dateexpiry), seems to fix this issue (at least for me). But: my comprehension of date handling in Koha is very far from perfect; I can't really guarantee this would be a proper fix. I guess the exact outcome may also depend on 'dateformat' setting in I18N/L10N preferences (we use dd/mm/yyyy; for yyyy-mm-dd unpatched script may actually work seemingly fine!).
Created attachment 25594 [details] [review] Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete Batch patron deletion/anonymization tool has some issues: 1) If 'dateformat' setting in I18N/L10N system preferences is set to anything other then 'iso' (eg. 'metric'), bulk deletion when using 'expiration date is before' criterion is not working properly. Date entered in this field will be efectivelly ignored (or possibly sometimes wrongly interpreted as different date, in other format) on the final patron deletion stage. This may result in deleting (or moving to trash) more borrower records then intended. 2) Bulk/batch patron deletion should skip borrowers with nonzero account balance (ones with oustanding fines or credits) 3) This tool shouldn't offer to choose as deletion criterion those patron categories which have category_type set to 'S' (= staff patron categories) This patch fixes abovementioned problems. It also adds an option to "test run" patron batch deletion, and makes this option the default choice in "warning" stage. Test plan: - prepare test database with some patron records (at least 2, the more the better) set up in such a way that they will be vulnerable to issues 1 & 2 - confirm issues 1,2 - restore test database - apply patch - ensure issues 1 & 2 are no longer present - first by using new "test run" option: for #1, record counts in "warning" stage and "final" stage should be now the same; for #2, observe that patron records with nonzero balance are now excluded from deletion - redo the tests, this time choosing "delete permanently" and "move to trash" instead of "test run" - test #3 by changing "Category type" to "S" in some test patron categories - after that, those categories should no longer be choosable as deletion criteria.
Created attachment 26047 [details] [review] Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete Batch patron deletion/anonymization tool has some issues: 1) If 'dateformat' setting in I18N/L10N system preferences is set to anything other then 'iso' (eg. 'metric'), bulk deletion when using 'expiration date is before' criterion is not working properly. Date entered in this field will be efectivelly ignored (or possibly sometimes wrongly interpreted as different date, in other format) on the final patron deletion stage. This may result in deleting (or moving to trash) more borrower records then intended. 2) Bulk/batch patron deletion should skip borrowers with nonzero account balance (ones with oustanding fines or credits) 3) This tool shouldn't offer to choose as deletion criterion those patron categories which have category_type set to 'S' (= staff patron categories) This patch fixes abovementioned problems. It also adds an option to "test run" patron batch deletion, and makes this option the default choice in "warning" stage. Test plan: - prepare test database with some patron records (at least 2, the more the better) set up in such a way that they will be vulnerable to issues 1 & 2 - confirm issues 1,2 - restore test database - apply patch - ensure issues 1 & 2 are no longer present - first by using new "test run" option: for #1, record counts in "warning" stage and "final" stage should be now the same; for #2, observe that patron records with nonzero balance are now excluded from deletion - redo the tests, this time choosing "delete permanently" and "move to trash" instead of "test run" - test #3 by changing "Category type" to "S" in some test patron categories - after that, those categories should no longer be choosable as deletion criteria. Signed-off-by: Magnus Enger <digitalutvikling@gmail.com> Tested with dateformat = dd/mm/yyyy. I tested with two expired patrons, one with fines and one without. Before the patch a lot of unexpected patrons were deleted along with the expected ones. After applying the patch only the expired patron was deleted, not the one with fines. The test run and the "real" run reported correct numbers. The patch also makes sure no patron categories with category_type = S are suggested for batch deletion. Note: The ergonomics of the "Batch delete/anonymize" tool is hardly optimal, but this patch fixes a real, data-loosing bug, so let's deal with the ergonomics later.
Created attachment 26488 [details] [review] Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete Batch patron deletion/anonymization tool has some issues: 1) If 'dateformat' setting in I18N/L10N system preferences is set to anything other then 'iso' (eg. 'metric'), bulk deletion when using 'expiration date is before' criterion is not working properly. Date entered in this field will be efectivelly ignored (or possibly sometimes wrongly interpreted as different date, in other format) on the final patron deletion stage. This may result in deleting (or moving to trash) more borrower records then intended. 2) Bulk/batch patron deletion should skip borrowers with nonzero account balance (ones with oustanding fines or credits) 3) This tool shouldn't offer to choose as deletion criterion those patron categories which have category_type set to 'S' (= staff patron categories) This patch fixes abovementioned problems. It also adds an option to "test run" patron batch deletion, and makes this option the default choice in "warning" stage. Test plan: - prepare test database with some patron records (at least 2, the more the better) set up in such a way that they will be vulnerable to issues 1 & 2 - confirm issues 1,2 - restore test database - apply patch - ensure issues 1 & 2 are no longer present - first by using new "test run" option: for #1, record counts in "warning" stage and "final" stage should be now the same; for #2, observe that patron records with nonzero balance are now excluded from deletion - redo the tests, this time choosing "delete permanently" and "move to trash" instead of "test run" - test #3 by changing "Category type" to "S" in some test patron categories - after that, those categories should no longer be choosable as deletion criteria. Signed-off-by: Magnus Enger <digitalutvikling@gmail.com> Tested with dateformat = dd/mm/yyyy. I tested with two expired patrons, one with fines and one without. Before the patch a lot of unexpected patrons were deleted along with the expected ones. After applying the patch only the expired patron was deleted, not the one with fines. The test run and the "real" run reported correct numbers. The patch also makes sure no patron categories with category_type = S are suggested for batch deletion. Note: The ergonomics of the "Batch delete/anonymize" tool is hardly optimal, but this patch fixes a real, data-loosing bug, so let's deal with the ergonomics later. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
I'm wondering if this one is not a critical one. I have a QA followup but I will open a new bug report for that in order not to slow down the push of this one. Marked as Passed QA.
I created and submitted a patch for my QA comments, see bug 11975.
I've opened another follow-up bug (bug 11983) regarding the fact that this patron introduces a /second/ implementation of logic to exclude patrons that have non-zero fine-balances (the other is in the patron deletion cronjob)
Pushed to master. Thanks, Jacek!
Patch pushed to 3.14.x, will be in 3.14.5
I've changed importance by mistake
Pushed to 3.12.x will be in 3.12.13