Bug 27395

Summary: Add warning to PatronSelfRegistrationDefaultCategory to avoid accidental patron deletion
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: System AdministrationAssignee: Eden <eden.bacani>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, david, eden.bacani, fridolin.somers, gmcharlt, jonathan.druart, m.de.rooy, nick, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27564
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28943
Change sponsored?: --- Patch complexity: String patch
Documentation contact: Documentation submission:
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.
Bug 27395: Add a warning to PatronSelfRegistrationDefaultCategory
Bug 27395: Add a warning to PatronSelfRegistrationDefaultCategory

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
Comment 19 Marcel de Rooy 2021-09-02 13:43:29 UTC
Noticed this warning just recently.

What is the suggested work flow here then? Should every registered patron be moved to another category? Cant this be done more intelligently?

We are using the verification step too. So why should we remove patrons from borrowers table? If they do not accept, they are just in some temporary table?
Comment 20 Andrew Fuerste-Henry 2021-09-02 13:56:06 UTC
> Should every registered patron be
> moved to another category? 

Yes, that's the intended workflow.

> Cant this be done more intelligently?

The assumption among libraries I've worked with is that staff need to manually verify validity of the account, which generally means physically checking some proof of address. In theory, one could automate the category change using the update_patrons_category cronjob, but I'm not sure more automated is more intelligently.

> We are using the verification step too. So why should we remove patrons from
> borrowers table? If they do not accept, they are just in some temporary
> table?

If you're also using the email verification step, then you're correct, those patrons aren't in the borrowers table at all until they click the link in their email.
Comment 21 Katrin Fischer 2021-09-02 14:00:48 UTC
I believe that for reasons of data privacy and such libraries often still require that something is signed in order to finish the registration process. The suggestion with update_patrons_category is a good one I think if you want to automate the process. You could also just NOT delete the patrons in the temporary category by disabling the cron. Empty pref might also work, not sure there.
Comment 22 Marcel de Rooy 2021-09-02 14:07:45 UTC
(In reply to Andrew Fuerste-Henry from comment #20)

Hi Andrew,
Thx for your fast reply.

> > Should every registered patron be
> > moved to another category? 
> 
> Yes, that's the intended workflow.

We are actually not interested to check each of these accounts (in our special library). We only check patrons that also register with us for a library card and come to our desk. Until now we had no need to change patron category at that time.

But running cleanup with 'del-exp-selfreg' would indeed be a disaster.. I still have the idea that this needs more attention. Especially when hearing that several libraries unfortunately did so already. Is putting a warning on an Admin page all we can do?
Comment 23 Marcel de Rooy 2021-09-02 14:10:52 UTC
(In reply to Katrin Fischer from comment #21)
> I believe that for reasons of data privacy and such libraries often still
> require that something is signed in order to finish the registration
> process. The suggestion with update_patrons_category is a good one I think
> if you want to automate the process. You could also just NOT delete the
> patrons in the temporary category by disabling the cron. Empty pref might
> also work, not sure there.

As long as you dont add the specific flag, you are safe. But I do no longer feel so safe as I did :)

Will open up a new report to think about some solution out of here
Comment 24 Marcel de Rooy 2021-09-02 14:20:21 UTC
So bug 28943.