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.

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

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?

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.

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 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.

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>

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"?

(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

+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

Pushed to master for 21.05, thanks to everybody involved!

Enhancement not pushed to 20.11.x

(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.

(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.

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.

(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!

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 :)

Backported: Pushed to 19.11.x branch for 19.11.15

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?

> 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.

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.

(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?

(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

So bug 28943.