From c882d9139e642d6a57282a676bc9c1398f873afc 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] 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.

- CalcDateDue was modified to be consistent with the intended behaviour
- Koha::Calendar->addDate was rewritten in a more sane way (also split into
  addHours and addDays for convenience).

Regards
To+

Sponsored-by: Universidad Nacional de Córdoba
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Increased the number of tests to 28
---
 C4/Circulation.pm |    7 ++-
 Koha/Calendar.pm  |  179 +++++++++++++++++++++++++++++++++++++++++------------
 t/Calendar.t      |   52 +++++++++++++++-
 3 files changed, 196 insertions(+), 42 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 5cbc211..beddf04 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -3007,20 +3007,21 @@ sub CalcDateDue {
     } else {
 
         # otherwise, calculate the datedue as normal
-        if ( C4::Context->preference('useDaysMode') eq 'Days' )
+        if ( C4::Context->preference('useDaysMode') ne 'Calendar' )
         {    # ignoring calendar
             my $dt =
               DateTime->now( time_zone => C4::Context->tz() )
               ->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;
             }
+
+            return $dt;
+
         } else {
             my $dur;
             if ($loanlength->{lengthunit} eq 'hours') {
diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm
index d4ea77e..22640c5 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 {
@@ -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;
@@ -274,9 +336,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 +374,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 +414,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..058f700 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 => 28;
 use Koha::DateUtils;
 
 BEGIN {
@@ -131,3 +132,52 @@ $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' );
+
+# next_open_day tests
+$dt = dt_from_string('2012-07-07','iso');
+cmp_ok( $dt->add(days => 1), '==', $cal->next_open_day(dt_from_string('2012-07-07','iso')), 'Next open day correctly calculated' );
+
+# prev_open_day tests
+# 2012-07-07 and 2012-07-06 are set to holidays previously, so previous open day should match 2012-07-05
+$dt = dt_from_string('2012-07-05','iso');
+cmp_ok( $dt, '==', $cal->prev_open_day(dt_from_string('2012-07-08','iso')), 'Previous open day correctly calculated' );
+
+######################
+# useDaysMode='Datedue' case tests
+subtest 'useDaysMode=Datedue' => sub { 
+    my $cal = Koha::Calendar->new( TEST_MODE => 1 );
+    my $dur = DateTime::Duration->new( days => 1 );
+    $cal->set_daysmode( 'Datedue' );
+    $cal->add_holiday( dt_from_string('2012-07-04','iso') );
+    $dt = dt_from_string( '2012-07-03','iso' );
+    is($cal->addDate( $dt, $dur, 'days' ), dt_from_string('2012-07-05','iso'), 'Single day add (Datedue, matches holiday, shift)' );
+    $dur = DateTime::Duration->new( days => 2 );
+    is($cal->addDate( $dt, $dur, 'days' ), dt_from_string('2012-07-05','iso'), 'Two days add, skips holiday (Datedue)' );
+
+};
+#######################
+
+#######################
+# useDaysMode='Calendar' case tests
+subtest 'useDaysMode=Calendar' => sub {
+    my $cal = Koha::Calendar->new( TEST_MODE => 1 );
+    my $dur = DateTime::Duration->new( days => 1 );
+    $cal->set_daysmode('Calendar');
+    $cal->add_holiday( dt_from_string('2012-07-04','iso') );
+    $dt = dt_from_string('2012-07-03','iso');
+    is($cal->addDate( $dt, $dur, 'days' ), dt_from_string('2012-07-05','iso'), 'Single day add (Calendar)' );
+};
+#######################
+
+#######################
+# useDaysMode='Days' case tests
+subtest 'useDaysMode=Days' => sub {
+    my $cal = Koha::Calendar->new( TEST_MODE => 1 );
+    my $dur = DateTime::Duration->new( days => 1 );
+    $cal->set_daysmode('Days');
+    $cal->add_holiday( dt_from_string('2012-07-04','iso') );
+    $dt = dt_from_string('2012-07-03','iso');
+    is($cal->addDate( $dt, $dur, 'days' ), dt_from_string('2012-07-04','iso'), 'Single day add (Days)' );
+};
+#######################
+
-- 
1.7.10.4