Bug 28924 - Allow checkout fine limit to be determined by patron category
Summary: Allow checkout fine limit to be determined by patron category
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Matt Blenkinsop
QA Contact: Nick Clemens (kidclamp)
URL: https://github.com/PTFS-Europe/koha/c...
Keywords:
Depends on:
Blocks: 38727 39261 37426
  Show dependency treegraph
 
Reported: 2021-08-30 21:05 UTC by Donna
Modified: 2025-03-06 09:09 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/1006
Text to go in the release notes:
This allows to set charge limits for checkouts by patron category. If the patron category level option is not used, the global system preferences will be used instead. The new options available are: - Checkout charge limit (`noissuescharge`) - Guarantee checkout charge limit (`NoIssuesChargeGuarantees preference`) - Guarantors with guarantees checkout charge limit (`NoIssuesChargeGuarantorsWithGuarantees`)
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 28924: Add 3 new columns to categories table (3.45 KB, patch)
2024-04-30 12:40 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add DBIC file (1.97 KB, patch)
2024-04-30 12:40 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add new columns to UI and controller (11.94 KB, patch)
2024-04-30 12:40 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods (7.18 KB, patch)
2024-04-30 12:40 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Update all usages of the current sysprefs (19.98 KB, patch)
2024-04-30 12:40 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add a template method and update template usages (3.77 KB, patch)
2024-04-30 12:40 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add unit tests for new methods and update existing tests (19.31 KB, patch)
2024-04-30 12:41 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add 3 new columns to categories table (3.45 KB, patch)
2024-05-02 09:55 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add DBIC file (1.97 KB, patch)
2024-05-02 09:55 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add new columns to UI and controller (940.45 KB, patch)
2024-05-02 09:55 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods (7.18 KB, patch)
2024-05-02 09:55 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Update all usages of the current sysprefs (19.98 KB, patch)
2024-05-02 09:55 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add a template method and update template usages (3.77 KB, patch)
2024-05-02 09:55 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add unit tests for new methods and update existing tests (19.31 KB, patch)
2024-05-02 09:55 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add 3 new columns to categories table (3.48 KB, patch)
2024-05-06 00:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 28924: Add DBIC file (2.00 KB, patch)
2024-05-06 00:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 28924: Add new columns to UI and controller (940.48 KB, patch)
2024-05-06 00:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods (7.21 KB, patch)
2024-05-06 00:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 28924: Update all usages of the current sysprefs (20.00 KB, patch)
2024-05-06 00:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 28924: Add a template method and update template usages (3.80 KB, patch)
2024-05-06 00:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 28924: Add unit tests for new methods and update existing tests (19.33 KB, patch)
2024-05-06 00:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 28924: Add 3 new columns to categories table (3.51 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add DBIC file (1.56 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add new columns to UI and controller (940.49 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods (7.18 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Update all usages of the current sysprefs (19.97 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add a template method and update template usages (3.77 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add unit tests for new methods and update existing tests (19.35 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up) Use $self instead of $patron (6.75 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Don't define charge values as 0 (4.37 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Remove unnecessary template method (3.35 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Fix unit tests (7.90 KB, patch)
2024-06-14 15:12 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add 3 new columns to categories table (3.56 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add DBIC file (1.61 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add new columns to UI and controller (940.54 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods (7.23 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Update all usages of the current sysprefs (20.02 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add a template method and update template usages (3.82 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add unit tests for new methods and update existing tests (19.40 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up) Use $self instead of $patron (6.75 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Don't define charge values as 0 (4.37 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Remove unnecessary template method (3.35 KB, patch)
2024-06-14 15:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Fix unit tests (7.90 KB, patch)
2024-06-14 15:16 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add 3 new columns to categories table (3.68 KB, patch)
2024-07-10 13:31 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: Add DBIC file (1.73 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: Add new columns to UI and controller (940.66 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods (7.34 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: Update all usages of the current sysprefs (20.14 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: Add a template method and update template usages (3.94 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: Add unit tests for new methods and update existing tests (19.51 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: (QA follow-up) Use $self instead of $patron (6.67 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Don't define charge values as 0 (4.49 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Remove unnecessary template method (3.47 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Fix unit tests (7.89 KB, patch)
2024-07-10 13:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 28924: Add 3 new columns to categories table (3.69 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add DBIC file (1.74 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add new columns to UI and controller (12.15 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods (7.36 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Update all usages of the current sysprefs (20.15 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add a template method and update template usages (3.95 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: Add unit tests for new methods and update existing tests (19.52 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up) Use $self instead of $patron (6.68 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Don't define charge values as 0 (4.50 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Remove unnecessary template method (3.48 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up): Fix unit tests (7.90 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (QA follow-up) Terminology fixes (13.17 KB, patch)
2024-07-15 14:22 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (follow-up): Fix API spec (1.45 KB, patch)
2024-07-19 07:17 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 28924: (follow-up) Decimal points count as characters (3.48 KB, patch)
2024-07-23 14:55 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Donna 2021-08-30 21:05:05 UTC
Currently, all patron categories have the NoIssuesCharge system preference amount applied uniformly.  However there are situations where one patron category should have a higher (or lower) threshold, and it would be helpful to set this by patron category.  For instance, for libraries that do ILL, items will frequently be marked as long overdue/lost due to the length of time they are checked out, and ILL account will be blocked from use.  Being able to set specific patron categories to different NoIssuesCharge would resolve this.
Comment 1 Kristi 2023-09-19 13:02:33 UTC
This would be particularly helpful for some of our unique patron categories.
Comment 2 Martin Renvoize (ashimema) 2024-03-27 11:12:27 UTC
Branch available at https://github.com/PTFS-Europe/koha/commits/bug_28924/.
Comment 3 Lucas Gass (lukeg) 2024-04-19 21:01:54 UTC
We were wondering if there is a test plan for this one?
Comment 4 Matt Blenkinsop 2024-04-30 12:39:48 UTC
Test plan added to latest commit on the branch for future reference and also here for testing

Patron category level noissuescharge

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5. 
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

    prove t/db_dependent/SIP/Transaction.t
    prove t/db_dependent/SIP/Patron.t
    prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
    prove t/db_dependent/Koha/Patron.t
Comment 5 Matt Blenkinsop 2024-04-30 12:40:46 UTC
Created attachment 165870 [details] [review]
Bug 28924: Add 3 new columns to categories table

This patch adds three new columns to the patron categories table:
noissuescharge
noissueschargegurantees
noissueschargeguarantorswithguarantees

These values will allow charge limits to be set at category level rather than globally for all patrons. If the values are not set at category level then the system will use the global level values
Comment 6 Matt Blenkinsop 2024-04-30 12:40:48 UTC
Created attachment 165871 [details] [review]
Bug 28924: Add DBIC file
Comment 7 Matt Blenkinsop 2024-04-30 12:40:50 UTC
Created attachment 165872 [details] [review]
Bug 28924: Add new columns to UI and controller

This patch adds the columns in the table and also updates the controller to allow these fields to be updated in the database from the UI form
Comment 8 Matt Blenkinsop 2024-04-30 12:40:53 UTC
Created attachment 165873 [details] [review]
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods

This patch adds two new methods. can_borrow is a top-level method for determining whether a patron is able to borrow. Currently this logic is repeataed in multiple methods and shuld be more DRY. This bug is specific to patron charge limits so that is the focus but the method can be developed in follow-up bugs to cover all areas.
The is_patron_inside_charge_limits method is now to be used where any of the three noissuescharge sysprefs are used. It will look at the patron category based on either a borrowernumber or patron object and return a hash detailing the charges, the charge limit and whether the patron is over the limit

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees
Comment 9 Matt Blenkinsop 2024-04-30 12:40:55 UTC
Created attachment 165874 [details] [review]
Bug 28924: Update all usages of the current sysprefs

This patch updates all instances where the current noissuescharge sysprefs are used. They will now use the is_patron_inside_charge_limits method to handle the patron category level limits
Comment 10 Matt Blenkinsop 2024-04-30 12:40:57 UTC
Created attachment 165875 [details] [review]
Bug 28924: Add a template method and update template usages

This patch adds a method to be used in the template where currently the Koha.Preference('noissuescharge') method is used. This currently does not take into account the new patron category level limits and so needs to be updated to its own method to handle these.
Comment 11 Matt Blenkinsop 2024-04-30 12:41:00 UTC
Created attachment 165876 [details] [review]
Bug 28924: Add unit tests for new methods and update existing tests

This commit adds unit tests for the two new methods introduced and also updates all unit tests that use the noissuecharge sysprefs

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Test plan:
prove t/db_dependent/SIP/Transaction.t
prove t/db_dependent/SIP/Patron.t
prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
prove t/db_dependent/Koha/Patron.t
Comment 12 Kristi 2024-04-30 18:25:11 UTC
I am using a ByWater sandbox to go through the test plan, and at step 6, the  No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees are not present when I edit a patron type. Would I need to have taken other steps than using one of these test sandboxes?
Comment 13 Katrin Fischer 2024-05-01 10:53:44 UTC
Hi Kristi, from looking at the patches I think they should be there. Maybe the patches didn't apply cleanly. Maybe try again on another sandbox?

@Matt: Please fix capitalization! No Issues Charge Guarantors With Guarantees: etc.
Comment 14 Matt Blenkinsop 2024-05-02 09:55:18 UTC
Created attachment 166042 [details] [review]
Bug 28924: Add 3 new columns to categories table

This patch adds three new columns to the patron categories table:
noissuescharge
noissueschargegurantees
noissueschargeguarantorswithguarantees

These values will allow charge limits to be set at category level rather than globally for all patrons. If the values are not set at category level then the system will use the global level values
Comment 15 Matt Blenkinsop 2024-05-02 09:55:21 UTC
Created attachment 166043 [details] [review]
Bug 28924: Add DBIC file
Comment 16 Matt Blenkinsop 2024-05-02 09:55:25 UTC
Created attachment 166044 [details] [review]
Bug 28924: Add new columns to UI and controller

This patch adds the columns in the table and also updates the controller to allow these fields to be updated in the database from the UI form
Comment 17 Matt Blenkinsop 2024-05-02 09:55:28 UTC
Created attachment 166045 [details] [review]
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods

This patch adds two new methods. can_borrow is a top-level method for determining whether a patron is able to borrow. Currently this logic is repeataed in multiple methods and shuld be more DRY. This bug is specific to patron charge limits so that is the focus but the method can be developed in follow-up bugs to cover all areas.
The is_patron_inside_charge_limits method is now to be used where any of the three noissuescharge sysprefs are used. It will look at the patron category based on either a borrowernumber or patron object and return a hash detailing the charges, the charge limit and whether the patron is over the limit

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees
Comment 18 Matt Blenkinsop 2024-05-02 09:55:31 UTC
Created attachment 166046 [details] [review]
Bug 28924: Update all usages of the current sysprefs

This patch updates all instances where the current noissuescharge sysprefs are used. They will now use the is_patron_inside_charge_limits method to handle the patron category level limits
Comment 19 Matt Blenkinsop 2024-05-02 09:55:34 UTC
Created attachment 166047 [details] [review]
Bug 28924: Add a template method and update template usages

This patch adds a method to be used in the template where currently the Koha.Preference('noissuescharge') method is used. This currently does not take into account the new patron category level limits and so needs to be updated to its own method to handle these.
Comment 20 Matt Blenkinsop 2024-05-02 09:55:36 UTC
Created attachment 166048 [details] [review]
Bug 28924: Add unit tests for new methods and update existing tests

This commit adds unit tests for the two new methods introduced and also updates all unit tests that use the noissuecharge sysprefs

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Test plan:
prove t/db_dependent/SIP/Transaction.t
prove t/db_dependent/SIP/Patron.t
prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
prove t/db_dependent/Koha/Patron.t
Comment 21 David Nind 2024-05-06 00:29:58 UTC
Created attachment 166171 [details] [review]
Bug 28924: Add 3 new columns to categories table

This patch adds three new columns to the patron categories table:
noissuescharge
noissueschargegurantees
noissueschargeguarantorswithguarantees

These values will allow charge limits to be set at category level rather than globally for all patrons. If the values are not set at category level then the system will use the global level values

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2024-05-06 00:30:01 UTC
Created attachment 166172 [details] [review]
Bug 28924: Add DBIC file

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 David Nind 2024-05-06 00:30:05 UTC
Created attachment 166173 [details] [review]
Bug 28924: Add new columns to UI and controller

This patch adds the columns in the table and also updates the controller to allow these fields to be updated in the database from the UI form

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2024-05-06 00:30:08 UTC
Created attachment 166174 [details] [review]
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods

This patch adds two new methods. can_borrow is a top-level method for determining whether a patron is able to borrow. Currently this logic is repeataed in multiple methods and shuld be more DRY. This bug is specific to patron charge limits so that is the focus but the method can be developed in follow-up bugs to cover all areas.
The is_patron_inside_charge_limits method is now to be used where any of the three noissuescharge sysprefs are used. It will look at the patron category based on either a borrowernumber or patron object and return a hash detailing the charges, the charge limit and whether the patron is over the limit

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2024-05-06 00:30:12 UTC
Created attachment 166175 [details] [review]
Bug 28924: Update all usages of the current sysprefs

This patch updates all instances where the current noissuescharge sysprefs are used. They will now use the is_patron_inside_charge_limits method to handle the patron category level limits

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2024-05-06 00:30:14 UTC
Created attachment 166176 [details] [review]
Bug 28924: Add a template method and update template usages

This patch adds a method to be used in the template where currently the Koha.Preference('noissuescharge') method is used. This currently does not take into account the new patron category level limits and so needs to be updated to its own method to handle these.

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2024-05-06 00:30:17 UTC
Created attachment 166177 [details] [review]
Bug 28924: Add unit tests for new methods and update existing tests

This commit adds unit tests for the two new methods introduced and also updates all unit tests that use the noissuecharge sysprefs

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Test plan:
prove t/db_dependent/SIP/Transaction.t
prove t/db_dependent/SIP/Patron.t
prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
prove t/db_dependent/Koha/Patron.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 Katrin Fischer 2024-05-06 06:17:52 UTC
I know I am late to this... but I've been thinking how "noissue" has never been a terribly good name. Wouldn't this be a chance to improve on it? Especially since "issue" is on the terminology list as no-use. But I see a point in consistency as the old prefs remain. (I hope I will survive this comment)
Comment 29 Nick Clemens (kidclamp) 2024-06-13 14:55:36 UTC
Patch doesn't apply, please rebase

From reading the code:
is_patron_inside_charge_limits checks for no issues charge limits like category/pref/defaults to 0
then you test if the charges are defined - but they always will be, because default 0

CanBookBeIssued then also checks if they look like a number, and they always will because defaulted to 0 - before these patches leaving the pref blank means it isn't active (except noissuescharge) I will attach a test I think will fail after applying, check if I am right there.

The two new routines in Patron.pm both take borrower arguments - is there a reason not to simply use 'self'?

circ/circulation.pl line 590
You add 'noissues=>1' without checking the fines override pref - is this correct?

The new GetPatronCharges routine in the Categories template plugin seems misplaced - I think we should be using your new routines directly from the patron object in the templates - it looks like there are other variables related to charges in the scripts - so we should try to avoid double work here and pass more info forward if we have already fetched it.

I don't think the tests are covering all cases and should be expanded a bit.

This is just a quick pass, so let me know if anything needs clarification
Comment 30 Matt Blenkinsop 2024-06-14 15:12:12 UTC
Created attachment 167719 [details] [review]
Bug 28924: Add 3 new columns to categories table

This patch adds three new columns to the patron categories table:
noissuescharge
noissueschargegurantees
noissueschargeguarantorswithguarantees

These values will allow charge limits to be set at category level rather than globally for all patrons. If the values are not set at category level then the system will use the global level values
Comment 31 Matt Blenkinsop 2024-06-14 15:12:14 UTC
Created attachment 167720 [details] [review]
Bug 28924: Add DBIC file
Comment 32 Matt Blenkinsop 2024-06-14 15:12:20 UTC
Created attachment 167721 [details] [review]
Bug 28924: Add new columns to UI and controller

This patch adds the columns in the table and also updates the controller to allow these fields to be updated in the database from the UI form
Comment 33 Matt Blenkinsop 2024-06-14 15:12:23 UTC
Created attachment 167722 [details] [review]
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods

This patch adds two new methods. can_borrow is a top-level method for determining whether a patron is able to borrow. Currently this logic is repeataed in multiple methods and shuld be more DRY. This bug is specific to patron charge limits so that is the focus but the method can be developed in follow-up bugs to cover all areas.
The is_patron_inside_charge_limits method is now to be used where any of the three noissuescharge sysprefs are used. It will look at the patron category based on either a borrowernumber or patron object and return a hash detailing the charges, the charge limit and whether the patron is over the limit

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees
Comment 34 Matt Blenkinsop 2024-06-14 15:12:27 UTC
Created attachment 167723 [details] [review]
Bug 28924: Update all usages of the current sysprefs

This patch updates all instances where the current noissuescharge sysprefs are used. They will now use the is_patron_inside_charge_limits method to handle the patron category level limits
Comment 35 Matt Blenkinsop 2024-06-14 15:12:30 UTC
Created attachment 167724 [details] [review]
Bug 28924: Add a template method and update template usages

This patch adds a method to be used in the template where currently the Koha.Preference('noissuescharge') method is used. This currently does not take into account the new patron category level limits and so needs to be updated to its own method to handle these.
Comment 36 Matt Blenkinsop 2024-06-14 15:12:33 UTC
Created attachment 167725 [details] [review]
Bug 28924: Add unit tests for new methods and update existing tests

This commit adds unit tests for the two new methods introduced and also updates all unit tests that use the noissuecharge sysprefs

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Test plan:
prove t/db_dependent/SIP/Transaction.t
prove t/db_dependent/SIP/Patron.t
prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
prove t/db_dependent/Koha/Patron.t
Comment 37 Matt Blenkinsop 2024-06-14 15:12:37 UTC
Created attachment 167726 [details] [review]
Bug 28924: (QA follow-up) Use $self instead of $patron
Comment 38 Matt Blenkinsop 2024-06-14 15:12:40 UTC
Created attachment 167727 [details] [review]
Bug 28924: (QA follow-up): Don't define charge values as 0
Comment 39 Matt Blenkinsop 2024-06-14 15:12:43 UTC
Created attachment 167728 [details] [review]
Bug 28924: (QA follow-up): Remove unnecessary template method
Comment 40 Matt Blenkinsop 2024-06-14 15:12:47 UTC
Created attachment 167729 [details] [review]
Bug 28924: (QA follow-up): Fix unit tests
Comment 41 Matt Blenkinsop 2024-06-14 15:15:22 UTC
Created attachment 167730 [details] [review]
Bug 28924: Add 3 new columns to categories table

This patch adds three new columns to the patron categories table:
noissuescharge
noissueschargegurantees
noissueschargeguarantorswithguarantees

These values will allow charge limits to be set at category level rather than globally for all patrons. If the values are not set at category level then the system will use the global level values

Signed-off-by: David Nind <david@davidnind.com>
Comment 42 Matt Blenkinsop 2024-06-14 15:15:26 UTC
Created attachment 167731 [details] [review]
Bug 28924: Add DBIC file

Signed-off-by: David Nind <david@davidnind.com>
Comment 43 Matt Blenkinsop 2024-06-14 15:15:31 UTC
Created attachment 167732 [details] [review]
Bug 28924: Add new columns to UI and controller

This patch adds the columns in the table and also updates the controller to allow these fields to be updated in the database from the UI form

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 Matt Blenkinsop 2024-06-14 15:15:35 UTC
Created attachment 167733 [details] [review]
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods

This patch adds two new methods. can_borrow is a top-level method for determining whether a patron is able to borrow. Currently this logic is repeataed in multiple methods and shuld be more DRY. This bug is specific to patron charge limits so that is the focus but the method can be developed in follow-up bugs to cover all areas.
The is_patron_inside_charge_limits method is now to be used where any of the three noissuescharge sysprefs are used. It will look at the patron category based on either a borrowernumber or patron object and return a hash detailing the charges, the charge limit and whether the patron is over the limit

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 Matt Blenkinsop 2024-06-14 15:15:39 UTC
Created attachment 167734 [details] [review]
Bug 28924: Update all usages of the current sysprefs

This patch updates all instances where the current noissuescharge sysprefs are used. They will now use the is_patron_inside_charge_limits method to handle the patron category level limits

Signed-off-by: David Nind <david@davidnind.com>
Comment 46 Matt Blenkinsop 2024-06-14 15:15:43 UTC
Created attachment 167735 [details] [review]
Bug 28924: Add a template method and update template usages

This patch adds a method to be used in the template where currently the Koha.Preference('noissuescharge') method is used. This currently does not take into account the new patron category level limits and so needs to be updated to its own method to handle these.

Signed-off-by: David Nind <david@davidnind.com>
Comment 47 Matt Blenkinsop 2024-06-14 15:15:47 UTC
Created attachment 167736 [details] [review]
Bug 28924: Add unit tests for new methods and update existing tests

This commit adds unit tests for the two new methods introduced and also updates all unit tests that use the noissuecharge sysprefs

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Test plan:
prove t/db_dependent/SIP/Transaction.t
prove t/db_dependent/SIP/Patron.t
prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
prove t/db_dependent/Koha/Patron.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 48 Matt Blenkinsop 2024-06-14 15:15:51 UTC
Created attachment 167737 [details] [review]
Bug 28924: (QA follow-up) Use $self instead of $patron
Comment 49 Matt Blenkinsop 2024-06-14 15:15:55 UTC
Created attachment 167738 [details] [review]
Bug 28924: (QA follow-up): Don't define charge values as 0
Comment 50 Matt Blenkinsop 2024-06-14 15:15:58 UTC
Created attachment 167739 [details] [review]
Bug 28924: (QA follow-up): Remove unnecessary template method
Comment 51 Matt Blenkinsop 2024-06-14 15:16:03 UTC
Created attachment 167740 [details] [review]
Bug 28924: (QA follow-up): Fix unit tests
Comment 52 Matt Blenkinsop 2024-06-14 15:20:49 UTC
Forgot David's sign off lines first time round

(In reply to Nick Clemens (kidclamp) from comment #29)
> Patch doesn't apply, please rebase

Rebased


> From reading the code:
> is_patron_inside_charge_limits checks for no issues charge limits like
> category/pref/defaults to 0
> then you test if the charges are defined - but they always will be, because
> default 0

Looks like I was confusing a falsy value with an undefined value - good spot. I've amended this now to not set the default


> CanBookBeIssued then also checks if they look like a number, and they always
> will because defaulted to 0 - before these patches leaving the pref blank
> means it isn't active (except noissuescharge) I will attach a test I think
> will fail after applying, check if I am right there.

As above

> The two new routines in Patron.pm both take borrower arguments - is there a
> reason not to simply use 'self'?

They should definitely be using $self, not sure what I was doing there
 
> circ/circulation.pl line 590
> You add 'noissues=>1' without checking the fines override pref - is this
> correct?

Don't think that should be there, have removed it
 
> The new GetPatronCharges routine in the Categories template plugin seems
> misplaced - I think we should be using your new routines directly from the
> patron object in the templates - it looks like there are other variables
> related to charges in the scripts - so we should try to avoid double work
> here and pass more info forward if we have already fetched it.

I've removed the method and passed the data through. The values already existed in the template so just needed adding into the inc files

> I don't think the tests are covering all cases and should be expanded a bit.

Can you give an example of a case you think is missing?

> This is just a quick pass, so let me know if anything needs clarification
Comment 53 Martin Renvoize (ashimema) 2024-07-09 14:18:34 UTC
Need to add the 'Sponsored-by: Cuyahoga' here Matt
Comment 54 Nick Clemens (kidclamp) 2024-07-10 13:31:59 UTC
Created attachment 168728 [details] [review]
Bug 28924: Add 3 new columns to categories table

This patch adds three new columns to the patron categories table:
noissuescharge
noissueschargegurantees
noissueschargeguarantorswithguarantees

These values will allow charge limits to be set at category level rather than globally for all patrons. If the values are not set at category level then the system will use the global level values

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Nick Clemens (kidclamp) 2024-07-10 13:32:02 UTC
Created attachment 168729 [details] [review]
Bug 28924: Add DBIC file

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens (kidclamp) 2024-07-10 13:32:06 UTC
Created attachment 168730 [details] [review]
Bug 28924: Add new columns to UI and controller

This patch adds the columns in the table and also updates the controller to allow these fields to be updated in the database from the UI form

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens (kidclamp) 2024-07-10 13:32:09 UTC
Created attachment 168731 [details] [review]
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods

This patch adds two new methods. can_borrow is a top-level method for determining whether a patron is able to borrow. Currently this logic is repeataed in multiple methods and shuld be more DRY. This bug is specific to patron charge limits so that is the focus but the method can be developed in follow-up bugs to cover all areas.
The is_patron_inside_charge_limits method is now to be used where any of the three noissuescharge sysprefs are used. It will look at the patron category based on either a borrowernumber or patron object and return a hash detailing the charges, the charge limit and whether the patron is over the limit

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Nick Clemens (kidclamp) 2024-07-10 13:32:12 UTC
Created attachment 168732 [details] [review]
Bug 28924: Update all usages of the current sysprefs

This patch updates all instances where the current noissuescharge sysprefs are used. They will now use the is_patron_inside_charge_limits method to handle the patron category level limits

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 59 Nick Clemens (kidclamp) 2024-07-10 13:32:16 UTC
Created attachment 168733 [details] [review]
Bug 28924: Add a template method and update template usages

This patch adds a method to be used in the template where currently the Koha.Preference('noissuescharge') method is used. This currently does not take into account the new patron category level limits and so needs to be updated to its own method to handle these.

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 60 Nick Clemens (kidclamp) 2024-07-10 13:32:19 UTC
Created attachment 168734 [details] [review]
Bug 28924: Add unit tests for new methods and update existing tests

This commit adds unit tests for the two new methods introduced and also updates all unit tests that use the noissuecharge sysprefs

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Test plan:
prove t/db_dependent/SIP/Transaction.t
prove t/db_dependent/SIP/Patron.t
prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
prove t/db_dependent/Koha/Patron.t

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 61 Nick Clemens (kidclamp) 2024-07-10 13:32:22 UTC
Created attachment 168735 [details] [review]
Bug 28924: (QA follow-up) Use $self instead of $patron

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 62 Nick Clemens (kidclamp) 2024-07-10 13:32:25 UTC
Created attachment 168736 [details] [review]
Bug 28924: (QA follow-up): Don't define charge values as 0

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 63 Nick Clemens (kidclamp) 2024-07-10 13:32:28 UTC
Created attachment 168737 [details] [review]
Bug 28924: (QA follow-up): Remove unnecessary template method

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 64 Nick Clemens (kidclamp) 2024-07-10 13:32:31 UTC
Created attachment 168738 [details] [review]
Bug 28924: (QA follow-up): Fix unit tests

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 65 Nick Clemens (kidclamp) 2024-07-10 13:33:52 UTC
(In reply to Matt Blenkinsop from comment #52)

> 
> Can you give an example of a case you think is missing?
> 
On second pass these looked sufficient :-)
Comment 66 Katrin Fischer 2024-07-11 11:31:26 UTC
1) Is this correct?

         [% IF ( charges ) %]
-            [% INCLUDE 'blocked-fines.inc' borrowernumber = patron.borrowernumber %]
+            [% INCLUDE 'blocked-fines.inc' NoIssuesCharge = chargesamount %]
         [% END %]

It looks like we are losing the borrowernumber? 

2) Terminology

https://wiki.koha-community.org/wiki/Terminology

a) +=head3 can_borrow
...
+This method determines whether a borrower is able to borrow based on various parameters.
+- Debarrments

I feel like this should be "can_checkout" to follow our terminology guide and because borrow/lend is a common one to get confused by non-native speakers.

Also some small stuff borrower = patron etc.

b) noissuecharge

The noissuecharge parameter is badly named and has always been. I feel like we will never will fully recover from it as it has spread to other system preferences by now, but I wonder if we could at least start to move into a better direction with the new database table columns:

noissuescharge
noissueschargeguarantees
noissueschargeguarantorswithguarantees

I can see the consistency here, so this is more a question than a blocker.
I was thinking along the lines of checkout_charge_limit or so.

c) Templates

So this is a blocker for me, because I can see the consistency with the codes/prefs, but I think we can do better in the templates and stick to the agreed on terms:

+                    <th scope="col">No issues charge</th>
+                    <th scope="col">No issues charge guarantees</th>
+                    <th scope="col">No issues charge guarantors with guarantees</th>

Also in the input form.

What about: 
Checkout charge limit... ? And maybe a good hint for the input form with reference and link to noissuescharge and the other preferences if we want to make that connection visible.

3) Don't include api/v1/swagger/swagger_bundle.json

It's in this patch:
Subject: [PATCH] Bug 28924: Add new columns to UI and controller

It will be automatically generated.

4) Translatability

This smells untranslatable:

-        my $noissuescharge = C4::Context->preference("noissuescharge") || 5;
-        $flaginfo{'message'} = sprintf 'Patron owes %.02f', $owing;
-        $flaginfo{'amount'}  = sprintf "%.02f", $owing;
-        if ( $owing > $noissuescharge && !C4::Context->preference("AllowFineOverride") ) {
+        my $noissuescharge = $patron_charge_limits->{noissuescharge}->{limit} || 5;
+        $flaginfo{'message'} = sprintf 'Patron owes %.02f', $patron_charge_limits->{noissuescharge}->{charge};
+        $flaginfo{'amount'}  = sprintf "%.02f", $patron_charge_limits->{noissuescharge}->{charge};

-            $flaginfo{'message'} = sprintf 'patron guarantees owe %.02f', $guarantees_non_issues_charges;
-            $flaginfo{'amount'}  = $guarantees_non_issues_charges;
+            $flaginfo{'message'} = sprintf 'patron guarantees owe %.02f', $patron_charge_limits->{NoIssuesChargeGuarantees}->{charge};
+            $flaginfo{'amount'}  = $patron_charge_limits->{NoIssuesChargeGuarantees}->{charge};


Also the Price formatting will be wrong (CurrentyFormat). I am not sure where this is used?

5) Documentation

+  `noissuescharge` int(11) DEFAULT NULL COMMENT 'define maximum amount withstanding before checkouts are blocked',
+  `noissueschargeguarantees` int(11) DEFAULT NULL COMMENT 'define maximum amount withstanding before checkouts are blocked',
+  `noissueschargeguarantorswithguarantees` int(11) DEFAULT NULL COMMENT 'define maximum amount withstanding before checkouts are blocked',

These all have the same description - what is the difference?

6) Formatting

Please use the $Price TT filter for proper formatting of the prices on display, including the 0.00 default:

+                        [% IF (category.noissuescharge) %]
+                            <td>[% category.noissuescharge | html %]</td>
+                        [% ELSE %]
+                            <td>0.00</td>
+                        [% END %]
+                        [% IF (category.noissueschargeguarantees) %]
+                            <td>[% category.noissueschargeguarantees | html %]</td>
+                        [% ELSE %]
+                            <td>0.00</td>
+                        [% END %]
+                        [% IF (category.noissueschargeguarantorswithguarantees) %]
+                            <td>[% category.noissueschargeguarantorswithguarantees | html %]</td>
+                        [% ELSE %]
+                            <td>0.00</td>
+                        [% END %]

7) Price handling on input fields

You need to use $Price on_editing on the input fields, I believe:

Example:
<input class="decimal" type="text" size="20" name="rrp" id="rrp" value="[% rrp | $Price on_editing => 1 %]
Comment 67 Matt Blenkinsop 2024-07-15 14:22:19 UTC
Created attachment 168984 [details] [review]
Bug 28924: Add 3 new columns to categories table

This patch adds three new columns to the patron categories table:
noissuescharge
noissueschargegurantees
noissueschargeguarantorswithguarantees

These values will allow charge limits to be set at category level rather than globally for all patrons. If the values are not set at category level then the system will use the global level values

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 68 Matt Blenkinsop 2024-07-15 14:22:22 UTC
Created attachment 168985 [details] [review]
Bug 28924: Add DBIC file

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 69 Matt Blenkinsop 2024-07-15 14:22:25 UTC
Created attachment 168986 [details] [review]
Bug 28924: Add new columns to UI and controller

This patch adds the columns in the table and also updates the controller to allow these fields to be updated in the database from the UI form

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 70 Matt Blenkinsop 2024-07-15 14:22:28 UTC
Created attachment 168987 [details] [review]
Bug 28924: Add can_borrow and is_patron_inside_charge_limits methods

This patch adds two new methods. can_borrow is a top-level method for determining whether a patron is able to borrow. Currently this logic is repeataed in multiple methods and shuld be more DRY. This bug is specific to patron charge limits so that is the focus but the method can be developed in follow-up bugs to cover all areas.
The is_patron_inside_charge_limits method is now to be used where any of the three noissuescharge sysprefs are used. It will look at the patron category based on either a borrowernumber or patron object and return a hash detailing the charges, the charge limit and whether the patron is over the limit

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 71 Matt Blenkinsop 2024-07-15 14:22:31 UTC
Created attachment 168988 [details] [review]
Bug 28924: Update all usages of the current sysprefs

This patch updates all instances where the current noissuescharge sysprefs are used. They will now use the is_patron_inside_charge_limits method to handle the patron category level limits

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 72 Matt Blenkinsop 2024-07-15 14:22:34 UTC
Created attachment 168989 [details] [review]
Bug 28924: Add a template method and update template usages

This patch adds a method to be used in the template where currently the Koha.Preference('noissuescharge') method is used. This currently does not take into account the new patron category level limits and so needs to be updated to its own method to handle these.

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 73 Matt Blenkinsop 2024-07-15 14:22:37 UTC
Created attachment 168990 [details] [review]
Bug 28924: Add unit tests for new methods and update existing tests

This commit adds unit tests for the two new methods introduced and also updates all unit tests that use the noissuecharge sysprefs

Test plan:
1. Choose a patron and note their patron category
2. Create a manual invoice on that patron for 6.00
3. Try to check an item out to the patron and it will show a message saying that checkout is blocked because the fine balance is over the limit.
4. This is because the default global value for the ‘noissuescharge’ is 5.00 and we are now over that limit
5. Navigate to Administration > Patron categories and click to edit the category relevant to your patron
6. At the bottom of the form will be fields called No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees. Set those fields to 7
7. Now try and checkout again - this time you will be allowed to checkout as the category level limit is higher than the fine we set.
8. Choose a different patron (patron 2) with a category that is different to the first patron (patron 1).
9. Repeat steps 2 and 3. This time checkout will be blocked again because the limit for this patron category is still the global value of 5.00
10. Choose a third patron with a category of child or similar (patron 3)
11. Make patron 1 a guarantor for patron 3
12. Edit patron 3’s patron category and set the limit for No Issues Charge, No Issues Charge Guarantees and No Issues Charge Guarantors With Guarantees to 5.
13. Try to check out to patron 3 and it will be blocked due to the fines on patron 1’s account
14. Try to checkout to patron 1 - it should still be possible
15. Add a fine to patron 3’s account for 2.00
16. Try to check out to patron 1 - the checkout will be blocked due to the total of 8 now owed by the patron and its guarantees

Test plan:
prove t/db_dependent/SIP/Transaction.t
prove t/db_dependent/SIP/Patron.t
prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
prove t/db_dependent/Koha/Patron.t

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 74 Matt Blenkinsop 2024-07-15 14:22:40 UTC
Created attachment 168991 [details] [review]
Bug 28924: (QA follow-up) Use $self instead of $patron

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Matt Blenkinsop 2024-07-15 14:22:43 UTC
Created attachment 168992 [details] [review]
Bug 28924: (QA follow-up): Don't define charge values as 0

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 76 Matt Blenkinsop 2024-07-15 14:22:46 UTC
Created attachment 168993 [details] [review]
Bug 28924: (QA follow-up): Remove unnecessary template method

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 77 Matt Blenkinsop 2024-07-15 14:22:49 UTC
Created attachment 168994 [details] [review]
Bug 28924: (QA follow-up): Fix unit tests

Sponsored-by: Cuyahoga County Public Library <https://cuyahogalibrary.org/>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 78 Matt Blenkinsop 2024-07-15 14:22:53 UTC
Created attachment 168995 [details] [review]
Bug 28924: (QA follow-up) Terminology fixes
Comment 79 Matt Blenkinsop 2024-07-15 14:23:41 UTC
(In reply to Katrin Fischer from comment #66)
> 1) Is this correct?
> 
>          [% IF ( charges ) %]
> -            [% INCLUDE 'blocked-fines.inc' borrowernumber =
> patron.borrowernumber %]
> +            [% INCLUDE 'blocked-fines.inc' NoIssuesCharge = chargesamount %]
>          [% END %]
> 
> It looks like we are losing the borrowernumber? 

The patron is still picked up by the template and populates the links correctly

> 
> 2) Terminology
> 
> https://wiki.koha-community.org/wiki/Terminology
> 
> a) +=head3 can_borrow
> ...
> +This method determines whether a borrower is able to borrow based on
> various parameters.
> +- Debarrments
> 
> I feel like this should be "can_checkout" to follow our terminology guide
> and because borrow/lend is a common one to get confused by non-native
> speakers.
> 
> Also some small stuff borrower = patron etc.

Fixed :) 

> 
> b) noissuecharge
> 
> The noissuecharge parameter is badly named and has always been. I feel like
> we will never will fully recover from it as it has spread to other system
> preferences by now, but I wonder if we could at least start to move into a
> better direction with the new database table columns:
> 
> noissuescharge
> noissueschargeguarantees
> noissueschargeguarantorswithguarantees
> 
> I can see the consistency here, so this is more a question than a blocker.
> I was thinking along the lines of checkout_charge_limit or so.

I think this probably needs breaking out into a separate bug with some discussion about how to change the terminology and agreement on the wording to use. Then this probably needs to be implemented across Koha more widely including the original sysprefs rather than doing it in just one place otherwise we risk introducing even more inconsistency in these patches

> 
> c) Templates
> 
> So this is a blocker for me, because I can see the consistency with the
> codes/prefs, but I think we can do better in the templates and stick to the
> agreed on terms:
> 
> +                    <th scope="col">No issues charge</th>
> +                    <th scope="col">No issues charge guarantees</th>
> +                    <th scope="col">No issues charge guarantors with
> guarantees</th>
> 
> Also in the input form.
> 
> What about: 
> Checkout charge limit... ? And maybe a good hint for the input form with
> reference and link to noissuescharge and the other preferences if we want to
> make that connection visible.

I've updated these as suggested and added a hint

 
> 3) Don't include api/v1/swagger/swagger_bundle.json
> 
> It's in this patch:
> Subject: [PATCH] Bug 28924: Add new columns to UI and controller
> 
> It will be automatically generated.

Done, its in gitignore so not sure how it made it into the patch

> 
> 4) Translatability
> 
> This smells untranslatable:
> 
> -        my $noissuescharge = C4::Context->preference("noissuescharge") || 5;
> -        $flaginfo{'message'} = sprintf 'Patron owes %.02f', $owing;
> -        $flaginfo{'amount'}  = sprintf "%.02f", $owing;
> -        if ( $owing > $noissuescharge &&
> !C4::Context->preference("AllowFineOverride") ) {
> +        my $noissuescharge =
> $patron_charge_limits->{noissuescharge}->{limit} || 5;
> +        $flaginfo{'message'} = sprintf 'Patron owes %.02f',
> $patron_charge_limits->{noissuescharge}->{charge};
> +        $flaginfo{'amount'}  = sprintf "%.02f",
> $patron_charge_limits->{noissuescharge}->{charge};
> 
> -            $flaginfo{'message'} = sprintf 'patron guarantees owe %.02f',
> $guarantees_non_issues_charges;
> -            $flaginfo{'amount'}  = $guarantees_non_issues_charges;
> +            $flaginfo{'message'} = sprintf 'patron guarantees owe %.02f',
> $patron_charge_limits->{NoIssuesChargeGuarantees}->{charge};
> +            $flaginfo{'amount'}  =
> $patron_charge_limits->{NoIssuesChargeGuarantees}->{charge};
> 
> 
> Also the Price formatting will be wrong (CurrentyFormat). I am not sure
> where this is used?

This is only used in the SIP module when creating the patron object. There isn't any template that I can see that this gets rendered into as I assume this is handled by the hardware supplier that SIP is connecting to

> 
> 5) Documentation
> 
> +  `noissuescharge` int(11) DEFAULT NULL COMMENT 'define maximum amount
> withstanding before checkouts are blocked',
> +  `noissueschargeguarantees` int(11) DEFAULT NULL COMMENT 'define maximum
> amount withstanding before checkouts are blocked',
> +  `noissueschargeguarantorswithguarantees` int(11) DEFAULT NULL COMMENT
> 'define maximum amount withstanding before checkouts are blocked',
> 
> These all have the same description - what is the difference?

Fixed the descriptions, clearly didn't change them after a copy and paste job!

> 
> 6) Formatting
> 
> Please use the $Price TT filter for proper formatting of the prices on
> display, including the 0.00 default:
> 
> +                        [% IF (category.noissuescharge) %]
> +                            <td>[% category.noissuescharge | html %]</td>
> +                        [% ELSE %]
> +                            <td>0.00</td>
> +                        [% END %]
> +                        [% IF (category.noissueschargeguarantees) %]
> +                            <td>[% category.noissueschargeguarantees | html
> %]</td>
> +                        [% ELSE %]
> +                            <td>0.00</td>
> +                        [% END %]
> +                        [% IF
> (category.noissueschargeguarantorswithguarantees) %]
> +                            <td>[%
> category.noissueschargeguarantorswithguarantees | html %]</td>
> +                        [% ELSE %]
> +                            <td>0.00</td>
> +                        [% END %]
> 

Done

> 7) Price handling on input fields
> 
> You need to use $Price on_editing on the input fields, I believe:
> 
> Example:
> <input class="decimal" type="text" size="20" name="rrp" id="rrp" value="[%
> rrp | $Price on_editing => 1 %]


Done
Comment 80 Katrin Fischer 2024-07-18 16:10:34 UTC
Thanks for the follow-up!
Comment 81 Katrin Fischer 2024-07-18 16:27:26 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 82 Katrin Fischer 2024-07-18 18:10:03 UTC
I believe this might have broken the tests:
t_db_dependent_api_v1_patron_categories_t
Build #636
Comment 83 Matt Blenkinsop 2024-07-19 07:17:02 UTC
Created attachment 169177 [details] [review]
Bug 28924: (follow-up): Fix API spec

This patch fixes the API definition for a patron category, as highlighted by the failing tests
Comment 84 Victor Grousset/tuxayo 2024-07-22 19:49:30 UTC
Here, something unintended made a DBIC schema file inconsistent and it blocks it's autogeneration, see Bug 37426
Comment 85 Martin Renvoize (ashimema) 2024-07-23 14:55:14 UTC
Created attachment 169428 [details] [review]
Bug 28924: (follow-up) Decimal points count as characters

The restriction to 3 characters in the new patron category fields was
too strict.. the default for $Price on_editing is 0.00 (makes sense
right).. and that's 4 Characters so we were failing validation.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 86 Martin Renvoize (ashimema) 2024-07-23 14:56:30 UTC
More failing tests so I've pushed the follow-up above to make those happy..

A '.' counts as a character.. who would think ;P