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