| Summary: | Critical error in Koha::Calendar::days_between calculation | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Paul Poulain <paul.poulain> |
| Component: | Architecture, internals, and plumbing | Assignee: | Colin Campbell <colin.campbell> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | blocker | ||
| Priority: | P5 - low | CC: | chris, koha |
| Version: | 3.8 | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Bug Depends on: | |||
| Bug Blocks: | 7420, 8110, 8251 | ||
| Attachments: |
Proposed Patch
Supplementary Patch (merges test files) Patch which should apply cleanly on 3.8.x Bug 8486 - Follow up making the tests db independent Bug 8486 - Follow up making the tests db independent |
||
ACK I'll add some tests for the methods, the original test script appears to have gone astray. Needs to be watertight Created attachment 11111 [details] [review] Proposed Patch Submitted patch should address the reported error. Need to add similar tests for hours_between will address that in a separate patch Colin, the test is t/Kalendar.t This patch edits the t/Calendar.t which was the test for the old C4::Calendar should we get rid of the t/Kalendar.t one? Created attachment 11121 [details] [review] Supplementary Patch (merges test files) This patch removes t/Kalendar.t and merges the tests from there into t/Calendar.t This bug is very critical and break many things. That's why I'll signoff-pass QA and push in one go. I made a lot of tests, things are much better. Before the patch = no tests, my script proves that things are wrong After the patch = tests added, my script return correct results (99 and 125) Just a tiny comment: there was a trailing ` cmp_ok( $ret->ymd(), 'eq', '2012-07-24', 'Simple Single Day Add (Calendar)`' ); That i've removed: -cmp_ok( $ret->ymd(), 'eq', '2012-07-24', 'Simple Single Day Add (Calendar)' ); thanks for your patch colin ! Patches do not apply on 3.8.x Applying: Bug 8486 Correct calculation of days_between Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging Koha/Calendar.pm CONFLICT (content): Merge conflict in Koha/Calendar.pm I'm unsure how to resolve the commit safely. Please reformat for 3.8.x Created attachment 11174 [details] [review] Patch which should apply cleanly on 3.8.x This patch should apply cleanly to 3.8.x I've squashed the two commits from master into one Thanks for the very quick turnaround Colin, it works well, pushed to 3.8.x will be in 3.8.4 Created attachment 11179 [details] [review] Bug 8486 - Follow up making the tests db independent Just the last patch needing signoff, it stops the tests needing a database. Created attachment 11248 [details] [review] Bug 8486 - Follow up making the tests db independent Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Makes the test db independant by specifying the date format. Thus, there is no call to ->preference() that retrieve a systempreference Patch signed off. Will also pass QA and push in one go, trivial & valid fix Follow-up making the tests db independant pushed on master May be I missed something (I was away for a long time) but at first sight, I suspect that this patch leaves bug 8251 unsolved ("Patrons systematically debarred at checkin") : in days_between subroutine, delta_days function returns always a positive number (see Description of bug 8251). If so so it would be necessary to add the following condition : if(DateTime->compare( $start_dt, $end_dt ) > -1){ $days = $duration->inverse; } else { for (my $dt = $start_dt->clone(); $dt <= $end_dt; $dt->add(days => 1) ) { if ($self->is_holiday($dt)) { $days--; } } } Sorry ! you should have to read this :
if(DateTime->compare( $start_dt, $end_dt ) > -1){
$days = $days->inverse;
} else {
for (my $dt = $start_dt->clone();
$dt <= $end_dt;
$dt->add(days => 1)
) {
if ($self->is_holiday($dt)) {
$days--;
}
}
}
with respect to 8251 the bug is in the routine in C4::Circulation the advertised interface for days_between is to return a positive value. The calling sub should not even call it unless the issue is overdue yet it is doing so. Not sure about the status of the patches on that call but I'll try and review them when I'm back in the office and, sign off/submit a new one as required (In reply to comment #17) > with respect to 8251 the bug is in the routine in C4::Circulation the > advertised interface for days_between is to return a positive value. The > calling sub should not even call it unless the issue is overdue yet it is > doing so. Not sure about the status of the patches on that call but I'll try > and review them when I'm back in the office and, sign off/submit a new one > as required Thanks Colin |
My setup has syspref finesCalendar set "Use Calendar to skip all days the library is closed". I made the small following script: #!/usr/bin/perl use Koha::Calendar; use Koha::DateUtils; use 5.010; my $calendar = Koha::Calendar->new( branchcode => 'MAURES' ); say "Test 1=".$calendar->days_between(dt_from_string("2012-01-10") , dt_from_string("2012-05-05") )->in_units('days'); say "Test 2=".$calendar->days_between(dt_from_string("2012-05-05") , dt_from_string("2012-01-01") )->in_units('days'); The result I get is: ./test_calendar.pl DURATION : $VAR1 = bless( { 'seconds' => 0, 'minutes' => 0, 'end_of_month' => 'wrap', 'nanoseconds' => 0, 'days' => 26, 'months' => 3 }, 'DateTime::Duration' ); Test 1=26 DURATION : $VAR1 = bless( { 'seconds' => 0, 'minutes' => 0, 'end_of_month' => 'preserve', 'nanoseconds' => 0, 'days' => -4, 'months' => -4 }, 'DateTime::Duration' ); Test 2=-4 (the DURATION line comes from a warn I put in days_between Whatever the month, I always get the same result. Digging into documentation, I found this: $ perldoc DateTime::Duration · in_units( ... ) Returns the length of the duration in the units (any of those that can be passed to new) given as arguments. All lengths are integral, but may be negative. Smaller units are computed from what remains after taking away the larger units given, so for example: my $dur = DateTime::Duration->new( years => 1, months => 15 ); $dur->in_units( 'years' ); # 2 $dur->in_units( 'months' ); # 27 $dur->in_units( 'years', 'months' ); # (2, 3) $dur->in_units( 'weeks', 'days' ); # (0, 0) ! The last example demonstrates that there will not be any conversion between units which don't have a fixed conversion rate. The only conversions possible are: · years <=> months · weeks <=> days · hours <=> minutes · seconds <=> nanoseconds For the explanation of why this is the case, please see the How Datetime Math Works section of the DateTime.pm documentation CONCLUSION = the days_between() sub is doing a completely wrong calculation. I think we should to a date1-date2 to get a number of days, then remove all closed days. 2012-07-01 to 2012-07-15 is 14 (days) 2012-07-06 closed => 13 (days) 2012-07-07 closed => 12 (days) This problem is probably the real cause of bug 8251 and the trouble I get to test bug 7420 Colin, I've assigned this bug to you because you're Koha::Calendar initial author. Anyone is welcomed to confirm and provide a fix. What is really missing is a test plan. With a strong one, we would have detected the problem before