[Bug 8486] New: Critical error in Koha::Calendar::days_between ?
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Priority: P5 - low Change sponsored?: --- Bug ID: 8486 Assignee: colin.campbell@ptfs-europe.com Blocks: 7420, 8251 Summary: Critical error in Koha::Calendar::days_between ? Severity: blocker Classification: Unclassified OS: All Reporter: paul.poulain@biblibre.com Hardware: All Status: ASSIGNED Version: master Component: Architecture, internals, and plumbing Product: Koha 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #1 from Colin Campbell <colin.campbell@ptfs-europe.com> --- ACK I'll add some tests for the methods, the original test script appears to have gone astray. Needs to be watertight -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #2 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 11111 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11111&action=edit Proposed Patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #3 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Submitted patch should address the reported error. Need to add similar tests for hours_between will address that in a separate patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion CC| |chris@bigballofwax.co.nz --- Comment #4 from Chris Cormack <chris@bigballofwax.co.nz> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #5 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 11121 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11121&action=edit Supplementary Patch (merges test files) This patch removes t/Kalendar.t and merges the tests from there into t/Calendar.t -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA --- Comment #6 from Paul Poulain <paul.poulain@biblibre.com> --- 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 ! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master Version|master |rel_3_8 Summary|Critical error in |Critical error in |Koha::Calendar::days_betwee |Koha::Calendar::days_betwee |n ? |n calculation -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #7 from Chris Cormack <chris@bigballofwax.co.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #8 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 11174 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11174&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #9 from Chris Cormack <chris@bigballofwax.co.nz> --- Thanks for the very quick turnaround Colin, it works well, pushed to 3.8.x will be in 3.8.4 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #10 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 11179 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11179&action=edit Bug 8486 - Follow up making the tests db independent -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #11 from Chris Cormack <chris@bigballofwax.co.nz> --- Just the last patch needing signoff, it stops the tests needing a database. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #11179|0 |1 is obsolete| | --- Comment #12 from Paul Poulain <paul.poulain@biblibre.com> --- Created attachment 11248 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11248&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #13 from Paul Poulain <paul.poulain@biblibre.com> --- Patch signed off. Will also pass QA and push in one go, trivial & valid fix -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #14 from Paul Poulain <paul.poulain@biblibre.com> --- Follow-up making the tests db independant pushed on master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |8110 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Koha Team Lyon 3 <koha@univ-lyon3.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |koha@univ-lyon3.fr --- Comment #15 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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--; } } } -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #16 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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--; } } } -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #17 from Colin Campbell <colin.campbell@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 --- Comment #18 from Chris Cormack <chris@bigballofwax.co.nz> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8486 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org