[Bug 24114] New: modules must not warn
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Bug ID: 24114 Summary: modules must not warn Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org Bug 21336 introduced warn statements in Koha::Patrons. That should be avoided, the warn must be done in the cronjob script. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |21336 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21336 [Bug 21336] GDPR: Handle unsubscribe requests automatically by optional (administrative) lock, anonymize and remove -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 95814 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95814&action=edit Bug 24114: Remove warn statements from Koha::Patrons The warn must be done in the cronjob -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Working here now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95814|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 95874 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95874&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 95875 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95875&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 95876 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95876&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|modules must not warn |Remove warn statements from | |Koha::Patrons -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |24153 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24153 [Bug 24153] Add a confirm flag to the cleanup_database.pl cronjob -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95874|0 |1 is obsolete| | --- Comment #14 from Bouzid Fergani <bouzid.fergani@inlibro.com> --- Created attachment 99446 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99446&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99446|0 |1 is obsolete| | --- Comment #15 from Bouzid Fergani <bouzid.fergani@inlibro.com> --- Created attachment 99447 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99447&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99447|0 |1 is obsolete| | --- Comment #16 from Bouzid Fergani <bouzid.fergani@inlibro.com> --- Created attachment 99448 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99448&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95875|0 |1 is obsolete| | --- Comment #17 from Bouzid Fergani <bouzid.fergani@inlibro.com> --- Created attachment 99449 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99449&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Bouzid Fergani <bouzid.fergani@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95876|0 |1 is obsolete| | --- Comment #18 from Bouzid Fergani <bouzid.fergani@inlibro.com> --- Created attachment 99450 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99450&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99448|0 |1 is obsolete| | --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 99499 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99499&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99449|0 |1 is obsolete| | --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 99500 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99500&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99450|0 |1 is obsolete| | --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 99501 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99501&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice improvement, no regressions found.. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com Status|Pushed to master |Pushed to stable Version(s)|20.05.00 |20.05.00, 19.11.05 released in| | --- Comment #24 from Joy Nelson <joy@bywatersolutions.com> --- PUshed to 19.11.x branch for 19.11.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to stable |Pushed to oldstable Version(s)|20.05.00, 19.11.05 |20.05.00, 19.11.05, released in| |19.05.10 --- Comment #25 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 19.05.x for 19.05.10 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24114 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |hayleymapley@catalyst.net.n | |z --- Comment #26 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Enhancement will not be backported to 18.11.x series -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org