Bug 24114

Summary: Remove warn statements from Koha::Patrons
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: 1joynelson, hayleypelham, lucas, m.de.rooy, martin.renvoize
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10
Bug Depends on: 21336    
Bug Blocks: 24153    
Attachments: Bug 24114: Remove warn statements from Koha::Patrons
Bug 24114: Remove warn statements from Koha::Patrons
Bug 24114: (follow-up) Resolve warning on non-numeric subtraction
Bug 24114: (QA follow-up) Fix counts for lock, anonymize and delete
Bug 24114: Remove warn statements from Koha::Patrons
Bug 24114: Remove warn statements from Koha::Patrons
Bug 24114: Remove warn statements from Koha::Patrons
Bug 24114: (follow-up) Resolve warning on non-numeric subtraction
Bug 24114: (QA follow-up) Fix counts for lock, anonymize and delete
Bug 24114: Remove warn statements from Koha::Patrons
Bug 24114: (follow-up) Resolve warning on non-numeric subtraction
Bug 24114: (QA follow-up) Fix counts for lock, anonymize and delete

Description Jonathan Druart 2019-11-26 10:10:12 UTC
Bug 21336 introduced warn statements in Koha::Patrons. That should be avoided, the warn must be done in the cronjob script.
Comment 1 Jonathan Druart 2019-11-26 10:12:09 UTC
Created attachment 95814 [details] [review]
Bug 24114: Remove warn statements from Koha::Patrons

The warn must be done in the cronjob
Comment 2 Jonathan Druart 2019-11-26 10:14:11 UTC
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.
Comment 3 Marcel de Rooy 2019-11-26 15:25:13 UTC
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
Comment 4 Marcel de Rooy 2019-11-26 15:26:15 UTC
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
Comment 5 Marcel de Rooy 2019-11-26 15:31:10 UTC
(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.
Comment 6 Jonathan Druart 2019-11-26 15:31:34 UTC
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.
Comment 7 Marcel de Rooy 2019-11-26 15:32:39 UTC
(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 ;)
Comment 8 Marcel de Rooy 2019-11-26 15:39:20 UTC
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 ?
Comment 9 Marcel de Rooy 2019-11-29 08:59:20 UTC
Working here now
Comment 10 Marcel de Rooy 2019-11-29 09:23:52 UTC
(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.
Comment 11 Marcel de Rooy 2019-11-29 10:15:04 UTC
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
Comment 12 Marcel de Rooy 2019-11-29 10:15:09 UTC
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>
Comment 13 Marcel de Rooy 2019-11-29 10:15:13 UTC
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>
Comment 14 Bouzid Fergani 2020-02-21 21:00:36 UTC
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>
Comment 15 Bouzid Fergani 2020-02-21 21:01:15 UTC
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>
Comment 16 Bouzid Fergani 2020-02-21 21:02:28 UTC
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>
Comment 17 Bouzid Fergani 2020-02-21 21:02:33 UTC
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>
Comment 18 Bouzid Fergani 2020-02-21 21:02:37 UTC
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>
Comment 19 Martin Renvoize 2020-02-24 12:22:09 UTC
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>
Comment 20 Martin Renvoize 2020-02-24 12:22:12 UTC
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>
Comment 21 Martin Renvoize 2020-02-24 12:22:15 UTC
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>
Comment 22 Martin Renvoize 2020-02-24 12:22:39 UTC
Nice improvement, no regressions found..

Passing QA
Comment 23 Martin Renvoize 2020-02-24 13:21:54 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 24 Joy Nelson 2020-03-30 22:02:15 UTC
PUshed to 19.11.x branch for 19.11.05
Comment 25 Lucas Gass 2020-04-08 15:04:48 UTC
backported to 19.05.x for 19.05.10
Comment 26 Hayley Pelham 2020-04-14 02:52:27 UTC
Enhancement will not be backported to 18.11.x series