Bug 26232 - undefined fine grace period kills koha
Summary: undefined fine grace period kills koha
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Didier Gautheron
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-18 05:27 UTC by Didier Gautheron
Modified: 2020-11-17 04:22 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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 (1009 bytes, patch)
2020-08-18 05:59 UTC, Didier Gautheron
Details | Diff | Splinter Review
Bug 26232: Test for empty smart rule fine grace period (2.25 KB, patch)
2020-08-18 06:00 UTC, Didier Gautheron
Details | Diff | Splinter Review
Bug 26232: Empty smart rule fine grace period (1009 bytes, patch)
2020-08-18 06:00 UTC, Didier Gautheron
Details | Diff | Splinter Review
Bug 26232: Empty smart rule fine grace period (1.03 KB, patch)
2020-10-01 11:33 UTC, Didier Gautheron
Details | Diff | Splinter Review
Bug 26232: Test for empty smart rule fine grace period (2.30 KB, patch)
2020-10-24 21:30 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 26232: Empty smart rule fine grace period (1.08 KB, patch)
2020-10-24 21:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 26232: Test for empty smart rule fine grace period (2.35 KB, patch)
2020-10-25 00:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26232: Empty smart rule fine grace period (1.14 KB, patch)
2020-10-25 00:05 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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