Bug 9031: (QA follow-up) Final changes to Calendar::days_between

The crash is caused by comparing two datetimes where one datetime is
floating and the other one was not. In that case the floating is
converted. Note too that DateTime overloads comparison operators.

This patch clones the two dates first. Puts them in floating both. And
just after that starts comparing etc.

Similar small change in hours_between.

Adding a test where the parameters are swapped for days_between.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm
index f6a276f..c66ab45 100644 (file)
--- a/Koha/Calendar.pm
+++ b/Koha/Calendar.pm
@@ -317,32 +317,26 @@ sub days_between {
     my $start_dt = shift;
     my $end_dt   = shift;
 
-    if ( $start_dt->compare($end_dt) > 0 ) {
-        # swap dates
-        my $int_dt = $end_dt;
-        $end_dt = $start_dt;
-        $start_dt = $int_dt;
+    # Change time zone for date math and swap if needed
+    $start_dt = $start_dt->clone->set_time_zone('floating');
+    $end_dt = $end_dt->clone->set_time_zone('floating');
+    if( $start_dt->compare($end_dt) > 0 ) {
+        ( $start_dt, $end_dt ) = ( $end_dt, $start_dt );
     }
 
-
     # start and end should not be closed days
     my $days = $start_dt->delta_days($end_dt)->delta_days;
-    for (my $dt = $start_dt->clone()->set_time_zone('floating');
-        $dt <= $end_dt;
-        $dt->add(days => 1)
-    ) {
-        if ($self->is_holiday($dt)) {
-            $days--;
-        }
+    while( $start_dt->compare($end_dt) < 1 ) {
+        $days-- if $self->is_holiday($start_dt);
+        $start_dt->add( days => 1 );
     }
     return DateTime::Duration->new( days => $days );
-
 }
 
 sub hours_between {
     my ($self, $start_date, $end_date) = @_;
-    my $start_dt = $start_date->clone();
-    my $end_dt = $end_date->clone();
+    my $start_dt = $start_date->clone()->set_time_zone('floating');
+    my $end_dt = $end_date->clone()->set_time_zone('floating');
     my $duration = $end_dt->delta_ms($start_dt);
     $start_dt->truncate( to => 'day' );
     $end_dt->truncate( to => 'day' );
@@ -351,7 +345,7 @@ sub hours_between {
     # take into account open/close times then it would be a duration
     # of library open hours
     my $skipped_days = 0;
-    for (my $dt = $start_dt->clone()->set_time_zone('floating');
+    for (my $dt = $start_dt->clone();
         $dt <= $end_dt;
         $dt->add(days => 1)
     ) {

diff --git a/t/db_dependent/Calendar.t b/t/db_dependent/Calendar.t
index b443420..3cfe961 100644 (file)
--- a/t/db_dependent/Calendar.t
+++ b/t/db_dependent/Calendar.t
@@ -68,13 +68,15 @@ is($forwarded_dt->ymd, $today->ymd, 'negative day should return start dt');
 
 subtest 'crossing_DST' => sub {
 
-    plan tests => 2;
+    plan tests => 3;
 
     my $tz = DateTime::TimeZone->new( name => 'America/New_York' );
     my $start_date = dt_from_string( "2016-03-09 02:29:00",undef,$tz );
     my $end_date = dt_from_string( "2017-01-01 00:00:00", undef, $tz );
     my $days_between = $calendar->days_between($start_date,$end_date);
     is( $days_between->delta_days, 298, "Days calculated correctly" );
+    $days_between = $calendar->days_between($end_date,$start_date);
+    is( $days_between->delta_days, 298, "Swapping returns the same" );
     my $hours_between = $calendar->hours_between($start_date,$end_date);
     is( $hours_between->delta_minutes, 298 * 24 * 60 - 149, "Hours (in minutes) calculated correctly" );