From 389de864282006b41b8db06621f17990a8f68991 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 3 Jun 2020 14:51:42 +0100 Subject: [PATCH] Bug 25663: Remove Koha::RefundLostItemFeeRule and uses This patch replaces all calls to RefundLostItemFeeRules with Koha::CirculationRules->get_lostreturn_policy and removes the module it makes redundant. Test plan 1/ Confirm that there are no longer any uses of RefundLostItemFeeRules in the codebase 2/ Confirm circulation tests still all pass 3/ Confirm you can still set and unset the lost return rules 4/ Signoff --- C4/Circulation.pm | 17 +- Koha/RefundLostItemFeeRules.pm | 179 ---------- admin/smart-rules.pl | 6 +- t/db_dependent/RefundLostItemFeeRule.t | 459 ------------------------- t/lib/TestBuilder.pm | 5 +- 5 files changed, 11 insertions(+), 655 deletions(-) delete mode 100644 Koha/RefundLostItemFeeRules.pm delete mode 100755 t/db_dependent/RefundLostItemFeeRule.t diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 7a3b21b903..a0fde11397 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -52,7 +52,6 @@ use Koha::Database; use Koha::Libraries; use Koha::Account::Lines; use Koha::Holds; -use Koha::RefundLostItemFeeRules; use Koha::Account::Lines; use Koha::Account::Offsets; use Koha::Config::SysPrefs; @@ -1450,11 +1449,10 @@ sub AddIssue { ## If item was lost, it has now been found, reverse any list item charges if necessary. if ( $item_object->itemlost ) { if ( - Koha::RefundLostItemFeeRules->should_refund( + Koha::CirculationRules->get_lostreturn_policy( { - current_branch => C4::Context->userenv->{branch}, - item_home_branch => $item_object->homebranch, - item_holding_branch => $item_object->holdingbranch, + return_branch => C4::Context->userenv->{branch}, + item => $item_object } ) ) @@ -2034,13 +2032,12 @@ sub AddReturn { $messages->{'WasLost'} = 1; unless ( C4::Context->preference("BlockReturnOfLostItems") ) { if ( - Koha::RefundLostItemFeeRules->should_refund( + Koha::CirculationRules->get_lostreturn_policy( { - current_branch => C4::Context->userenv->{branch}, - item_home_branch => $item->homebranch, - item_holding_branch => $item_holding_branch + return_branch => C4::Context->userenv->{branch}, + item => $item, } - ) + ) ) { _FixAccountForLostAndFound( $item->itemnumber, diff --git a/Koha/RefundLostItemFeeRules.pm b/Koha/RefundLostItemFeeRules.pm deleted file mode 100644 index 9cb32d55f5..0000000000 --- a/Koha/RefundLostItemFeeRules.pm +++ /dev/null @@ -1,179 +0,0 @@ -package Koha::RefundLostItemFeeRules; - -# Copyright Theke Solutions 2016 -# -# This file is part of Koha. -# -# Koha is free software; you can redistribute it and/or modify it -# under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 3 of the License, or -# (at your option) any later version. -# -# Koha is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Koha; if not, see . - -use Modern::Perl; - -use Koha::Database; -use Koha::Exceptions; - -use Koha::CirculationRule; - -use base qw(Koha::CirculationRules); - -=head1 NAME - -Koha::RefundLostItemFeeRules - Koha RefundLostItemFeeRules object set class - -=head1 API - -=head2 Class Methods - -=cut - -=head3 _type - -=cut - -sub _type { - return 'CirculationRule'; -} - -=head3 object_class - -=cut - -sub object_class { - return 'Koha::CirculationRule'; -} - -=head3 should_refund - -Koha::RefundLostItemFeeRules->should_refund() - -Returns a boolean telling if the fee needs to be refund given the -passed params, and the current rules/sysprefs configuration. - -=cut - -sub should_refund { - - my $self = shift; - my $params = shift; - - return $self->_effective_branch_rule( $self->_choose_branch( $params ) ); -} - - -=head3 _effective_branch_rule - -Koha::RefundLostItemFeeRules->_effective_branch_rule - -Given a branch, returns a boolean representing the resulting rule. -It tries the branch-specific first. Then falls back to the defined default. - -=cut - -sub _effective_branch_rule { - - my $self = shift; - my $branch = shift; - - my $specific_rule = $self->search( - { - branchcode => $branch, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - )->next(); - - return ( defined $specific_rule ) - ? $specific_rule->rule_value - : $self->_default_rule; -} - -=head3 _choose_branch - -my $branch = Koha::RefundLostItemFeeRules->_choose_branch({ - current_branch => 'current_branch_code', - item_home_branch => 'item_home_branch', - item_holding_branch => 'item_holding_branch' -}); - -Helper function that determines the branch to be used to apply the rule. - -=cut - -sub _choose_branch { - - my $self = shift; - my $param = shift; - - my $behaviour = C4::Context->preference( 'RefundLostOnReturnControl' ) // 'CheckinLibrary'; - - my $param_mapping = { - CheckinLibrary => 'current_branch', - ItemHomeBranch => 'item_home_branch', - ItemHoldingBranch => 'item_holding_branch' - }; - - my $branch = $param->{ $param_mapping->{ $behaviour } }; - - if ( !defined $branch ) { - Koha::Exceptions::MissingParameter->throw( - "$behaviour requires the " . - $param_mapping->{ $behaviour } . - " param" - ); - } - - return $branch; -} - -=head3 Koha::RefundLostItemFeeRules->find(); - -Inherit from Koha::Objects->find(), but forces rule_name => 'refund' - -=cut - -sub find { - my ( $self, @pars ) = @_; - - if ( ref($pars[0]) eq 'HASH' ) { - $pars[0]->{rule_name} = 'refund'; - } - - return $self->SUPER::find(@pars); -} - -=head3 _default_rule (internal) - -This function returns the default rule defined for refunding lost -item fees on return. It defaults to 1 if no rule is defined. - -=cut - -sub _default_rule { - - my $self = shift; - my $default_rule = $self->search( - { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - )->next(); - - return (defined $default_rule) - ? $default_rule->rule_value - : 1; -} - -1; diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index 195ca818af..d72599faae 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -27,7 +27,6 @@ use C4::Debug; use Koha::DateUtils; use Koha::Database; use Koha::Logger; -use Koha::RefundLostItemFeeRules; use Koha::Libraries; use Koha::CirculationRules; use Koha::Patron::Categories; @@ -549,10 +548,11 @@ elsif ( $op eq 'mod-refund-lost-item-fee-rule' ) { } } -my $refundLostItemFeeRule = Koha::RefundLostItemFeeRules->find({ branchcode => ($branch eq '*') ? undef : $branch }); +my $refundLostItemFeeRule = Koha::CirculationRules->find({ branchcode => ($branch eq '*') ? undef : $branch, rule_name => 'refund' }); +my $defaultLostItemFeeRule = Koha::CirculationRules->find({ branchcode => undef, rule_name => 'refund' }); $template->param( refundLostItemFeeRule => $refundLostItemFeeRule, - defaultRefundRule => Koha::RefundLostItemFeeRules->_default_rule + defaultRefundRule => $defaultLostItemFeeRule ? $defaultLostItemFeeRule->rule_value : 1 ); my $patron_categories = Koha::Patron::Categories->search({}, { order_by => ['description'] }); diff --git a/t/db_dependent/RefundLostItemFeeRule.t b/t/db_dependent/RefundLostItemFeeRule.t deleted file mode 100755 index da53f3ab1b..0000000000 --- a/t/db_dependent/RefundLostItemFeeRule.t +++ /dev/null @@ -1,459 +0,0 @@ -#!/usr/bin/perl - -# This file is part of Koha. -# -# Koha is free software; you can redistribute it and/or modify it -# under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 3 of the License, or -# (at your option) any later version. -# -# Koha is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Koha; if not, see . - -use Modern::Perl; - -use Test::More tests => 9; -use t::lib::Mocks; -use t::lib::TestBuilder; - -use C4::Context; -use Koha::Database; - -BEGIN { - use_ok('Koha::Object'); - use_ok('Koha::CirculationRule'); - use_ok('Koha::RefundLostItemFeeRules'); -} - -my $schema = Koha::Database->new->schema; -my $builder = t::lib::TestBuilder->new; - -subtest 'Koha::RefundLostItemFeeRule::delete() tests' => sub { - - plan tests => 5; - - # Start transaction - $schema->storage->txn_begin; - - # Clean the table - $schema->resultset('CirculationRule')->search()->delete; - - my $generated_default_rule = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - } - ); - my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $generated_other_rule = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => $branchcode, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - } - ); - - my $default_rule = Koha::CirculationRules->search( - { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - )->next(); - ok( defined $default_rule, 'Default rule created' ); - ok( $default_rule->_result->in_storage, 'Default rule actually in storage'); - - my $other_rule = Koha::CirculationRules->search( - { - branchcode => $generated_other_rule->{branchcode}, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - )->next(); - ok( defined $other_rule, 'Other rule created' ); - ok( $other_rule->_result->in_storage, 'Other rule actually in storage'); - - # deleting the regular rule - $other_rule->delete; - ok( !$other_rule->_result->in_storage, 'Other rule deleted from storage' ); - - # Rollback transaction - $schema->storage->txn_rollback; -}; - -subtest 'Koha::RefundLostItemFeeRules::_default_rule() tests' => sub { - - plan tests => 6; - - # Start transaction - $schema->storage->txn_begin; - - # Clean the table - $schema->resultset('CirculationRule')->search()->delete; - - my $generated_default_rule = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 1, - } - } - ); - my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $generated_other_rule = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => $branchcode, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - } - ); - - my $default_rule = Koha::CirculationRules->search( - { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - )->next(); - ok( defined $default_rule, 'Default rule created' ); - ok( $default_rule->_result->in_storage, 'Default rule actually in storage'); - is( Koha::RefundLostItemFeeRules->_default_rule, 1, 'Default rule is set to refund' ); - - # Change default rule to "Don't refund" - $default_rule->rule_value(0); - $default_rule->store; - # Re-read from DB, to be sure - $default_rule = Koha::CirculationRules->search( - { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - )->next(); - ok( !Koha::RefundLostItemFeeRules->_default_rule, 'Default rule is set to not refund' ); - - $default_rule->delete; - ok( !$default_rule->_result->in_storage, 'Default rule effectively deleted from storage' ); - - ok( Koha::RefundLostItemFeeRules->_default_rule, 'Default rule is set to refund if no default rule is present' ); - - # Rollback transaction - $schema->storage->txn_rollback; -}; - -subtest 'Koha::RefundLostItemFeeRules::_effective_branch_rule() tests' => sub { - - plan tests => 3; - - # Start transaction - $schema->storage->txn_begin; - - # Clean the table - $schema->resultset('CirculationRule')->search()->delete; - - my $default_rule = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 1, - } - } - ); - my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_false = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => $branchcode, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 0, - } - } - ); - my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_true = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => $branchcode2, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 1, - } - } - ); - - is( Koha::RefundLostItemFeeRules->_effective_branch_rule( $specific_rule_true->{ branchcode } ), - 1,'Specific rule is applied (true)'); - is( Koha::RefundLostItemFeeRules->_effective_branch_rule( $specific_rule_false->{ branchcode } ), - 0,'Specific rule is applied (false)'); - # Delete specific rules - Koha::RefundLostItemFeeRules->find({ branchcode => $specific_rule_false->{ branchcode } })->delete; - is( Koha::RefundLostItemFeeRules->_effective_branch_rule( $specific_rule_false->{ branchcode } ), - 1,'No specific rule defined, fallback to global (true)'); - - # Rollback transaction - $schema->storage->txn_rollback; -}; - -subtest 'Koha::RefundLostItemFeeRules::_choose_branch() tests' => sub { - - plan tests => 9; - - # Start transaction - $schema->storage->txn_begin; - - my $params = { - current_branch => 'current_branch_code', - item_holding_branch => 'item_holding_branch_code', - item_home_branch => 'item_home_branch_code' - }; - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); - - is( Koha::RefundLostItemFeeRules->_choose_branch( $params ), - 'current_branch_code', 'CheckinLibrary is honoured'); - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHomeBranch' ); - is( Koha::RefundLostItemFeeRules->_choose_branch( $params ), - 'item_home_branch_code', 'ItemHomeBranch is honoured'); - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHoldingBranch' ); - is( Koha::RefundLostItemFeeRules->_choose_branch( $params ), - 'item_holding_branch_code', 'ItemHoldingBranch is honoured'); - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); - eval { - Koha::RefundLostItemFeeRules->_choose_branch(); - }; - is( ref($@), 'Koha::Exceptions::MissingParameter', - 'Missing parameter exception' ); - is( $@->message, 'CheckinLibrary requires the current_branch param', - 'Exception message is correct' ); - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHomeBranch' ); - eval { - Koha::RefundLostItemFeeRules->_choose_branch(); - }; - is( ref($@), 'Koha::Exceptions::MissingParameter', - 'Missing parameter exception' ); - is( $@->message, 'ItemHomeBranch requires the item_home_branch param', - 'Exception message is correct' ); - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHoldingBranch' ); - eval { - Koha::RefundLostItemFeeRules->_choose_branch(); - }; - is( ref($@), 'Koha::Exceptions::MissingParameter', - 'Missing parameter exception' ); - is( $@->message, 'ItemHoldingBranch requires the item_holding_branch param', - 'Exception message is correct' ); - - # Rollback transaction - $schema->storage->txn_rollback; -}; - -subtest 'Koha::RefundLostItemFeeRules::should_refund() tests' => sub { - - plan tests => 3; - - # Start transaction - $schema->storage->txn_begin; - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); - - $schema->resultset('CirculationRule')->search()->delete; - - my $default_rule = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 1 - } - } - ); - my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_false = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => $branchcode, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 0 - } - } - ); - my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_true = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => $branchcode2, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 1 - } - } - ); - # Make sure we have an unused branchcode - my $branchcode3 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_dummy = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => $branchcode3, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - } - } - ); - my $branch_without_rule = $specific_rule_dummy->{ branchcode }; - Koha::CirculationRules - ->search( - { - branchcode => $branch_without_rule, - categorycode => undef, - itemtype => undef, - rule_name => 'refund' - } - ) - ->next - ->delete; - - my $params = { - current_branch => $specific_rule_true->{ branchcode }, - # patron_branch => $specific_rule_false->{ branchcode }, - item_holding_branch => $branch_without_rule, - item_home_branch => $branch_without_rule - }; - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); - is( Koha::RefundLostItemFeeRules->should_refund( $params ), - 1,'Specific rule is applied (true)'); - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHomeBranch' ); - is( Koha::RefundLostItemFeeRules->should_refund( $params ), - 1,'No rule for branch, global rule applied (true)'); - - # Change the default value just to try - Koha::CirculationRules->search({ branchcode => undef, rule_name => 'refund' })->next->rule_value(0)->store; - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHoldingBranch' ); - is( Koha::RefundLostItemFeeRules->should_refund( $params ), - 0,'No rule for branch, global rule applied (false)'); - - # Rollback transaction - $schema->storage->txn_rollback; -}; - -subtest 'Koha::RefundLostItemFeeRules::find() tests' => sub { - - plan tests => 5; - - # Start transaction - $schema->storage->txn_begin; - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); - - $schema->resultset('CirculationRule')->search()->delete; - - my $default_non_refund = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'non_refund_rule', - rule_value => 1 - } - } - ); - - ok(defined Koha::RefundLostItemFeeRules->find($default_non_refund->{id}), 'Find should continue to work when passed an id'); - - my $specific_non_refund = $builder->build( - { - source => 'CirculationRule', - value => { - categorycode => undef, - itemtype => undef, - rule_name => 'non_refund_rule', - rule_value => 0 - } - } - ); - - ok(!defined Koha::RefundLostItemFeeRules->find({ branchcode => undef }), 'Non refund default rules are not found'); - ok(!defined Koha::RefundLostItemFeeRules->find({ branchcode => $specific_non_refund->{branchcode} }), 'Non refund specific rules are not found'); - - my $default_refund = $builder->build( - { - source => 'CirculationRule', - value => { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 1 - } - } - ); - my $specific_refund = $builder->build( - { - source => 'CirculationRule', - value => { - categorycode => undef, - itemtype => undef, - rule_name => 'refund', - rule_value => 0 - } - } - ); - - ok(defined Koha::RefundLostItemFeeRules->find({ branchcode => undef }), 'Refund default rules are found'); - ok(defined Koha::RefundLostItemFeeRules->find({ branchcode => $specific_refund->{branchcode} }), 'Refund specific rules are found'); - - # Rollback transaction - $schema->storage->txn_rollback; -}; diff --git a/t/lib/TestBuilder.pm b/t/lib/TestBuilder.pm index 7c9d4e8d1e..5c22c213c2 100644 --- a/t/lib/TestBuilder.pm +++ b/t/lib/TestBuilder.pm @@ -576,10 +576,7 @@ sub _gen_default_values { }, AuthHeader => { marcxml => '', - }, - RefundLostItemFeeRules => { - rule_name => 'refund', - }, + } }; } -- 2.20.1