Description
Marcel de Rooy
2018-10-11 12:27:03 UTC
Essentially, we need something like: Koha::Patrons->search_expired({ days => $days })->search({ login_attempts => { '!=' => -1 } })->lock({ remove => 1, verbose => $verbose }); 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> 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> 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> 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> Rebased. Please test ! Hi, plaease rebase on master. Thank you. Sorry, Patch wors for me now. My fault. Back to NSO. 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> 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> (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 -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. 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> 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> 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> 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> (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. 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> 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> 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> 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> 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> It seems that we should use filter_by_last_update (inherited from Koha::Objects) here. (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? 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 <=. 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> 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> 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> 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> 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> 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" Created attachment 119463 [details] [review] Bug 21549: Rename filter_by_dateexpiry with filter_by_expiration_date Created attachment 119464 [details] [review] Bug 21549: Make filter_by_last_update returns a Koha::Objects-based object Do you agree with those 3 patches, Marcel? 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> 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> 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> (In reply to Jonathan Druart from comment #34) > Do you agree with those 3 patches, Marcel? Sure, thanks. Found a solution in the middle ;) Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x |