Bug 31937 - Prevent cleanup_database.pl from locking too many accounts
Summary: Prevent cleanup_database.pl from locking too many accounts
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 21336
Blocks:
  Show dependency treegraph
 
Reported: 2022-10-24 10:32 UTC by Marcel de Rooy
Modified: 2023-06-08 22:28 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31937: Fix Patrons::search_unsubscribed - sharpen condition (1.19 KB, patch)
2022-10-27 12:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
k (1.26 KB, patch)
2022-11-16 23:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31937: Fix Patrons::search_unsubscribed - sharpen condition (1.20 KB, patch)
2022-11-16 23:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31937: Fix Patrons::search_unsubscribed - sharpen condition (1.28 KB, patch)
2022-11-17 07:05 UTC, Jonathan Druart
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 2022-10-24 10:32:39 UTC Comment hidden (obsolete)
Comment 1 Marcel de Rooy 2022-10-24 13:16:27 UTC
Nothing better than being hit by your own development :)
We need to refine the condition of search_unsubscribed in cleanup_database.
I added a lot of refusals for another consent type (still customization), but they were picked up nicely by cleanup_database :)

So no more casualties expected after all..
Comment 2 Marcel de Rooy 2022-10-27 12:00:37 UTC
Created attachment 142712 [details] [review]
Bug 31937: Fix Patrons::search_unsubscribed - sharpen condition

The condition is fine as long as we only have GDPR consents.
But if you dont, you would have trouble. I know :)
Lets prevent that here, although the table still has an ENUM. So
double safety.

Test plan:
Run t/db_dependent/Koha/Patrons.t
Bonus: Run cleanup_database.pl and check for unusual number of
locked accounts:
     misc/cronjobs/cleanup_database.pl --confirm -v --sessions

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2022-10-27 12:05:06 UTC
Note for QA: I did explicitly leave this on bug, not enh. Since it does not hurt to backport it too.
Comment 4 Marcel de Rooy 2022-10-27 12:06:16 UTC
(In reply to Marcel de Rooy from comment #0)
> The first run of cleanupdatabase after the upgrade to 21.11 did indeed
> cleanup a lot ;)
> It seems that this statement
>     my $expired_patrons = Koha::Patrons->filter_by_expiration_date({ days =>
> $lock_days })->search({ login_attempts => { '!=' => -1 } });
> 
> works different than before. It must have selected patrons that had an empty
> dateexpiry in 20.11 ?

In order to be complete, this comment was not the cause. See comment1.
Comment 5 Katrin Fischer 2022-11-16 23:04:31 UTC
Created attachment 143979 [details] [review]
k
Comment 6 Katrin Fischer 2022-11-16 23:05:33 UTC
Created attachment 143980 [details] [review]
Bug 31937: Fix Patrons::search_unsubscribed - sharpen condition

The condition is fine as long as we only have GDPR consents.
But if you dont, you would have trouble. I know :)
Lets prevent that here, although the table still has an ENUM. So
double safety.

Test plan:
Run t/db_dependent/Koha/Patrons.t
Bonus: Run cleanup_database.pl and check for unusual number of
locked accounts:
     misc/cronjobs/cleanup_database.pl --confirm -v --sessions

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Marcel de Rooy 2022-11-17 06:32:25 UTC
(In reply to Katrin Fischer from comment #6)
> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Great. Thx
Comment 8 Jonathan Druart 2022-11-17 07:05:00 UTC
Created attachment 143982 [details] [review]
Bug 31937: Fix Patrons::search_unsubscribed - sharpen condition

The condition is fine as long as we only have GDPR consents.
But if you dont, you would have trouble. I know :)
Lets prevent that here, although the table still has an ENUM. So
double safety.

Test plan:
Run t/db_dependent/Koha/Patrons.t
Bonus: Run cleanup_database.pl and check for unusual number of
locked accounts:
     misc/cronjobs/cleanup_database.pl --confirm -v --sessions

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Tomás Cohen Arazi 2022-11-17 16:42:57 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!