From 4b519f12d4b7d522dba0cb38d75043d2b1e07a11 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 4 Sep 2015 10:39:48 +0100 Subject: [PATCH] Bug 14293: The suspension should be calculated in days 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: Nick Clemens --- C4/Circulation.pm | 11 +++----- .../Circulation/IssuingRules/maxsuspensiondays.t | 31 +++++++++++++++++++++- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 63b6543a7d..87155e2cc4 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2231,21 +2231,18 @@ sub _debar_user_on_return { ); 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); if ($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 ); - my $deltadays = DateTime::Duration->new( - days => $chargeable_units + my $deltadays = C4::Overdues::get_chargeable_units('days', $dt_due, $return_date, $branchcode); + $deltadays = DateTime::Duration->new( + days => $deltadays, ); + if ( $deltadays->subtract($grace)->is_positive() ) { my $suspension_days = $deltadays * $finedays; diff --git a/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t b/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t index 196b0fe79e..abdef9daad 100644 --- a/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t +++ b/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t @@ -1,5 +1,5 @@ use Modern::Perl; -use Test::More tests => 2; +use Test::More tests => 3; use MARC::Record; use MARC::Field; @@ -101,6 +101,35 @@ is( ); DelDebarment( $debarments->[0]->{borrower_debarment_id} ); +# Test with lengthunit => 'hours' +Koha::IssuingRules->search->delete; +$builder->build( + { + source => 'Issuingrule', + value => { + categorycode => '*', + itemtype => '*', + branchcode => '*', + firstremind => 0, + finedays => 2, + lengthunit => 'hours', + suspension_chargeperiod => 1, + maxsuspensiondays => 100, + } + } +); + +$daysafter10 = dt_from_string->add_duration(DateTime::Duration->new(days => 10)); +AddIssue( $borrower, $barcode, $daysago20 ); +AddReturn( $barcode, $branchcode ); +$debarments = GetDebarments({borrowernumber => $borrower->{borrowernumber}}); +is( + $debarments->[0]->{expiration}, + output_pref({ dt => $daysafter40, dateformat => 'iso', dateonly => 1 }), + 'calculate suspension with lengthunit hours.' +); +DelDebarment( $debarments->[0]->{borrower_debarment_id} ); + $schema->storage->txn_rollback; # C4::Context->userenv -- 2.13.7