Bug 25531

Summary: Patron may not be debarred if backdated return
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: major    
Priority: P5 - low CC: 1joynelson, gmcharlt, kyle.m.hall, martin.renvoize, nick, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06, 19.05.12
Bug Depends on:    
Bug Blocks: 25551    
Attachments: Bug 25531: Add tests
Bug 25531: Debar patron if needed when checkin is backdated
Bug 25531: Add tests
Bug 25531: Add tests
Bug 25531: Debar patron if needed when checkin is backdated
Bug 25531: Add tests
Bug 25531: Debar patron if needed when checkin is backdated
Bug 24881: [19.11.x] Try to fix random failure from Circulation.t

Description Jonathan Druart 2020-05-18 15:48:18 UTC
This has been caught by a test failing randomly:

15:41:14 koha_1       |     #   Failed test 'AddReturn must have debarred the patron'
15:41:14 koha_1       |     #   at t/db_dependent/Circulation.t line 88.
15:41:14 koha_1       |     #          got: ''
15:41:14 koha_1       |     #     expected: '1'
15:41:14 koha_1       |     # AddReturn returned message $VAR1 = {
15:41:14 koha_1       |     #           'WasReturned' => 1
15:41:14 koha_1       |     #         };
15:41:14 koha_1       | 
15:41:14 koha_1       |     #   Failed test 'Test at line 2282'
15:41:14 koha_1       |     #   at t/db_dependent/Circulation.t line 92.
15:41:14 koha_1       |     #          got: '0'
15:41:14 koha_1       |     #     expected: '1'
15:41:14 koha_1       | 
15:41:14 koha_1       |     #   Failed test 'Test at line 2282'
15:41:14 koha_1       |     #   at t/db_dependent/Circulation.t line 95.
15:41:14 koha_1       |     #          got: undef
15:41:14 koha_1       |     #     expected: '2020-06-01'
15:41:14 koha_1       |     # Looks like you failed 3 tests of 21.
15:41:14 koha_1       | 
15:41:14 koha_1       | #   Failed test 'AddReturn + suspension_chargeperiod'
15:41:14 koha_1       | #   at t/db_dependent/Circulation.t line 2291.
15:41:28 koha_1       | # Looks like you failed 1 test of 48.
15:41:28 koha_1       | [13:41:28] t/db_dependent/Circulation.t


To know if an issue is overdue, we compared it with today. But we should compare it with the return date is provided
Comment 1 Jonathan Druart 2020-05-18 15:54:49 UTC
Created attachment 105030 [details] [review]
Bug 25531: Add tests
Comment 2 Jonathan Druart 2020-05-18 15:54:52 UTC
Created attachment 105031 [details] [review]
Bug 25531: Debar patron if needed when checkin is backdated

If think this case does not apply to real-life, but the logic needs to
be fixed.
If an item is due now, and AddReturn is called now with a return date in
the future, the issue is overdue and the patron must be debarred.
However it is not as we compare with now and not the return date
Comment 3 Victor Grousset/tuxayo 2020-05-18 21:07:02 UTC
Shouldn't the test fail when the implementation isn't applied?
My plan was
- apply 1st patch (test)
- run => fail
- apply second patch (fix)
- run => ok
Comment 4 Jonathan Druart 2020-05-19 08:44:42 UTC
Created attachment 105064 [details] [review]
Bug 25531: Add tests
Comment 5 Jonathan Druart 2020-05-19 08:45:37 UTC
(In reply to Victor Grousset/tuxayo from comment #3)
> Shouldn't the test fail when the implementation isn't applied?
> My plan was
> - apply 1st patch (test)
> - run => fail
> - apply second patch (fix)
> - run => ok

Hum yes, it should, and it did for me twice in a row yesterday and assumed it was *always*.
Now the adjusted test should fail on each run.
Comment 6 Jonathan Druart 2020-05-19 10:33:24 UTC
Original failure appeared again, Master_U18 #791
Comment 7 Victor Grousset/tuxayo 2020-05-19 13:46:44 UTC
Created attachment 105096 [details] [review]
Bug 25531: Add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 8 Victor Grousset/tuxayo 2020-05-19 13:46:47 UTC
Created attachment 105097 [details] [review]
Bug 25531: Debar patron if needed when checkin is backdated

If think this case does not apply to real-life, but the logic needs to
be fixed.
If an item is due now, and AddReturn is called now with a return date in
the future, the issue is overdue and the patron must be debarred.
However it is not as we compare with now and not the return date

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2020-05-19 13:47:50 UTC
It works :D
Comment 10 Martin Renvoize 2020-05-19 15:46:46 UTC
Created attachment 105102 [details] [review]
Bug 25531: Add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-05-19 15:46:49 UTC
Created attachment 105103 [details] [review]
Bug 25531: Debar patron if needed when checkin is backdated

If think this case does not apply to real-life, but the logic needs to
be fixed.
If an item is due now, and AddReturn is called now with a return date in
the future, the issue is overdue and the patron must be debarred.
However it is not as we compare with now and not the return date

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-05-19 15:47:30 UTC
I agree the test is a little 'unreal', but the logic is sound and the fix seems sound.

Passing QA
Comment 13 Martin Renvoize 2020-05-19 15:49:43 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-05-20 21:26:43 UTC
tests do not pass on 19.11.x  
  FAIL   valid
                Global symbol "$now" requires explicit package name (did you forget to declare "my $now"?)
                Global symbol "$now" requires explicit package name (did you forget to declare "my $now"?)
                Global symbol "$now" requires explicit package name (did you forget to declare "my $now"?)
                t/db_dependent/Circulation.t had compilation errors.


Not backported to 19.11.x
Comment 15 Jonathan Druart 2020-05-21 08:22:53 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2020-05-21 08:25:52 UTC
(In reply to Joy Nelson from comment #14)
> tests do not pass on 19.11.x  
>   FAIL   valid
>                 Global symbol "$now" requires explicit package name (did you
> forget to declare "my $now"?)
>                 Global symbol "$now" requires explicit package name (did you
> forget to declare "my $now"?)
>                 Global symbol "$now" requires explicit package name (did you
> forget to declare "my $now"?)
>                 t/db_dependent/Circulation.t had compilation errors.
> 
> 
> Not backported to 19.11.x

Hi Joy, I attached a patch for 19.11.x on bug 24881. The tests should pass after you applied it.
Comment 17 Joy Nelson 2020-05-21 20:54:50 UTC
pushed to 19.11.x for 19.11.06
Comment 18 Victor Grousset/tuxayo 2020-06-11 16:10:36 UTC
Backported to 19.05.x branch for 19.05.12