From ad26a130e477f6625bf55b592cacd682329fab38 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Wed, 12 Feb 2020 12:55:51 -0500 Subject: [PATCH] Bug 20815: Add ability to choose if lost fee is refunded based on length of time item has been lost This adds the ability to not refund lost item fees on return if the item has been lost for more than a given number of days. Test Plan: 1) Set the new system preference NoRefundOnLostReturnedItemsAge to a number of days 2) Find a lost item that has been lost longer than that NoRefundOnLostReturnedItemsAge days which would have otherwise been refunded 3) Return the item 4) Note no refund on the lost item fee was processed, the fee remains unchanged 5) prove t/db_dependent/Circulation.t --- C4/Circulation.pm | 32 +- .../data/mysql/atomicupdate/bug_20815.perl | 10 + installer/data/mysql/sysprefs.sql | 1 + .../admin/preferences/circulation.pref | 5 + t/db_dependent/Circulation.t | 302 +++++++++++++++++- 5 files changed, 346 insertions(+), 4 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_20815.perl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index dc12cd3352..742afdc881 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1445,11 +1445,24 @@ sub AddIssue { ## If item was lost, it has now been found, reverse any list item charges if necessary. if ( $item_object->itemlost ) { + my $refund = 1; + my $no_refund_after_days = C4::Context->preference('NoRefundOnLostReturnedItemsAge'); + if ($no_refund_after_days) { + my $today = dt_from_string(); + my $lost_age_in_days = + dt_from_string( $item_object->itemlost_on ) + ->delta_days($today) + ->in_units('days'); + + $refund = 0 unless ( $lost_age_in_days < $no_refund_after_days ); + } + if ( - Koha::RefundLostItemFeeRules->should_refund( + $refund + && Koha::RefundLostItemFeeRules->should_refund( { - current_branch => C4::Context->userenv->{branch}, - item_home_branch => $item_object->homebranch, + current_branch => C4::Context->userenv->{branch}, + item_home_branch => $item_object->homebranch, item_holding_branch => $item_object->holdingbranch, } ) @@ -2027,7 +2040,20 @@ sub AddReturn { if ( $item->itemlost ) { $messages->{'WasLost'} = 1; unless ( C4::Context->preference("BlockReturnOfLostItems") ) { + my $refund = 1; + my $no_refund_after_days = C4::Context->preference('NoRefundOnLostReturnedItemsAge'); + if ($no_refund_after_days) { + my $today = dt_from_string(); + my $lost_age_in_days = + dt_from_string( $item->itemlost_on ) + ->delta_days($today) + ->in_units('days'); + + $refund = 0 unless ( $lost_age_in_days < $no_refund_after_days ); + } + if ( + $refund && Koha::RefundLostItemFeeRules->should_refund( { current_branch => C4::Context->userenv->{branch}, diff --git a/installer/data/mysql/atomicupdate/bug_20815.perl b/installer/data/mysql/atomicupdate/bug_20815.perl new file mode 100644 index 0000000000..50c7ef54ce --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_20815.perl @@ -0,0 +1,10 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + $dbh->do(q{ + INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES + ('NoRefundOnLostReturnedItemsAge','','','Do not refund lost item fees if item is lost for more than this number of days','Integer') + }); + + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug XXXXX - description)\n"; +} diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index cfa295e197..410a212252 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -327,6 +327,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('noissuescharge','5','','Define maximum amount withstanding before check outs are blocked','Integer'), ('NoIssuesChargeGuarantees','','','Define maximum amount withstanding before check outs are blocked','Integer'), ('noItemTypeImages','0',NULL,'If ON, disables item-type images','YesNo'), +('NoRefundOnLostReturnedItemsAge','','','Do not refund lost item fees if item is lost for more than this number of days','Integer'), ('NoRenewalBeforePrecision','exact_time','date|exact_time','Calculate "No renewal before" based on date only or exact time of due date','Choice'), ('NotesBlacklist','',NULL,'List of notes fields that should not appear in the title notes/description separator of details','free'), ('NotHighlightedWords','and|or|not',NULL,'List of words to NOT highlight when OpacHitHighlight is enabled','free'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 6eb5af0660..a86b02a499 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -878,6 +878,11 @@ Circulation: yes: Forgive no: "Don't Forgive" - the fines on an item when it is lost. + - + - "Don't refund lost fees if a lost item is returned more than" + - pref: NoRefundOnLostReturnedItemsAge + class: integer + - days after it was marked lost. - - pref: WhenLostChargeReplacementFee choices: diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 08822d765c..bcac1a987b 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -18,7 +18,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 46; +use Test::More tests => 50; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -3721,6 +3721,306 @@ subtest "Test Backdating of Returns" => sub { is( $accountline->amount+0, 0, 'Fee amount was reduced to 0' ); }; +subtest 'Tests for NoRefundOnLostReturnedItemsAge = undef' => sub { + plan tests => 3; + + t::lib::Mocks::mock_preference( 'WhenLostChargeReplacementFee', 1 ); + t::lib::Mocks::mock_preference( 'NoRefundOnLostReturnedItemsAge', undef ); + + my $lost_on = dt_from_string->subtract( days => 7 )->date; + + my $library = $builder->build( { source => 'Branch' } ); + my $patron = $builder->build( + { + source => 'Borrower', + value => { categorycode => $patron_category->{categorycode} } + } + ); + + my $biblionumber = $builder->build_sample_biblio( + { + branchcode => $library->{branchcode}, + } + )->biblionumber; + my $item = $builder->build_sample_item( + { + biblionumber => $biblionumber, + library => $library->{branchcode}, + replacementprice => '42.00', + } + ); + + # And the circulation rule + Koha::CirculationRules->search->delete; + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { + issuelength => 14, + lengthunit => 'days', + } + } + ); + $builder->build( + { + source => 'CirculationRule', + value => { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'refund', + rule_value => 1 + } + } + ); + + # Test with NoRefundOnLostReturnedItemsAge disabled + my $issue = AddIssue( $patron, $item->barcode ); + LostItem( $item->itemnumber, 'cli', 0 ); + $item->itemlost(1); + $item->itemlost_on( $lost_on ); + $item->store(); + + my $a = Koha::Account::Lines->find( + { + itemnumber => $item->id, + borrowernumber => $patron->{borrowernumber} + } + ); + ok( $a, "Found accountline for lost fee" ); + is( $a->amountoutstanding, '42.000000', "Lost fee charged correctly" ); + my ( $doreturn, $messages ) = AddReturn( $item->barcode, $library->{branchcode}, undef, dt_from_string ); + $a = Koha::Account::Lines->find( $a->id ); + is( $a->amountoutstanding, '0.000000', "Lost fee was refunded" ); +}; + +subtest 'Tests for NoRefundOnLostReturnedItemsAge > length of days item has been lost' => sub { + plan tests => 3; + + t::lib::Mocks::mock_preference( 'WhenLostChargeReplacementFee', 1 ); + t::lib::Mocks::mock_preference( 'NoRefundOnLostReturnedItemsAge', 7 ); + + my $lost_on = dt_from_string->subtract( days => 6 )->date; + + my $library = $builder->build( { source => 'Branch' } ); + my $patron = $builder->build( + { + source => 'Borrower', + value => { categorycode => $patron_category->{categorycode} } + } + ); + + my $biblionumber = $builder->build_sample_biblio( + { + branchcode => $library->{branchcode}, + } + )->biblionumber; + my $item = $builder->build_sample_item( + { + biblionumber => $biblionumber, + library => $library->{branchcode}, + replacementprice => '42.00', + } + ); + + # And the circulation rule + Koha::CirculationRules->search->delete; + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { + issuelength => 14, + lengthunit => 'days', + } + } + ); + $builder->build( + { + source => 'CirculationRule', + value => { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'refund', + rule_value => 1 + } + } + ); + + # Test with NoRefundOnLostReturnedItemsAge disabled + my $issue = AddIssue( $patron, $item->barcode ); + LostItem( $item->itemnumber, 'cli', 0 ); + $item->itemlost(1); + $item->itemlost_on( $lost_on ); + $item->store(); + + my $a = Koha::Account::Lines->find( + { + itemnumber => $item->id, + borrowernumber => $patron->{borrowernumber} + } + ); + ok( $a, "Found accountline for lost fee" ); + is( $a->amountoutstanding, '42.000000', "Lost fee charged correctly" ); + my ( $doreturn, $messages ) = AddReturn( $item->barcode, $library->{branchcode}, undef, dt_from_string ); + $a = Koha::Account::Lines->find( $a->id ); + is( $a->amountoutstanding, '0.000000', "Lost fee was refunded" ); +}; + +subtest 'Tests for NoRefundOnLostReturnedItemsAge = length of days item has been lost' => sub { + plan tests => 3; + + t::lib::Mocks::mock_preference( 'WhenLostChargeReplacementFee', 1 ); + t::lib::Mocks::mock_preference( 'NoRefundOnLostReturnedItemsAge', 7 ); + + my $lost_on = dt_from_string->subtract( days => 7 )->date; + + my $library = $builder->build( { source => 'Branch' } ); + my $patron = $builder->build( + { + source => 'Borrower', + value => { categorycode => $patron_category->{categorycode} } + } + ); + + my $biblionumber = $builder->build_sample_biblio( + { + branchcode => $library->{branchcode}, + } + )->biblionumber; + my $item = $builder->build_sample_item( + { + biblionumber => $biblionumber, + library => $library->{branchcode}, + replacementprice => '42.00', + } + ); + + # And the circulation rule + Koha::CirculationRules->search->delete; + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { + issuelength => 14, + lengthunit => 'days', + } + } + ); + $builder->build( + { + source => 'CirculationRule', + value => { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'refund', + rule_value => 1 + } + } + ); + + # Test with NoRefundOnLostReturnedItemsAge disabled + my $issue = AddIssue( $patron, $item->barcode ); + LostItem( $item->itemnumber, 'cli', 0 ); + $item->itemlost(1); + $item->itemlost_on( $lost_on ); + $item->store(); + + my $a = Koha::Account::Lines->find( + { + itemnumber => $item->id, + borrowernumber => $patron->{borrowernumber} + } + ); + ok( $a, "Found accountline for lost fee" ); + is( $a->amountoutstanding, '42.000000', "Lost fee charged correctly" ); + my ( $doreturn, $messages ) = AddReturn( $item->barcode, $library->{branchcode}, undef, dt_from_string ); + $a = Koha::Account::Lines->find( $a->id ); + is( $a->amountoutstanding, '42.000000', "Lost fee was not refunded" ); +}; + +subtest 'Tests for NoRefundOnLostReturnedItemsAge < length of days item has been lost' => sub { + plan tests => 3; + + t::lib::Mocks::mock_preference( 'WhenLostChargeReplacementFee', 1 ); + t::lib::Mocks::mock_preference( 'NoRefundOnLostReturnedItemsAge', 7 ); + + my $lost_on = dt_from_string->subtract( days => 8 )->date; + + my $library = $builder->build( { source => 'Branch' } ); + my $patron = $builder->build( + { + source => 'Borrower', + value => { categorycode => $patron_category->{categorycode} } + } + ); + + my $biblionumber = $builder->build_sample_biblio( + { + branchcode => $library->{branchcode}, + } + )->biblionumber; + my $item = $builder->build_sample_item( + { + biblionumber => $biblionumber, + library => $library->{branchcode}, + replacementprice => '42.00', + } + ); + + # And the circulation rule + Koha::CirculationRules->search->delete; + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { + issuelength => 14, + lengthunit => 'days', + } + } + ); + $builder->build( + { + source => 'CirculationRule', + value => { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'refund', + rule_value => 1 + } + } + ); + + # Test with NoRefundOnLostReturnedItemsAge disabled + my $issue = AddIssue( $patron, $item->barcode ); + LostItem( $item->itemnumber, 'cli', 0 ); + $item->itemlost(1); + $item->itemlost_on( $lost_on ); + $item->store(); + + my $a = Koha::Account::Lines->find( + { + itemnumber => $item->id, + borrowernumber => $patron->{borrowernumber} + } + ); + ok( $a, "Found accountline for lost fee" ); + is( $a->amountoutstanding, '42.000000', "Lost fee charged correctly" ); + my ( $doreturn, $messages ) = AddReturn( $item->barcode, $library->{branchcode}, undef, dt_from_string ); + $a = Koha::Account::Lines->find( $a->id ); + is( $a->amountoutstanding, '42.000000', "Lost fee was not refunded" ); +}; + $schema->storage->txn_rollback; C4::Context->clear_syspref_cache(); $cache->clear_from_cache('single_holidays'); -- 2.21.1 (Apple Git-122.3)