From 1d69301468a50e6154e0c4de0ebc6cfbf0c3e4a2 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 21 Sep 2012 12:50:26 -0300 Subject: [PATCH] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit useDaysMode=Datedue wasn't used as advertised in the docs. Added next_open_day and prev_open_day subs to Koha::Calendar and some tests for them. - Koha::Calendar->addDate was rewritten in a more sane way (also split into addHours and addDays for convenience). - Fixed a bug introduced in Bug 8966 regarding dt truncation and dtSets->contains - Minor docs typos - Use the passed Calendar mode or default to 'Calendar' in Koha::Calendar->_mockinit. - Tests I'm writing some db-dependent tests for is_holiday, and hopefully for CalcDateDue so any rewrite/followup doesn't break things. Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Kyle M Hall Signed-off-by: Katrin Fischer Passed-QA-by: Paul Poulain Signed-off-by: Jared Camins-Esakov Conflicts: t/Calendar.t Signed-off-by: Chris Cormack --- C4/Circulation.pm | 5 +- Koha/Calendar.pm | 190 ++++++++++++++++++++++++++++++++++++---------- t/Calendar.t | 73 +++++++++++++++--- 3 files changed, 213 insertions(+), 55 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 42119ef462..a23b070d85 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2828,13 +2828,14 @@ sub CalcDateDue { ->truncate( to => 'minute' ); if ( $loanlength->{lengthunit} eq 'hours' ) { $dt->add( hours => $loanlength->{issuelength} ); - return $dt; } else { # days $dt->add( days => $loanlength->{issuelength} ); $dt->set_hour(23); $dt->set_minute(59); - return $dt; } + # break + return $dt; + } else { my $dur; if ($loanlength->{lengthunit} eq 'hours') { diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm index d4ea77ed9e..3986038fda 100644 --- a/Koha/Calendar.pm +++ b/Koha/Calendar.pm @@ -85,57 +85,93 @@ sub _init { sub addDate { my ( $self, $startdate, $add_duration, $unit ) = @_; - my $base_date = $startdate->clone(); + + # Default to days duration (legacy support I guess) if ( ref $add_duration ne 'DateTime::Duration' ) { $add_duration = DateTime::Duration->new( days => $add_duration ); } - $unit ||= q{}; # default days ? - my $days_mode = $self->{days_mode}; - Readonly::Scalar my $return_by_hour => 10; - my $day_dur = DateTime::Duration->new( days => 1 ); - if ( $add_duration->is_negative() ) { - $day_dur = DateTime::Duration->new( days => -1 ); + + $unit ||= 'days'; # default days ? + my $dt; + + if ( $unit eq 'hours' ) { + # Fixed for legacy support. Should be set as a branch parameter + Readonly::Scalar my $return_by_hour => 10; + + $dt = $self->addHours($startdate, $add_duration, $return_by_hour); + } else { + # days + $dt = $self->addDays($startdate, $add_duration); } - if ( $days_mode eq 'Datedue' ) { - my $dt = $base_date + $add_duration; - while ( $self->is_holiday($dt) ) { + return $dt; +} - $dt->add_duration($day_dur); - if ( $unit eq 'hours' ) { - $dt->set_hour($return_by_hour); # Staffs specific - } +sub addHours { + my ( $self, $startdate, $hours_duration, $return_by_hour ) = @_; + my $base_date = $startdate->clone(); + + $base_date->add_duration($hours_duration); + + # If we are using the calendar behave for now as if Datedue + # was the chosen option (current intended behaviour) + + if ( $self->{days_mode} ne 'Days' && + $self->is_holiday($base_date) ) { + + if ( $hours_duration->is_negative() ) { + $base_date = $self->prev_open_day($base_date); + } else { + $base_date = $self->next_open_day($base_date); } - return $dt; - } elsif ( $days_mode eq 'Calendar' ) { - if ( $unit eq 'hours' ) { - $base_date->add_duration($add_duration); - while ( $self->is_holiday($base_date) ) { - $base_date->add_duration($day_dur); - } + $base_date->set_hour($return_by_hour); + + } + + return $base_date; +} +sub addDays { + my ( $self, $startdate, $days_duration ) = @_; + my $base_date = $startdate->clone(); + + if ( $self->{days_mode} eq 'Calendar' ) { + # use the calendar to skip all days the library is closed + # when adding + my $days = abs $days_duration->in_units('days'); + + if ( $days_duration->is_negative() ) { + while ($days) { + $base_date = $self->prev_open_day($base_date); + --$days; + } } else { - my $days = abs $add_duration->in_units('days'); while ($days) { - $base_date->add_duration($day_dur); - if ( $self->is_holiday($base_date) ) { - next; - } else { - --$days; - } + $base_date = $self->next_open_day($base_date); + --$days; } } - if ( $unit eq 'hours' ) { - my $dt = $base_date->clone()->subtract( days => 1 ); - if ( $self->is_holiday($dt) ) { - $base_date->set_hour($return_by_hour); # Staffs specific + + } else { # Days or Datedue + # use straight days, then use calendar to push + # the date to the next open day if Datedue + $base_date->add_duration($days_duration); + + if ( $self->{days_mode} eq 'Datedue' ) { + # Datedue, then use the calendar to push + # the date to the next open day if holiday + if ( $self->is_holiday($base_date) ) { + if ( $days_duration->is_negative() ) { + $base_date = $self->prev_open_day($base_date); + } else { + $base_date = $self->next_open_day($base_date); + } } } - return $base_date; - } else { # Days - return $base_date + $add_duration; } + + return $base_date; } sub is_holiday { @@ -154,10 +190,10 @@ sub is_holiday { if ( exists $self->{day_month_closed_days}->{$month}->{$day} ) { return 1; } - if ( $self->{exception_holidays}->contains($dt) ) { + if ( $self->{exception_holidays}->contains($localdt) ) { return 1; } - if ( $self->{single_holidays}->contains($dt) ) { + if ( $self->{single_holidays}->contains($localdt) ) { return 1; } @@ -165,6 +201,32 @@ sub is_holiday { return 0; } +sub next_open_day { + my ( $self, $dt ) = @_; + my $base_date = $dt->clone(); + + $base_date->add(days => 1); + + while ($self->is_holiday($base_date)) { + $base_date->add(days => 1); + } + + return $base_date; +} + +sub prev_open_day { + my ( $self, $dt ) = @_; + my $base_date = $dt->clone(); + + $base_date->add(days => -1); + + while ($self->is_holiday($base_date)) { + $base_date->add(days => -1); + } + + return $base_date; +} + sub days_between { my $self = shift; my $start_dt = shift; @@ -228,7 +290,12 @@ sub _mockinit { ); push @{$dates}, $special; $self->{single_holidays} = DateTime::Set->from_datetimes( dates => $dates ); - $self->{days_mode} = 'Calendar'; + + # if not defined, days_mode defaults to 'Calendar' + if ( !defined($self->{days_mode}) ) { + $self->{days_mode} = 'Calendar'; + } + $self->{test} = 1; return; } @@ -274,9 +341,9 @@ This documentation refers to Koha::Calendar version 0.0.1 =head1 SYNOPSIS - use Koha::Calendat + use Koha::Calendar - my $c = Koha::Calender->new( branchcode => 'MAIN' ); + my $c = Koha::Calendar->new( branchcode => 'MAIN' ); my $dt = DateTime->now(); # are we open @@ -312,11 +379,36 @@ Currently unit is only used to invoke Staffs return Monday at 10 am rule this parameter will be removed when issuingrules properly cope with that +=head2 addHours + + my $dt = $calendar->addHours($date, $dur, $return_by_hour ) + +C<$date> is a DateTime object representing the starting date of the interval. + +C<$offset> is a DateTime::Duration to add to it + +C<$return_by_hour> is an integer value representing the opening hour for the branch + + +=head2 addDays + + my $dt = $calendar->addDays($date, $dur) + +C<$date> is a DateTime object representing the starting date of the interval. + +C<$offset> is a DateTime::Duration to add to it + +C<$unit> is a string value 'days' or 'hours' toflag granularity of duration + +Currently unit is only used to invoke Staffs return Monday at 10 am rule this +parameter will be removed when issuingrules properly cope with that + + =head2 is_holiday $yesno = $calendar->is_holiday($dt); -passed at DateTime object returns 1 if it is a closed day +passed a DateTime object returns 1 if it is a closed day 0 if not according to the calendar =head2 days_between @@ -327,6 +419,22 @@ Passed two dates returns a DateTime::Duration object measuring the length betwee ignoring closed days. Always returns a positive number irrespective of the relative order of the parameters +=head2 next_open_day + +$datetime = $calendar->next_open_day($duedate_dt) + +Passed a Datetime returns another Datetime representing the next open day. It is +intended for use to calculate the due date when useDaysMode syspref is set to either +'Datedue' or 'Calendar'. + +=head2 prev_open_day + +$datetime = $calendar->prev_open_day($duedate_dt) + +Passed a Datetime returns another Datetime representing the previous open day. It is +intended for use to calculate the due date when useDaysMode syspref is set to either +'Datedue' or 'Calendar'. + =head2 set_daysmode For testing only allows the calling script to change days mode diff --git a/t/Calendar.t b/t/Calendar.t index 80926eb786..d6972d0acc 100755 --- a/t/Calendar.t +++ b/t/Calendar.t @@ -3,7 +3,8 @@ use strict; use warnings; use DateTime; -use Test::More tests => 23; +use DateTime::Duration; +use Test::More tests => 22; use Koha::DateUtils; BEGIN { @@ -89,18 +90,8 @@ 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' ); +my $ret; -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'); # see bugzilla #8966 @@ -131,3 +122,61 @@ $cal->add_holiday( dt_from_string('2012-07-07','iso') ); $daycount = $cal->days_between( dt_from_string("2012-07-01",'iso'), dt_from_string("2012-07-15",'iso') )->in_units('days'); cmp_ok( $daycount, '==', 12, 'multiple holidays correctly recognized' ); + +my $one_day_dur = DateTime::Duration->new( days => 1 ); +my $two_day_dur = DateTime::Duration->new( days => 2 ); +my $seven_day_dur = DateTime::Duration->new( days => 7 ); + +subtest '\'Datedue\' tests' => sub { + my $cal = Koha::Calendar->new( TEST_MODE => 1 , + days_mode => 'Datedue'); + + $cal->add_holiday( dt_from_string('2012-07-04','iso') ); + $dt = dt_from_string( '2012-07-03','iso' ); + + is($cal->addDate( $dt, $one_day_dur, 'days' ), + dt_from_string('2012-07-05','iso'), + 'Single day add (Datedue, matches holiday, shift)' ); + + is($cal->addDate( $dt, $two_day_dur, 'days' ), + dt_from_string('2012-07-05','iso'), + 'Two days add, skips holiday (Datedue)' ); + + cmp_ok($cal->addDate( $test_dt, $seven_day_dur, 'days' ), 'eq', + '2012-07-30T11:53:00', + 'Add 7 days (Datedue)' ); +}; + + +subtest '\'Calendar\' tests' => sub { + my $cal = Koha::Calendar->new( TEST_MODE => 1, + days_mode => 'Calendar' ); + + $cal->add_holiday( dt_from_string('2012-07-04','iso') ); + $dt = dt_from_string('2012-07-03','iso'); + + is($cal->addDate( $dt, $one_day_dur, 'days' ), + dt_from_string('2012-07-05','iso'), + 'Single day add (Calendar)' ); + + cmp_ok($cal->addDate( $test_dt, $seven_day_dur, 'days' ), 'eq', + '2012-07-31T11:53:00', + 'Add 7 days (Calendar)' ); +}; + + +subtest '\'Days\' tests' => sub { + my $cal = Koha::Calendar->new( TEST_MODE => 1, + days_mode => 'Days' ); + + $cal->add_holiday( dt_from_string('2012-07-04','iso') ); + $dt = dt_from_string('2012-07-03','iso'); + + is($cal->addDate( $dt, $one_day_dur, 'days' ), + dt_from_string('2012-07-04','iso'), + 'Single day add (Days)' ); + + cmp_ok($cal->addDate( $test_dt, $seven_day_dur, 'days' ),'eq', + '2012-07-30T11:53:00', + 'Add 7 days (Days)' ); +}; -- 2.39.5