From 76e1939c82bc0aae5a8de05516c9db27c380f812 Mon Sep 17 00:00:00 2001
From: Thibaud Guillot <thibaud.guillot@biblibre.com>
Date: Fri, 4 Sep 2015 10:39:48 +0100
Subject: [PATCH] Bug 14293: The suspension should be calculated in days
Content-Type: text/plain; charset=utf-8

There was a mismatch between bug 13909 and bug 5549.
The get_chargeable_units call from _debar_user_on_return should specify
that we want a suspension in days.

However I don't understand the comment let by the author of bug 5549
f61a9617184ec4b24100c1d99150bfd4ebf13336

1/
> finedays is in days, so hourly loans must multiply by 24
Yes but the suspension is in days, so no need to * 24

2/ And we should have a look on this one too:
> grace period is measured in the same units as the loan
But the grace period is always in days. On the circ rules the column is
"Fine grace period (day)".
I think we should replace
  DateTime::Duration->new( $unit => $issuingrule->{firstremind} );
with
  DateTime::Duration->new( days => $issuingrule->{firstremind} );

Anyway, we definitelly need more tests in this area!

Test plan:
1/ Define an issuing rule with a unit=hour
2/ Set a suspension in days
3/ Check an item out and specify a past due date.
4/ Check the item in
5/ The patron should be debarred correctly

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 C4/Circulation.pm                             |  10 +-
 C4/Overdues.pm                                |  33 ++---
 .../Circulation/maxsuspensiondays.t           | 121 +++++++++++++++++-
 3 files changed, 133 insertions(+), 31 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 29abc95ac7..bafd23c2a6 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -20,7 +20,7 @@ package C4::Circulation;
 
 use Modern::Perl;
 use DateTime;
-use POSIX qw( floor );
+use POSIX qw( floor ceil );
 use YAML::XS;
 use Encode;
 
@@ -2718,14 +2718,10 @@ sub _calculate_new_debar_dt {
     );
     my $finedays = $issuing_rule ? $issuing_rule->{finedays} : undef;
     my $unit     = $issuing_rule ? $issuing_rule->{lengthunit} : undef;
-    my $chargeable_units = C4::Overdues::get_chargeable_units($unit, $dt_due, $return_date, $branchcode);
+    my $chargeable_units = C4::Overdues::get_chargeable_units('days', $dt_due, $return_date, $branchcode);
 
     return unless $finedays;
 
-    # finedays is in days, so hourly loans must multiply by 24
-    # thus 1 hour late equals 1 day suspension * finedays rate
-    $finedays = $finedays * 24 if ( $unit eq 'hours' );
-
     # grace period is measured in the same units as the loan
     my $grace =
       DateTime::Duration->new( $unit => $issuing_rule->{firstremind} // 0);
@@ -2740,7 +2736,7 @@ sub _calculate_new_debar_dt {
         if ( defined $issuing_rule->{suspension_chargeperiod} && $issuing_rule->{suspension_chargeperiod} > 1 ) {
             # No need to / 1 and do not consider / 0
             $suspension_days = DateTime::Duration->new(
-                days => floor( $suspension_days->in_units('days') / $issuing_rule->{suspension_chargeperiod} )
+                days => ceil( $suspension_days->in_units('days') / $issuing_rule->{suspension_chargeperiod} )
             );
         }
 
diff --git a/C4/Overdues.pm b/C4/Overdues.pm
index 41674a468a..eac4208e8d 100644
--- a/C4/Overdues.pm
+++ b/C4/Overdues.pm
@@ -321,26 +321,21 @@ sub get_chargeable_units {
 
     my $charge_units = 0;
     my $charge_duration;
-    if ($unit eq 'hours') {
-        if(C4::Context->preference('finesCalendar') eq 'noFinesWhenClosed') {
-            my $calendar = Koha::Calendar->new( branchcode => $branchcode );
-            $charge_duration = $calendar->hours_between( $date_due, $date_returned );
-        } else {
-            $charge_duration = $date_returned->delta_ms( $date_due );
-        }
-        if($charge_duration->in_units('hours') == 0 && $charge_duration->in_units('seconds') > 0){
-            return 1;
-        }
-        return $charge_duration->in_units('hours');
+    if(C4::Context->preference('finesCalendar') eq 'noFinesWhenClosed') {
+        my $calendar = Koha::Calendar->new( branchcode => $branchcode );
+        $charge_duration = $calendar->hours_between( $date_due, $date_returned );
+    } else {
+        $charge_duration = $date_returned->delta_ms( $date_due );
     }
-    else { # days
-        if(C4::Context->preference('finesCalendar') eq 'noFinesWhenClosed') {
-            my $calendar = Koha::Calendar->new( branchcode => $branchcode );
-            $charge_duration = $calendar->days_between( $date_due, $date_returned );
-        } else {
-            $charge_duration = $date_returned->delta_days( $date_due );
-        }
-        return $charge_duration->in_units('days');
+    if($charge_duration->in_units('hours') == 0 && $charge_duration->in_units('seconds') > 0){
+        return 1;
+    }
+
+    my $hours = $charge_duration->in_units('hours');
+    if ($unit eq 'hours') {
+        return $hours;
+    } else {
+        return ( $hours > 0 ? ceil($hours / 24) : 0 );
     }
 }
 
diff --git a/t/db_dependent/Circulation/maxsuspensiondays.t b/t/db_dependent/Circulation/maxsuspensiondays.t
index 614a32303a..bcd25e1a48 100755
--- a/t/db_dependent/Circulation/maxsuspensiondays.t
+++ b/t/db_dependent/Circulation/maxsuspensiondays.t
@@ -15,6 +15,8 @@ use Koha::Patrons;
 use t::lib::TestBuilder;
 use t::lib::Mocks;
 
+use POSIX qw( ceil );
+
 my $schema = Koha::Database->schema;
 $schema->storage->txn_begin;
 my $builder = t::lib::TestBuilder->new;
@@ -129,9 +131,12 @@ subtest "suspension_chargeperiod" => sub {
     my $last_year = dt_from_string->clone->subtract( years => 1 );
     my $today = dt_from_string;
     my $new_debar_dt = C4::Circulation::_calculate_new_debar_dt( $patron, $item, $last_year, $today );
-    is( $new_debar_dt->truncate( to => 'day' ),
-        $today->clone->add( days => 365 / 15 * 7 )->truncate( to => 'day' ) );
-
+    is(
+        $new_debar_dt->truncate( to => 'day' ),
+        $today->clone->add( days => ceil( 365 / 15 * 7 ) )
+          ->truncate( to => 'day' )
+      ),
+      'the truncated dates are equal.'
 };
 
 subtest "maxsuspensiondays" => sub {
@@ -156,7 +161,113 @@ subtest "maxsuspensiondays" => sub {
     my $today = dt_from_string;
     my $new_debar_dt = C4::Circulation::_calculate_new_debar_dt( $patron, $item, $last_year, $today );
     is( $new_debar_dt->truncate( to => 'day' ),
-        $today->clone->add( days => 333 )->truncate( to => 'day' ) );
-};
+        $today->clone->add( days => 333 )->truncate( to => 'day' ));
+
+
+    Koha::CirculationRules->set_rules(
+        {
+            categorycode => undef,
+            itemtype     => undef,
+            branchcode   => undef,
+            rules        => {
+                firstremind             => 0,
+                finedays                => 2,
+                lengthunit              => 'hours',
+                suspension_chargeperiod => 1,
+                maxsuspensiondays       => 100,
+            }
+        }
+    );
+
+    my $yesterday  = dt_from_string->clone->subtract( days => 1 );
+    my $daysafter2 = dt_from_string->clone->add( days => 2 );
+    AddIssue( $patron, $barcode, $yesterday );
+    AddReturn( $barcode, $branchcode );
+    $debarments    = $patron->restrictions;
+    $THE_debarment = $debarments->next;
+    is(
+        $THE_debarment->expiration,
+        output_pref(
+            { dt => $daysafter2, dateformat => 'iso', dateonly => 1 }
+        ),
+        'calculate suspension with lengthunit hours.'
+    );
+
+    DelDebarment( $THE_debarment->borrower_debarment_id );
+
+    my $hoursago2 = dt_from_string->clone->subtract( hours => 2 );
+    AddIssue( $patron, $barcode, $hoursago2 );
+    AddReturn( $barcode, $branchcode );
+    $debarments    = $patron->restrictions;
+    $THE_debarment = $debarments->next;
+    is(
+        $THE_debarment->expiration,
+        output_pref(
+            { dt => $daysafter2, dateformat => 'iso', dateonly => 1 }
+        ),
+        'calculate suspension with lengthunit hours.'
+    );
 
+    DelDebarment( $THE_debarment->borrower_debarment_id );
+
+    my $hoursafter2 = dt_from_string->clone->add( hours => 2 );
+    AddIssue( $patron, $barcode, $hoursafter2 );
+    AddReturn( $barcode, $branchcode );
+    $debarments    = $patron->restrictions;
+    $THE_debarment = $debarments->next;
+    is_deeply( $THE_debarment, undef, 'No debarment if in time' );
+
+    # Add 1 day every 2 days
+    Koha::CirculationRules->set_rules(
+        {
+            categorycode => undef,
+            itemtype     => undef,
+            branchcode   => undef,
+            rules        => {
+                firstremind             => 0,
+                finedays                => 1,
+                lengthunit              => 'hours',
+                suspension_chargeperiod => 2,
+                maxsuspensiondays       => 100,
+            }
+        }
+    );
+
+    # 20 days late => 20/2 * 1 => 10 days of suspension
+    AddIssue( $patron, $barcode, $daysago20 );
+    AddReturn( $barcode, $branchcode );
+    $debarments    = $patron->restrictions;
+    $THE_debarment = $debarments->next;
+    is(
+        $THE_debarment->expiration,
+        output_pref(
+            { dt => $daysafter10, dateformat => 'iso', dateonly => 1 }
+        ),
+        'calculate suspension with lengthunit hours.'
+    );
+
+    DelDebarment( $THE_debarment->borrower_debarment_id );
+
+    # 1 day late => ceil(1/2) * 1 => 1 day of suspension
+    my $tomorrow = dt_from_string->clone->add( days => 1 );
+    AddIssue( $patron, $barcode, $yesterday );
+    AddReturn( $barcode, $branchcode );
+    $debarments    = $patron->restrictions;
+    $THE_debarment = $debarments->next;
+    is(
+        $THE_debarment->expiration,
+        output_pref( { dt => $tomorrow, dateformat => 'iso', dateonly => 1 } ),
+        'calculate suspension with lengthunit hours.'
+    );
+
+    DelDebarment( $THE_debarment->borrower_debarment_id );
+
+    # 2 hours late => ceil(.x/2) * 1 => 1 day of suspension
+    AddIssue( $patron, $barcode, $hoursafter2 );
+    AddReturn( $barcode, $branchcode );
+    $debarments    = $patron->restrictions;
+    $THE_debarment = $debarments->next;
+    is_deeply( $THE_debarment, undef, 'No debarment if in time' );
+
+};
 $schema->storage->txn_rollback;
-- 
2.30.2