Description
Liz Rea
2020-04-17 16:25:17 UTC
Maybe instead of deleting them wholly the first go, we put them in deleted borrowers for a time, and then purge them from deleted borrowers once a year? It could be a privacy issue. If the patron never finished the registration process, there might be no legal base for being allowed to store their data. deleted_borrowers is difficult in that regard anyway, but you could argue that usually patrons in there have been properly registered before. Often that would mean they have signed something on their first library visit. We still have no Koha script to clean deleted_borrowers afaik. :( Long argument short... I'd like to keep the option to delete them right away instead of moving them. That's fine by me, making it an option. :) The Koha tool (delete patrons) provides a way to send patrons to the trash or permanently delete. This would be a good option to include for self reg patrons Patron deletion options in Tools: Permanently delete these patrons Delete patrons directly from the database. Patron data will not be recoverable. Move these patrons to the trash Move patrons to the deleted patrons table. They can be deleted permanently by the cleanup_database script. Created attachment 103603 [details] [review] Bug 25183: cleanup-database.pl --del-exp-selfreg fully deletes borrowers - should go into deleted-borrowers The cleanup database job for deleting expired self registrations fully deletes borrowers instead of transferring them to deleted borrowers. This is a problem when libraries think they are deleting email unverified patrons instead of ones that haven't moved out of the category, an easy thing to be confused about. Test Plan: 1) Apply this patch 2) Try running cleanup_database.pl with the new -del-exp-selfreg-type parameters 'soft' and 'hard' 3) Param value 'soft' should move patrons to deletedborrowers 4) Param value 'hard' should follow existing behavior 5) Leave our -del-exp-selfreg-type, you should get the existing behavior What is the exact syntax for running it? I obviously forgot some detail because it keeps printing me the help again and again. Here are the things I tried: kohadev-koha@54d1fbaadc5c:/kohadevbox/koha$ misc/cronjobs/cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type=soft kohadev-koha@54d1fbaadc5c:/kohadevbox/koha$ misc/cronjobs/cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type soft kohadev-koha@54d1fbaadc5c:/kohadevbox/koha$ misc/cronjobs/cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type soft --confirm Created attachment 103784 [details] [review] Bug 25183: cleanup-database.pl --del-exp-selfreg fully deletes borrowers - should go into deleted-borrowers The cleanup database job for deleting expired self registrations fully deletes borrowers instead of transferring them to deleted borrowers. This is a problem when libraries think they are deleting email unverified patrons instead of ones that haven't moved out of the category, an easy thing to be confused about. Test Plan: 1) Apply this patch 2) Try running cleanup_database.pl with the new -del-exp-selfreg-type parameters 'soft' and 'hard' 3) Param value 'soft' should move patrons to deletedborrowers 4) Param value 'hard' should follow existing behavior 5) Leave our -del-exp-selfreg-type, you should get the existing behavior Please try again! (In reply to Victor Grousset/tuxayo from comment #6) > What is the exact syntax for running it? I obviously forgot some detail > because it keeps printing me the help again and again. > > Here are the things I tried: > > kohadev-koha@54d1fbaadc5c:/kohadevbox/koha$ > misc/cronjobs/cleanup_database.pl --del-exp-selfreg > --del-exp-selfreg-type=soft > > kohadev-koha@54d1fbaadc5c:/kohadevbox/koha$ > misc/cronjobs/cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type > soft > > kohadev-koha@54d1fbaadc5c:/kohadevbox/koha$ > misc/cronjobs/cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type > soft --confirm - created a self registration - I see it with - select * from borrowers order by borrowernumber DESC limit 1\G - Applied the patch - ./cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type=soft - woops, wrong syntax, got the help printed - ./cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type soft - it seems it did something! - self registration not here anymore - select * from borrowers order by borrowernumber DESC limit 1\G - it's not it deletedborrowers despite "soft" - select * from deletedborrowers order by borrowernumber DESC limit 1\G So, test failed. Hypothesis: I didn't get the param syntax right and the it's falling back to the default "del-exp-selfreg-type" value which is "hard" Created attachment 119019 [details] [review] Bug 25183: cleanup-database.pl --del-exp-selfreg fully deletes borrowers - should go into deleted-borrowers The cleanup database job for deleting expired self registrations fully deletes borrowers instead of transferring them to deleted borrowers. This is a problem when libraries think they are deleting email unverified patrons instead of ones that haven't moved out of the category, an easy thing to be confused about. Test Plan: 1) Apply this patch 2) Try running cleanup_database.pl with the new -del-exp-selfreg-type parameters 'soft' and 'hard' 3) Param value 'soft' should move patrons to deletedborrowers 4) Param value 'hard' should follow existing behavior 5) Leave our -del-exp-selfreg-type, you should get the existing behavior Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Worked as expected in testing for me.. hope the QA person can work out where Victor may have gone wrong. > Worked as expected in testing for me
Is the following the right syntax?
./cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type soft
QA comments: 1. This patch adds a parameter to subroutine DeleteExpiredOpacRegistrations. This new parameter should be tested. 2. The new constant DEFAULT_EXP_SELFREG_TYPE is not used 3. The script parameter --del-exp-selfreg-type is not correctly defined. As it is, it takes no value: % misc/cronjobs/cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type=soft Option del-exp-selfreg-type does not take an argument (In reply to Martin Renvoize from comment #11) > Worked as expected in testing for me.. hope the QA person can work out where > Victor may have gone wrong. I don't know how that could work for you. Could that be a difference in Getopt::Long versions ? I'm using 2.5 (In reply to Julian Maurice from comment #14) > I'm using 2.5 I meant 2.50 Created attachment 119593 [details] [review] Bug 25183: Add option to cleanup-database.pl to move expired self registrations to deleted borrowers table The cleanup database job for deleting expired self registrations fully deletes borrowers instead of transferring them to deleted borrowers. This is a problem when libraries think they are deleting email unverified patrons instead of ones that haven't moved out of the category, an easy thing to be confused about. Test Plan: 1) Apply this patch 2) Try running cleanup_database.pl with the new -del-exp-selfreg-type parameters 'soft' and 'hard' 3) Param value 'soft' should move patrons to deletedborrowers 4) Param value 'hard' should follow existing behavior 5) Leave our -del-exp-selfreg-type, you should get the existing behavior Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119594 [details] [review] Bug 25183: Add unit tests Comment on attachment 119593 [details] [review] Bug 25183: Add option to cleanup-database.pl to move expired self registrations to deleted borrowers table Review of attachment 119593 [details] [review]: ----------------------------------------------------------------- ::: misc/cronjobs/cleanup_database.pl @@ +27,4 @@ > use constant DEFAULT_SEARCHHISTORY_PURGEDAYS => 30; > use constant DEFAULT_SHARE_INVITATION_EXPIRY_DAYS => 14; > use constant DEFAULT_DEBARMENTS_PURGEDAYS => 30; > +use constant DEFAULT_EXP_SELFREG_TYPE => 'hard'; This is not used anywhere @@ +167,4 @@ > 'restrictions:i' => \$pDebarments, > 'all-restrictions' => \$allDebarments, > 'del-exp-selfreg' => \$pExpSelfReg, > + 'del-exp-selfreg-type:s' => \$pExpSelfRegType, ':s' means the value is optional. In this case the value should be mandatory. Use '=s' instead @@ +229,4 @@ > || $pPseudoTransactionsTo > || $pMessages > || defined $lock_days && $lock_days ne q{} > + || $pExpSelfRegType !~ m/^hard|soft$/ I guess this line is supposed to avoid giving wrong value for the parameter, but it doesn't work. I was able to give --del-exp-selfreg-type foo without the script complaining |