Bugzilla – Attachment 11174 Details for
Bug 8486
Critical error in Koha::Calendar::days_between calculation
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch which should apply cleanly on 3.8.x
0001-Bug-8486-Correct-calculation-of-days_between.patch (text/plain), 11.93 KB, created by
Colin Campbell
on 2012-07-27 16:43:01 UTC
(
hide
)
Description:
Patch which should apply cleanly on 3.8.x
Filename:
MIME Type:
Creator:
Colin Campbell
Created:
2012-07-27 16:43:01 UTC
Size:
11.93 KB
patch
obsolete
>From d2af63343e06b36871f144fe4a8e5769aaca04a6 Mon Sep 17 00:00:00 2001 >From: Colin Campbell <colin.campbell@ptfs-europe.com> >Date: Fri, 27 Jul 2012 17:31:30 +0100 >Subject: [PATCH] Bug 8486 Correct calculation of days_between` > >This patch backports the following patches >from master to 3.8.x: > >commit d786d7c7b6c2894cca6231cae3deee9c9d43120b >Author: Colin Campbell <colin.campbell@ptfs-europe.com> >Date: Tue Jul 24 15:16:10 2012 +0100 > > Bug 8486 Correct calculation of days_between > > There were errors in the calculation used for days_between > which caused incorrect values to be returned > Added tests to validate calculation > > NB Tests still need to be provided for the hourly > calculations > >commit 8379a9781dbf16bcb53b55b20f15dcd55ff948Oc2 >Author: Colin Campbell <colin.campbell@ptfs-europe.com> >Date: Wed Jul 25 10:55:56 2012 +0100 > > Bug 8486 Merge Calendar test files > > Moves the tests previously in t/Kalendar.t to t/Calendar.t > and removes the old file. > Note the existing minimal test of C4/Calendar still called in > t/Calendar >--- > Koha/Calendar.pm | 87 +++++++++++++++++++++++++++++-------- > t/Calendar.t | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- > t/Kalendar.t | 62 -------------------------- > 3 files changed, 192 insertions(+), 86 deletions(-) > delete mode 100755 t/Kalendar.t > >diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm >index 03146fa..0a66c52 100644 >--- a/Koha/Calendar.pm >+++ b/Koha/Calendar.pm >@@ -79,6 +79,7 @@ sub _init { > } > $self->{single_holidays} = DateTime::Set->from_datetimes( dates => $dates ); > $self->{days_mode} = C4::Context->preference('useDaysMode'); >+ $self->{test} = 0; > return; > } > >@@ -100,7 +101,6 @@ sub addDate { > my $dt = $base_date + $add_duration; > while ( $self->is_holiday($dt) ) { > >- # TODOP if hours set to 10 am > $dt->add_duration($day_dur); > if ( $unit eq 'hours' ) { > $dt->set_hour($return_by_hour); # Staffs specific >@@ -169,25 +169,25 @@ sub days_between { > my $start_dt = shift; > my $end_dt = shift; > >- my $datestart_temp = $start_dt->clone(); >- my $dateend_temp = $end_dt->clone(); > > # start and end should not be closed days >- my $duration = $dateend_temp->delta_days($datestart_temp); >- $datestart_temp->truncate( to => 'days' ); >- $dateend_temp->truncate( to => 'days' ); >- while ( DateTime->compare( $datestart_temp, $dateend_temp ) == -1 ) { >- $datestart_temp->add( days => 1 ); >- if ( $self->is_holiday($datestart_temp) ) { >- $duration->subtract( days => 1 ); >+ my $days = $start_dt->delta_days($end_dt)->delta_days; >+ for (my $dt = $start_dt->clone(); >+ $dt <= $end_dt; >+ $dt->add(days => 1) >+ ) { >+ if ($self->is_holiday($dt)) { >+ $days--; > } > } >- return $duration; >+ return DateTime::Duration->new( days => $days ); > > } > > sub hours_between { >- my ($self, $start_dt, $end_dt) = @_; >+ my ($self, $start_date, $end_date) = @_; >+ my $start_dt = $start_date->clone(); >+ my $end_dt = $end_date->clone(); > my $duration = $end_dt->delta_ms($start_dt); > $start_dt->truncate( to => 'days' ); > $end_dt->truncate( to => 'days' ); >@@ -195,12 +195,19 @@ sub hours_between { > # However for hourly loans the logic should be expanded to > # take into account open/close times then it would be a duration > # of library open hours >- while ( DateTime->compare( $start_dt, $end_dt ) == -1 ) { >- $start_dt->add( days => 1 ); >- if ( $self->is_holiday($start_dt) ) { >- $duration->subtract( hours => 24 ); >+ my $skipped_days = 0; >+ for (my $dt = $start_dt->clone(); >+ $dt <= $end_dt; >+ $dt->add(days => 1) >+ ) { >+ if ($self->is_holiday($dt)) { >+ ++$skipped_days; > } > } >+ if ($skipped_days) { >+ $duration->subtract_duration(DateTime::Duration->new( hours => 24 * $skipped_days)); >+ } >+ > return $duration; > > } >@@ -221,6 +228,35 @@ sub _mockinit { > push @{$dates}, $special; > $self->{single_holidays} = DateTime::Set->from_datetimes( dates => $dates ); > $self->{days_mode} = 'Calendar'; >+ $self->{test} = 1; >+ return; >+} >+ >+sub set_daysmode { >+ my ( $self, $mode ) = @_; >+ >+ # if not testing this is a no op >+ if ( $self->{test} ) { >+ $self->{days_mode} = $mode; >+ } >+ >+ return; >+} >+ >+sub clear_weekly_closed_days { >+ my $self = shift; >+ $self->{weekly_closed_days} = [ 0, 0, 0, 0, 0, 0, 0 ]; # Sunday only >+ return; >+} >+ >+sub add_holiday { >+ my $self = shift; >+ my $new_dt = shift; >+ my @dt = $self->{exception_holidays}->as_list; >+ push @dt, $new_dt; >+ $self->{exception_holidays} = >+ DateTime::Set->from_datetimes( dates => \@dt ); >+ > return; > } > >@@ -287,7 +323,24 @@ passed at DateTime object returns 1 if it is a closed day > $duration = $calendar->days_between($start_dt, $end_dt); > > Passed two dates returns a DateTime::Duration object measuring the length between them >-ignoring closed days >+ignoring closed days. Always returns a positive number irrespective of the >+relative order of the parameters >+ >+=head2 set_daysmode >+ >+For testing only allows the calling script to change days mode >+ >+=head2 clear_weekly_closed_days >+ >+In test mode changes the testing set of closed days to a new set with >+no closed days. TODO passing an array of closed days to this would >+allow testing of more configurations >+ >+=head2 add_holiday >+ >+Passed a datetime object this will add it to the calendar's list of >+closed days. This is for testing so that we can alter the Calenfar object's >+list of specified dates > > =head1 DIAGNOSTICS > >diff --git a/t/Calendar.t b/t/Calendar.t >index d2690f7..859d60b 100755 >--- a/t/Calendar.t >+++ b/t/Calendar.t >@@ -1,14 +1,129 @@ >-#!/usr/bin/perl >-# >-# This Koha test module is a stub! >-# Add more tests here!!! >+#!/usr/bin/env perl > > use strict; > use warnings; >- >-use Test::More tests => 1; >+use DateTime; >+use Test::More tests => 21; >+use Koha::DateUtils; > > BEGIN { >- use_ok('C4::Calendar'); >+ use_ok('Koha::Calendar'); >+ >+ # This was the only test C4 had >+ # Remove when no longer used >+ use_ok('C4::Calendar'); > } > >+my $cal = Koha::Calendar->new( TEST_MODE => 1 ); >+ >+isa_ok( $cal, 'Koha::Calendar', 'Calendar class returned' ); >+ >+my $saturday = DateTime->new( >+ year => 2011, >+ month => 6, >+ day => 25, >+ time_zone => 'Europe/London', >+); >+my $sunday = DateTime->new( >+ year => 2011, >+ month => 6, >+ day => 26, >+ time_zone => 'Europe/London', >+); >+my $monday = DateTime->new( >+ year => 2011, >+ month => 6, >+ day => 27, >+ time_zone => 'Europe/London', >+); >+my $bloomsday = DateTime->new( >+ year => 2011, >+ month => 6, >+ day => 16, >+ time_zone => 'Europe/London', >+); # should be a holiday >+my $special = DateTime->new( >+ year => 2011, >+ month => 6, >+ day => 1, >+ time_zone => 'Europe/London', >+); # should be a holiday >+my $notspecial = DateTime->new( >+ year => 2011, >+ month => 6, >+ day => 2, >+ time_zone => 'Europe/London', >+); # should NOT be a holiday >+ >+is( $cal->is_holiday($sunday), 1, 'Sunday is a closed day' ); # wee free test; >+is( $cal->is_holiday($monday), 0, 'Monday is not a closed day' ); # alas >+is( $cal->is_holiday($bloomsday), 1, 'month/day closed day test' ); >+is( $cal->is_holiday($special), 1, 'special closed day test' ); >+is( $cal->is_holiday($notspecial), 0, 'open day test' ); >+ >+my $dt = $cal->addDate( $saturday, 1, 'days' ); >+is( $dt->day_of_week, 1, 'addDate skips closed Sunday' ); >+ >+$dt = $cal->addDate( $bloomsday, -1 ); >+is( $dt->ymd(), '2011-06-15', 'Negative call to addDate' ); >+ >+my $test_dt = DateTime->new( # Monday >+ year => 2012, >+ month => 7, >+ day => 23, >+ hour => 11, >+ minute => 53, >+ time_zone => 'Europe/London', >+); >+ >+my $later_dt = DateTime->new( # Monday >+ year => 2012, >+ month => 9, >+ day => 17, >+ hour => 17, >+ minute => 30, >+ time_zone => 'Europe/London', >+); >+ >+my $daycount = $cal->days_between( $test_dt, $later_dt ); >+cmp_ok( $daycount->in_units('days'), >+ '==', 48, 'days_between calculates correctly' ); >+ >+my $ret = $cal->addDate( $test_dt, 1, 'days' ); >+ >+cmp_ok( $ret->ymd(), 'eq', '2012-07-24', 'Simple Single Day Add (Calendar)`' ); >+ >+$ret = $cal->addDate( $test_dt, 7, 'days' ); >+cmp_ok( $ret->ymd(), 'eq', '2012-07-31', 'Add 7 days Calendar mode' ); >+$cal->set_daysmode('Datedue'); >+$ret = $cal->addDate( $test_dt, 7, 'days' ); >+cmp_ok( $ret->ymd(), 'eq', '2012-07-30', 'Add 7 days Datedue mode' ); >+$cal->set_daysmode('Days'); >+$ret = $cal->addDate( $test_dt, 7, 'days' ); >+cmp_ok( $ret->ymd(), 'eq', '2012-07-30', 'Add 7 days Days mode' ); >+$cal->set_daysmode('Calendar'); >+ >+# example tests for bug report >+$cal->clear_weekly_closed_days(); >+ >+$daycount = $cal->days_between( dt_from_string('2012-01-10'), >+ dt_from_string("2012-05-05") )->in_units('days'); >+cmp_ok( $daycount, '==', 116, 'test larger intervals' ); >+$daycount = $cal->days_between( dt_from_string("2012-01-01"), >+ dt_from_string("2012-05-05") )->in_units('days'); >+cmp_ok( $daycount, '==', 125, 'test positive intervals' ); >+my $daycount2 = $cal->days_between( dt_from_string("2012-05-05"), >+ dt_from_string("2012-01-01") )->in_units('days'); >+cmp_ok( $daycount2, '==', $daycount, 'test parameter order not relevant' ); >+$daycount = $cal->days_between( dt_from_string("2012-07-01"), >+ dt_from_string("2012-07-15") )->in_units('days'); >+cmp_ok( $daycount, '==', 14, 'days_between calculates correctly' ); >+$cal->add_holiday( dt_from_string('2012-07-06') ); >+$daycount = $cal->days_between( dt_from_string("2012-07-01"), >+ dt_from_string("2012-07-15") )->in_units('days'); >+cmp_ok( $daycount, '==', 13, 'holiday correctly recognized' ); >+ >+$cal->add_holiday( dt_from_string('2012-07-07') ); >+$daycount = $cal->days_between( dt_from_string("2012-07-01"), >+ dt_from_string("2012-07-15") )->in_units('days'); >+cmp_ok( $daycount, '==', 12, 'multiple holidays correctly recognized' ); >diff --git a/t/Kalendar.t b/t/Kalendar.t >deleted file mode 100755 >index 5d219b2..0000000 >--- a/t/Kalendar.t >+++ /dev/null >@@ -1,62 +0,0 @@ >-use strict; >-use warnings; >-use 5.010; >-use DateTime; >-use DateTime::TimeZone; >- >-use C4::Context; >-use Test::More tests => 9; >- >-BEGIN { use_ok('Koha::Calendar'); } >- >-my $cal = Koha::Calendar->new( TEST_MODE => 1 ); >- >-isa_ok( $cal, 'Koha::Calendar', 'Calendar class returned' ); >- >-my $saturday = DateTime->new( >- year => 2011, >- month => 6, >- day => 25, >- time_zone => 'Europe/London', >-); >-my $sunday = DateTime->new( >- year => 2011, >- month => 6, >- day => 26, >- time_zone => 'Europe/London', >-); >-my $monday = DateTime->new( >- year => 2011, >- month => 6, >- day => 27, >- time_zone => 'Europe/London', >-); >-my $bloomsday = DateTime->new( >- year => 2011, >- month => 6, >- day => 16, >- time_zone => 'Europe/London', >-); # should be a holiday >-my $special = DateTime->new( >- year => 2011, >- month => 6, >- day => 1, >- time_zone => 'Europe/London', >-); # should be a holiday >-my $notspecial = DateTime->new( >- year => 2011, >- month => 6, >- day => 2, >- time_zone => 'Europe/London', >-); # should NOT be a holiday >-is( $cal->is_holiday($sunday), 1, 'Sunday is a closed day' ); # wee free test; >-is( $cal->is_holiday($monday), 0, 'Monday is not a closed day' ); # alas >-is( $cal->is_holiday($bloomsday), 1, 'month/day closed day test' ); >-is( $cal->is_holiday($special), 1, 'special closed day test' ); >-is( $cal->is_holiday($notspecial), 0, 'open day test' ); >- >-my $dt = $cal->addDate( $saturday, 1, 'days' ); >-is( $dt->day_of_week, 1, 'addDate skips closed Sunday' ); >- >-$dt = $cal->addDate( $bloomsday, -1 ); >-is( $dt->ymd(), '2011-06-15', 'Negative call to addDate' ); >-- >1.7.11.2.249.g31c7954 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 8486
:
11111
|
11121
| 11174 |
11179
|
11248