Bug 28457 - cleanup_database.pl self-registration options are not configurable
Summary: cleanup_database.pl self-registration options are not configurable
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-05-26 01:05 UTC by David Cook
Modified: 2024-01-10 18:30 UTC (History)
3 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 28457: Add configuration to cleanup_database.pl for self-registration (4.25 KB, patch)
2021-05-26 01:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 28457: Add configuration to cleanup_database.pl for self-registration (3.85 KB, patch)
2023-11-02 23:41 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2021-05-26 01:05:44 UTC
It doesn't make sense to rely on CLI options for cleanup_database.pl, especially not when it comes to self-registration options.

Instead, cleanup_database.pl should consult system preferences.
Comment 1 David Cook 2021-05-26 01:18:04 UTC
I notice that --del-unv-selfreg doesn't actually work correctly already either...
Comment 2 David Cook 2021-05-26 01:28:35 UTC
Created attachment 121401 [details] [review]
Bug 28457: Add configuration to cleanup_database.pl for self-registration

This patch fixes the --del-unv-selfreg option so that it can actually
accept a DAYS parameter. (Note that backwards compatibility is preserved
so that not specifying DAYS will still work.)

This patch also uses the following self registration system preferences
to trigger the cleanup:
PatronSelfRegistrationExpireTemporaryAccountsDelay
PatronSelfRegistrationVerifyByEmail

Note that --del-unv-selfreg will override an instance's system preference.

This means that on a package install the cleanup will just work.

Test plan:
0. Apply patch
1. perl ./misc/cronjobs/cleanup_database.pl -v
You did not specify any cleanup work for the script to do.

2. perl ./misc/cronjobs/cleanup_database.pl -v --del-exp-selfreg
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
self-registered borrowers may be deleted

3. perl ./misc/cronjobs/cleanup_database.pl -v --del-unv-selfreg
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
unverified self-registrations may be deleted after 1 days

4. perl ./misc/cronjobs/cleanup_database.pl -v --del-unv-selfreg 3
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
unverified self-registrations may be deleted after 3 days

5. Set "PatronSelfRegistrationExpireTemporaryAccountsDelay" to 1
6. Set "PatronSelfRegistrationVerifyByEmail" to "Require"

7. perl ./misc/cronjobs/cleanup_database.pl -v
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
self-registered borrowers may be deleted
unverified self-registrations may be deleted after 1 days

8. perl ./misc/cronjobs/cleanup_database.pl -v --del-unv-selfreg 2
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
self-registered borrowers may be deleted
unverified self-registrations may be deleted after 2 days
Comment 3 Jonathan Druart 2021-05-27 07:04:41 UTC
The option vs syspref question for this script has been discussed recently on bug 25429.

The last version of the patches there *should* reflect the consensus we found (I haven't had the time to review however).
Comment 4 David Cook 2021-05-27 23:35:47 UTC
(In reply to Jonathan Druart from comment #3)
> The option vs syspref question for this script has been discussed recently
> on bug 25429.
> 
> The last version of the patches there *should* reflect the consensus we
> found (I haven't had the time to review however).

Hmm that last patch doesn't make sense to me...
Comment 5 Katrin Fischer 2022-10-01 08:02:38 UTC
Please don't forget to set assignee, thx!
Comment 6 Lucas Gass 2023-11-02 21:31:31 UTC
Hey David, looks like this one needs a simple rebase.
Comment 7 David Cook 2023-11-02 23:41:02 UTC
Created attachment 158297 [details] [review]
Bug 28457: Add configuration to cleanup_database.pl for self-registration

This patch fixes the --del-unv-selfreg option so that it can actually
accept a DAYS parameter. (Note that backwards compatibility is preserved
so that not specifying DAYS will still work.)

This patch also uses the following self registration system preferences
to trigger the cleanup:
PatronSelfRegistrationExpireTemporaryAccountsDelay
PatronSelfRegistrationVerifyByEmail

Note that --del-unv-selfreg will override an instance's system preference.

This means that on a package install the cleanup will just work.

Test plan:
0. Apply patch
1. perl ./misc/cronjobs/cleanup_database.pl -v
You did not specify any cleanup work for the script to do.

2. perl ./misc/cronjobs/cleanup_database.pl -v --del-exp-selfreg
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
self-registered borrowers may be deleted

3. perl ./misc/cronjobs/cleanup_database.pl -v --del-unv-selfreg
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
unverified self-registrations may be deleted after 1 days

4. perl ./misc/cronjobs/cleanup_database.pl -v --del-unv-selfreg 3
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
unverified self-registrations may be deleted after 3 days

5. Set "PatronSelfRegistrationExpireTemporaryAccountsDelay" to 1
6. Set "PatronSelfRegistrationVerifyByEmail" to "Require"

7. perl ./misc/cronjobs/cleanup_database.pl -v
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
self-registered borrowers may be deleted
unverified self-registrations may be deleted after 1 days

8. perl ./misc/cronjobs/cleanup_database.pl -v --del-unv-selfreg 2
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
self-registered borrowers may be deleted
unverified self-registrations may be deleted after 2 days
Comment 8 David Cook 2023-11-02 23:41:19 UTC
(In reply to Lucas Gass from comment #6)
> Hey David, looks like this one needs a simple rebase.

Thanks, Lucas.

I rebased but haven't re-tested myself.
Comment 9 Matthias Le Gac 2024-01-10 18:30:07 UTC
In step 3 I don't have the message : 
Confirm flag not passed, running in dry-run mode...
Start lock unsubscribed, anonymize and delete
0 patrons would have been locked
0 patrons would have been anonymized
0 patrons would have been deleted
unverified self-registrations may be deleted after 1 days

But I have : 
You did not specify any cleanup work for the script to do.

The step 3 works only after I did the next steps