Bug 26593 - Rental discounts are applied in wrong precedence order
Summary: Rental discounts are applied in wrong precedence order
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2020-10-01 11:14 UTC by Nick Clemens
Modified: 2021-02-11 22:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.02,20.05.08


Attachments
Bug 26593: Remove _get_discount_from_rule (6.22 KB, patch)
2020-11-19 15:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26593: Remove _get_discount_from_rule (6.26 KB, patch)
2020-12-21 13:06 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26593: Remove _get_discount_from_rule (6.33 KB, patch)
2021-01-11 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-10-01 11:14:39 UTC
GetIssuingCharges uses its own private sub to choose the correct rental discount rule, specifying the rules returned in order:

1 - same library,  same category, same itemtype
2 - all libraries, same category, same itemtype
3 - same library,  same category, all itemtypes
4 - all libraries, same category, all itemtypes

It doesn't match what the circ rules page says, and is missing several options:

    same library, same patron category, same item type
    same library, same patron category, all item types
    same library, all patron categories, same item type
    same library, all patron categories, all item types
    default (all libraries), same patron category, same item type
    default (all libraries), same patron category, all item types
    default (all libraries), all patron categories, same item type
    default (all libraries), all patron categories, all item types
Comment 1 Martin Renvoize 2020-10-06 06:53:01 UTC
Hmm, I'm wondering if there was a deliberate reason behind this or whether it's just a historical mistake.
Comment 2 Nick Clemens 2020-11-19 15:54:03 UTC
Created attachment 113823 [details] [review]
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
Comment 3 Victor Grousset/tuxayo 2020-12-21 13:06:45 UTC
Created attachment 114552 [details] [review]
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>
Comment 4 Martin Renvoize 2021-01-11 12:09:56 UTC
Created attachment 115013 [details] [review]
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>
Comment 5 Martin Renvoize 2021-01-11 12:10:59 UTC
I could have sworn I'd already QA'd this a couple of months back!

All works as expected.. Passing QA
Comment 6 Jonathan Druart 2021-01-12 10:42:54 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 7 Fridolin Somers 2021-01-14 16:21:03 UTC
Pushed to 20.11.x for 20.11.02
Comment 8 Andrew Fuerste-Henry 2021-01-20 02:17:14 UTC
Cannot replicate issue on 20.05. Discounts are being applied correctly based on library when following test plan without . Not backporting.
Comment 9 Andrew Fuerste-Henry 2021-01-20 15:59:09 UTC
Reverted a different patch and retested this one.
Pushed to 20.05.x for 20.05.08
Comment 10 Victor Grousset/tuxayo 2021-01-22 19:56:57 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.