Bugzilla – Attachment 115013 Details for
Bug 26593
Rental discounts are applied in wrong precedence order
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 26593: Remove _get_discount_from_rule
Bug-26593-Remove-getdiscountfromrule.patch (text/plain), 6.33 KB, created by
Martin Renvoize
on 2021-01-11 12:09:56 UTC
(
hide
)
Description:
Bug 26593: Remove _get_discount_from_rule
Filename:
MIME Type:
Creator:
Martin Renvoize
Created:
2021-01-11 12:09:56 UTC
Size:
6.33 KB
patch
obsolete
>From 27b34208b81b2049ca8ccdaf9f35e33ad1f3a5d0 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> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 26593
:
113823
|
114552
| 115013