Bug 11352

Summary: Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete
Product: Koha Reporter: Danielle Elder <danielle>
Component: PatronsAssignee: Jacek Ablewicz <abl>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: critical    
Priority: P3 CC: 1joynelson, abl, chris, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, kyle, magnus, nengard
Version: 3.14   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11642
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15398
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 11983, 11975    
Attachments: Warning 1
Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete
Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete
Bug 11352 - Batch Patron Deletion/Anonmyzation deletes more than warning states it will delete

Description Danielle Elder 2013-12-07 01:03:58 UTC
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.
Comment 1 Chris Cormack 2013-12-10 21:27:19 UTC
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?
Comment 2 Danielle Elder 2013-12-10 21:33:07 UTC
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.
Comment 3 Joy Nelson 2014-01-30 15:11:43 UTC
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.
Comment 4 Kyle M Hall 2014-01-30 16:45:32 UTC
(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
Comment 5 Jacek Ablewicz 2014-02-02 11:46:27 UTC
(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!).
Comment 6 Jacek Ablewicz 2014-02-24 17:56:05 UTC Comment hidden (obsolete)
Comment 7 Magnus Enger 2014-03-11 08:42:40 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-03-20 15:38:49 UTC
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>
Comment 9 Jonathan Druart 2014-03-20 15:40:57 UTC
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.
Comment 10 Jonathan Druart 2014-03-20 15:49:03 UTC
I created and submitted a patch for my QA comments, see bug 11975.
Comment 11 Galen Charlton 2014-03-22 13:55:04 UTC
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)
Comment 12 Galen Charlton 2014-03-22 13:56:42 UTC
Pushed to master.  Thanks, Jacek!
Comment 13 Fridolin Somers 2014-03-25 08:37:34 UTC
Patch pushed to 3.14.x, will be in 3.14.5
Comment 14 Fridolin Somers 2014-03-25 09:11:19 UTC
I've changed importance by mistake
Comment 15 Chris Cormack 2014-03-26 07:08:06 UTC
Pushed to 3.12.x will be in 3.12.13