From 4d994773f83468b561896939f16131d0332d9cfc Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Mon, 24 Oct 2022 11:13:32 +0000 Subject: [PATCH] Bug 23012: (QA follow-up) Combine method to get both values The code for get_processingreturn_policy was very similar to get_lostreturn_policy. Combining the two methods allows for use of get_effective_rules which uses get_effective_rule_value which is cached. This should reduce lines of code and improve performance Tests updated and adjusted as well Signed-off-by: Nick Clemens Signed-off-by: Tomas Cohen Arazi --- Koha/CirculationRules.pm | 47 ++---- Koha/Item.pm | 12 +- t/db_dependent/Koha/CirculationRules.t | 221 +++++++++---------------- 3 files changed, 95 insertions(+), 185 deletions(-) diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index 64999533b5..f96df496fb 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -577,9 +577,9 @@ sub get_onshelfholds_policy { =head3 get_lostreturn_policy - my $lostrefund_policy = Koha::CirculationRules->get_lostreturn_policy( { return_branch => $return_branch, item => $item } ); + my $lost_proc_refund_policy = Koha::CirculationRules->get_lostreturn_policy( { return_branch => $return_branch, item => $item } ); -Return values are: +lostreturn return values are: =over 2 @@ -593,37 +593,7 @@ Return values are: =back -=cut - -sub get_lostreturn_policy { - my ( $class, $params ) = @_; - - my $item = $params->{item}; - - my $behaviour = C4::Context->preference( 'RefundLostOnReturnControl' ) // 'CheckinLibrary'; - my $behaviour_mapping = { - CheckinLibrary => $params->{'return_branch'} // $item->homebranch, - ItemHomeBranch => $item->homebranch, - ItemHoldingBranch => $item->holdingbranch - }; - - my $branch = $behaviour_mapping->{ $behaviour }; - - my $rule = Koha::CirculationRules->get_effective_rule( - { - branchcode => $branch, - rule_name => 'lostreturn', - } - ); - - return $rule ? $rule->rule_value : 'refund'; -} - -=head3 get_processingreturn_policy - - my $processingrefund_policy = Koha::CirculationRules->get_processingreturn_policy( { return_branch => $return_branch, item => $item } ); - -Return values are: +processing return return values are: =over 2 @@ -637,9 +607,10 @@ Return values are: =back + =cut -sub get_processingreturn_policy { +sub get_lostreturn_policy { my ( $class, $params ) = @_; my $item = $params->{item}; @@ -653,14 +624,16 @@ sub get_processingreturn_policy { my $branch = $behaviour_mapping->{ $behaviour }; - my $rule = Koha::CirculationRules->get_effective_rule( + my $rules = Koha::CirculationRules->get_effective_rules( { branchcode => $branch, - rule_name => 'processingreturn', + rules => ['lostreturn','processingreturn'] } ); - return $rule ? $rule->rule_value : 'refund'; + $rules->{lostreturn} //= 'refund'; + $rules->{processingreturn} //= 'refund'; + return $rules; } =head3 article_requestable_rules diff --git a/Koha/Item.pm b/Koha/Item.pm index 37d2ee343c..25b4e54dc3 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -1159,7 +1159,7 @@ sub _set_found_trigger { return $self unless $lost_age_in_days < $no_refund_after_days; } - my $lostreturn_policy = Koha::CirculationRules->get_lostreturn_policy( + my $lost_proc_return_policy = Koha::CirculationRules->get_lostreturn_policy( { item => $self, return_branch => C4::Context->userenv @@ -1167,6 +1167,7 @@ sub _set_found_trigger { : undef, } ); + my $lostreturn_policy = $lost_proc_return_policy->{lostreturn}; if ( $lostreturn_policy ) { @@ -1324,14 +1325,7 @@ sub _set_found_trigger { } } - my $processingreturn_policy = Koha::CirculationRules->get_processingreturn_policy( - { - item => $self, - return_branch => C4::Context->userenv - ? C4::Context->userenv->{'branch'} - : undef, - } - ); + my $processingreturn_policy = $lost_proc_return_policy->{lostreturn}; if ( $processingreturn_policy ) { diff --git a/t/db_dependent/Koha/CirculationRules.t b/t/db_dependent/Koha/CirculationRules.t index 8664505524..c4009c6a96 100755 --- a/t/db_dependent/Koha/CirculationRules.t +++ b/t/db_dependent/Koha/CirculationRules.t @@ -20,7 +20,7 @@ use Modern::Perl; use Benchmark; -use Test::More tests => 8; +use Test::More tests => 7; use Test::Deep qw( cmp_methods ); use Test::Exception; @@ -29,6 +29,7 @@ use Koha::Database; use t::lib::Mocks; use t::lib::TestBuilder; +use Koha::Cache::Memory::Lite; my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; @@ -687,65 +688,61 @@ subtest 'get_lostreturn_policy() tests' => sub { $schema->resultset('CirculationRule')->search()->delete; - my $default_rule_charge = $builder->build( + my $default_proc_rule_charge = $builder->build( { source => 'CirculationRule', value => { branchcode => undef, categorycode => undef, itemtype => undef, - rule_name => 'lostreturn', + rule_name => 'processingreturn', rule_value => 'charge' } } ); - my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_false = $builder->build( + my $default_lost_rule_charge = $builder->build( { source => 'CirculationRule', value => { - branchcode => $branchcode, + branchcode => undef, categorycode => undef, itemtype => undef, rule_name => 'lostreturn', - rule_value => 0 + rule_value => 'charge' } } ); - my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_refund = $builder->build( + my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; + my $specific_lost_rule_false = $builder->build( { source => 'CirculationRule', value => { - branchcode => $branchcode2, + branchcode => $branchcode, categorycode => undef, itemtype => undef, rule_name => 'lostreturn', - rule_value => 'refund' + rule_value => 0 } } ); - my $branchcode3 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_restore = $builder->build( + my $specific_proc_rule_false = $builder->build( { source => 'CirculationRule', value => { - branchcode => $branchcode3, + branchcode => $branchcode, categorycode => undef, itemtype => undef, - rule_name => 'lostreturn', - rule_value => 'restore' + rule_name => 'processingreturn', + rule_value => 0 } } ); - - # Make sure we have an unused branchcode - my $branchcode4 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_dummy = $builder->build( + my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; + my $specific_lost_rule_refund = $builder->build( { source => 'CirculationRule', value => { - branchcode => $branchcode4, + branchcode => $branchcode2, categorycode => undef, itemtype => undef, rule_name => 'lostreturn', @@ -753,140 +750,59 @@ subtest 'get_lostreturn_policy() tests' => sub { } } ); - my $branch_without_rule = $specific_rule_dummy->{ branchcode }; - Koha::CirculationRules - ->search( - { - branchcode => $branch_without_rule, - categorycode => undef, - itemtype => undef, - rule_name => 'lostreturn', - rule_value => 'refund' - } - ) - ->next - ->delete; - - my $item = $builder->build_sample_item( - { - homebranch => $specific_rule_restore->{branchcode}, - holdingbranch => $specific_rule_false->{branchcode} - } - ); - my $params = { - return_branch => $specific_rule_refund->{ branchcode }, - item => $item - }; - - # Specific rules - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); - is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 'refund','Specific rule for checkin branch is applied (refund)'); - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHomeBranch' ); - is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 'restore','Specific rule for home branch is applied (restore)'); - - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHoldingBranch' ); - is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 0,'Specific rule for holding branch is applied (false)'); - - # Default rule check - t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); - $params->{return_branch} = $branch_without_rule; - is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 'charge','No rule for branch, global rule applied (charge)'); - - # Change the default value just to try - Koha::CirculationRules->search({ branchcode => undef, rule_name => 'lostreturn' })->next->rule_value(0)->store; - is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 0,'No rule for branch, global rule applied (false)'); - - # No default rule defined check - Koha::CirculationRules - ->search( - { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rule_name => 'lostreturn' - } - ) - ->next - ->delete; - is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 'refund','No rule for branch, no default rule, fallback default (refund)'); - - # Fallback to ItemHoldBranch if CheckinLibrary is undefined - $params->{return_branch} = undef; - is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 'restore','return_branch undefined, fallback to ItemHomeBranch rule (restore)'); - - $schema->storage->txn_rollback; -}; - -subtest 'get_processingreturn_policy() tests' => sub { - plan tests => 7; - - $schema->storage->txn_begin; - - $schema->resultset('CirculationRule')->search()->delete; - - my $default_rule_charge = $builder->build( + my $specific_proc_rule_refund = $builder->build( { source => 'CirculationRule', value => { - branchcode => undef, + branchcode => $branchcode2, categorycode => undef, itemtype => undef, rule_name => 'processingreturn', - rule_value => 'charge' + rule_value => 'refund' } } ); - my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_false = $builder->build( + my $branchcode3 = $builder->build( { source => 'Branch' } )->{branchcode}; + my $specific_lost_rule_restore = $builder->build( { source => 'CirculationRule', value => { - branchcode => $branchcode, + branchcode => $branchcode3, categorycode => undef, itemtype => undef, - rule_name => 'processingreturn', - rule_value => 0 + rule_name => 'lostreturn', + rule_value => 'restore' } } ); - my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_refund = $builder->build( + my $specific_proc_rule_restore = $builder->build( { source => 'CirculationRule', value => { - branchcode => $branchcode2, + branchcode => $branchcode3, categorycode => undef, itemtype => undef, rule_name => 'processingreturn', - rule_value => 'refund' + rule_value => 'restore' } } ); - my $branchcode3 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_restore = $builder->build( + + # Make sure we have an unused branchcode + my $branchcode4 = $builder->build( { source => 'Branch' } )->{branchcode}; + my $specific_lost_rule_dummy = $builder->build( { source => 'CirculationRule', value => { - branchcode => $branchcode3, + branchcode => $branchcode4, categorycode => undef, itemtype => undef, - rule_name => 'processingreturn', - rule_value => 'restore' + rule_name => 'lostreturn', + rule_value => 'refund' } } ); - - # Make sure we have an unused branchcode - my $branchcode4 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $specific_rule_dummy = $builder->build( + my $specific_proc_rule_dummy = $builder->build( { source => 'CirculationRule', value => { @@ -898,7 +814,19 @@ subtest 'get_processingreturn_policy() tests' => sub { } } ); - my $branch_without_rule = $specific_rule_dummy->{ branchcode }; + my $branch_without_rule = $specific_lost_rule_dummy->{ branchcode }; + Koha::CirculationRules + ->search( + { + branchcode => $branch_without_rule, + categorycode => undef, + itemtype => undef, + rule_name => 'lostreturn', + rule_value => 'refund' + } + ) + ->next + ->delete; Koha::CirculationRules ->search( { @@ -914,40 +842,55 @@ subtest 'get_processingreturn_policy() tests' => sub { my $item = $builder->build_sample_item( { - homebranch => $specific_rule_restore->{branchcode}, - holdingbranch => $specific_rule_false->{branchcode} + homebranch => $specific_lost_rule_restore->{branchcode}, + holdingbranch => $specific_lost_rule_false->{branchcode} } ); my $params = { - return_branch => $specific_rule_refund->{ branchcode }, + return_branch => $specific_lost_rule_refund->{ branchcode }, item => $item }; # Specific rules t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); - is( Koha::CirculationRules->get_processingreturn_policy( $params ), - 'refund','Specific rule for checkin branch is applied (refund)'); + is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), + { lostreturn => 'refund', processingreturn => 'refund' },'Specific rule for checkin branch is applied (refund)'); t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHomeBranch' ); - is( Koha::CirculationRules->get_processingreturn_policy( $params ), - 'restore','Specific rule for home branch is applied (restore)'); + is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), + { lostreturn => 'restore', processingreturn => 'restore' },'Specific rule for home branch is applied (restore)'); t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHoldingBranch' ); - is( Koha::CirculationRules->get_processingreturn_policy( $params ), - 0,'Specific rule for holding branch is applied (false)'); + is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), + { lostreturn => 0, processingreturn => 0 },'Specific rule for holding branch is applied (false)'); # Default rule check t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); $params->{return_branch} = $branch_without_rule; - is( Koha::CirculationRules->get_processingreturn_policy( $params ), - 'charge','No rule for branch, global rule applied (charge)'); + is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), + { lostreturn => 'charge', processingreturn => 'charge' },'No rule for branch, global rule applied (charge)'); # Change the default value just to try + Koha::CirculationRules->search({ branchcode => undef, rule_name => 'lostreturn' })->next->rule_value(0)->store; Koha::CirculationRules->search({ branchcode => undef, rule_name => 'processingreturn' })->next->rule_value(0)->store; - is( Koha::CirculationRules->get_processingreturn_policy( $params ), - 0,'No rule for branch, global rule applied (false)'); + my $memory_cache = Koha::Cache::Memory::Lite->get_instance; + $memory_cache->flush(); + is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), + { lostreturn => 0, processingreturn => 0 },'No rule for branch, global rule applied (false)'); # No default rule defined check + Koha::CirculationRules + ->search( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'lostreturn' + } + ) + ->next + ->delete; + # No default rule defined check Koha::CirculationRules ->search( { @@ -959,13 +902,13 @@ subtest 'get_processingreturn_policy() tests' => sub { ) ->next ->delete; - is( Koha::CirculationRules->get_processingreturn_policy( $params ), - 'refund','No rule for branch, no default rule, fallback default (refund)'); + is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), + { lostreturn => 'refund', processingreturn => 'refund' },'No rule for branch, no default rule, fallback default (refund)'); # Fallback to ItemHoldBranch if CheckinLibrary is undefined $params->{return_branch} = undef; - is( Koha::CirculationRules->get_processingreturn_policy( $params ), - 'restore','return_branch undefined, fallback to ItemHomeBranch rule (restore)'); + is_deeply( Koha::CirculationRules->get_lostreturn_policy( $params ), + { lostreturn => 'restore', processingreturn => 'restore' },'return_branch undefined, fallback to ItemHomeBranch rule (restore)'); $schema->storage->txn_rollback; }; -- 2.39.5