From 0929b5616bce3aa5f734de0cea2646f1233d34b7 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Thu, 19 Nov 2020 15:46:52 +0000
Subject: [PATCH] Bug 26593: Remove _get_discount_from_rule

This patch remove the private sub used in GetIssuingCharges in favor
of get_effective_rule

It corrects the wrong precedence for rules and adds tests to cover this
subroutine

NOTE: the 'branch' for the discount will be determined by the signed in branch,
    this is a bug to be fixed in the future

To test:
1 - Define a rentalcharge for an itemtype
2 - Define a 10% discount for library A, category A, all itemtypes
3 - Define a 50% discount for all libraries, category A, same itemtype
4 - Attempt to checkout an item from library A of the matching itemtype
5 - The 50% discount is applied
6 - Apply patches
7 - Attempt to checkout an item from library A of the matching itemtype
8 - The 10% discount is applied

prove -v t/db_dependent/Circulation.t

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
---
 C4/Circulation.pm            | 54 ++++++------------------------------
 t/db_dependent/Circulation.t | 47 ++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 47 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 602bc54f6b..3fd6ceb55a 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -3394,12 +3394,17 @@ sub GetIssuingCharges {
     if ( my $item_data = $sth->fetchrow_hashref ) {
         $item_type = $item_data->{itemtype};
         $charge    = $item_data->{rentalcharge};
+        # FIXME This should follow CircControl
         my $branch = C4::Context::mybranch();
         my $patron = Koha::Patrons->find( $borrowernumber );
-        my $discount = _get_discount_from_rule($patron->categorycode, $branch, $item_type);
+        my $discount = Koha::CirculationRules->get_effective_rule({
+            categorycode => $patron->categorycode,
+            branchcode   => $branch,
+            itemtype     => $item_type,
+            rule_name    => 'rentaldiscount'
+        });
         if ($discount) {
-            # We may have multiple rules so get the most specific
-            $charge = ( $charge * ( 100 - $discount ) ) / 100;
+            $charge = ( $charge * ( 100 - $discount->rule_value ) ) / 100;
         }
         if ($charge) {
             $charge = sprintf '%.2f', $charge; # ensure no fractions of a penny returned
@@ -3409,49 +3414,6 @@ sub GetIssuingCharges {
     return ( $charge, $item_type );
 }
 
-# Select most appropriate discount rule from those returned
-sub _get_discount_from_rule {
-    my ($categorycode, $branchcode, $itemtype) = @_;
-
-    # Set search precedences
-    my @params = (
-        {
-            branchcode   => $branchcode,
-            itemtype     => $itemtype,
-            categorycode => $categorycode,
-        },
-        {
-            branchcode   => undef,
-            categorycode => $categorycode,
-            itemtype     => $itemtype,
-        },
-        {
-            branchcode   => $branchcode,
-            categorycode => $categorycode,
-            itemtype     => undef,
-        },
-        {
-            branchcode   => undef,
-            categorycode => $categorycode,
-            itemtype     => undef,
-        },
-    );
-
-    foreach my $params (@params) {
-        my $rule = Koha::CirculationRules->search(
-            {
-                rule_name => 'rentaldiscount',
-                %$params,
-            }
-        )->next();
-
-        return $rule->rule_value if $rule;
-    }
-
-    # none of the above
-    return 0;
-}
-
 =head2 AddIssuingCharge
 
   &AddIssuingCharge( $checkout, $charge, $type )
diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
index f29c175aef..6258ed80a8 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 => 51;
+use Test::More tests => 52;
 use Test::Exception;
 use Test::MockModule;
 use Test::Deep qw( cmp_deeply );
@@ -282,6 +282,51 @@ Koha::CirculationRules->set_rules(
     }
 );
 
+subtest "GetIssuingCharges tests" => sub {
+    plan tests => 4;
+    my $branch_discount = $builder->build_object({ class => 'Koha::Libraries' });
+    my $branch_no_discount = $builder->build_object({ class => 'Koha::Libraries' });
+    Koha::CirculationRules->set_rule(
+        {
+            categorycode => undef,
+            branchcode   => $branch_discount->branchcode,
+            itemtype     => undef,
+            rule_name    => 'rentaldiscount',
+            rule_value   => 15
+        }
+    );
+    my $itype_charge = $builder->build_object({
+        class => 'Koha::ItemTypes',
+        value => {
+            rentalcharge => 10
+        }
+    });
+    my $itype_no_charge = $builder->build_object({
+        class => 'Koha::ItemTypes',
+        value => {
+            rentalcharge => 0
+        }
+    });
+    my $patron = $builder->build_object({ class => 'Koha::Patrons' });
+    my $item_1 = $builder->build_sample_item({ itype => $itype_charge->itemtype });
+    my $item_2 = $builder->build_sample_item({ itype => $itype_no_charge->itemtype });
+
+    t::lib::Mocks::mock_userenv({ branchcode => $branch_no_discount->branchcode });
+    # For now the sub always uses the env branch, this should follow CircControl instead
+    my ($charge, $itemtype) = GetIssuingCharges( $item_1->itemnumber, $patron->borrowernumber);
+    is( $charge + 0, 10.00, "Charge fetched correctly when no discount exists");
+    ($charge, $itemtype) = GetIssuingCharges( $item_2->itemnumber, $patron->borrowernumber);
+    is( $charge + 0, 0.00, "Charge fetched correctly when no discount exists and no charge");
+
+    t::lib::Mocks::mock_userenv({ branchcode => $branch_discount->branchcode });
+    # For now the sub always uses the env branch, this should follow CircControl instead
+    ($charge, $itemtype) = GetIssuingCharges( $item_1->itemnumber, $patron->borrowernumber);
+    is( $charge + 0, 8.50, "Charge fetched correctly when discount exists");
+    ($charge, $itemtype) = GetIssuingCharges( $item_2->itemnumber, $patron->borrowernumber);
+    is( $charge + 0, 0.00, "Charge fetched correctly when discount exists and no charge");
+
+};
+
 my ( $reused_itemnumber_1, $reused_itemnumber_2 );
 subtest "CanBookBeRenewed tests" => sub {
     plan tests => 89;
-- 
2.29.2