Attachment 13591 Details for Bug 8800 – Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)

[patch] Bug 8800 - useDaysMode=Datedue wrong behaviour (revisited)

Bug-8800---useDaysModeDatedue-wrong-behaviour-revi.patch (text/plain), 13.16 KB, created by Kyle M Hall (khall) on 2012-11-21 14:55:11 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2012-11-21 14:55:11 UTC

Size: 13.16 KB

patch

obsolete

>From b939b07037ca7335f25d5dc002a8fd196fd3e871 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
>Content-Type: text/plain; charset="utf-8"
>
>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>
>---
> 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.2.5

Actions: View | Diff | Splinter Review

Attachments on bug 8800: 12444 | 12445 | 12446 | 12448 | 12449 | 12527 | 12600 | 12919 | 13367 | 13402 | 13447 | 13571 | 13591 | 13605 | 13619 | 13620