Bugzilla – Attachment 11111 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]
Proposed Patch
0001-Bug-8486-Correct-calculation-of-days_between.patch (text/plain), 7.90 KB, created by
Colin Campbell
on 2012-07-24 14:24:27 UTC
(
hide
)
Description:
Proposed Patch
Filename:
MIME Type:
Creator:
Colin Campbell
Created:
2012-07-24 14:24:27 UTC
Size:
7.90 KB
patch
obsolete
>From 6f11e0e8719a0ef925e4c676fbf47cf584eadd10 Mon Sep 17 00:00:00 2001 >From: Colin Campbell <colin.campbell@ptfs-europe.com> >Date: Tue, 24 Jul 2012 15:16:10 +0100 >Subject: [PATCH] Bug 8486 Correct calculation of days_between > >Thee 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 >--- > Koha/Calendar.pm | 87 +++++++++++++++++++++++++++++++++++++++++++++----------- > t/Calendar.t | 80 ++++++++++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 143 insertions(+), 24 deletions(-) > >diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm >index 495f8ce..298cdb2 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 >- $datestart_temp->truncate( to => 'day' ); >- $dateend_temp->truncate( to => 'day' ); >- my $duration = $dateend_temp - $datestart_temp; >- 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 => 'day' ); > $end_dt->truncate( to => 'day' ); >@@ -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..65c2622 100755 >--- a/t/Calendar.t >+++ b/t/Calendar.t >@@ -1,14 +1,80 @@ >-#!/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 => 14; >+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' ); >+ >+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' ); >-- >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