Bug 25183 - cleanup-database.pl --del-exp-selfreg fully deletes borrowers - should go into deleted-borrowers
Summary: cleanup-database.pl --del-exp-selfreg fully deletes borrowers - should go int...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-17 16:25 UTC by Liz Rea
Modified: 2021-04-16 08:03 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25183: cleanup-database.pl --del-exp-selfreg fully deletes borrowers - should go into deleted-borrowers (5.06 KB, patch)
2020-04-23 19:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25183: cleanup-database.pl --del-exp-selfreg fully deletes borrowers - should go into deleted-borrowers (5.06 KB, patch)
2020-04-27 13:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25183: cleanup-database.pl --del-exp-selfreg fully deletes borrowers - should go into deleted-borrowers (5.23 KB, patch)
2021-03-30 15:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25183: Add option to cleanup-database.pl to move expired self registrations to deleted borrowers table (5.23 KB, patch)
2021-04-14 16:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25183: Add unit tests (2.10 KB, patch)
2021-04-14 16:05 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Liz Rea 2020-04-17 16:25:17 UTC
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.
Comment 1 Liz Rea 2020-04-17 17:59:21 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?
Comment 2 Katrin Fischer 2020-04-18 03:09:39 UTC
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.
Comment 3 Liz Rea 2020-04-20 19:30:58 UTC
That's fine by me, making it an option. :)
Comment 4 Joy Nelson 2020-04-23 17:02:00 UTC
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.
Comment 5 Kyle M Hall 2020-04-23 19:18:46 UTC
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
Comment 6 Victor Grousset/tuxayo 2020-04-26 22:47:30 UTC
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
Comment 7 Kyle M Hall 2020-04-27 13:03:52 UTC
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
Comment 8 Kyle M Hall 2020-04-27 13:04:35 UTC
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
Comment 9 Victor Grousset/tuxayo 2020-04-29 00:45:29 UTC
- 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"
Comment 10 Martin Renvoize 2021-03-30 15:10:56 UTC
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>
Comment 11 Martin Renvoize 2021-03-30 15:11:33 UTC
Worked as expected in testing for me.. hope the QA person can work out where Victor may have gone wrong.
Comment 12 Victor Grousset/tuxayo 2021-03-30 21:07:44 UTC
> Worked as expected in testing for me

Is the following the right syntax?
./cleanup_database.pl --del-exp-selfreg --del-exp-selfreg-type soft
Comment 13 Julian Maurice 2021-04-09 09:47:36 UTC
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
Comment 14 Julian Maurice 2021-04-09 09:51:13 UTC
(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
Comment 15 Julian Maurice 2021-04-09 09:53:10 UTC
(In reply to Julian Maurice from comment #14)
> I'm using 2.5

I meant 2.50
Comment 16 Kyle M Hall 2021-04-14 16:05:09 UTC
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>
Comment 17 Kyle M Hall 2021-04-14 16:05:21 UTC
Created attachment 119594 [details] [review]
Bug 25183: Add unit tests
Comment 18 Julian Maurice 2021-04-16 08:03:32 UTC
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