Bug 28962 - Unverified self registrations should be removed shortly
Summary: Unverified self registrations should be removed shortly
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-07 14:23 UTC by Marcel de Rooy
Modified: 2022-02-07 13:11 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the cleanup_database script so that the option to delete unverified self registrations (del-unv-selfreg) works. It also adds this option to the standard crontab, defaulting to 14 days (the default notice says unverified registrations will expire "shortly"). Note: If you don't use self registration with a verification email, this does not affect you. If you do use self registration, check the system preferences and settings - particularly PatronSelfRegistrationDefaultCategory.
Version(s) released in:
22.05.00


Attachments
Bug 28962: Add del-unv-selfreg to crontab (4.50 KB, patch)
2021-09-07 14:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28962: Add del-unv-selfreg to crontab (4.51 KB, patch)
2021-12-18 22:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28962: Add del-unv-selfreg to crontab (4.59 KB, patch)
2022-01-13 06:30 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2021-09-07 14:23:04 UTC
The flag del-unv-selfreg could be inserted into the standard set of the cleanup database cron job.

The verification mail tells us:
If you did not initiate this request, you may safely ignore this one-time message. The request will expire shortly.

This actually is not true. But we could make it a bit more true when deleting them by default within say 14 days? Of course this could always become a syspref one day :)

Note that running this sub when you do not use self registration does not hurt. It only selects the records without a borrowernumber.
Comment 1 Marcel de Rooy 2021-09-07 14:35:26 UTC
Ha, the advertized

--del-unv-selfreg  DAYS  Delete unverified self registrations older than DAYS

does not even work !
The cmdline value is silently ignored.
Will fix that too now.
Comment 2 Marcel de Rooy 2021-09-07 14:40:08 UTC
Created attachment 124617 [details] [review]
Bug 28962: Add del-unv-selfreg to crontab

We also add a test by inserting a simulated borrower modification
record in the same table which should not be deleted.

NOTE: This patch fixes the del-unv-selfreg parameter in the
cleanup db script. It did not even do what it promised :)

Test plan:
Verify if the crontab change is correct.
Run t/db_dependent/Members.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2021-09-07 14:43:24 UTC
I have intentionally kept the BUG flag instead of ENH since this fixes a tiny but very sloppy bug in the cleanup_database parameter setting of this flag.
Comment 4 David Nind 2021-12-18 22:53:02 UTC
Created attachment 128704 [details] [review]
Bug 28962: Add del-unv-selfreg to crontab

We also add a test by inserting a simulated borrower modification
record in the same table which should not be deleted.

NOTE: This patch fixes the del-unv-selfreg parameter in the
cleanup db script. It did not even do what it promised :)

Test plan:
Verify if the crontab change is correct.
Run t/db_dependent/Members.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2021-12-18 23:30:55 UTC
The crontab entry looks fine to me. The tests pass.

No updates are required to the cron jobs and crontab sections of manual. The flag is already covered in the description of the script, although it doesn't work!
Comment 6 Marcel de Rooy 2021-12-21 12:09:30 UTC
(In reply to David Nind from comment #4)
> Signed-off-by: David Nind <david@davidnind.com>

Thx for your signoff's, David !
Comment 7 Jonathan Druart 2022-01-13 06:26:10 UTC
Not a big deal I think, but prior to this patch we were actually removing faster as the "default" was 1.
Comment 8 Jonathan Druart 2022-01-13 06:27:26 UTC
Also note that with this patch the script will now fail (displaying the help) if the flag is called without value. I don't think we should backport this.
Comment 9 Jonathan Druart 2022-01-13 06:30:59 UTC
Created attachment 129377 [details] [review]
Bug 28962: Add del-unv-selfreg to crontab

We also add a test by inserting a simulated borrower modification
record in the same table which should not be deleted.

NOTE: This patch fixes the del-unv-selfreg parameter in the
cleanup db script. It did not even do what it promised :)

Test plan:
Verify if the crontab change is correct.
Run t/db_dependent/Members.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Marcel de Rooy 2022-01-13 07:33:11 UTC
(In reply to Jonathan Druart from comment #7)
> Not a big deal I think, but prior to this patch we were actually removing
> faster as the "default" was 1.

This is arguable. First, saying that "default" is 1 leans on the true value (not a great feature). But more importantly, the crontab did not include this flag. So unless an admin added it, the records were not deleted at all.
Comment 11 Marcel de Rooy 2022-01-13 07:34:44 UTC
(In reply to Jonathan Druart from comment #8)
> Also note that with this patch the script will now fail (displaying the
> help) if the flag is called without value. I don't think we should backport
> this.

Agreed. Moving to enh. (Altough it fixes a bug..)
Comment 12 Fridolin Somers 2022-01-22 07:40:23 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄