Bug 19382 - Add ability to block guarantees based on fees owed by guarantor and other guarantees
Summary: Add ability to block guarantees based on fees owed by guarantor and other gua...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 14570 14577
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-28 13:01 UTC by Kyle M Hall
Modified: 2020-11-13 14:46 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (38.82 KB, patch)
2020-01-10 20:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (38.82 KB, patch)
2020-01-10 20:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (38.84 KB, patch)
2020-01-13 11:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (38.99 KB, patch)
2020-02-18 18:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (39.08 KB, patch)
2020-03-30 17:36 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (39.54 KB, patch)
2020-07-05 01:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19382: Add blocking of ability to checkout on circulation.pl (6.16 KB, patch)
2020-07-24 13:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (39.46 KB, patch)
2020-08-26 22:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19382: Add blocking of ability to checkout on circulation.pl (6.23 KB, patch)
2020-08-26 22:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19382: Tests refactoring (13.85 KB, patch)
2020-08-31 10:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19382: Tests refactoring [alternate] (5.08 KB, patch)
2020-08-31 12:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (39.44 KB, patch)
2020-09-03 18:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add blocking of ability to checkout on circulation.pl (5.85 KB, patch)
2020-09-03 18:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: (QA follow-up) Throw exception if only_this_guarantor is set for a non-guarantor (23.92 KB, patch)
2020-09-03 18:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: (follow-up) Alternate tests (36.25 KB, patch)
2020-09-10 10:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19382: (follow-up) Alternate tests (36.32 KB, patch)
2020-09-11 12:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees (39.47 KB, patch)
2020-10-13 17:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Add blocking of ability to checkout on circulation.pl (5.85 KB, patch)
2020-10-13 17:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: (QA follow-up) Throw exception if only_this_guarantor is set for a non-guarantor (23.92 KB, patch)
2020-10-13 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: (follow-up) Alternate tests (36.32 KB, patch)
2020-10-13 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: (QA follow-up) Fix typos (3.04 KB, patch)
2020-10-13 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19382: Adjust comment in test (1.03 KB, patch)
2020-10-14 12:20 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-09-28 13:01:39 UTC
Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.
Comment 1 Kyle M Hall 2020-01-10 20:26:16 UTC
Created attachment 97234 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items
Comment 2 Kyle M Hall 2020-01-10 20:34:36 UTC
Created attachment 97235 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items
Comment 3 Katrin Fischer 2020-01-11 20:35:38 UTC
Should this be NSO?
Comment 4 Kyle M Hall 2020-01-13 11:42:28 UTC
Created attachment 97303 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items
Comment 5 Donna 2020-02-14 16:59:03 UTC
Updated testing plan:
Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3.  Set NoIssuesChargeGuarantees to $14
4) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
5) Give 3 of the 4 a $5 fine
6) None of them should be able to check out items
Comment 6 Donna 2020-02-14 17:02:07 UTC
When checking out to parent, the total that shows is the total fine amount for the guarantees, does not include the other guarantor dollar amount.

For example, Child 1 owes $5, Child 2 owes $5, Parent 1 owes $5.  When parent 2 checks out, message states $10 owed by guarantees.

When Child 3 (no fines) checks out, message states: The patron's guarantors and their other guarantees collectively have a debt of 15.00.
Comment 7 Kyle M Hall 2020-02-18 18:12:15 UTC
Created attachment 99196 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items
Comment 8 Barbara Johnson 2020-03-30 17:34:55 UTC
The test plan worked exactly as stated so I will sign off on it.  

I have some additional observations.  The blocking message during checkout showing the total amount owed by the family of four is great.  However, it seems like it will take some time to locate who has the fines in order to make payments and get the family below the threshold amount for blocking.

It would be helpful if the blocking message on circulation.pl could include a summary on the screen that listed each family member with their associated fines along with clickable links to make immediate payments.  I can open a new buf that addresses this extra functionality.
Comment 9 ByWater Sandboxes 2020-03-30 17:36:30 UTC
Created attachment 102090 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 10 Katrin Fischer 2020-07-05 01:05:33 UTC
Created attachment 106534 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 11 Katrin Fischer 2020-07-05 01:05:48 UTC
What I've done on a fresh database:

- Apply patch
- Run updatedatabase
- restart_all
- NoIssuesChargeGuarantorsWithGuarantees to 1
- Added a daughter to one of the existing patrons
- Added a 2$ manual invoice to both
- Both can still check out and no warning is shown.

Can you check please?

I have uploaded the patches as I did a minor rebase (sysprefs.sql)
Comment 12 Katrin Fischer 2020-07-05 01:07:46 UTC
(In reply to Katrin Fischer from comment #11)
> What I've done on a fresh database:
> 
> - Apply patch
> - Run updatedatabase
> - restart_all
> - NoIssuesChargeGuarantorsWithGuarantees to 1
> - Added a daughter to one of the existing patrons
> - Added a 2$ manual invoice to both
> - Both can still check out and no warning is shown.
> 
> Can you check please?
> 
> I have uploaded the patches as I did a minor rebase (sysprefs.sql)

Also tried adding a second daughter - not blocked either :(
Comment 13 Kyle M Hall 2020-07-24 13:00:31 UTC
Created attachment 107344 [details] [review]
Bug 19382: Add blocking of ability to checkout on circulation.pl
Comment 14 Katrin Fischer 2020-08-22 21:44:39 UTC
Hi Kyle,

I spotted an unresolved TODO:

+        my $guarantees_fines_amount = $flags->{CHARGES_GUARANTEES} ? $flags->{CHARGES_GUARANTEES}->{amount} : 0; #TODO: Replace with $patron->relationships_debt

Do you want to fix this?

The "blocking" worked this time for me. :)
Comment 15 Kyle M Hall 2020-08-24 17:30:26 UTC
(In reply to Katrin Fischer from comment #14)
> Hi Kyle,
> 
> I spotted an unresolved TODO:
> 
> +        my $guarantees_fines_amount = $flags->{CHARGES_GUARANTEES} ?
> $flags->{CHARGES_GUARANTEES}->{amount} : 0; #TODO: Replace with
> $patron->relationships_debt
> 
> Do you want to fix this?

Those are really just ideas for future backend enhancements. I'd be happy to file them!

> The "blocking" worked this time for me. :)

Excellent!
Comment 16 Katrin Fischer 2020-08-26 22:33:04 UTC
Created attachment 109210 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2020-08-26 22:33:08 UTC
Created attachment 109211 [details] [review]
Bug 19382: Add blocking of ability to checkout on circulation.pl

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Jonathan Druart 2020-08-31 10:35:17 UTC
Comment on attachment 109210 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Review of attachment 109210 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Koha/Patron.t
@@ +153,5 @@
> +    is( $child_1->relationships_debt({ only_this_guarantor => 0, include_guarantors => 0, include_this_patron => 0 }), 10, 'Family debt is correct' );
> +    is( $child_1->relationships_debt({ only_this_guarantor => 0, include_guarantors => 0, include_this_patron => 1 }), 10, 'Family debt is correct' );
> +    is( $child_1->relationships_debt({ only_this_guarantor => 0, include_guarantors => 1, include_this_patron => 0 }), 10, 'Family debt is correct' );
> +    is( $child_1->relationships_debt({ only_this_guarantor => 0, include_guarantors => 1, include_this_patron => 1 }), 10, 'Family debt is correct' );
> +    is( $child_1->relationships_debt({ only_this_guarantor => 1, include_guarantors => 0, include_this_patron => 0 }), 0, 'Family debt is correct' );

Is this correct? $child_1 is not a guarantor so I don't think it should follow "only_this_guarantor". I would have expected 10 here, can you confirm?
Comment 19 Jonathan Druart 2020-08-31 10:38:32 UTC
Created attachment 109349 [details] [review]
Bug 19382: Tests refactoring
Comment 20 Jonathan Druart 2020-08-31 10:39:12 UTC
Asking because I was trying to make the tests more readable, see the last patch, some testes are failing.
Comment 21 Kyle M Hall 2020-08-31 12:33:30 UTC
Created attachment 109357 [details] [review]
Bug 19382: Tests refactoring [alternate]
Comment 22 Kyle M Hall 2020-08-31 12:34:54 UTC
(In reply to Jonathan Druart from comment #20)
> Asking because I was trying to make the tests more readable, see the last
> patch, some testes are failing.

I think putting the tests in a loop here just adds obfuscation, especially my attempt ;)
Comment 23 Jonathan Druart 2020-08-31 13:35:30 UTC
Yes indeed.
My idea was to make the tests more readable, and this:

+            $amount = 0 if ( $patron->id == $child_2->id && ( $i == 0 || $i == 2 || $i == 4 || $i == 5 || $i == 6 ) );

is something I'd like to avoid :)


What about the previous question?
(In reply to Jonathan Druart from comment #18)
> > +    is( $child_1->relationships_debt({ only_this_guarantor => 1, include_guarantors => 0, include_this_patron => 0 }), 0, 'Family debt is correct' );
> 
> Is this correct? $child_1 is not a guarantor so I don't think it should
> follow "only_this_guarantor". I would have expected 10 here, can you confirm?
Comment 24 Kyle M Hall 2020-08-31 15:32:41 UTC
(In reply to Jonathan Druart from comment #23)
> Yes indeed.
> My idea was to make the tests more readable, and this:
> 
> +            $amount = 0 if ( $patron->id == $child_2->id && ( $i == 0 || $i
> == 2 || $i == 4 || $i == 5 || $i == 6 ) );
> 
> is something I'd like to avoid :)

Yes, I agree what I wrote there is just awful ;)

> What about the previous question?
> (In reply to Jonathan Druart from comment #18)
> > > +    is( $child_1->relationships_debt({ only_this_guarantor => 1, include_guarantors => 0, include_this_patron => 0 }), 0, 'Family debt is correct' );
> > 
> > Is this correct? $child_1 is not a guarantor so I don't think it should
> > follow "only_this_guarantor". I would have expected 10 here, can you confirm?

In this case, since the patron is not a guarantor, we have no guarantors to get debt from, we are also saying don't include this patron. Basically with this combination for a guarantee, the result will always 0.

I think this makes more sense than simply ignoring only_this_guarantor if the patron is not a guarantor. As it is written it follows a consistent internal logic.
Comment 25 Jonathan Druart 2020-08-31 15:52:10 UTC
Take a look at this:

child 1 debt is correct (only_this_guarantor => 1, include_guarantors => 0, include_this_patron => 0)
=> 0

child 1 debt is correct (only_this_guarantor => 0, include_guarantors => 0, include_this_patron => 0)
=> 10

That does not make a lot of sense IMO, both returns should be the same. Don't you think?
Comment 26 Kyle M Hall 2020-08-31 16:35:40 UTC
(In reply to Jonathan Druart from comment #25)
> Take a look at this:
> 
> child 1 debt is correct (only_this_guarantor => 1, include_guarantors => 0,
> include_this_patron => 0)
> => 0
> 
> child 1 debt is correct (only_this_guarantor => 0, include_guarantors => 0,
> include_this_patron => 0)
> => 10
> 
> That does not make a lot of sense IMO, both returns should be the same.
> Don't you think?

I'll take a look! 
Which line numbers are these tests on?
Comment 27 Jonathan Druart 2020-09-01 10:53:38 UTC
With only the 2 first patches applied:

153     is( $child_1->relationships_debt({ only_this_guarantor => 0, include_guarantors => 0, include_this_patron => 0 }), 10, 'Family debt is correct' );

157     is( $child_1->relationships_debt({ only_this_guarantor => 1, include_guarantors => 0, include_this_patron => 0 }), 0, 'Family debt is correct' );
Comment 28 Kyle M Hall 2020-09-01 12:36:50 UTC
(In reply to Jonathan Druart from comment #27)
> With only the 2 first patches applied:
> 
> 153     is( $child_1->relationships_debt({ only_this_guarantor => 0,
> include_guarantors => 0, include_this_patron => 0 }), 10, 'Family debt is
> correct' );

In this case, we are not specifying only_this_guarantor, and we are specifying not to include this patron, and not to include the guarantors themselves, so the code will total the charges from the guarantors other guarantees. Since the sibling owes 10, the returned value is 10.
 
> 157     is( $child_1->relationships_debt({ only_this_guarantor => 1,
> include_guarantors => 0, include_this_patron => 0 }), 0, 'Family debt is
> correct' );

This one is much simpler, since only_this_guarantor is specified, but the patron is not a guarantor, there are no guarantors to check. No guarantors means no guarantees so the total returned is 0.
Comment 29 Jonathan Druart 2020-09-01 13:33:57 UTC
This is confusing to me.
First, child_1 is not a guarantor so only_this_guarantor should not modify the returned value. Then, in the same logic, only_this_guarantor should not have any effects if include_guarantors is not set.

Asking Tomas and Martin for additional feedbacks.
Comment 30 Tomás Cohen Arazi 2020-09-03 12:33:59 UTC
(In reply to Jonathan Druart from comment #29)
> This is confusing to me.
> First, child_1 is not a guarantor so only_this_guarantor should not modify
> the returned value. Then, in the same logic, only_this_guarantor should not
> have any effects if include_guarantors is not set.
> 
> Asking Tomas and Martin for additional feedbacks.

Shouldn't we just throw an exception on bad combination of parameters? If patron is not a guarantor, we should throw a Koha::Exception::BadParameter if 'only_this_guarantor' is passed, right?
Comment 31 Kyle M Hall 2020-09-03 18:27:40 UTC
Created attachment 109634 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Kyle M Hall 2020-09-03 18:27:56 UTC
Created attachment 109635 [details] [review]
Bug 19382: Add blocking of ability to checkout on circulation.pl

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Kyle M Hall 2020-09-03 18:28:02 UTC
Created attachment 109636 [details] [review]
Bug 19382: (QA follow-up) Throw exception if only_this_guarantor is set for a non-guarantor
Comment 34 Jonathan Druart 2020-09-09 14:10:28 UTC
I am sorry Kyle but I really don't think we should push the tests as they are written. We should expect one loop and a sum following some conditions, like I tried in my patch. That will highlight inconsistencies (like the one I raised in the previous comments).
Also the amounts are always 10, so we cannot know which ones is really picked (reading the tests I mean).

We should improve our test suite, and those tests are not in the right direction in my opinion.

Asking for another QA point of view.
Comment 35 Martin Renvoize 2020-09-09 17:04:07 UTC
Dang; Improveing the tests here is alluding me so far.. I've tried a few variations of loops and it just ends up getting complicated and obfuscated..

I'm going to have one last try tomorrow but I think we might end up just improving the output messages of the existing tests (and improve the numbers to ensure catching more mathematical variations).
Comment 36 Martin Renvoize 2020-09-10 10:05:06 UTC
Created attachment 109841 [details] [review]
Bug 19382: (follow-up) Alternate tests
Comment 37 Martin Renvoize 2020-09-10 10:06:23 UTC
Hmm, not sure if my followup improves the tests or not to be honest.. they all pass but I'm not entirely pleased with the set of logic in the middle of the newly introduced sub
Comment 38 Kyle M Hall 2020-09-11 12:30:21 UTC
Created attachment 109937 [details] [review]
Bug 19382: (follow-up) Alternate tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Jonathan Druart 2020-09-30 12:46:00 UTC
+# 3 params, count from 0 to 6 in binary ( 3 places ) to get the set of switches, then do that 4 times, one for each parent and child
+    for my $i ( 0 .. 7 ) {

Kyle, in a previous patch I replaced "0 to 6" with "0 to 7", but you reverted it.
Can you explain? "0 to 7" seems what we are doing here.
Comment 40 Jonathan Druart 2020-09-30 12:50:33 UTC
C4/Circulation.pm:        my $guarantors_non_issues_charges += $patron->relationships_debt({ include_guarantors => 1, only_this_guaranor => 0, include_this_patron => 1 });

C4/SIP/ILS/Patron.pm:        $fines_amount += $patron->relationships_debt({ include_guarantors => 1, only_this_guaranor => 0, include_this_patron => 1 });


circ/circulation.pl:        my $guarantors_non_issues_charges += $patron->relationships_debt({ include_guarantors => 1, only_this_guaranor => 0, include_this_patron => 1 });

Looks like there is a typo in the second parameter name.
Comment 41 Kyle M Hall 2020-10-13 17:42:09 UTC
(In reply to Jonathan Druart from comment #39)
> +# 3 params, count from 0 to 6 in binary ( 3 places ) to get the set of
> switches, then do that 4 times, one for each parent and child
> +    for my $i ( 0 .. 7 ) {
> 
> Kyle, in a previous patch I replaced "0 to 6" with "0 to 7", but you
> reverted it.
> Can you explain? "0 to 7" seems what we are doing here.

That change was made by Martin.
Comment 42 Kyle M Hall 2020-10-13 17:43:30 UTC
Created attachment 111570 [details] [review]
Bug 19382: Add ability to block guarantees based on fees owed by guarantor and other guarantees

Some libraries would like to not only block the circulation of a guarantor based on fines owed by guarantees, but would also like to block circulation for all guarantees as well. Basically, if a family as a whole reaches a certain threshold of fines, the entire family will be blocked from checking out items.

Test Plan:
1) Apply this patch
2) Set NoIssuesChargeGuarantorsWithGuarantees to $14
3) Create a family of four ( 2 guarantors, 2 guarantees ) where the parents guarantee both children
4) Give 3 of the 4 a $5 fine
5) None of them should be able to check out items

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 43 Kyle M Hall 2020-10-13 17:43:57 UTC
Created attachment 111571 [details] [review]
Bug 19382: Add blocking of ability to checkout on circulation.pl

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 44 Kyle M Hall 2020-10-13 17:44:08 UTC
Created attachment 111572 [details] [review]
Bug 19382: (QA follow-up) Throw exception if only_this_guarantor is set for a non-guarantor
Comment 45 Kyle M Hall 2020-10-13 17:44:13 UTC
Created attachment 111573 [details] [review]
Bug 19382: (follow-up) Alternate tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Kyle M Hall 2020-10-13 17:44:17 UTC
Created attachment 111574 [details] [review]
Bug 19382: (QA follow-up) Fix typos
Comment 47 Jonathan Druart 2020-10-14 12:20:12 UTC
Created attachment 111609 [details] [review]
Bug 19382: Adjust comment in test
Comment 48 Jonathan Druart 2020-10-14 12:53:47 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 49 Lucas Gass 2020-11-13 14:46:49 UTC
enhancement will not be backported to 20.05.x