From c3e1de843a05a676259211d51afebaed3b6381c7 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 16 Jan 2015 09:07:48 -0500 Subject: [PATCH] Bug 13590 - Add ability to charge fines at start of charge period Right now, Koha only charges fines at the end of a given charge period. For example, let us assume a circulation rule has a charge period of one week ( 7 days ) and a fine of $5. This means that an item can be overdue for 6 days without accruing a fine. Koha should allow circulation rules to be configured to place the charge at the start of the end of the charge period so the library can decide when the fine should accrue. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) prove t/db_dependent/Circulation_Issuingrule.t 4) prove t/db_dependent/Circulation.t 5) prove t/db_dependent/Fines.t 6) Ensure you can still create/edit circulation rules --- C4/Overdues.pm | 99 +++++++------ Koha/Schema/Result/Issuingrule.pm | 12 ++- admin/smart-rules.pl | 40 ++++-- installer/data/mysql/kohastructure.sql | 1 + installer/data/mysql/updatedatabase.pl | 8 + .../prog/en/modules/admin/smart-rules.tt | 8 + t/db_dependent/Circulation_Issuingrule.t | 153 ++++++++++---------- t/db_dependent/Fines.t | 56 +++++++ 8 files changed, 244 insertions(+), 133 deletions(-) create mode 100644 t/db_dependent/Fines.t diff --git a/C4/Overdues.pm b/C4/Overdues.pm index 1797923..3a50274 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -24,6 +24,8 @@ use strict; use Date::Calc qw/Today Date_to_Days/; use Date::Manip qw/UnixDate/; use List::MoreUtils qw( uniq ); +use POSIX qw( floor ceil ); +use Readonly; use C4::Circulation; use C4::Context; @@ -33,46 +35,51 @@ use C4::Debug; use vars qw($VERSION @ISA @EXPORT); +Readonly::Scalar our $INTERVAL_START => 1; +Readonly::Scalar our $INTERVAL_END => 0; + BEGIN { - # set the version for version checking + # set the version for version checking $VERSION = 3.07.00.049; - require Exporter; - @ISA = qw(Exporter); - # subs to rename (and maybe merge some...) - push @EXPORT, qw( - &CalcFine - &Getoverdues - &checkoverdues - &NumberNotifyId - &AmountNotify - &UpdateFine - &GetFine - - &CheckItemNotify - &GetOverduesForBranch - &RemoveNotifyLine - &AddNotifyLine - &GetOverdueMessageTransportTypes - ); - # subs to remove - push @EXPORT, qw( - &BorType - ); - - # check that an equivalent don't exist already before moving - - # subs to move to Circulation.pm - push @EXPORT, qw( - &GetIssuesIteminfo - ); - - # &GetIssuingRules - delete. - # use C4::Circulation::GetIssuingRule instead. - - # subs to move to Biblio.pm - push @EXPORT, qw( - &GetItems - ); + require Exporter; + @ISA = qw(Exporter); + + # subs to rename (and maybe merge some...) + push @EXPORT, qw( + &CalcFine + &Getoverdues + &checkoverdues + &NumberNotifyId + &AmountNotify + &UpdateFine + &GetFine + + &CheckItemNotify + &GetOverduesForBranch + &RemoveNotifyLine + &AddNotifyLine + &GetOverdueMessageTransportTypes + ); + + # subs to remove + push @EXPORT, qw( + &BorType + ); + + # check that an equivalent don't exist already before moving + + # subs to move to Circulation.pm + push @EXPORT, qw( + &GetIssuesIteminfo + ); + + # &GetIssuingRules - delete. + # use C4::Circulation::GetIssuingRule instead. + + # subs to move to Biblio.pm + push @EXPORT, qw( + &GetItems + ); } =head1 NAME @@ -251,15 +258,13 @@ sub CalcFine { my $chargeable_units = _get_chargeable_units($fine_unit, $start_date, $end_date, $branchcode); my $units_minus_grace = $chargeable_units - $data->{firstremind}; my $amount = 0; - if ($data->{'chargeperiod'} && ($units_minus_grace > 0) ) { - if ( C4::Context->preference('FinesIncludeGracePeriod') ) { - $amount = int($chargeable_units / $data->{'chargeperiod'}) * $data->{'fine'};# TODO fine calc should be in cents - } else { - $amount = int($units_minus_grace / $data->{'chargeperiod'}) * $data->{'fine'}; - } - } else { - # a zero (or null) chargeperiod or negative units_minus_grace value means no charge. - } + if ( $data->{'chargeperiod'} && ( $units_minus_grace > 0 ) ) { + my $units = C4::Context->preference('FinesIncludeGracePeriod') ? $chargeable_units : $units_minus_grace; + my $charge_periods = $units / $data->{'chargeperiod'}; + $charge_periods = $data->{'chargeperiod_charge_at'} == $INTERVAL_START ? ceil($charge_periods) : floor($charge_periods); + $amount = $charge_periods * $data->{'fine'}; + } # else { # a zero (or null) chargeperiod or negative units_minus_grace value means no charge. } + $amount = $data->{overduefinescap} if $data->{overduefinescap} && $amount > $data->{overduefinescap}; $debug and warn sprintf("CalcFine returning (%s, %s, %s, %s)", $amount, $data->{'chargename'}, $units_minus_grace, $chargeable_units); return ($amount, $data->{'chargename'}, $units_minus_grace, $chargeable_units); diff --git a/Koha/Schema/Result/Issuingrule.pm b/Koha/Schema/Result/Issuingrule.pm index a8cf014..2983ad3 100644 --- a/Koha/Schema/Result/Issuingrule.pm +++ b/Koha/Schema/Result/Issuingrule.pm @@ -80,6 +80,12 @@ __PACKAGE__->table("issuingrules"); data_type: 'integer' is_nullable: 1 +=head2 chargeperiod_charge_at + + data_type: 'tinyint' + default_value: 0 + is_nullable: 0 + =head2 accountsent data_type: 'integer' @@ -184,6 +190,8 @@ __PACKAGE__->add_columns( { data_type => "integer", is_nullable => 1 }, "chargeperiod", { data_type => "integer", is_nullable => 1 }, + "chargeperiod_charge_at", + { data_type => "tinyint", default_value => 0, is_nullable => 0 }, "accountsent", { data_type => "integer", is_nullable => 1 }, "chargename", @@ -236,8 +244,8 @@ __PACKAGE__->add_columns( __PACKAGE__->set_primary_key("branchcode", "categorycode", "itemtype"); -# Created by DBIx::Class::Schema::Loader v0.07039 @ 2014-09-17 21:06:58 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:vmlv13CeQ99RO0VsviRABg +# Created by DBIx::Class::Schema::Loader v0.07040 @ 2015-01-16 07:20:41 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:6AKNiL/+m3EyTjvAAOz9Cw # You can replace this text with custom content, and it will be preserved on regeneration diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index 942e9bf..63c9919 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -27,6 +27,7 @@ use C4::Koha; use C4::Debug; use C4::Branch; # GetBranches use C4::Dates qw/format_date format_date_in_iso/; +use Koha::Database; my $input = CGI->new; my $dbh = C4::Context->dbh; @@ -100,10 +101,6 @@ elsif ($op eq 'delete-branch-item') { } # save the values entered elsif ($op eq 'add') { - my $sth_search = $dbh->prepare('SELECT COUNT(*) AS total FROM issuingrules WHERE branchcode=? AND categorycode=? AND itemtype=?'); - my $sth_insert = $dbh->prepare('INSERT INTO issuingrules (branchcode, categorycode, itemtype, maxissueqty, renewalsallowed, renewalperiod, norenewalbefore, auto_renew, reservesallowed, issuelength, lengthunit, hardduedate, hardduedatecompare, fine, finedays, maxsuspensiondays, firstremind, chargeperiod,rentaldiscount, overduefinescap) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)'); - my $sth_update=$dbh->prepare("UPDATE issuingrules SET fine=?, finedays=?, maxsuspensiondays=?, firstremind=?, chargeperiod=?, maxissueqty=?, renewalsallowed=?, renewalperiod=?, norenewalbefore=?, auto_renew=?, reservesallowed=?, issuelength=?, lengthunit = ?, hardduedate=?, hardduedatecompare=?, rentaldiscount=?, overduefinescap=? WHERE branchcode=? AND categorycode=? AND itemtype=?"); - my $br = $branch; # branch my $bor = $input->param('categorycode'); # borrower category my $cat = $input->param('itemtype'); # item type @@ -113,6 +110,7 @@ elsif ($op eq 'add') { $maxsuspensiondays = undef if $maxsuspensiondays eq q||; my $firstremind = $input->param('firstremind'); my $chargeperiod = $input->param('chargeperiod'); + my $chargeperiod_charge_at = $input->param('chargeperiod_charge_at'); my $maxissueqty = $input->param('maxissueqty'); my $renewalsallowed = $input->param('renewalsallowed'); my $renewalperiod = $input->param('renewalperiod'); @@ -131,13 +129,33 @@ elsif ($op eq 'add') { my $overduefinescap = $input->param('overduefinescap') || undef; $debug and warn "Adding $br, $bor, $cat, $fine, $maxissueqty"; - $sth_search->execute($br,$bor,$cat); - my $res = $sth_search->fetchrow_hashref(); - if ($res->{total}) { - $sth_update->execute($fine, $finedays, $maxsuspensiondays, $firstremind, $chargeperiod, $maxissueqty, $renewalsallowed, $renewalperiod, $norenewalbefore, $auto_renew, $reservesallowed, $issuelength,$lengthunit, $hardduedate,$hardduedatecompare,$rentaldiscount,$overduefinescap, $br,$bor,$cat); - } else { - $sth_insert->execute($br,$bor,$cat,$maxissueqty,$renewalsallowed, $renewalperiod, $norenewalbefore, $auto_renew, $reservesallowed,$issuelength,$lengthunit,$hardduedate,$hardduedatecompare,$fine,$finedays, $maxsuspensiondays, $firstremind,$chargeperiod,$rentaldiscount,$overduefinescap); - } + my $rs = Koha::Database->new()->schema()->resultset('Issuingrule'); + + $rs->update_or_create( + { + branchcode => $br, + categorycode => $bor, + itemtype => $cat, + fine => $fine, + finedays => $finedays, + maxsuspensiondays => $maxsuspensiondays, + firstremind => $firstremind, + chargeperiod => $chargeperiod, + chargeperiod_charge_at => $chargeperiod_charge_at, + maxissueqty => $maxissueqty, + renewalsallowed => $renewalsallowed, + renewalperiod => $renewalperiod, + norenewalbefore => $norenewalbefore, + auto_renew => $auto_renew, + reservesallowed => $reservesallowed, + issuelength => $issuelength, + lengthunit => $lengthunit, + hardduedate => $hardduedate, + hardduedatecompare => $hardduedatecompare, + rentaldiscount => $rentaldiscount, + overduefinescap => $overduefinescap, + } + ); } elsif ($op eq "set-branch-defaults") { my $categorycode = $input->param('categorycode'); diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index facde44..8897960 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1178,6 +1178,7 @@ CREATE TABLE `issuingrules` ( -- circulation and fine rules `maxsuspensiondays` int(11) default NULL, -- max suspension days `firstremind` int(11) default NULL, -- fine grace period `chargeperiod` int(11) default NULL, -- how often the fine amount is charged + `chargeperiod_charge_at` tinyint(1) NOT NULL DEFAULT '0', -- Should fine be given at the start ( 1 ) or the end ( 0 ) of the period `accountsent` int(11) default NULL, -- not used? always NULL `chargename` varchar(100) default NULL, -- not used? always NULL `maxissueqty` int(4) default NULL, -- total number of checkouts allowed diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 4653c02..0c56701 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -9669,6 +9669,14 @@ if ( CheckVersion($DBversion) ) { SetVersion ($DBversion); } +$DBversion = "3.19.00.XXX"; +if ( CheckVersion($DBversion) ) { + $dbh->do("ALTER TABLE issuingrules ADD chargeperiod_charge_at BOOLEAN NOT NULL DEFAULT '0' AFTER chargeperiod"); + + print "Upgrade to $DBversion done (Bug 11944 - Convert DB tables to utf8_unicode_ci)\n"; + SetVersion ($DBversion); +} + =head1 FUNCTIONS =head2 TableExists($table) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt index 8bc5809..3ed1674 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt @@ -147,6 +147,7 @@ for="tobranch">Clone these rules to: Clone these rules to: [% rule.finedays %] @@ -261,6 +263,12 @@ for="tobranch">Clone these rules to: + + + diff --git a/t/db_dependent/Circulation_Issuingrule.t b/t/db_dependent/Circulation_Issuingrule.t index 6e4f44f..5f6623e 100644 --- a/t/db_dependent/Circulation_Issuingrule.t +++ b/t/db_dependent/Circulation_Issuingrule.t @@ -108,82 +108,85 @@ $dbh->do( #Test GetIssuingRule my $sampleissuingrule1 = { - reservecharge => '0.000000', - chargename => 'Null', - restrictedtype => 0, - accountsent => 0, - maxissueqty => 5, - finedays => 0, - lengthunit => 'Null', - renewalperiod => 5, - norenewalbefore => 6, - auto_renew => 0, - issuelength => 5, - chargeperiod => 0, - rentaldiscount => '2.000000', - reservesallowed => 0, - hardduedate => '2013-01-01', - branchcode => $samplebranch1->{branchcode}, - fine => '0.000000', - hardduedatecompare => 5, - overduefinescap => '0.000000', - renewalsallowed => 0, - firstremind => 0, - itemtype => 'BOOK', - categorycode => $samplecat->{categorycode}, - maxsuspensiondays => 0, + reservecharge => '0.000000', + chargename => 'Null', + restrictedtype => 0, + accountsent => 0, + maxissueqty => 5, + finedays => 0, + lengthunit => 'Null', + renewalperiod => 5, + norenewalbefore => 6, + auto_renew => 0, + issuelength => 5, + chargeperiod => 0, + chargeperiod_charge_at => 0, + rentaldiscount => '2.000000', + reservesallowed => 0, + hardduedate => '2013-01-01', + branchcode => $samplebranch1->{branchcode}, + fine => '0.000000', + hardduedatecompare => 5, + overduefinescap => '0.000000', + renewalsallowed => 0, + firstremind => 0, + itemtype => 'BOOK', + categorycode => $samplecat->{categorycode}, + maxsuspensiondays => 0, }; my $sampleissuingrule2 = { - branchcode => $samplebranch2->{branchcode}, - categorycode => $samplecat->{categorycode}, - itemtype => 'BOOK', - maxissueqty => 2, - renewalsallowed => 'Null', - renewalperiod => 2, - norenewalbefore => 7, - auto_renew => 0, - reservesallowed => 'Null', - issuelength => 2, - lengthunit => 'Null', - hardduedate => 2, - hardduedatecompare => 'Null', - fine => 'Null', - finedays => 'Null', - firstremind => 'Null', - chargeperiod => 'Null', - rentaldiscount => 2.00, - overduefinescap => 'Null', - accountsent => 'Null', - reservecharge => 'Null', - chargename => 'Null', - restrictedtype => 'Null', - maxsuspensiondays => 0, + branchcode => $samplebranch2->{branchcode}, + categorycode => $samplecat->{categorycode}, + itemtype => 'BOOK', + maxissueqty => 2, + renewalsallowed => 'Null', + renewalperiod => 2, + norenewalbefore => 7, + auto_renew => 0, + reservesallowed => 'Null', + issuelength => 2, + lengthunit => 'Null', + hardduedate => 2, + hardduedatecompare => 'Null', + fine => 'Null', + finedays => 'Null', + firstremind => 'Null', + chargeperiod => 'Null', + chargeperiod_charge_at => 0, + rentaldiscount => 2.00, + overduefinescap => 'Null', + accountsent => 'Null', + reservecharge => 'Null', + chargename => 'Null', + restrictedtype => 'Null', + maxsuspensiondays => 0, }; my $sampleissuingrule3 = { - branchcode => $samplebranch1->{branchcode}, - categorycode => $samplecat->{categorycode}, - itemtype => 'DVD', - maxissueqty => 3, - renewalsallowed => 'Null', - renewalperiod => 3, - norenewalbefore => 8, - auto_renew => 0, - reservesallowed => 'Null', - issuelength => 3, - lengthunit => 'Null', - hardduedate => 3, - hardduedatecompare => 'Null', - fine => 'Null', - finedays => 'Null', - firstremind => 'Null', - chargeperiod => 'Null', - rentaldiscount => 3.00, - overduefinescap => 'Null', - accountsent => 'Null', - reservecharge => 'Null', - chargename => 'Null', - restrictedtype => 'Null', - maxsuspensiondays => 0, + branchcode => $samplebranch1->{branchcode}, + categorycode => $samplecat->{categorycode}, + itemtype => 'DVD', + maxissueqty => 3, + renewalsallowed => 'Null', + renewalperiod => 3, + norenewalbefore => 8, + auto_renew => 0, + reservesallowed => 'Null', + issuelength => 3, + lengthunit => 'Null', + hardduedate => 3, + hardduedatecompare => 'Null', + fine => 'Null', + finedays => 'Null', + firstremind => 'Null', + chargeperiod => 'Null', + chargeperiod_charge_at => 0, + rentaldiscount => 3.00, + overduefinescap => 'Null', + accountsent => 'Null', + reservecharge => 'Null', + chargename => 'Null', + restrictedtype => 'Null', + maxsuspensiondays => 0, }; $query = 'INSERT INTO issuingrules ( branchcode, @@ -203,6 +206,7 @@ $query = 'INSERT INTO issuingrules ( finedays, firstremind, chargeperiod, + chargeperiod_charge_at, rentaldiscount, overduefinescap, accountsent, @@ -210,7 +214,7 @@ $query = 'INSERT INTO issuingrules ( chargename, restrictedtype, maxsuspensiondays - ) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)'; + ) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)'; my $sth = $dbh->prepare($query); $sth->execute( $sampleissuingrule1->{branchcode}, @@ -230,6 +234,7 @@ $sth->execute( $sampleissuingrule1->{finedays}, $sampleissuingrule1->{firstremind}, $sampleissuingrule1->{chargeperiod}, + $sampleissuingrule1->{chargeperiod_charge_at}, $sampleissuingrule1->{rentaldiscount}, $sampleissuingrule1->{overduefinescap}, $sampleissuingrule1->{accountsent}, @@ -256,6 +261,7 @@ $sth->execute( $sampleissuingrule2->{finedays}, $sampleissuingrule2->{firstremind}, $sampleissuingrule2->{chargeperiod}, + $sampleissuingrule2->{chargeperiod_charge_at}, $sampleissuingrule2->{rentaldiscount}, $sampleissuingrule2->{overduefinescap}, $sampleissuingrule2->{accountsent}, @@ -282,6 +288,7 @@ $sth->execute( $sampleissuingrule3->{finedays}, $sampleissuingrule3->{firstremind}, $sampleissuingrule3->{chargeperiod}, + $sampleissuingrule3->{chargeperiod_charge_at}, $sampleissuingrule3->{rentaldiscount}, $sampleissuingrule3->{overduefinescap}, $sampleissuingrule3->{accountsent}, diff --git a/t/db_dependent/Fines.t b/t/db_dependent/Fines.t new file mode 100644 index 0000000..1f273ba --- /dev/null +++ b/t/db_dependent/Fines.t @@ -0,0 +1,56 @@ +#!/usr/bin/perl + +use Modern::Perl; + +use C4::Context; +use C4::Overdues; +use Koha::Database; +use Koha::DateUtils; + +use Test::More tests => 5; + +#Start transaction +my $dbh = C4::Context->dbh; +my $schema = Koha::Database->new()->schema(); + +$dbh->{RaiseError} = 1; +$dbh->{AutoCommit} = 0; + +$dbh->do(q|DELETE FROM issuingrules|); + +my $issuingrule = $schema->resultset('Issuingrule')->create( + { + categorycode => '*', + itemtype => '*', + branchcode => '*', + fine => 1, + finedays => 0, + chargeperiod => 7, + chargeperiod_charge_at => 0, + lengthunit => 'days', + issuelength => 1, + } +); + +ok( $issuingrule, 'Issuing rule created' ); + +my $period_start = dt_from_string('2000-01-01'); +my $period_end = dt_from_string('2000-01-05'); + +my ( $fine ) = CalcFine( {}, q{}, q{}, $period_start, $period_end ); +is( $fine, 0, '4 days overdue, charge period 7 days, charge at end of interval gives fine of $0' ); + +$period_end = dt_from_string('2000-01-10'); +( $fine ) = CalcFine( {}, q{}, q{}, $period_start, $period_end ); +is( $fine, 1, '9 days overdue, charge period 7 days, charge at end of interval gives fine of $1' ); + +# Test charging fine at the *beginning* of each charge period +$issuingrule->update( { chargeperiod_charge_at => 1 } ); + +$period_end = dt_from_string('2000-01-05'); +( $fine ) = CalcFine( {}, q{}, q{}, $period_start, $period_end ); +is( $fine, 1, '4 days overdue, charge period 7 days, charge at start of interval gives fine of $1' ); + +$period_end = dt_from_string('2000-01-10'); +( $fine ) = CalcFine( {}, q{}, q{}, $period_start, $period_end ); +is( $fine, 2, '9 days overdue, charge period 7 days, charge at start of interval gives fine of $2' ); -- 1.7.2.5