Bug 28943 - Lower the risk of accidental patron deletion by cleanup_database.pl
Summary: Lower the risk of accidental patron deletion by cleanup_database.pl
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords: Manual
Depends on:
Blocks:
 
Reported: 2021-09-02 14:15 UTC by Marcel de Rooy
Modified: 2022-04-20 07:35 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
If you use self registration but you do not use a temporary self registration patron category, you should actually clear the preference PatronSelfRegistrationExpireTemporaryAccountsDelay.
Version(s) released in:
22.05.00,21.11.03,21.05.14,20.11.18


Attachments
Bug 28943: Lower the risk of accidental patron deletion (8.34 KB, patch)
2021-09-07 13:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28943: Lower the risk of accidental patron deletion (8.34 KB, patch)
2021-09-07 13:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28943: Lower the risk of accidental patron deletion (6.45 KB, patch)
2021-10-11 11:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28943: Lower the risk of accidental patron deletion (6.46 KB, patch)
2021-12-17 11:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 28943: Lower the risk of accidental patron deletion (6.52 KB, patch)
2022-01-19 10:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28943: (QA follow-up) Fix opac.pref (2.76 KB, patch)
2022-01-22 01:59 UTC, Fridolin Somers
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-02 14:15:37 UTC
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?
Comment 1 Marcel de Rooy 2021-09-02 14:24:05 UTC
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.
Comment 2 Marcel de Rooy 2021-09-02 14:25:48 UTC
(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.
Comment 3 Katrin Fischer 2021-09-05 22:04:37 UTC
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?
Comment 4 Marcel de Rooy 2021-09-07 11:44:31 UTC
(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||;
Comment 5 Marcel de Rooy 2021-09-07 13:04:58 UTC
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>
Comment 6 Marcel de Rooy 2021-09-07 13:07:33 UTC
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>
Comment 7 Marcel de Rooy 2021-09-07 13:12:45 UTC
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 ;)
Comment 8 Marcel de Rooy 2021-09-07 13:18:32 UTC
For discussion: Do we need a preference that indicates that a library uses a temporary patron category for self registration or not?
Comment 9 Marcel de Rooy 2021-09-07 13:20:35 UTC
(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 ?
Comment 10 Andrew Fuerste-Henry 2021-09-07 14:25:42 UTC
"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.
Comment 11 Marcel de Rooy 2021-10-07 14:23:07 UTC
Reconsidering and looking at wording

Thx Andrew
Comment 12 Marcel de Rooy 2021-10-11 11:54:44 UTC
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>
Comment 13 David Nind 2021-12-17 11:13:51 UTC
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>
Comment 14 Jonathan Druart 2022-01-13 06:42:39 UTC
Isn't supporting "0" actually a feature?
Comment 15 Marcel de Rooy 2022-01-13 07:20:12 UTC
(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.
Comment 16 Martin Renvoize 2022-01-19 10:23:30 UTC
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>
Comment 17 Martin Renvoize 2022-01-19 10:24:15 UTC
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
Comment 18 Fridolin Somers 2022-01-22 01:55:35 UTC
(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.
Comment 19 Fridolin Somers 2022-01-22 01:56:55 UTC
Oh I see now it was already said in test plan.
Comment 20 Fridolin Somers 2022-01-22 01:59:39 UTC
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.
Comment 21 Fridolin Somers 2022-01-22 02:00:59 UTC
I move to component "Patrons", makes more sens
Comment 22 Fridolin Somers 2022-01-22 02:01:55 UTC
And rise to major since one can delete a lot of patrons
Comment 23 Fridolin Somers 2022-01-22 07:40:18 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 24 Kyle M Hall 2022-02-07 11:56:26 UTC
Pushed to 21.11.x for 21.11.03
Comment 25 Andrew Fuerste-Henry 2022-04-15 16:48:53 UTC
Pushed to 21.05.x for 21.05.14
Comment 26 Victor Grousset/tuxayo 2022-04-17 20:04:38 UTC
Backported: Pushed to 20.11.x branch for 20.11.18