Bug 23382 - Issuing rules failing after bug 20912
Summary: Issuing rules failing after bug 20912
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
: 23586 (view as bug list)
Depends on: 20912
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-26 12:37 UTC by Nick Clemens
Modified: 2019-11-05 16:42 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 23382: Improve test coverage (11.73 KB, patch)
2019-08-15 13:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add tests for hourly calculations (2.87 KB, patch)
2019-08-15 13:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Improve test coverage (11.73 KB, patch)
2019-08-15 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add tests for hourly calculations (2.83 KB, patch)
2019-08-15 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add hourly tests for Circulation (9.47 KB, patch)
2019-08-15 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Improve test coverage (11.73 KB, patch)
2019-08-16 15:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add tests for hourly calculations (2.83 KB, patch)
2019-08-16 15:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add hourly tests for Circulation (9.47 KB, patch)
2019-08-16 15:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued (2.99 KB, patch)
2019-08-16 15:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Improve test coverage (11.73 KB, patch)
2019-10-02 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add tests for hourly calculations (2.83 KB, patch)
2019-10-02 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add hourly tests for Circulation (9.47 KB, patch)
2019-10-02 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued (2.98 KB, patch)
2019-10-02 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Clarify variable names (4.53 KB, patch)
2019-10-02 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Ensure tests pass on Wednesdays (1.79 KB, patch)
2019-10-02 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Improve test coverage (11.73 KB, patch)
2019-10-16 12:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add tests for hourly calculations (2.83 KB, patch)
2019-10-16 12:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add hourly tests for Circulation (9.46 KB, patch)
2019-10-16 12:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued (2.98 KB, patch)
2019-10-16 12:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Clarify variable names (4.53 KB, patch)
2019-10-16 12:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Ensure tests pass on Wednesdays (1.52 KB, patch)
2019-10-16 12:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Further improve test coverage (6.01 KB, patch)
2019-10-16 12:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Truncate dates to minutes for comparison. (2.39 KB, patch)
2019-10-16 12:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: hours_between should match the logic of days_between (1.46 KB, patch)
2019-10-16 12:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Improve test coverage (11.73 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add tests for hourly calculations (2.83 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Add hourly tests for Circulation (9.46 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued (2.98 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Clarify variable names (4.53 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Ensure tests pass on Wednesdays (1.52 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Further improve test coverage (6.00 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Truncate dates to minutes for comparison. (2.39 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: hours_between should match the logic of days_between (1.46 KB, patch)
2019-10-25 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: (follow-up) Ensure closed day is inside charge window (2.24 KB, patch)
2019-10-25 10:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23382: Use floating time zone to prevent tests to fail because of DST (1.16 KB, patch)
2019-10-25 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: Improve test coverage (11.81 KB, patch)
2019-10-25 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: Add tests for hourly calculations (2.91 KB, patch)
2019-10-25 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: Add hourly tests for Circulation (9.54 KB, patch)
2019-10-25 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued (3.06 KB, patch)
2019-10-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: (follow-up) Clarify variable names (4.61 KB, patch)
2019-10-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: (follow-up) Ensure tests pass on Wednesdays (1.59 KB, patch)
2019-10-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: (follow-up) Further improve test coverage (6.09 KB, patch)
2019-10-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: Truncate dates to minutes for comparison. (2.46 KB, patch)
2019-10-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: hours_between should match the logic of days_between (1.53 KB, patch)
2019-10-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: (follow-up) Ensure closed day is inside charge window (2.31 KB, patch)
2019-10-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: Use floating time zone to prevent tests to fail because of DST (1.23 KB, patch)
2019-10-25 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23382: Improve test coverage (11.87 KB, patch)
2019-10-25 14:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: Add tests for hourly calculations (2.96 KB, patch)
2019-10-25 14:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: Add hourly tests for Circulation (9.60 KB, patch)
2019-10-25 14:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued (3.11 KB, patch)
2019-10-25 14:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: (follow-up) Clarify variable names (4.66 KB, patch)
2019-10-25 14:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: (follow-up) Ensure tests pass on Wednesdays (1.65 KB, patch)
2019-10-25 14:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: (follow-up) Further improve test coverage (6.14 KB, patch)
2019-10-25 14:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: Truncate dates to minutes for comparison. (2.52 KB, patch)
2019-10-25 14:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: hours_between should match the logic of days_between (1.59 KB, patch)
2019-10-25 14:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: (follow-up) Ensure closed day is inside charge window (2.37 KB, patch)
2019-10-25 14:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23382: Use floating time zone to prevent tests to fail because of DST (1.29 KB, patch)
2019-10-25 14:41 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 Nick Clemens 2019-07-26 12:37:40 UTC
To recreate:
1 - Define an hourly rental fee for an item type, and set it's circ rule to hourly units
2 - Check out an item of that type
3 - Nothing is charged
4 - Enable RentalFeesCheckoutConfirmation syspref
5 - Checkout another item of that type
6 - Note you are notified of the correct charge
7 - Note that confirmation does not charge
Comment 1 Nick Clemens 2019-07-26 12:39:48 UTC
I believe the issue is two calls to accumulate_rentalcharge() that do not pass current date and due date - so there is no time difference to calculate
Comment 2 Martin Renvoize 2019-08-15 13:12:52 UTC
Created attachment 92221 [details] [review]
Bug 23382: Improve test coverage
Comment 3 Martin Renvoize 2019-08-15 13:12:55 UTC
Created attachment 92222 [details] [review]
Bug 23382: Add tests for hourly calculations
Comment 4 Martin Renvoize 2019-08-15 13:57:42 UTC
Created attachment 92236 [details] [review]
Bug 23382: Improve test coverage
Comment 5 Martin Renvoize 2019-08-15 13:57:45 UTC
Created attachment 92237 [details] [review]
Bug 23382: Add tests for hourly calculations
Comment 6 Martin Renvoize 2019-08-15 13:57:47 UTC
Created attachment 92238 [details] [review]
Bug 23382: Add hourly tests for Circulation
Comment 7 Martin Renvoize 2019-08-15 13:58:30 UTC
Hmm..

Well, I've greatly increased the test coverage.. but I've not yet managed to replicate the issue :(
Comment 8 Martin Renvoize 2019-08-15 13:59:17 UTC
oooh.. RentalFeesCheckoutConfirmation plays into it.. I missed that..
Comment 9 Martin Renvoize 2019-08-16 08:52:07 UTC
OK, it's not that either... :(

I have however found some annoying flaws in our DateTime maths which are highlighted by the hourly tests.. I have no idea how to resolve them..

In short.. according to DateTime:

`DateTime->now()->delta_ms(DateTime->now()->add( hours => 4 ) = DateTime::Duration->new( hours => 3, minutes => 59, seconds => 15 );`
Comment 10 Martin Renvoize 2019-08-16 15:45:48 UTC
Created attachment 92273 [details] [review]
Bug 23382: Improve test coverage
Comment 11 Martin Renvoize 2019-08-16 15:45:51 UTC
Created attachment 92274 [details] [review]
Bug 23382: Add tests for hourly calculations
Comment 12 Martin Renvoize 2019-08-16 15:45:55 UTC
Created attachment 92275 [details] [review]
Bug 23382: Add hourly tests for Circulation
Comment 13 Martin Renvoize 2019-08-16 15:45:59 UTC
Created attachment 92276 [details] [review]
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued

It looks like over progressive rebases of bug 20912 a clause was lost
within CanBookBeIssued such that a fatal error may be triggered if an
item with no corresponding itemtype was passed into the routine.

Additionally the we were passing a Koha::Library object to CalcDateDue
rather than a branchcode which resulted in a different duedate being
used in 'CanBookBeIssued' when compared to 'AddIssue'.
Comment 14 Martin Renvoize 2019-08-16 15:49:18 UTC
Pretty sure the last patch resolves the issue reported here.. but I've not added tests to cover that fix yet so that'll have to come in a followup. (CanBookBeIssued seems to be entirely untested with regards to these charges)

The increased test coverage does still seem to highlight an issue with DateTime maths but I'm not entirely sure if it's a test suit issue or if it'll result in weird fees for end-users as yet.

Either way.. some feedback as to whether this resolves the original issue reported would be great so setting to NSO
Comment 15 Kyle M Hall 2019-09-06 10:47:54 UTC
root@2620803d2f65:koha(bug23382-qa)$ prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 43/45
    #   Failed test 'Hourly rental charge calculated correctly'
    #   at t/db_dependent/Circulation.t line 3357.
    #          got: '7.500000'
    #     expected: '10.000000'
    # Looks like you failed 1 test of 16.

#   Failed test 'Incremented fee tests'
#   at t/db_dependent/Circulation.t line 3368.

    #   Failed test 'Item needs rentalcharge confirmation to be issued'
    #   at t/db_dependent/Circulation.t line 3419.
    #     Structures begin differing at:
    #          $got->{RENTALCHARGE} = '1.00'
    #     $expected->{RENTALCHARGE} = '1'
    # Looks like you failed 1 test of 2.
t/db_dependent/Circulation.t .. 45/45
#   Failed test 'CanBookBeIssued & RentalFeesCheckoutConfirmation'
#   at t/db_dependent/Circulation.t line 3425.
# Looks like you failed 2 tests of 45.
t/db_dependent/Circulation.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/45 subtests

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 512 Tests: 45 Failed: 2)
  Failed tests:  44-45
  Non-zero exit status: 2
Files=1, Tests=45, 23 wallclock secs ( 0.03 usr  0.03 sys + 16.31 cusr  3.22 csys = 19.59 CPU)
Result: FAIL
Comment 16 Martin Renvoize 2019-10-02 07:33:39 UTC
*** Bug 23586 has been marked as a duplicate of this bug. ***
Comment 17 Martin Renvoize 2019-10-02 07:36:06 UTC
Changed title to reflect the severity and scope of bug when merged with bug 23586.

"Hourly rental fees are not charged" => "Issuing rules failing after bug 20912"
Comment 18 Martin Renvoize 2019-10-02 10:21:35 UTC
Created attachment 93407 [details] [review]
Bug 23382: Improve test coverage
Comment 19 Martin Renvoize 2019-10-02 10:21:38 UTC
Created attachment 93408 [details] [review]
Bug 23382: Add tests for hourly calculations
Comment 20 Martin Renvoize 2019-10-02 10:21:41 UTC
Created attachment 93409 [details] [review]
Bug 23382: Add hourly tests for Circulation
Comment 21 Martin Renvoize 2019-10-02 10:21:44 UTC
Created attachment 93410 [details] [review]
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued

It looks like over progressive rebases of bug 20912 a clause was lost
within CanBookBeIssued such that a fatal error may be triggered if an
item with no corresponding itemtype was passed into the routine.

Additionally the we were passing a Koha::Library object to CalcDateDue
rather than a branchcode which resulted in a different duedate being
used in 'CanBookBeIssued' when compared to 'AddIssue'.
Comment 22 Martin Renvoize 2019-10-02 10:21:48 UTC
Created attachment 93411 [details] [review]
Bug 23382: (follow-up) Clarify variable names

This patch updates the variable name '$branch' to '$branchcode' in a
number of places to make it clearer what the variable contains.
Comment 23 Martin Renvoize 2019-10-02 10:21:51 UTC
Created attachment 93412 [details] [review]
Bug 23382: (follow-up) Ensure tests pass on Wednesdays
Comment 24 Martin Renvoize 2019-10-02 10:25:16 UTC
Tests now pass.
Comment 25 Nick Clemens 2019-10-03 10:30:16 UTC
Discussed with Martin, checkout calculations are coming up short
Comment 26 Katrin Fischer 2019-10-09 16:53:36 UTC
(In reply to Nick Clemens from comment #25)
> Discussed with Martin, checkout calculations are coming up short

This sounds like a scary bug - any news here?
Comment 27 Martin Renvoize 2019-10-16 07:11:19 UTC
It is indeed scary.. I'm working on it today but I think I've just had a breakthrough.. just trying to write a bunch more tests to prove my theory
Comment 28 Martin Renvoize 2019-10-16 12:12:44 UTC
Created attachment 94281 [details] [review]
Bug 23382: Improve test coverage
Comment 29 Martin Renvoize 2019-10-16 12:12:49 UTC
Created attachment 94282 [details] [review]
Bug 23382: Add tests for hourly calculations
Comment 30 Martin Renvoize 2019-10-16 12:12:52 UTC
Created attachment 94283 [details] [review]
Bug 23382: Add hourly tests for Circulation
Comment 31 Martin Renvoize 2019-10-16 12:12:56 UTC
Created attachment 94284 [details] [review]
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued

It looks like over progressive rebases of bug 20912 a clause was lost
within CanBookBeIssued such that a fatal error may be triggered if an
item with no corresponding itemtype was passed into the routine.

Additionally the we were passing a Koha::Library object to CalcDateDue
rather than a branchcode which resulted in a different duedate being
used in 'CanBookBeIssued' when compared to 'AddIssue'.
Comment 32 Martin Renvoize 2019-10-16 12:12:59 UTC
Created attachment 94285 [details] [review]
Bug 23382: (follow-up) Clarify variable names

This patch updates the variable name '$branch' to '$branchcode' in a
number of places to make it clearer what the variable contains.
Comment 33 Martin Renvoize 2019-10-16 12:13:03 UTC
Created attachment 94286 [details] [review]
Bug 23382: (follow-up) Ensure tests pass on Wednesdays
Comment 34 Martin Renvoize 2019-10-16 12:13:06 UTC
Created attachment 94287 [details] [review]
Bug 23382: (follow-up) Further improve test coverage
Comment 35 Martin Renvoize 2019-10-16 12:13:09 UTC
Created attachment 94288 [details] [review]
Bug 23382: Truncate dates to minutes for comparison.

It's somewhat of a mess in C4::Circulation as to when dates are
truncated and when they are not and as such Koha::Charges::Fees could
not reliably assume that the dates passed in were consistent with each
other. As such, we take the approach of always truncating to the
greatest minute smaller than the passed in dates so we are comparing
like for like.
Comment 36 Martin Renvoize 2019-10-16 12:13:14 UTC
Created attachment 94289 [details] [review]
Bug 23382: hours_between should match the logic of days_between

The loops for subtraction holiday dates in hours_between and
days_between differed and as such their handling of start and end
boundaries for days also differed.  This patch makes them handle the
boundary days consistently.
Comment 37 Jonathan Druart 2019-10-21 08:37:56 UTC
(In reply to Martin Renvoize from comment #31)
> Created attachment 94284 [details] [review] [review]
> Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued
> 
> It looks like over progressive rebases of bug 20912 a clause was lost
> within CanBookBeIssued such that a fatal error may be triggered if an
> item with no corresponding itemtype was passed into the routine.

Did not we agree we should not test the assumption? We now have a script to catch such inconsistencies, moreover we are suppose to add a FK to unsure it will never happen again.

(In reply to Martin Renvoize from comment #35)
> Created attachment 94288 [details] [review] [review]
> Bug 23382: Truncate dates to minutes for comparison.
> 
> It's somewhat of a mess in C4::Circulation as to when dates are
> truncated and when they are not and as such Koha::Charges::Fees could
> not reliably assume that the dates passed in were consistent with each
> other. As such, we take the approach of always truncating to the
> greatest minute smaller than the passed in dates so we are comparing
> like for like.

How can we test that patch? Do we have test to cover it?
Comment 38 Martin Renvoize 2019-10-22 10:26:13 UTC
Comment on attachment 94284 [details] [review]
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued

Review of attachment 94284 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +707,4 @@
>      unless ( $duedate ) {
>          my $issuedate = $now->clone();
>  
> +        $duedate = CalcDateDue( $issuedate, $effective_itemtype, $circ_library->branchcode, $patron_unblessed );

Updating $branch to $circ_library->branchcode here was the change corrected the main issue in this bug report.

@@ +1006,4 @@
>          my ($rentalCharge) = GetIssuingCharges( $item_object->itemnumber, $patron->borrowernumber );
> +
> +        my $itemtype_object = Koha::ItemTypes->find( $item_object->effective_itemtype );
> +        if ($itemtype_object) {

I only added this check to be consistent with other area's where we are testing for such cases. As you suggest, I'd love to see these checks disappear, but likely as a self-contained bug that catches all cases.
Comment 39 Martin Renvoize 2019-10-22 10:28:08 UTC
As for testing the truncate dates patch.. I would suggest reading the tests added within this patchset to confirm you agree that they make sense (from a datetime math perspective) then running them before this particular patch (and note they fail) then running them again once this patch is applied (noting they now pass).

Hope that helps.
Comment 40 Martin Renvoize 2019-10-25 10:01:13 UTC
Created attachment 94723 [details] [review]
Bug 23382: Improve test coverage
Comment 41 Martin Renvoize 2019-10-25 10:01:16 UTC
Created attachment 94724 [details] [review]
Bug 23382: Add tests for hourly calculations
Comment 42 Martin Renvoize 2019-10-25 10:01:20 UTC
Created attachment 94725 [details] [review]
Bug 23382: Add hourly tests for Circulation
Comment 43 Martin Renvoize 2019-10-25 10:01:24 UTC
Created attachment 94726 [details] [review]
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued

It looks like over progressive rebases of bug 20912 a clause was lost
within CanBookBeIssued such that a fatal error may be triggered if an
item with no corresponding itemtype was passed into the routine.

Additionally the we were passing a Koha::Library object to CalcDateDue
rather than a branchcode which resulted in a different duedate being
used in 'CanBookBeIssued' when compared to 'AddIssue'.
Comment 44 Martin Renvoize 2019-10-25 10:01:27 UTC
Created attachment 94727 [details] [review]
Bug 23382: (follow-up) Clarify variable names

This patch updates the variable name '$branch' to '$branchcode' in a
number of places to make it clearer what the variable contains.
Comment 45 Martin Renvoize 2019-10-25 10:01:31 UTC
Created attachment 94728 [details] [review]
Bug 23382: (follow-up) Ensure tests pass on Wednesdays
Comment 46 Martin Renvoize 2019-10-25 10:01:34 UTC
Created attachment 94729 [details] [review]
Bug 23382: (follow-up) Further improve test coverage
Comment 47 Martin Renvoize 2019-10-25 10:01:38 UTC
Created attachment 94730 [details] [review]
Bug 23382: Truncate dates to minutes for comparison.

It's somewhat of a mess in C4::Circulation as to when dates are
truncated and when they are not and as such Koha::Charges::Fees could
not reliably assume that the dates passed in were consistent with each
other. As such, we take the approach of always truncating to the
greatest minute smaller than the passed in dates so we are comparing
like for like.
Comment 48 Martin Renvoize 2019-10-25 10:01:41 UTC
Created attachment 94731 [details] [review]
Bug 23382: hours_between should match the logic of days_between

The loops for subtraction holiday dates in hours_between and
days_between differed and as such their handling of start and end
boundaries for days also differed.  This patch makes them handle the
boundary days consistently.
Comment 49 Martin Renvoize 2019-10-25 10:31:33 UTC
Created attachment 94743 [details] [review]
Bug 23382: (follow-up) Ensure closed day is inside charge window

We're testing with an hourly loan of 96 hours to catch a closed day.
However, our closed day was hard coded to be a Wednesday so if you ran
the tests on a Friday the 96 hour loan would not catch a Wednesday.
Comment 50 Jonathan Druart 2019-10-25 13:30:56 UTC
Created attachment 94755 [details] [review]
Bug 23382: Use floating time zone to prevent tests to fail because of DST

Today Friday Oct 25th, DST next Sunday => Date math are wrong, 96 hours
diff becomes 95.
To avoid that we use the floating timezone
Comment 51 Jonathan Druart 2019-10-25 13:31:49 UTC
Created attachment 94756 [details] [review]
Bug 23382: Improve test coverage

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 52 Jonathan Druart 2019-10-25 13:31:53 UTC
Created attachment 94757 [details] [review]
Bug 23382: Add tests for hourly calculations

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 53 Jonathan Druart 2019-10-25 13:31:59 UTC
Created attachment 94758 [details] [review]
Bug 23382: Add hourly tests for Circulation

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 54 Jonathan Druart 2019-10-25 13:32:03 UTC
Created attachment 94759 [details] [review]
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued

It looks like over progressive rebases of bug 20912 a clause was lost
within CanBookBeIssued such that a fatal error may be triggered if an
item with no corresponding itemtype was passed into the routine.

Additionally the we were passing a Koha::Library object to CalcDateDue
rather than a branchcode which resulted in a different duedate being
used in 'CanBookBeIssued' when compared to 'AddIssue'.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 55 Jonathan Druart 2019-10-25 13:32:08 UTC
Created attachment 94760 [details] [review]
Bug 23382: (follow-up) Clarify variable names

This patch updates the variable name '$branch' to '$branchcode' in a
number of places to make it clearer what the variable contains.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 56 Jonathan Druart 2019-10-25 13:32:13 UTC
Created attachment 94761 [details] [review]
Bug 23382: (follow-up) Ensure tests pass on Wednesdays

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 57 Jonathan Druart 2019-10-25 13:32:18 UTC
Created attachment 94762 [details] [review]
Bug 23382: (follow-up) Further improve test coverage

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 58 Jonathan Druart 2019-10-25 13:32:26 UTC
Created attachment 94763 [details] [review]
Bug 23382: Truncate dates to minutes for comparison.

It's somewhat of a mess in C4::Circulation as to when dates are
truncated and when they are not and as such Koha::Charges::Fees could
not reliably assume that the dates passed in were consistent with each
other. As such, we take the approach of always truncating to the
greatest minute smaller than the passed in dates so we are comparing
like for like.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 59 Jonathan Druart 2019-10-25 13:32:31 UTC
Created attachment 94764 [details] [review]
Bug 23382: hours_between should match the logic of days_between

The loops for subtraction holiday dates in hours_between and
days_between differed and as such their handling of start and end
boundaries for days also differed.  This patch makes them handle the
boundary days consistently.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 60 Jonathan Druart 2019-10-25 13:32:36 UTC
Created attachment 94765 [details] [review]
Bug 23382: (follow-up) Ensure closed day is inside charge window

We're testing with an hourly loan of 96 hours to catch a closed day.
However, our closed day was hard coded to be a Wednesday so if you ran
the tests on a Friday the 96 hour loan would not catch a Wednesday.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 61 Jonathan Druart 2019-10-25 13:32:41 UTC
Created attachment 94766 [details] [review]
Bug 23382: Use floating time zone to prevent tests to fail because of DST

Today Friday Oct 25th, DST next Sunday => Date math are wrong, 96 hours
diff becomes 95.
To avoid that we use the floating timezone

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 62 Martin Renvoize 2019-10-25 14:06:48 UTC
Thanks dude :)
Comment 63 Nick Clemens 2019-10-25 14:40:36 UTC
Created attachment 94774 [details] [review]
Bug 23382: Improve test coverage

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 64 Nick Clemens 2019-10-25 14:40:40 UTC
Created attachment 94775 [details] [review]
Bug 23382: Add tests for hourly calculations

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 65 Nick Clemens 2019-10-25 14:40:43 UTC
Created attachment 94776 [details] [review]
Bug 23382: Add hourly tests for Circulation

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 66 Nick Clemens 2019-10-25 14:40:47 UTC
Created attachment 94777 [details] [review]
Bug 23382: Fix logic in C4::Circulation::CanBookBeIssued

It looks like over progressive rebases of bug 20912 a clause was lost
within CanBookBeIssued such that a fatal error may be triggered if an
item with no corresponding itemtype was passed into the routine.

Additionally the we were passing a Koha::Library object to CalcDateDue
rather than a branchcode which resulted in a different duedate being
used in 'CanBookBeIssued' when compared to 'AddIssue'.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 67 Nick Clemens 2019-10-25 14:40:51 UTC
Created attachment 94778 [details] [review]
Bug 23382: (follow-up) Clarify variable names

This patch updates the variable name '$branch' to '$branchcode' in a
number of places to make it clearer what the variable contains.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 68 Nick Clemens 2019-10-25 14:40:54 UTC
Created attachment 94779 [details] [review]
Bug 23382: (follow-up) Ensure tests pass on Wednesdays

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 69 Nick Clemens 2019-10-25 14:40:58 UTC
Created attachment 94780 [details] [review]
Bug 23382: (follow-up) Further improve test coverage

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 70 Nick Clemens 2019-10-25 14:41:01 UTC
Created attachment 94781 [details] [review]
Bug 23382: Truncate dates to minutes for comparison.

It's somewhat of a mess in C4::Circulation as to when dates are
truncated and when they are not and as such Koha::Charges::Fees could
not reliably assume that the dates passed in were consistent with each
other. As such, we take the approach of always truncating to the
greatest minute smaller than the passed in dates so we are comparing
like for like.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 71 Nick Clemens 2019-10-25 14:41:06 UTC
Created attachment 94782 [details] [review]
Bug 23382: hours_between should match the logic of days_between

The loops for subtraction holiday dates in hours_between and
days_between differed and as such their handling of start and end
boundaries for days also differed.  This patch makes them handle the
boundary days consistently.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 72 Nick Clemens 2019-10-25 14:41:10 UTC
Created attachment 94783 [details] [review]
Bug 23382: (follow-up) Ensure closed day is inside charge window

We're testing with an hourly loan of 96 hours to catch a closed day.
However, our closed day was hard coded to be a Wednesday so if you ran
the tests on a Friday the 96 hour loan would not catch a Wednesday.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 73 Nick Clemens 2019-10-25 14:41:14 UTC
Created attachment 94784 [details] [review]
Bug 23382: Use floating time zone to prevent tests to fail because of DST

Today Friday Oct 25th, DST next Sunday => Date math are wrong, 96 hours
diff becomes 95.
To avoid that we use the floating timezone

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 74 Martin Renvoize 2019-10-26 06:54:59 UTC
Nice work!

Pushed to master for 19.11.00