Bug 8486 - Critical error in Koha::Calendar::days_between calculation
Summary: Critical error in Koha::Calendar::days_between calculation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low blocker (vote)
Target Milestone: Oldversions
Assignee: Colin Campbell
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 7420 8110 8251
  Show dependency treegraph
 
Reported: 2012-07-21 06:30 UTC by Paul Poulain
Modified: 2018-12-16 19:32 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (7.90 KB, patch)
2012-07-24 14:24 UTC, Colin Campbell
Details | Diff | Splinter Review
Supplementary Patch (merges test files) (4.67 KB, patch)
2012-07-25 10:04 UTC, Colin Campbell
Details | Diff | Splinter Review
Patch which should apply cleanly on 3.8.x (11.93 KB, patch)
2012-07-27 16:43 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 8486 - Follow up making the tests db independent (2.63 KB, patch)
2012-07-28 08:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8486 - Follow up making the tests db independent (2.82 KB, patch)
2012-08-01 10:38 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2012-07-21 06:30:40 UTC
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
Comment 1 Colin Campbell 2012-07-21 10:54:23 UTC
ACK I'll add some tests for the methods, the original test script appears to have gone astray. Needs to be watertight
Comment 2 Colin Campbell 2012-07-24 14:24:27 UTC
Created attachment 11111 [details] [review]
Proposed Patch
Comment 3 Colin Campbell 2012-07-24 14:27:49 UTC
Submitted patch should address the reported error. Need to add similar tests for
hours_between will address that in a separate patch
Comment 4 Chris Cormack 2012-07-25 01:55:27 UTC
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?
Comment 5 Colin Campbell 2012-07-25 10:04:51 UTC
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
Comment 6 Paul Poulain 2012-07-25 14:53:55 UTC
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 !
Comment 7 Chris Cormack 2012-07-26 07:59:49 UTC
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
Comment 8 Colin Campbell 2012-07-27 16:43:01 UTC
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
Comment 9 Chris Cormack 2012-07-28 02:49:54 UTC
Thanks for the very quick turnaround Colin, it works well, pushed to 3.8.x will be in 3.8.4
Comment 10 Chris Cormack 2012-07-28 08:10:02 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2012-07-28 08:11:32 UTC
Just the last patch needing signoff, it stops the tests needing a database.
Comment 12 Paul Poulain 2012-08-01 10:38:04 UTC
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
Comment 13 Paul Poulain 2012-08-01 10:38:39 UTC
Patch signed off. Will also pass QA and push in one go, trivial & valid fix
Comment 14 Paul Poulain 2012-08-01 10:39:11 UTC
Follow-up making the tests db independant pushed on master
Comment 15 Koha Team Lyon 3 2012-09-03 10:53:03 UTC
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--;
         }
    }
}
Comment 16 Koha Team Lyon 3 2012-09-03 10:57:38 UTC
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--;
         }
    }
}
Comment 17 Colin Campbell 2012-09-03 23:01:42 UTC
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
Comment 18 Chris Cormack 2012-09-03 23:09:33 UTC
(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