Bug 21336 introduced warn statements in Koha::Patrons. That should be avoided, the warn must be done in the cronjob script.
Created attachment 95814 [details] [review] Bug 24114: Remove warn statements from Koha::Patrons The warn must be done in the cronjob
I do not understand how to test properly this patch, why do not we have a separated flag for this task? The following commit 2ca2585b1c5b61024351c32d84e605f61d04a418 Bug 21336: Adjust cleanup_database.pl says "Run cleanup_database.pl --logs (or any other flag) for the first time." But it sounds wrong to me, I am expecting the script to do what I am asking it to do.
git grep -l -E "warn [\'\"]" Account.pm ArticleRequest.pm AuthUtils.pm Authorities.pm BiblioUtils.pm Cache.pm Club/Hold.pm I18N.pm Item/Search/Field.pm Logger.pm Middleware/SetEnv.pm Patron.pm Plugins/Base.pm Plugins/Handler.pm QueryParser/Driver/PQF.pm REST/Plugin/PluginRoutes.pm Schema/Result/Item.pm SearchEngine/Elasticsearch/QueryBuilder.pm SimpleMARC.pm Template/Plugin/Asset.pm UploadedFile.pm Util/MARC.pm XSLT_Handler.pm
C4 git grep -l -E "warn [\'\"]" Acquisition.pm Auth.pm Auth_with_cas.pm Auth_with_ldap.pm Auth_with_shibboleth.pm AuthoritiesMarc.pm Barcodes.pm Barcodes/EAN13.pm Barcodes/ValueBuilder.pm Barcodes/annual.pm Barcodes/hbyymmincr.pm Biblio.pm Bookseller.pm Charset.pm Circulation.pm ClassSortRoutine.pm ClassSplitRoutine/LCC.pm Context.pm CourseReserves.pm Creators/PDF.pm Creators/Profile.pm Debug.pm External/BakerTaylor.pm External/OverDrive.pm External/Syndetics.pm HoldsQueue.pm Images.pm ImportExportFramework.pm InstallAuth.pm Installer.pm Items.pm Koha.pm Labels/Label.pm Languages.pm Letters.pm Matcher.pm Members/Attributes.pm Members/Messaging.pm Message.pm OAI/Sets.pm Output.pm Overdues.pm Record.pm Reserves.pm Ris.pm SIP/ILS.pm SIP/ILS/Item.pm SIP/ILS/Patron.pm SIP/ILS/Transaction/Checkin.pm SIP/ILS/Transaction/Checkout.pm SIP/Sip/Checksum.pm SIP/Sip/MsgType.pm SMS.pm Search.pm Search/History.pm Serials.pm Service.pm SocialData.pm Suggestions.pm Tags.pm Templates.pm Utils/DataTables/ColumnsSettings.pm XISBN.pm XSLT.pm
(In reply to Jonathan Druart from comment #2) > I do not understand how to test properly this patch, why do not we have a > separated flag for this task? > > The following commit 2ca2585b1c5b61024351c32d84e605f61d04a418 > Bug 21336: Adjust cleanup_database.pl > says > "Run cleanup_database.pl --logs (or any other flag) for the first time." > > But it sounds wrong to me, I am expecting the script to do what I am asking > it to do. Underlying thought is we do not want to override the prefs here. If you run cleanup, it will cleanup what the prefs control. Do we really need 30 options or something like that in cleanup? For each simple task it does. I guess this is again not the only place where this is done.
Marcel, to me they are completely different. Those are warnings displayed in the Koha logs because we encountered something wrong (or debug is on). The ones I am removing here are information displayed in the cronjob. The proof they are wrong is that the cronjob is printing (STDOUT) all other messages, but warn (STDERR) these ones, whereas they are not errors/warnings but verbose messages.
(In reply to Jonathan Druart from comment #6) > Marcel, to me they are completely different. > Those are warnings displayed in the Koha logs because we encountered > something wrong (or debug is on). > > The ones I am removing here are information displayed in the cronjob. The > proof they are wrong is that the cronjob is printing (STDOUT) all other > messages, but warn (STDERR) these ones, whereas they are not errors/warnings > but verbose messages. You have a point. I cannot see what all these warns do. But probably there are bad ones ;)
I can test later this week. I doubt if all ->count calls are working correctly after the changes but will check. Adjust title and status ?
Working here now
(In reply to Marcel de Rooy from comment #8) > I doubt if all ->count calls are working > correctly after the changes but will check. Yes, the counts are wrong. Will add a followup.
Created attachment 95874 [details] [review] Bug 24114: Remove warn statements from Koha::Patrons The warn must be done in the cronjob. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Followed this test plan (with two follow-ups applied): [1] Prefs: UnsubscribeReflectionDelay=1, PatronAnonymizeDelay=2, PatronRemovalDelay=3, FailedLoginAttempts was undef [2] Pick borrower and set expiry to NOW-2, and lock him (login_attempts=-1) Could be achieved too by settings FailedLoginAttempts and trying wrong passwords. Run cleanup job: Locked 0 patrons Anonymized 1 patrons Deleted 0 patrons [3] Pick borrower, set expiry to NOW-3. Run cleanup job: Locked 0 patrons Anonymized 0 patrons Deleted 1 patrons
Created attachment 95875 [details] [review] Bug 24114: (follow-up) Resolve warning on non-numeric subtraction Argument "" isn't numeric in subtraction (-) at /usr/share/koha/Koha/Patrons.pm line 290. Coming from an empty or undefined FailedLoginAttempts. Test plan: Verify that Koha/Patrons.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 95876 [details] [review] Bug 24114: (QA follow-up) Fix counts for lock, anonymize and delete Since these operations impact on the resultset, the counts should be saved before. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 99446 [details] [review] Bug 24114: Remove warn statements from Koha::Patrons The warn must be done in the cronjob. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Followed this test plan (with two follow-ups applied): [1] Prefs: UnsubscribeReflectionDelay=1, PatronAnonymizeDelay=2, PatronRemovalDelay=3, FailedLoginAttempts was undef [2] Pick borrower and set expiry to NOW-2, and lock him (login_attempts=-1) Could be achieved too by settings FailedLoginAttempts and trying wrong passwords. Run cleanup job: Locked 0 patrons Anonymized 1 patrons Deleted 0 patrons [3] Pick borrower, set expiry to NOW-3. Run cleanup job: Locked 0 patrons Anonymized 0 patrons Deleted 1 patrons Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Created attachment 99447 [details] [review] Bug 24114: Remove warn statements from Koha::Patrons The warn must be done in the cronjob. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Followed this test plan (with two follow-ups applied): [1] Prefs: UnsubscribeReflectionDelay=1, PatronAnonymizeDelay=2, PatronRemovalDelay=3, FailedLoginAttempts was undef [2] Pick borrower and set expiry to NOW-2, and lock him (login_attempts=-1) Could be achieved too by settings FailedLoginAttempts and trying wrong passwords. Run cleanup job: Locked 0 patrons Anonymized 1 patrons Deleted 0 patrons [3] Pick borrower, set expiry to NOW-3. Run cleanup job: Locked 0 patrons Anonymized 0 patrons Deleted 1 patrons Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Created attachment 99448 [details] [review] Bug 24114: Remove warn statements from Koha::Patrons The warn must be done in the cronjob. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Followed this test plan (with two follow-ups applied): [1] Prefs: UnsubscribeReflectionDelay=1, PatronAnonymizeDelay=2, PatronRemovalDelay=3, FailedLoginAttempts was undef [2] Pick borrower and set expiry to NOW-2, and lock him (login_attempts=-1) Could be achieved too by settings FailedLoginAttempts and trying wrong passwords. Run cleanup job: Locked 0 patrons Anonymized 1 patrons Deleted 0 patrons [3] Pick borrower, set expiry to NOW-3. Run cleanup job: Locked 0 patrons Anonymized 0 patrons Deleted 1 patrons Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Created attachment 99449 [details] [review] Bug 24114: (follow-up) Resolve warning on non-numeric subtraction Argument "" isn't numeric in subtraction (-) at /usr/share/koha/Koha/Patrons.pm line 290. Coming from an empty or undefined FailedLoginAttempts. Test plan: Verify that Koha/Patrons.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Created attachment 99450 [details] [review] Bug 24114: (QA follow-up) Fix counts for lock, anonymize and delete Since these operations impact on the resultset, the counts should be saved before. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Created attachment 99499 [details] [review] Bug 24114: Remove warn statements from Koha::Patrons The warn must be done in the cronjob. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Followed this test plan (with two follow-ups applied): [1] Prefs: UnsubscribeReflectionDelay=1, PatronAnonymizeDelay=2, PatronRemovalDelay=3, FailedLoginAttempts was undef [2] Pick borrower and set expiry to NOW-2, and lock him (login_attempts=-1) Could be achieved too by settings FailedLoginAttempts and trying wrong passwords. Run cleanup job: Locked 0 patrons Anonymized 1 patrons Deleted 0 patrons [3] Pick borrower, set expiry to NOW-3. Run cleanup job: Locked 0 patrons Anonymized 0 patrons Deleted 1 patrons Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99500 [details] [review] Bug 24114: (follow-up) Resolve warning on non-numeric subtraction Argument "" isn't numeric in subtraction (-) at /usr/share/koha/Koha/Patrons.pm line 290. Coming from an empty or undefined FailedLoginAttempts. Test plan: Verify that Koha/Patrons.t still passes. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99501 [details] [review] Bug 24114: (QA follow-up) Fix counts for lock, anonymize and delete Since these operations impact on the resultset, the counts should be saved before. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice improvement, no regressions found.. Passing QA
Nice work everyone! Pushed to master for 20.05
PUshed to 19.11.x branch for 19.11.05
backported to 19.05.x for 19.05.10
Enhancement will not be backported to 18.11.x series