Bug 27259 - HomeOrHoldingBranch is not used in all places
Summary: HomeOrHoldingBranch is not used in all places
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Joonas Kylmälä
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-17 15:17 UTC by Joonas Kylmälä
Modified: 2023-06-08 22:28 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 27259: Test HomeOrHoldingBranch usage in TooMany() (3.76 KB, patch)
2020-12-22 10:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in _CalculateAndUpdateFine() (5.11 KB, patch)
2020-12-22 10:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from (8.66 KB, patch)
2020-12-22 10:51 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in TooMany() (3.75 KB, patch)
2022-04-23 18:30 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in _CalculateAndUpdateFine() (5.10 KB, patch)
2022-04-23 18:30 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from (8.76 KB, patch)
2022-04-23 18:30 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from (8.76 KB, patch)
2022-09-11 12:33 UTC, Andrii Nugged
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in TooMany() (3.75 KB, patch)
2022-09-11 12:34 UTC, Andrii Nugged
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in _CalculateAndUpdateFine() (5.10 KB, patch)
2022-09-11 12:34 UTC, Andrii Nugged
Details | Diff | Splinter Review
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from (8.76 KB, patch)
2022-09-11 12:34 UTC, Andrii Nugged
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in TooMany() (3.80 KB, patch)
2022-09-27 09:14 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in _CalculateAndUpdateFine() (5.15 KB, patch)
2022-09-27 09:14 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from (8.81 KB, patch)
2022-09-27 09:14 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in TooMany() (3.87 KB, patch)
2022-11-02 16:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27259: Test HomeOrHoldingBranch usage in _CalculateAndUpdateFine() (5.22 KB, patch)
2022-11-02 16:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from (8.88 KB, patch)
2022-11-02 16:59 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 Joonas Kylmälä 2020-12-17 15:17:30 UTC
If you grep for CircControl in Koha source code you will find that there are few places that are missing the check for HomeOrHoldingBranch and just use item's homebranch. This for example leads to calculating renewability and fines with rules from item's homebranch and not from the holdingbranch as was wanted.
Comment 1 Joonas Kylmälä 2020-12-22 10:51:04 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2020-12-22 10:51:07 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2020-12-22 10:51:09 UTC Comment hidden (obsolete)
Comment 4 Joonas Kylmälä 2022-04-23 18:30:48 UTC Comment hidden (obsolete)
Comment 5 Joonas Kylmälä 2022-04-23 18:30:52 UTC Comment hidden (obsolete)
Comment 6 Joonas Kylmälä 2022-04-23 18:30:56 UTC Comment hidden (obsolete)
Comment 7 Andrii Nugged 2022-09-11 12:33:27 UTC Comment hidden (obsolete)
Comment 8 Andrii Nugged 2022-09-11 12:34:49 UTC
Created attachment 140389 [details] [review]
Bug 27259: Test HomeOrHoldingBranch usage in TooMany()

This shows that HomeOrHoldingBranch syspref is incorrectly not used by
TooMany() when it decides which circ rule to use.

Run "prove t/db_dependent/Circulation/TooMany.t" to notice the tests
now fail.
Comment 9 Andrii Nugged 2022-09-11 12:34:53 UTC
Created attachment 140390 [details] [review]
Bug 27259: Test HomeOrHoldingBranch usage in _CalculateAndUpdateFine()

This shows that HomeOrHoldingBranch syspref is incorrectly not used by
_CalculateAndUpdateFine() when it decides which circ rule to use.

Run "prove t/db_dependent/Circulation/_CalculateAndUpdateFine.t" to
notice the tests now fail.
Comment 10 Andrii Nugged 2022-09-11 12:34:58 UTC
Created attachment 140391 [details] [review]
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from

The TooMany() function and fine calculation functions were incorrectly
hard coded to use homebranch for fetching the circulation rules. Those
ignored completely the syspref HomeOrHoldingBranch where the user
might have set it to holdingbranch and therefore the fines and whether
patron has too many checkouts (TooMany()) were counted using the
unintended branch's rules. This problem only arises in the cases where
there are branch specific circulation rules defined.

Test plan:
1. Make sure following tests pass:
   $ prove t/db_dependent/Circulation/_CalculateAndUpdateFine.t
   $ prove t/db_dependent/Circulation/TooMany.t

Test plan for fines.pl:
1. Add branch specific fine rules for branches A and B. A having a
   fine of 1 per day and B having a fine of 0 per day.
2. Set sysprefs:
   CircControl = the library the items is from
   finesMode = Calculate and charge
   HomeOrHoldingBranch = holdingbranch
3. Create an item with home and holding branch of A
4. Checkout the item with a due date in the past (the past due date can be
   specified by clicking "Checkout settings" in the checkout page) and
   make sure the branch you are checking from is B.
5. Run perl /usr/share/koha/bin/cronjobs/fines.pl
6. Notice that fines have popped up now to the patron incorrectly
7. Apply patch
8. Pay fines, Check-in the item and check it out again
9. Run perl /usr/share/koha/bin/cronjobs/fines.pl
10. Notice that fine is now 0. This means that the branch
    B (holdingbranch of the checked-out item) specific rule is used.

Test plan for staticfines.pl:
1. Add branch specific fine rules for branches A and B. A having a
   fine of 1 per day and B having a fine of 0 per day.
2. Set sysprefs:
   CircControl = the library the items is from
   finesMode = Calculate and charge
   HomeOrHoldingBranch = holdingbranch
3. Create an item with homebranch A and holding branch of A
4. Checkout the item with a due date in the past (the past due date can be
   specified by clicking "Checkout settings" in the checkout page) and
   make sure the branch you are checking from is B.
5. Run perl staticfines.pl --library A --library B --category <PATRONS_CATEGORYCODE>
   and notice that now there is inccorectly fines
6. Apply patch
7. Pay fines, Check-in the item and check it out again
8. Run perl staticfines.pl --library A --library B --category <PATRONS_CATEGORYCODE>
    and notice the fines are now not generated

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 11 Andrii Nugged 2022-09-11 12:35:22 UTC
rebased to latest master
Comment 12 Peter Vashchuk 2022-09-27 09:14:22 UTC
Created attachment 141021 [details] [review]
Bug 27259: Test HomeOrHoldingBranch usage in TooMany()

This shows that HomeOrHoldingBranch syspref is incorrectly not used by
TooMany() when it decides which circ rule to use.

Run "prove t/db_dependent/Circulation/TooMany.t" to notice the tests
now fail.

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 13 Peter Vashchuk 2022-09-27 09:14:26 UTC
Created attachment 141022 [details] [review]
Bug 27259: Test HomeOrHoldingBranch usage in _CalculateAndUpdateFine()

This shows that HomeOrHoldingBranch syspref is incorrectly not used by
_CalculateAndUpdateFine() when it decides which circ rule to use.

Run "prove t/db_dependent/Circulation/_CalculateAndUpdateFine.t" to
notice the tests now fail.

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 14 Peter Vashchuk 2022-09-27 09:14:31 UTC
Created attachment 141023 [details] [review]
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from

The TooMany() function and fine calculation functions were incorrectly
hard coded to use homebranch for fetching the circulation rules. Those
ignored completely the syspref HomeOrHoldingBranch where the user
might have set it to holdingbranch and therefore the fines and whether
patron has too many checkouts (TooMany()) were counted using the
unintended branch's rules. This problem only arises in the cases where
there are branch specific circulation rules defined.

Test plan:
1. Make sure following tests pass:
   $ prove t/db_dependent/Circulation/_CalculateAndUpdateFine.t
   $ prove t/db_dependent/Circulation/TooMany.t

Test plan for fines.pl:
1. Add branch specific fine rules for branches A and B. A having a
   fine of 1 per day and B having a fine of 0 per day.
2. Set sysprefs:
   CircControl = the library the items is from
   finesMode = Calculate and charge
   HomeOrHoldingBranch = holdingbranch
3. Create an item with home and holding branch of A
4. Checkout the item with a due date in the past (the past due date can be
   specified by clicking "Checkout settings" in the checkout page) and
   make sure the branch you are checking from is B.
5. Run perl /usr/share/koha/bin/cronjobs/fines.pl
6. Notice that fines have popped up now to the patron incorrectly
7. Apply patch
8. Pay fines, Check-in the item and check it out again
9. Run perl /usr/share/koha/bin/cronjobs/fines.pl
10. Notice that fine is now 0. This means that the branch
    B (holdingbranch of the checked-out item) specific rule is used.

Test plan for staticfines.pl:
1. Add branch specific fine rules for branches A and B. A having a
   fine of 1 per day and B having a fine of 0 per day.
2. Set sysprefs:
   CircControl = the library the items is from
   finesMode = Calculate and charge
   HomeOrHoldingBranch = holdingbranch
3. Create an item with homebranch A and holding branch of A
4. Checkout the item with a due date in the past (the past due date can be
   specified by clicking "Checkout settings" in the checkout page) and
   make sure the branch you are checking from is B.
5. Run perl staticfines.pl --library A --library B --category <PATRONS_CATEGORYCODE>
   and notice that now there is inccorectly fines
6. Apply patch
7. Pay fines, Check-in the item and check it out again
8. Run perl staticfines.pl --library A --library B --category <PATRONS_CATEGORYCODE>
    and notice the fines are now not generated

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 15 Martin Renvoize 2022-11-02 16:59:29 UTC
Created attachment 143013 [details] [review]
Bug 27259: Test HomeOrHoldingBranch usage in TooMany()

This shows that HomeOrHoldingBranch syspref is incorrectly not used by
TooMany() when it decides which circ rule to use.

Run "prove t/db_dependent/Circulation/TooMany.t" to notice the tests
now fail.

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-11-02 16:59:35 UTC
Created attachment 143014 [details] [review]
Bug 27259: Test HomeOrHoldingBranch usage in _CalculateAndUpdateFine()

This shows that HomeOrHoldingBranch syspref is incorrectly not used by
_CalculateAndUpdateFine() when it decides which circ rule to use.

Run "prove t/db_dependent/Circulation/_CalculateAndUpdateFine.t" to
notice the tests now fail.

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-11-02 16:59:40 UTC
Created attachment 143015 [details] [review]
Bug 27259: Add HomeOrHoldingBranch checks where it was missing from

The TooMany() function and fine calculation functions were incorrectly
hard coded to use homebranch for fetching the circulation rules. Those
ignored completely the syspref HomeOrHoldingBranch where the user
might have set it to holdingbranch and therefore the fines and whether
patron has too many checkouts (TooMany()) were counted using the
unintended branch's rules. This problem only arises in the cases where
there are branch specific circulation rules defined.

Test plan:
1. Make sure following tests pass:
   $ prove t/db_dependent/Circulation/_CalculateAndUpdateFine.t
   $ prove t/db_dependent/Circulation/TooMany.t

Test plan for fines.pl:
1. Add branch specific fine rules for branches A and B. A having a
   fine of 1 per day and B having a fine of 0 per day.
2. Set sysprefs:
   CircControl = the library the items is from
   finesMode = Calculate and charge
   HomeOrHoldingBranch = holdingbranch
3. Create an item with home and holding branch of A
4. Checkout the item with a due date in the past (the past due date can be
   specified by clicking "Checkout settings" in the checkout page) and
   make sure the branch you are checking from is B.
5. Run perl /usr/share/koha/bin/cronjobs/fines.pl
6. Notice that fines have popped up now to the patron incorrectly
7. Apply patch
8. Pay fines, Check-in the item and check it out again
9. Run perl /usr/share/koha/bin/cronjobs/fines.pl
10. Notice that fine is now 0. This means that the branch
    B (holdingbranch of the checked-out item) specific rule is used.

Test plan for staticfines.pl:
1. Add branch specific fine rules for branches A and B. A having a
   fine of 1 per day and B having a fine of 0 per day.
2. Set sysprefs:
   CircControl = the library the items is from
   finesMode = Calculate and charge
   HomeOrHoldingBranch = holdingbranch
3. Create an item with homebranch A and holding branch of A
4. Checkout the item with a due date in the past (the past due date can be
   specified by clicking "Checkout settings" in the checkout page) and
   make sure the branch you are checking from is B.
5. Run perl staticfines.pl --library A --library B --category <PATRONS_CATEGORYCODE>
   and notice that now there is inccorectly fines
6. Apply patch
7. Pay fines, Check-in the item and check it out again
8. Run perl staticfines.pl --library A --library B --category <PATRONS_CATEGORYCODE>
    and notice the fines are now not generated

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-11-02 17:00:18 UTC
Nicely worked through.. good to see unit tests and qa script is happy.

Passing QA
Comment 19 Tomás Cohen Arazi 2022-11-04 22:14:25 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!