Attachment 13402 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), 11.89 KB, created by Paul Poulain on 2012-11-12 17:16:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Paul Poulain

Created: 2012-11-12 17:16:30 UTC

Size: 11.89 KB

patch

obsolete

>From fa1e5ab0976fecd3c03aedd4ce2ea67a644f60ac 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
>
>Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
>---
> C4/Circulation.pm |    7 ++-
> Koha/Calendar.pm  |  179 +++++++++++++++++++++++++++++++++++++++++------------
> t/Calendar.t      |   51 ++++++++++++++-
> 3 files changed, 195 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..4e5fd73 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..ee67dc4 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,51 @@ $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.9.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