From 3ba461253e861925265c5c5cf6f723ade486cce1 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
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 <nick@bywatersolutions.com>
---
 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.30.2