Bug 27395 - Add warning to PatronSelfRegistrationDefaultCategory to avoid accidental patron deletion
Summary: Add warning to PatronSelfRegistrationDefaultCategory to avoid accidental patr...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Eden
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-11 21:25 UTC by Katrin Fischer
Modified: 2021-06-14 21:34 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: String patch
Text to go in the release notes:
This patch adds a warning to the PatronSelfRegistrationDefaultCategory system preference to not use a regular patron category for self registration. If a regular patron category code is used and the cleanup_database cronjob is setup to delete unverified and unfinished OPAC self registrations, it permanently and and unrecoverably deletes all patrons that have registered more than PatronSelfRegistrationExpireTemporaryAccountsDelay days ago. It also removes unnecessary apostrophes at the end of two self registration and modification system preference descriptions.
Version(s) released in:
21.05.00,20.11.03,20.05.09,19.11.15


Attachments
Bug 27395: Add warning to PatronSelfRegistrationDefaultCategory. (1.61 KB, patch)
2021-01-19 21:27 UTC, Eden
Details | Diff | Splinter Review
Bug 27395: Add a warning to PatronSelfRegistrationDefaultCategory (3.46 KB, patch)
2021-01-25 01:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 27395: Add a warning to PatronSelfRegistrationDefaultCategory (3.52 KB, patch)
2021-01-26 21:55 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2021-01-11 21:25:10 UTC
If you have set up the cronjobs for deleting unverified and "unfinished" opac self registrations, a wrong setting in the PatronSelfRegistrationDefaultCategory system preference can lead to accidental deletion of a lot of your patrons. This will happen if you enter one of your regular patron category codes instead of using a specific one for self registrations. It will then delete all patrons that have registere more than PatronSelfRegistrationExpireTemporaryAccountsDelay days ago. This is also a complete deletion, not moving the data to deletedborrowers, making it even harder to fix the mistake.

I think it would be great to add a hint or warning about this to the system preference description. 

It currently reads:

Use the patron category code ____ as the default patron category for patrons registered via the OPAC.
Comment 1 Eden 2021-01-19 21:27:09 UTC
Created attachment 115364 [details] [review]
Bug 27395: Add warning to PatronSelfRegistrationDefaultCategory.

Test Plan
1. Check that warning message appears under PatronSelfRegistrationDefaultCategory when searched for in  system preferences.
Comment 2 David Nind 2021-01-19 22:00:41 UTC
Hi Eden.

A slight wording change.

Current proposed wording:

WARNING: If user enters one of there regular patron category codes instead of using a specific one for self registrations, some patrons may be accidentally deleted.

Suggested wording: 

WARNING: If a regular patron category code is used instead of a specific code for self registration, some patrons may be deleted if cronjobs are setup to delete unverified and unfinished OPAC self registrations.

David
Comment 3 David Nind 2021-01-24 21:59:03 UTC
I have read this again, and have come up with some alternative wording:

WARNING: Do not use a regular patron category code for self registration. If the misc/cronjobs/cleanup_database.pl cronjob is setup to delete unverified and unfinished OPAC self registrations, it may result in the permanent and unrecoverable deletion of all patrons that have registered more than PatronSelfRegistrationExpireTemporaryAccountsDelay days ago.

As this was an academy bug, I'm happy to do a follow up patch, or amend.

It's a bit long, but the consequence of changing this system preference to a regular patron category code seems quite major.

Also, is "it may result" correct? That is, rather than "it may result" in deletion, it will result in deletion? Or are there other factors involved?
Comment 4 Katrin Fischer 2021-01-24 22:06:01 UTC
It had quite severe results here, so I am in favor of warning that is hard to ignore. It will delete if the cron is set up according to the delay configured. So unless the delay is very high, deleting patrons is a very sure thing.
Comment 5 David Nind 2021-01-25 01:56:57 UTC
Created attachment 115734 [details] [review]
Bug 27395: Add a warning to PatronSelfRegistrationDefaultCategory

This patch adds a warning to the PatronSelfRegistrationDefaultCategory system
preference to not use a regular patron category for self registration.

If a regular patron category code is used and the cleanup_database cronjob is setup
to delete unverified and unfinished OPAC self registrations, it permanently and
and unrecoverably deletes all patrons that have registered more than
PatronSelfRegistrationExpireTemporaryAccountsDelay days ago.

It also removes unnecessary apostrophes at the end of two self registration
and modification system preference descriptions.

Test plan:
1. Apply the patch.
2. Check that the warning message for the PatronSelfRegistrationDefaultCategory
   system preference is clear and makes sense.
3. Sign off!

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2021-01-25 02:00:54 UTC
Comment on attachment 115364 [details] [review]
Bug 27395: Add warning to PatronSelfRegistrationDefaultCategory.

I amended the commit message and updated the warning to:

WARNING: Do not use a regular patron category for self registration. If the misc/cronjobs/cleanup_database.pl cronjob is setup to delete unverified and unfinished OPAC self registrations, it will permanently and unrecoverably delete all patrons that have registered more than PatronSelfRegistrationExpireTemporaryAccountsDelay days ago.
Comment 7 Katrin Fischer 2021-01-26 21:55:25 UTC
Created attachment 115846 [details] [review]
Bug 27395: Add a warning to PatronSelfRegistrationDefaultCategory

This patch adds a warning to the PatronSelfRegistrationDefaultCategory system
preference to not use a regular patron category for self registration.

If a regular patron category code is used and the cleanup_database cronjob is setup
to delete unverified and unfinished OPAC self registrations, it permanently and
and unrecoverably deletes all patrons that have registered more than
PatronSelfRegistrationExpireTemporaryAccountsDelay days ago.

It also removes unnecessary apostrophes at the end of two self registration
and modification system preference descriptions.

Test plan:
1. Apply the patch.
2. Check that the warning message for the PatronSelfRegistrationDefaultCategory
   system preference is clear and makes sense.
3. Sign off!

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Jonathan Druart 2021-01-27 12:13:57 UTC
I am wondering how librarians moved the selfreg users out of the default selfreg category
My feeling is that the first part of the WARNING is ok, but the second one (about the cronjob) is confusing and add too much info

"WARNING: DON'T DO IT" is enough imo

Should not we add a warning on the patron's detail page, like "This patron has the temporary SELFREG category"?
Comment 9 Nick Clemens 2021-01-27 13:48:16 UTC
(In reply to Jonathan Druart from comment #8)
> I am wondering how librarians moved the selfreg users out of the default
> selfreg category
> My feeling is that the first part of the WARNING is ok, but the second one
> (about the cronjob) is confusing and add too much info


I think the second part of the warning is helpful and gives context on why they should not use a 'regular' category. 

> Should not we add a warning on the patron's detail page, like "This patron
> has the temporary SELFREG category"?

Indeed, a warning that  'patron will be deleted in x days (based on the pref) would be an excellent enhancement on another bug
Comment 10 Katrin Fischer 2021-01-27 22:17:54 UTC
+1 to what Nick said. I think it's nicer to explain than just forbid. In my experience that is less likely to be ignored :)

I have filed a new bug for Jonathan's suggestion:
Bug 27564 - Add a hint to patron records that still use the self registration patron category
Comment 11 Jonathan Druart 2021-01-28 16:34:49 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 12 Fridolin Somers 2021-02-05 15:21:20 UTC
Enhancement not pushed to 20.11.x
Comment 13 Katrin Fischer 2021-02-06 18:53:26 UTC
(In reply to Fridolin Somers from comment #12)
> Enhancement not pushed to 20.11.x

Hi Frido, could you please reconsider this one? We had a library hurt badly by this one and I'd like to see others not run into it.
Comment 14 Fridolin Somers 2021-02-08 08:19:56 UTC
(In reply to Katrin Fischer from comment #13)
> (In reply to Fridolin Somers from comment #12)
> > Enhancement not pushed to 20.11.x
> 
> Hi Frido, could you please reconsider this one? We had a library hurt badly
> by this one and I'd like to see others not run into it.

OK I'm on it.
Comment 15 Fridolin Somers 2021-02-08 13:40:10 UTC
Pushed to 20.11.x for 20.11.03

I had to review a bit the patch, 20.11.x does not have links preferences in .pref files.
Comment 16 Katrin Fischer 2021-02-08 19:26:36 UTC
(In reply to Fridolin Somers from comment #15)
> Pushed to 20.11.x for 20.11.03
> 
> I had to review a bit the patch, 20.11.x does not have links preferences in
> .pref files.

Thx Frido and sorry for the extra work!
Comment 17 Andrew Fuerste-Henry 2021-02-09 22:04:13 UTC
Pushed to 20.05.x for 20.05.09.
I have also lived in fear of someone inadvertently deleting a lot of patrons with this syspref :)
Comment 18 Victor Grousset/tuxayo 2021-02-11 06:23:21 UTC
Backported: Pushed to 19.11.x branch for 19.11.15