Bug 21549

Summary: Lock expired patron accounts after x days
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: PatronsAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Nick Clemens (kidclamp) <nick>
Severity: enhancement    
Priority: P5 - low CC: amit.gupta, amitddng135, black23, dcook, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, nick
Version: MainKeywords: Manual
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This report adds pref LockExpiredDelay. When you enter a value there, the cleanup_database.pl cron job will lock the accounts of patrons that expired at least the specified number of days. This follows the same pattern as existing code that already allows you to anonymize or delete locked patrons.
Version(s) released in:
21.05.00
Bug Depends on:    
Bug Blocks: 28000    
Attachments: Bug 21549: Lock expired patron accounts
Bug 21549: Database revision for new preference
Bug 21549: Lock expired patron accounts
Bug 21549: Database revision for new preference
Bug 21549: Lock expired patron accounts
Bug 21549: Database revision for new preference
Bug 21549: Lock expired patron accounts
Bug 21549: Database revision for new preference
Bug 21549: (follow-up) Switch to filter_by
Bug 21549: (follow-up) Adjust preference description
Bug 21549: Lock expired patron accounts
Bug 21549: Database revision for new preference
Bug 21549: (follow-up) Switch to filter_by
Bug 21549: (follow-up) Adjust preference description
Bug 21549: (QA follow-up) Get pref at start of run and rename variable
Bug 21549: Lock expired patron accounts
Bug 21549: Database revision for new preference
Bug 21549: (follow-up) Switch to filter_by
Bug 21549: (follow-up) Adjust preference description
Bug 21549: (QA follow-up) Get pref at start of run and rename variable
Bug 21549: Use filter_by_last_update from filter_by_dateexpiry
Bug 21549: Rename filter_by_dateexpiry with filter_by_expiration_date
Bug 21549: Make filter_by_last_update returns a Koha::Objects-based object
Bug 21549: Use filter_by_last_update from filter_by_dateexpiry
Bug 21549: Rename filter_by_dateexpiry with filter_by_expiration_date
Bug 21549: Make filter_by_last_update returns a Koha::Objects-based object

Description Marcel de Rooy 2018-10-11 12:27:03 UTC
See also bug 21336.
We could do the same for expired patron accounts. We would need an additional pref to define the number of days to wait before locking the account. After that they run in the same workflow as the unsubscribe requests in 21336 (lock, anonymize, delete).

Would it be useful?
Comment 1 Marcel de Rooy 2018-10-15 07:37:49 UTC
Essentially, we need something like:

Koha::Patrons->search_expired({ days => $days })->search({ login_attempts => { '!=' => -1 } })->lock({ remove => 1, verbose => $verbose });
Comment 2 Marcel de Rooy 2020-08-10 13:45:56 UTC
Created attachment 108000 [details] [review]
Bug 21549: Lock expired patron accounts

Adding a search on locked patrons to the search_expired in cron script.
This prevents relocking.

Test plan:
Prove Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2020-08-10 13:46:01 UTC
Created attachment 108001 [details] [review]
Bug 21549: Database revision for new preference

Introducing LockExpiredDelay.

Test plan:
Run upgrade. Check syspref.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2020-10-12 08:04:07 UTC
Created attachment 111480 [details] [review]
Bug 21549: Lock expired patron accounts

Adding a search on locked patrons to the search_expired in cron script.
This prevents relocking.

Test plan:
Prove Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2020-10-12 08:04:12 UTC
Created attachment 111481 [details] [review]
Bug 21549: Database revision for new preference

Introducing LockExpiredDelay.

Test plan:
Run upgrade. Check syspref.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2020-10-12 08:06:28 UTC
Rebased. Please test !
Comment 7 Michal Denar 2020-11-21 22:20:07 UTC
Hi,
plaease rebase on master.

Thank you.
Comment 8 Michal Denar 2020-11-21 22:27:14 UTC
Sorry, Patch wors for me now. My fault. Back to NSO.
Comment 9 Michal Denar 2020-11-21 22:29:09 UTC
Created attachment 113914 [details] [review]
Bug 21549: Lock expired patron accounts

Adding a search on locked patrons to the search_expired in cron script.
This prevents relocking.

Test plan:
Prove Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 10 Michal Denar 2020-11-21 22:29:12 UTC
Created attachment 113915 [details] [review]
Bug 21549: Database revision for new preference

Introducing LockExpiredDelay.

Test plan:
Run upgrade. Check syspref.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 11 Marcel de Rooy 2020-11-23 07:04:25 UTC
(In reply to Michal Denar from comment #10)
> Created attachment 113915 [details] [review] [review]
> Bug 21549: Database revision for new preference
> 
> Introducing LockExpiredDelay.
> 
> Test plan:
> Run upgrade. Check syspref.
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
> 
> Signed-off-by: Michal Denar <black23@gmail.com>

Thanks Michal
Comment 12 Nick Clemens (kidclamp) 2021-01-11 13:32:05 UTC
-I think the method needs to be 'filter_by' -see bug 11983

-The pref description should include reliance on the cleanup database cron:
NOTE: This system preference requires the misc/cronjobs/cleanup_database.pl cronjob. Ask your system administrator to schedule it.
Comment 13 Marcel de Rooy 2021-03-19 08:20:24 UTC
Created attachment 118493 [details] [review]
Bug 21549: Lock expired patron accounts

Adding a search on locked patrons to the search_expired in cron script.
This prevents relocking.

Test plan:
Prove Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 14 Marcel de Rooy 2021-03-19 08:20:27 UTC
Created attachment 118494 [details] [review]
Bug 21549: Database revision for new preference

Introducing LockExpiredDelay.

Test plan:
Run upgrade. Check syspref.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 15 Marcel de Rooy 2021-03-19 08:20:31 UTC
Created attachment 118495 [details] [review]
Bug 21549: (follow-up) Switch to filter_by

See QA remarks.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2021-03-19 08:20:34 UTC
Created attachment 118496 [details] [review]
Bug 21549: (follow-up) Adjust preference description

See QA remarks.

Test plan:
Check the preference in staff client.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2021-03-19 08:21:19 UTC
(In reply to Nick Clemens from comment #12)
> -I think the method needs to be 'filter_by' -see bug 11983
> 
> -The pref description should include reliance on the cleanup database cron:
> NOTE: This system preference requires the misc/cronjobs/cleanup_database.pl
> cronjob. Ask your system administrator to schedule it.

Thx for QAing, Nick. The two follow-ups should do the job.
Comment 18 Nick Clemens (kidclamp) 2021-03-19 12:04:37 UTC
Created attachment 118517 [details] [review]
Bug 21549: Lock expired patron accounts

Adding a search on locked patrons to the search_expired in cron script.
This prevents relocking.

Test plan:
Prove Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens (kidclamp) 2021-03-19 12:04:40 UTC
Created attachment 118518 [details] [review]
Bug 21549: Database revision for new preference

Introducing LockExpiredDelay.

Test plan:
Run upgrade. Check syspref.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens (kidclamp) 2021-03-19 12:04:43 UTC
Created attachment 118519 [details] [review]
Bug 21549: (follow-up) Switch to filter_by

See QA remarks.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Nick Clemens (kidclamp) 2021-03-19 12:04:46 UTC
Created attachment 118520 [details] [review]
Bug 21549: (follow-up) Adjust preference description

See QA remarks.

Test plan:
Check the preference in staff client.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens (kidclamp) 2021-03-19 12:04:49 UTC
Created attachment 118521 [details] [review]
Bug 21549: (QA follow-up) Get pref at start of run and rename variable

This patch ensures the script will lock patrons if the pref is set but no other actions
specified

It also changes the variable name to be a bit more explicit

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Jonathan Druart 2021-04-07 11:09:59 UTC
It seems that we should use filter_by_last_update (inherited from Koha::Objects) here.
Comment 24 Marcel de Rooy 2021-04-09 06:58:59 UTC
(In reply to Jonathan Druart from comment #23)
> It seems that we should use filter_by_last_update (inherited from
> Koha::Objects) here.

I do not consider this as a valid reason for failing this patch. Note that a filter on dateexpiry is not a filter on "last update".
Perhaps this routine should be made more generic first?
Comment 25 Jonathan Druart 2021-04-09 13:53:18 UTC
Well, I do consider it's a valid reason. Adding a method that is already inherited is not acceptable (duplication of code).

If you are not willing to do it I will have a look and investigate when possible.

Note that there is a difference between the 2 methods, < vs <=.
Comment 26 Jonathan Druart 2021-04-12 14:23:28 UTC
Created attachment 119457 [details] [review]
Bug 21549: Lock expired patron accounts

Adding a search on locked patrons to the search_expired in cron script.
This prevents relocking.

Test plan:
Prove Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Jonathan Druart 2021-04-12 14:23:32 UTC
Created attachment 119458 [details] [review]
Bug 21549: Database revision for new preference

Introducing LockExpiredDelay.

Test plan:
Run upgrade. Check syspref.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Jonathan Druart 2021-04-12 14:23:37 UTC
Created attachment 119459 [details] [review]
Bug 21549: (follow-up) Switch to filter_by

See QA remarks.

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Jonathan Druart 2021-04-12 14:23:42 UTC
Created attachment 119460 [details] [review]
Bug 21549: (follow-up) Adjust preference description

See QA remarks.

Test plan:
Check the preference in staff client.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Jonathan Druart 2021-04-12 14:23:47 UTC
Created attachment 119461 [details] [review]
Bug 21549: (QA follow-up) Get pref at start of run and rename variable

This patch ensures the script will lock patrons if the pref is set but no other actions
specified

It also changes the variable name to be a bit more explicit

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Jonathan Druart 2021-04-12 14:23:52 UTC
Created attachment 119462 [details] [review]
Bug 21549: Use filter_by_last_update from filter_by_dateexpiry

To not reengineer the patch this patch does not remove the new
subroutine but make it use the logic from
Koha::Objects->filter_by_last_update

It also adds a new parameter "days_inclusive"
Comment 32 Jonathan Druart 2021-04-12 14:23:57 UTC
Created attachment 119463 [details] [review]
Bug 21549: Rename filter_by_dateexpiry with filter_by_expiration_date
Comment 33 Jonathan Druart 2021-04-12 14:24:02 UTC
Created attachment 119464 [details] [review]
Bug 21549: Make filter_by_last_update returns a Koha::Objects-based object
Comment 34 Jonathan Druart 2021-04-12 14:24:43 UTC
Do you agree with those 3 patches, Marcel?
Comment 35 Marcel de Rooy 2021-04-13 11:12:42 UTC
Created attachment 119525 [details] [review]
Bug 21549: Use filter_by_last_update from filter_by_dateexpiry

To not reengineer the patch this patch does not remove the new
subroutine but make it use the logic from
Koha::Objects->filter_by_last_update

It also adds a new parameter "days_inclusive"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2021-04-13 11:12:46 UTC
Created attachment 119526 [details] [review]
Bug 21549: Rename filter_by_dateexpiry with filter_by_expiration_date

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2021-04-13 11:12:50 UTC
Created attachment 119527 [details] [review]
Bug 21549: Make filter_by_last_update returns a Koha::Objects-based object

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2021-04-13 11:13:31 UTC
(In reply to Jonathan Druart from comment #34)
> Do you agree with those 3 patches, Marcel?

Sure, thanks. Found a solution in the middle ;)
Comment 39 Jonathan Druart 2021-04-14 13:36:05 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 40 Fridolin Somers 2021-04-19 13:11:22 UTC
Enhancement not pushed to 20.11.x