From 27b34208b81b2049ca8ccdaf9f35e33ad1f3a5d0 Mon Sep 17 00:00:00 2001 From: Nick Clemens 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 Signed-off-by: Martin Renvoize --- 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 2960c30bb9..61076407ed 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3390,12 +3390,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 @@ -3405,49 +3410,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.20.1