See the discussion on bug 27395. If you do not want to change the patron category for self registrations, you should not use this flag. But can we do something to prevent accidents which seemed to have happened already?
My first idea (biased a bit by what we do ourselves..): If you have enabled Verification by mail and you would set a new pref like PreventAccidentalRemoval [needs a better name], then cleanup_database will not remove patrons enrolled in category X before certain date.
(In reply to Marcel de Rooy from comment #1) > My first idea (biased a bit by what we do ourselves..): > > If you have enabled Verification by mail and you would set a new pref like > PreventAccidentalRemoval [needs a better name], then cleanup_database will > not remove patrons enrolled in category X before certain date. This comes down to moving the cmd line flag to a preference. I have seen that transition happen much more already.
Hm, we already have PatronSelfRegistrationExpireTemporaryAccountsDelay for the number of days. It seems like a double up to introduce an additional pref to secure another one. Maybe we need to make sure that this one can also be used to deactivate?
(In reply to Katrin Fischer from comment #3) > Hm, we already have PatronSelfRegistrationExpireTemporaryAccountsDelay for > the number of days. > > It seems like a double up to introduce an additional pref to secure another > one. Maybe we need to make sure that this one can also be used to deactivate? my $delay = C4::Context->preference('PatronSelfRegistrationExpireTemporaryAccountsDelay'); return 0 if not $category_code or not defined $delay or $delay eq q||;
Created attachment 124579 [details] [review] Bug 28943: Lower the risk of accidental patron deletion If you do not use a temporary self registration patron category, you should actually make the preference PatronSelfRegistrationExpireTemporaryAccountsDelay empty. This patch adds a comment in the subroutine that does the deletion to not remove the test on empty delay. It also adds a test in Members.t for the same reason. It corrects the wrong explanation and default value in the sysprefs.sql file. Also it adds further explanation to both related sysprefs. Test plan: Run t/db_dependent/Members. Check in Administration the two adjusted OPAC pref descriptions. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124580 [details] [review] Bug 28943: Lower the risk of accidental patron deletion If you do not use a temporary self registration patron category, you should actually make the preference PatronSelfRegistrationExpireTemporaryAccountsDelay empty. This patch adds a comment in the subroutine that does the deletion to not remove the test on empty delay. It also adds a test in Members.t for the same reason. It corrects the wrong explanation and default value in the sysprefs.sql file. Also it adds further explanation to both related sysprefs. Test plan: Run t/db_dependent/Members.t Check in Administration the two adjusted OPAC pref descriptions. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
NOTE: I have intentionally set the importance to Normal since I do not only consider this as an enhancement. See the other bug. It refers to a case where accidentally patrons were deleted. Note that correcting the default of the syspref here may prevent accidents in new libraries. And the adjusted pref descriptions may help too. Could we do more? Probably, but not per se on this report. I was thinking about some limit to be used in the delete sub to not inadvertently remove a large number of patrons. If you agree, you are invited to open up a new report and add me in CC ;)
For discussion: Do we need a preference that indicates that a library uses a temporary patron category for self registration or not?
(In reply to Marcel de Rooy from comment #8) > For discussion: Do we need a preference that indicates that a library uses a > temporary patron category for self registration or not? Or should that be a flag on patron categories ?
"A zero will remove all patrons; but if you make the pref EMPTY, it will not delete any patron." This sentence needs to be reworked for specificity and to keep the style consistent with other similar sysprefs. I'd recommend: "Leave this field empty if you don't want to activate this feature. A zero in this system preference will delete all patrons in the associated category." I'd be inclined not to add new wording to PatronSelfRegistrationDefaultCategory as well, that seems a little redundant. If you maintain a new message there, it should also be updated to not use all caps for "UNLESS" and "EMPTY." CCing Katrin on this for wording and style guidance.
Reconsidering and looking at wording Thx Andrew
Created attachment 126038 [details] [review] Bug 28943: Lower the risk of accidental patron deletion If you do not use a temporary self registration patron category, you should actually make the preference PatronSelfRegistrationExpireTemporaryAccountsDelay empty. As the comment in sysprefs.sql already said, we should not let a zero value in the pref delete patrons too. The module is changed now, the test adjusted and the description of both related sysprefs modified. Test plan: Run t/db_dependent/Members.t Check in Administration the two adjusted OPAC pref descriptions. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 128652 [details] [review] Bug 28943: Lower the risk of accidental patron deletion If you do not use a temporary self registration patron category, you should actually make the preference PatronSelfRegistrationExpireTemporaryAccountsDelay empty. As the comment in sysprefs.sql already said, we should not let a zero value in the pref delete patrons too. The module is changed now, the test adjusted and the description of both related sysprefs modified. Test plan: Run t/db_dependent/Members.t Check in Administration the two adjusted OPAC pref descriptions. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Isn't supporting "0" actually a feature?
(In reply to Jonathan Druart from comment #14) > Isn't supporting "0" actually a feature? Hmm. Theoretically perhaps? I would have said No in absolute sense, were it not for the code part stripping the time by ->ymd. So yes, I you run with 0 at e.g. 23:00, you only remove things from yesterday (still a potential disaster); which might be what you want when using the temp category. If you run at 01:00, you remove everything except the last hour; which might not be what you like (too quick). But I think that the risk of deleting almost all patrons (no temp category) outweighs the gain of running 0 at 23:00 with temp category. So, in short: No we should not support 0 as a feature.
Created attachment 129593 [details] [review] Bug 28943: Lower the risk of accidental patron deletion If you do not use a temporary self registration patron category, you should actually make the preference PatronSelfRegistrationExpireTemporaryAccountsDelay empty. As the comment in sysprefs.sql already said, we should not let a zero value in the pref delete patrons too. The module is changed now, the test adjusted and the description of both related sysprefs modified. Test plan: Run t/db_dependent/Members.t Check in Administration the two adjusted OPAC pref descriptions. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I think Marcle argues the point well and I'm inclined to agree.. As such, I've worked through and tested this. Everyone works as expected and the QA scripts and tests are all happy. Passing QA
(In reply to Jonathan Druart from comment #14) > Isn't supporting "0" actually a feature? Looks like when feature was added, zero was for disable. Also since zero is default value, it makes sens to be a not dangerous value. From sysprefs.sql : ('PatronSelfRegistrationExpireTemporaryAccountsDelay','0',NULL,'If PatronSelfRegistrationDefaultCategory is enabled, this system preference controls how long a patron can have a temporary status before the account is deleted automatically. It is an integer value representing a number of days to wait before deleting a temporary patron account. Setting it to 0 disables the deleting of temporary accounts.','Integer'), Looks like the testing of the preference has been added by : https://git.koha-community.org/Koha-community/Koha/commit/fbc4e94d7f389d6bac37d9145283239d872f82f5 So I agree with this change.
Oh I see now it was already said in test plan.
Created attachment 129711 [details] [review] Bug 28943: (QA follow-up) Fix opac.pref Fix YAML syntax of opac.pref, we must use dash for new lines. Also removes duplicate text before syspref. No need to say "code" since it is now a list of categories.
I move to component "Patrons", makes more sens
And rise to major since one can delete a lot of patrons
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.03
Pushed to 21.05.x for 21.05.14
Backported: Pushed to 20.11.x branch for 20.11.18