Bug 26232

Summary: undefined fine grace period kills koha
Product: Koha Reporter: Didier Gautheron <didier.gautheron>
Component: CirculationAssignee: Didier Gautheron <didier.gautheron>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: major    
Priority: P5 - low CC: aleisha, fridolin.somers, gmcharlt, kyle.m.hall, lucas, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06
Attachments: Bug 26232: Empty smart rule fine grace period
Bug 26232: Test for empty smart rule fine grace period
Bug 26232: Empty smart rule fine grace period
Bug 26232: Empty smart rule fine grace period
Bug 26232: Test for empty smart rule fine grace period
Bug 26232: Empty smart rule fine grace period
Bug 26232: Test for empty smart rule fine grace period
Bug 26232: Empty smart rule fine grace period

Description Didier Gautheron 2020-08-18 05:27:47 UTC
Staff can create smart rules with empty (undefined) grace period, but then koha returns an internal error in :

DateTime::Duration::new('DateTime::Duration', 'days', undef) called at /kohadevbox/koha/C4/Circulation.pm line 2395
C4::Circulation::_calculate_new_debar_dt(HASH(0x564b11658e58)', 'HASH(0x564b11836c28)', 'DateTime=HASH(0x564b1164d688)', 'DateTime=HASH(0x564b116787d0)') called at /kohadevbox/koha/C4/Circulation.pm line 2451
Comment 1 Didier Gautheron 2020-08-18 05:59:38 UTC
Created attachment 108432 [details] [review]
Bug 26232: Empty smart rule fine grace period

Staff can create smart rules with empty (undefined) fine grace period.
In Overdues.pm undefined firstremind means 0, do the same in Circulation.pm.

To test prove t/db_dependent/Circulation.t
Comment 2 Didier Gautheron 2020-08-18 06:00:53 UTC
Created attachment 108433 [details] [review]
Bug 26232: Test for empty smart rule fine grace period
Comment 3 Didier Gautheron 2020-08-18 06:00:56 UTC
Created attachment 108434 [details] [review]
Bug 26232: Empty smart rule fine grace period

Staff can create smart rules with empty (undefined) fine grace period.
In Overdues.pm undefined firstremind means 0, do the same in Circulation.pm.

To test prove t/db_dependent/Circulation.t
Comment 4 Katrin Fischer 2020-09-29 18:07:41 UTC
Hi didier, can you please add a test plan here?
Comment 5 Didier Gautheron 2020-10-01 11:33:02 UTC
Created attachment 111034 [details] [review]
Bug 26232: Empty smart rule fine grace period

Staff can create smart rules with empty (undefined) fine grace period.
In Overdues.pm undefined firstremind means 0, do the same in Circulation.pm.

To test:
1. Apply these patches
2. Run:
   kshell
   prove t/db_dependent/Circulation.t
Comment 6 Chris Cormack 2020-10-24 21:30:50 UTC
Created attachment 112403 [details] [review]
Bug 26232: Test for empty smart rule fine grace period

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 7 Chris Cormack 2020-10-24 21:31:12 UTC
Created attachment 112404 [details] [review]
Bug 26232: Empty smart rule fine grace period

Staff can create smart rules with empty (undefined) fine grace period.
In Overdues.pm undefined firstremind means 0, do the same in Circulation.pm.

To test:
1. Apply these patches
2. Run:
   kshell
   prove t/db_dependent/Circulation.t

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 8 Nick Clemens 2020-10-25 00:05:30 UTC
Created attachment 112427 [details] [review]
Bug 26232: Test for empty smart rule fine grace period

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>


Current status: Passed QA
Comment 9 Nick Clemens 2020-10-25 00:05:33 UTC
Created attachment 112428 [details] [review]
Bug 26232: Empty smart rule fine grace period

Staff can create smart rules with empty (undefined) fine grace period.
In Overdues.pm undefined firstremind means 0, do the same in Circulation.pm.

To test:
1. Apply these patches
2. Run:
   kshell
   prove t/db_dependent/Circulation.t

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2020-10-25 00:06:07 UTC
I cannot recreate the error adding rules via the stff client, but I believe the change makes sense
Comment 11 Jonathan Druart 2020-10-25 23:06:29 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 12 Lucas Gass 2020-11-13 18:28:08 UTC
backported to 20.05.x for 20.05.06
Comment 13 Aleisha Amohia 2020-11-17 04:22:07 UTC
a bunch of failed tests on 19.11.x:

$ prove t/db_dependent/Circulation.t 
t/db_dependent/Circulation.t .. 38/47 
    #   Failed test 'Item can be issued to this patron'
    #   at t/db_dependent/Circulation.t line 3005.
    #     Structures begin differing at:
    #          $got->{PATRON_CANT} = '1'
    #     $expected->{PATRON_CANT} = Does not exist
    # Looks like you failed 1 test of 2.

#   Failed test 'CanBookBeIssued | item-level_itypes=biblio'
#   at t/db_dependent/Circulation.t line 3007.

        #   Failed test 'Item can be issued to this patron'
        #   at t/db_dependent/Circulation.t line 3040.
        #     Structures begin differing at:
        #          $got->{TOO_MANY} = 'NO_RULE_DEFINED'
        #     $expected->{TOO_MANY} = Does not exist

        #   Failed test 'No confirmation needed, AllowNotForLoanOverride=0'
        #   at t/db_dependent/Circulation.t line 3046.
        #     Structures begin differing at:
        #          $got->{current_loan_count} = undef
        #     $expected->{current_loan_count} = Does not exist

        #   Failed test 'No confirmation needed, AllowNotForLoanOverride=0'
        #   at t/db_dependent/Circulation.t line 3057.
        #     Structures begin differing at:
        #          $got->{current_loan_count} = undef
        #     $expected->{current_loan_count} = Does not exist
        # Looks like you failed 3 tests of 6.

    #   Failed test 'item-level_itypes = 1'
    #   at t/db_dependent/Circulation.t line 3063.

        #   Failed test 'Item can be issued to this patron'
        #   at t/db_dependent/Circulation.t line 3082.
        #     Structures begin differing at:
        #          $got->{TOO_MANY} = 'NO_RULE_DEFINED'
        #     $expected->{TOO_MANY} = Does not exist

        #   Failed test 'No confirmation needed, AllowNotForLoanOverride=0'
        #   at t/db_dependent/Circulation.t line 3088.
        #     Structures begin differing at:
        #          $got->{PATRON_CANT} = '1'
        #     $expected->{PATRON_CANT} = Does not exist

        #   Failed test 'No confirmation needed, AllowNotForLoanOverride=0'
        #   at t/db_dependent/Circulation.t line 3099.
        #     Structures begin differing at:
        #          $got->{PATRON_CANT} = '1'
        #     $expected->{PATRON_CANT} = Does not exist
        # Looks like you failed 3 tests of 6.

    #   Failed test 'item-level_itypes = 0'
    #   at t/db_dependent/Circulation.t line 3105.
    # Looks like you failed 2 tests of 2.

#   Failed test 'CanBookBeIssued | notforloan'
#   at t/db_dependent/Circulation.t line 3108.
t/db_dependent/Circulation.t .. 44/47 
    #   Failed test 'Item needs rentalcharge confirmation to be issued'
    #   at t/db_dependent/Circulation.t line 3488.
    #     Structures begin differing at:
    #          $got->{current_loan_count} = undef
    #     $expected->{current_loan_count} = Does not exist

    #   Failed test 'Item needs rentalcharge confirmation to be issued, increment'
    #   at t/db_dependent/Circulation.t line 3492.
    #     Structures begin differing at:
    #          $got->{TOO_MANY} = 'NO_RULE_DEFINED'
    #     $expected->{TOO_MANY} = Does not exist
    # Looks like you failed 2 tests of 2.

#   Failed test 'CanBookBeIssued & RentalFeesCheckoutConfirmation'
#   at t/db_dependent/Circulation.t line 3494.
# Looks like you failed 3 tests of 47.
t/db_dependent/Circulation.t .. Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/47 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 768 Tests: 47 Failed: 3)
  Failed tests:  39-40, 45
  Non-zero exit status: 3
Files=1, Tests=47, 20 wallclock secs ( 0.06 usr  0.01 sys + 15.52 cusr  1.96 csys = 17.55 CPU)
Result: FAIL

please fix if needed