Bug 25663: Remove Koha::RefundLostItemFeeRule and uses
authorMartin Renvoize <martin.renvoize@ptfs-europe.com>
Wed, 3 Jun 2020 13:51:42 +0000 (14:51 +0100)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Thu, 13 Aug 2020 05:55:44 +0000 (07:55 +0200)
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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

C4/Circulation.pm
Koha/RefundLostItemFeeRules.pm [deleted file]
admin/smart-rules.pl
t/db_dependent/RefundLostItemFeeRule.t [deleted file]
t/lib/TestBuilder.pm

index a9cf02c..c94b773 100644 (file)
@@ -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;
@@ -1470,12 +1469,10 @@ sub AddIssue {
                 }
 
                 if (
-                    $refund
-                    && Koha::RefundLostItemFeeRules->should_refund(
+                    $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
                         }
                     )
                   )
@@ -2074,13 +2071,12 @@ sub AddReturn {
 
             if (
                 $refund &&
-                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 (file)
index 9cb32d5..0000000
+++ /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 <http://www.gnu.org/licenses>.
-
-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;
index 0a33493..04ddb26 100755 (executable)
@@ -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;
@@ -550,10 +549,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 (executable)
index da53f3a..0000000
+++ /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 <http://www.gnu.org/licenses>.
-
-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;
-};
index 3feac1c..4564620 100644 (file)
@@ -577,10 +577,7 @@ sub _gen_default_values {
         },
         AuthHeader => {
             marcxml => '',
-        },
-        RefundLostItemFeeRules => {
-            rule_name => 'refund',
-        },
+        }
     };
 }