From e453f74cfd2358ea795f2f0677946fba75ad7841 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 Signed-off-by: Deb Stephenson Signed-off-by: Katrin Fischer Signed-off-by: Jonathan Druart --- 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 4fa208f0ab..34fe73583f 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1456,11 +1456,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, } ) @@ -2040,7 +2053,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 e73912dfc7..ded953724a 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -333,6 +333,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 itemtype images in the staff interface','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 b89fd8fad7..29c076d729 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 @@ -941,6 +941,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 bd0b9a29cc..2be657dbd4 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 => 47; +use Test::More tests => 51; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -4062,6 +4062,306 @@ subtest 'Filling a hold should cancel existing transfer' => sub { is( Koha::Item::Transfers->search({ itemnumber => $item->itemnumber, datearrived => undef })->count, 0, "No outstanding transfers when hold is waiting"); }; +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->_result->itemlost(1); + $item->_result->itemlost_on( $lost_on ); + $item->_result->update(); + + 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->_result->itemlost(1); + $item->_result->itemlost_on( $lost_on ); + $item->_result->update(); + + 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->_result->itemlost(1); + $item->_result->itemlost_on( $lost_on ); + $item->_result->update(); + + 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->_result->itemlost(1); + $item->_result->itemlost_on( $lost_on ); + $item->_result->update(); + + 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(); $branches = Koha::Libraries->search(); -- 2.20.1