From 964fa43f610c77c1d9bb6276fdd787ea7ee86d05 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 Signed-off-by: Arthur Suzuki Signed-off-by: Mark Tompsett --- C4/Circulation.pm | 10 +- C4/Overdues.pm | 17 +-- .../IssuingRules/maxsuspensiondays.t | 133 ++++++++++++++++++ 3 files changed, 142 insertions(+), 18 deletions(-) create mode 100644 t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t diff --git a/C4/Circulation.pm b/C4/Circulation.pm index ac28b2e74b..4fdea76536 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; @@ -2699,14 +2699,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); @@ -2721,7 +2717,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..3cc7c256b9 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -321,7 +321,6 @@ 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 ); @@ -331,16 +330,12 @@ sub get_chargeable_units { if($charge_duration->in_units('hours') == 0 && $charge_duration->in_units('seconds') > 0){ return 1; } - return $charge_duration->in_units('hours'); - } - 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'); + + 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/IssuingRules/maxsuspensiondays.t b/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t new file mode 100644 index 0000000000..b969baa960 --- /dev/null +++ b/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t @@ -0,0 +1,133 @@ +use Modern::Perl; +use Test::More tests => 3; + +use MARC::Record; +use MARC::Field; +use C4::Context; + +use C4::Circulation qw( AddIssue AddReturn ); +use C4::Items qw( AddItem ); +use C4::Biblio qw( AddBiblio ); +use Koha::Database; +use Koha::DateUtils; +use Koha::Patron::Debarments qw( GetDebarments DelDebarment ); +use Koha::Patrons; + +use t::lib::TestBuilder; +use t::lib::Mocks; + +my $schema = Koha::Database->schema; +$schema->storage->txn_begin; +my $builder = t::lib::TestBuilder->new; +my $dbh = C4::Context->dbh; +$dbh->{RaiseError} = 1; + +my $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; +my $itemtype = $builder->build({ source => 'Itemtype' })->{itemtype}; +my $patron_category = $builder->build({ source => 'Category' }); + +t::lib::Mocks::mock_userenv({ branchcode => $branchcode }); + +# Test without maxsuspensiondays set +Koha::IssuingRules->search->delete; +$builder->build( + { + source => 'Issuingrule', + value => { + categorycode => '*', + itemtype => '*', + branchcode => '*', + firstremind => 0, + finedays => 2, + lengthunit => 'days', + suspension_chargeperiod => 1, + } + } +); + +my $borrowernumber = Koha::Patron->new({ + firstname => 'my firstname', + surname => 'my surname', + categorycode => $patron_category->{categorycode}, + branchcode => $branchcode, +})->store->borrowernumber; +my $borrower = Koha::Patrons->find( $borrowernumber )->unblessed; + +my $record = MARC::Record->new(); +$record->append_fields( + MARC::Field->new('100', ' ', ' ', a => 'My author'), + MARC::Field->new('245', ' ', ' ', a => 'My title'), +); + +my $barcode = 'bc_maxsuspensiondays'; +my ($biblionumber, $biblioitemnumber) = AddBiblio($record, ''); +my (undef, undef, $itemnumber) = AddItem({ + homebranch => $branchcode, + holdingbranch => $branchcode, + barcode => $barcode, + itype => $itemtype + } , $biblionumber); + +# clear any holidays to avoid throwing off the suspension day +# calculations +$dbh->do('DELETE FROM special_holidays'); +$dbh->do('DELETE FROM repeatable_holidays'); + +my $daysago20 = dt_from_string->add_duration(DateTime::Duration->new(days => -20)); +my $daysafter40 = dt_from_string->add_duration(DateTime::Duration->new(days => 40)); + +AddIssue( $borrower, $barcode, $daysago20 ); +AddReturn( $barcode, $branchcode ); +my $debarments = GetDebarments({borrowernumber => $borrower->{borrowernumber}}); +is( + $debarments->[0]->{expiration}, + output_pref({ dt => $daysafter40, dateformat => 'iso', dateonly => 1 }), + 'calculate suspension with no maximum set' +); +DelDebarment( $debarments->[0]->{borrower_debarment_id} ); + +# Test with maxsuspensiondays = 10 days +my $issuing_rule = Koha::IssuingRules->search->next; +$issuing_rule->maxsuspensiondays( 10 )->store; + +my $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 => $daysafter10, dateformat => 'iso', dateonly => 1 }), + 'calculate suspension with a maximum set' +); +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; -- 2.30.2