From 829b61a4bdce2c454860e64f08fa81e767c03cbc Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@gmail.com>
Date: Fri, 21 Sep 2012 12:50:26 -0300
Subject: [PATCH 1/3] [SIGNED-OFF] Bug 8800 - useDaysMode=Datedue wrong
 behaviour (revisited)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
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 <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
---
 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 5cbc211..ad86d41 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -3014,13 +3014,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 d4ea77e..3986038 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 2b719b8..d6972d0 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)' );
+};
-- 
1.7.9.5