Description
Nick Clemens (kidclamp)
2019-07-26 12:37:40 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 Created attachment 92221 [details] [review] Bug 23382: Improve test coverage Created attachment 92222 [details] [review] Bug 23382: Add tests for hourly calculations Created attachment 92236 [details] [review] Bug 23382: Improve test coverage Created attachment 92237 [details] [review] Bug 23382: Add tests for hourly calculations Created attachment 92238 [details] [review] Bug 23382: Add hourly tests for Circulation Hmm.. Well, I've greatly increased the test coverage.. but I've not yet managed to replicate the issue :( oooh.. RentalFeesCheckoutConfirmation plays into it.. I missed that.. 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 );` Created attachment 92273 [details] [review] Bug 23382: Improve test coverage Created attachment 92274 [details] [review] Bug 23382: Add tests for hourly calculations Created attachment 92275 [details] [review] Bug 23382: Add hourly tests for Circulation 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'. 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 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 *** Bug 23586 has been marked as a duplicate of this bug. *** 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" Created attachment 93407 [details] [review] Bug 23382: Improve test coverage Created attachment 93408 [details] [review] Bug 23382: Add tests for hourly calculations Created attachment 93409 [details] [review] Bug 23382: Add hourly tests for Circulation 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'. 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. Created attachment 93412 [details] [review] Bug 23382: (follow-up) Ensure tests pass on Wednesdays Tests now pass. Discussed with Martin, checkout calculations are coming up short (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? 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 Created attachment 94281 [details] [review] Bug 23382: Improve test coverage Created attachment 94282 [details] [review] Bug 23382: Add tests for hourly calculations Created attachment 94283 [details] [review] Bug 23382: Add hourly tests for Circulation 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'. 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. Created attachment 94286 [details] [review] Bug 23382: (follow-up) Ensure tests pass on Wednesdays Created attachment 94287 [details] [review] Bug 23382: (follow-up) Further improve test coverage 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. 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. (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 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. 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. Created attachment 94723 [details] [review] Bug 23382: Improve test coverage Created attachment 94724 [details] [review] Bug 23382: Add tests for hourly calculations Created attachment 94725 [details] [review] Bug 23382: Add hourly tests for Circulation 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'. 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. Created attachment 94728 [details] [review] Bug 23382: (follow-up) Ensure tests pass on Wednesdays Created attachment 94729 [details] [review] Bug 23382: (follow-up) Further improve test coverage 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. 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. 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. 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 Created attachment 94756 [details] [review] Bug 23382: Improve test coverage Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 94757 [details] [review] Bug 23382: Add tests for hourly calculations Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 94758 [details] [review] Bug 23382: Add hourly tests for Circulation Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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> 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> 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> Created attachment 94762 [details] [review] Bug 23382: (follow-up) Further improve test coverage Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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> 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> 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> 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> Thanks dude :) 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Nice work! Pushed to master for 19.11.00 backported to 19.05.x for 19.05.07 *** Bug 24437 has been marked as a duplicate of this bug. *** Created attachment 107456 [details] [review] Bug 23382: (RM follow-up) Revert "hours_between should match..." This reverts commit a693c7243c23f888e2fad38a4fff9f37ff4a9301 which caused regressions. The original loop compared start date to end date and iterated all the way to start date equals end date. The alternate implimentation inadvertantly looped from start date, skipped the first day then iterated up to one day beyond end date. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 107457 [details] [review] Bug 23382: (RM follow-up) Ensure tests pass on Wednesdays Koha::Calendar assumes we never issue an item on a closed day.. this is a fair assumption but the hardcoded closed day in our tests means it does not hold true in this case. This patch sets the closed day dynamically to prevent such a case. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> The last 2 patches were pushed to master (19.11) but not attached here and so missed for 19.05.x They fix bug 25665. |