Bug 33411 - SIP2 includes other guarantees with the same guarantor when calculating against NoIssuesChargeGuarantees
Summary: SIP2 includes other guarantees with the same guarantor when calculating again...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 29755
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-04 18:25 UTC by Nick Clemens
Modified: 2023-12-28 20:46 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.01,22.11.07


Attachments
Bug 33411: Unit test update (2.01 KB, patch)
2023-04-07 18:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees (2.09 KB, patch)
2023-04-07 18:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33411: Unit test update (2.06 KB, patch)
2023-04-07 20:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees (2.13 KB, patch)
2023-04-07 20:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 33411: Unit test update (2.06 KB, patch)
2023-05-16 11:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees (2.12 KB, patch)
2023-05-16 11:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33411: Unit test update (2.06 KB, patch)
2023-05-16 12:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees (2.16 KB, patch)
2023-05-16 12:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 33411: (follow-up) Set debit type for mocked fines (776 bytes, patch)
2023-06-01 18:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33411: Unit test update (2.16 KB, patch)
2023-06-02 07:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees (2.26 KB, patch)
2023-06-02 07:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33411: (follow-up) Set debit type for mocked fines (873 bytes, patch)
2023-06-02 07:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-04-04 18:25:07 UTC
Bug 29755 seems to have made a bad assumption when moving code to use Koha::Patron->relationships_debt for calculating fine thresholds

It seems that routine finds the patrons guarantor, and adds all related guarantees when called like:
$patron->relationships_debt({ include_guarantors => 0, only_this_guarantor => 0, include_this_patron => 0 });

It seems NoIssuesChargeGuarantees should only count a patrons direct guarantees, not the fines of patrons sharing their guarantor
Comment 1 Nick Clemens 2023-04-07 18:57:31 UTC
Created attachment 149289 [details] [review]
Bug 33411: Unit test update
Comment 2 Nick Clemens 2023-04-07 18:57:33 UTC
Created attachment 149290 [details] [review]
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees

To test:
 1 - Find an adult patron in Koha
 2 - Add two guarantees
 3 - Add $1 fine to parent, $5 fine to each guarantee
 4 - Set no issues charge guarantees to 10
 5 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 6 - Note the 64 message starts with Y's that mean patron is blocked
 7 - Repeat with each child barcode
 8 - They are blocked?
 9 - Apply patch, restart all
10 - Try again
11 - Parent blocked, children should not be
Comment 3 David Nind 2023-04-07 20:42:11 UTC
Created attachment 149292 [details] [review]
Bug 33411: Unit test update

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2023-04-07 20:42:14 UTC
Created attachment 149293 [details] [review]
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees

To test:
 1 - Find an adult patron in Koha
 2 - Add two guarantees
 3 - Add $1 fine to parent, $5 fine to each guarantee
 4 - Set no issues charge guarantees to 10
 5 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 6 - Note the 64 message starts with Y's that mean patron is blocked
 7 - Repeat with each child barcode
 8 - They are blocked?
 9 - Apply patch, restart all
10 - Try again
11 - Parent blocked, children should not be

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2023-04-07 20:45:01 UTC
Testing notes using KTD:

1. I used Ruben Harper (23529000686353) as the guarantor and Joyce Gaines (23529000224510) and Bobbie Middleton (23529000080862) as the guarantees.

2. I wasn't sure how to restart the SIP emulator after applying the patch, so ending up shutting down and starting up KTD (otherwise there was no change in the responses received).

3. Tests passed after applying the patches: t/db_dependent/SIP/Patron.t 

Results before patch applied
============================

kohadev-koha@kohadevbox:koha(master)$ perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron 23529000686353
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'patron_information'
SEND: 6300120230407    201515          AOCPL|AA23529000686353|ACterm1|
READ: 64YYYY          00120230407    201515000000000000000100000000AOCPL|AA23529000686353|AERuben Harper|BLY|BV1|CC5|BD6963 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB19170924|PCS|PIY|AFGreetings from Koha.  -- Patron blocked by fines (11) on guaranteed accounts -- Patron owes 1.00|


kohadev-koha@kohadevbox:koha(master)$ perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron 23529000224510
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'patron_information'
SEND: 6300120230407    201635          AOCPL|AA23529000224510|ACterm1|
READ: 64              00120230407    201635000000000000000100000000AOCPL|AA23529000224510|AEJoyce Gaines|BLY|BV5|CC5|BD6012 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB20080407|PCYA|PIY|AFGreetings from Koha.  -- Patron owes 5.00|


kohadev-koha@kohadevbox:koha(master)$ perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron 23529000080862
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'patron_information'
SEND: 6300120230407    201658          AOCPL|AA23529000080862|ACterm1|
READ: 64              00120230407    201658000000000000000100000000AOCPL|AA23529000080862|AEBobbie Middleton|BLY|BV5|CC5|BD4619 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB20080407|PCJ|PIY|AFGreetings from Koha.  -- Patron owes 5.00|


Results after patch applied
============================

kohadev-koha@kohadevbox:koha(bz33411)$ perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron 23529000686353
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'patron_information'
SEND: 6300120230407    202802          AOCPL|AA23529000686353|ACterm1|
READ: 64              00120230407    202802000000000000000000000000AOCPL|AA23529000686353|AERuben Harper|BLY|BV0|CC5|BD6963 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB19170924|PCS|PIY|AFGreetings from Koha. |


kohadev-koha@kohadevbox:koha(bz33411)$ perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron 23529000224510
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'patron_information'
SEND: 6300120230407    203519          AOCPL|AA23529000224510|ACterm1|
READ: 64              00120230407    203519000000000000000000000000AOCPL|AA23529000224510|AEJoyce Gaines|BLY|BV0|CC5|BD6012 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB20080407|PCYA|PIY|AFGreetings from Koha. |


kohadev-koha@kohadevbox:koha(bz33411)$ perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron 23529000080862
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'patron_information'
SEND: 6300120230407    203605          AOCPL|AA23529000080862|ACterm1|
READ: 64              00120230407    203605000000000000000000000000AOCPL|AA23529000080862|AEBobbie Middleton|BLY|BV0|CC5|BD4619 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB20080407|PCJ|PIY|AFGreetings from Koha. |
Comment 6 Victor Grousset/tuxayo 2023-05-09 23:30:09 UTC
> 11 - Parent blocked, children should not be

David's test have the parent unblocked thanks to the patch. Is the test plan wrong then?

I can't get a difference with and without the patch. Even after doing "koha-sip --restart" is that the right way? (testing on ktd)
Comment 7 Victor Grousset/tuxayo 2023-05-15 18:09:56 UTC
Moving off the QA queue pending clarification of the test plan and holding off new test attempt.
Comment 8 Nick Clemens 2023-05-16 11:39:17 UTC
Created attachment 151271 [details] [review]
Bug 33411: Unit test update

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Nick Clemens 2023-05-16 11:39:19 UTC
Created attachment 151272 [details] [review]
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees

To test:
 1 - Find an adult patron in Koha
 2 - Add two guarantees
 3 - Add $1 fine to parent, $5 fine to each guarantee
 4 - Set no issues charge guarantees to 9
 5 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 6 - Note the 64 message starts with Y's that mean patron is blocked
 7 - Repeat with each child barcode
 8 - They are blocked?
 9 - Apply patch, restart all (restart_all will restart SIP)
10 - Try again
11 - Parent blocked, children should not be
Comment 10 Nick Clemens 2023-05-16 11:41:08 UTC
(In reply to David Nind from comment #5)
> Testing notes using KTD:
> 
> 1. I used Ruben Harper (23529000686353) as the guarantor and Joyce Gaines
> (23529000224510) and Bobbie Middleton (23529000080862) as the guarantees.
> 
> 2. I wasn't sure how to restart the SIP emulator after applying the patch,
> so ending up shutting down and starting up KTD (otherwise there was no
> change in the responses received).
> 
restart_all will restart SIP - I had a mistake in the test plan, set the pref to 9 for the guarantees to trigger the block



(In reply to Victor Grousset/tuxayo from comment #6)
> > 11 - Parent blocked, children should not be
> 
> David's test have the parent unblocked thanks to the patch. Is the test plan
> wrong then?
> 
> I can't get a difference with and without the patch. Even after doing
> "koha-sip --restart" is that the right way? (testing on ktd)

When KTD is shut down and restarted, all data is cleared, so the patrons no longer had fines - that is why the parent was unblocked

The problem was the mistake in plan, now fixed, ready for testing
Comment 11 David Nind 2023-05-16 12:49:54 UTC
Created attachment 151278 [details] [review]
Bug 33411: Unit test update

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2023-05-16 12:49:56 UTC
Created attachment 151279 [details] [review]
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees

To test:
 1 - Find an adult patron in Koha
 2 - Add two guarantees
 3 - Add $1 fine to parent, $5 fine to each guarantee
 4 - Set no issues charge guarantees to 9
 5 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 6 - Note the 64 message starts with Y's that mean patron is blocked
 7 - Repeat with each child barcode
 8 - They are blocked?
 9 - Apply patch, restart all (restart_all will restart SIP)
10 - Try again
11 - Parent blocked, children should not be

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 Marcel de Rooy 2023-05-17 14:32:53 UTC
# Subtest: NoIssuesChargeGuarantees tests
    1..6
    not ok 1 - Only patron's fines are reported in total
    #   Failed test 'Only patron's fines are reported in total'
    #   at t/db_dependent/SIP/Patron.t line 327.
    #          got: '0'
    #     expected: '11'
    ok 2 - Guarantor blocked
    not ok 3 - Screen message includes related fines total
    #   Failed test 'Screen message includes related fines total'
    #   at t/db_dependent/SIP/Patron.t line 329.
    #                   'Greetings from Koha. GxXo8ZD -- Patron blocked by fines (11.22) on guaranteed accounts'
    #     doesn't match '(?^u:Patron blocked by fines \(22\.22\) on guaranteed accounts)'
    ok 4 - Guarantee only fines correctly counted
    ok 5 - Guarantee not blocked by guarantor fines
    ok 6 - Screen message does not include blocked message
    # Looks like you failed 2 tests of 6.
not ok 9 - NoIssuesChargeGuarantees tests
#   Failed test 'NoIssuesChargeGuarantees tests'
#   at t/db_dependent/SIP/Patron.t line 338.
Comment 14 Nick Clemens 2023-06-01 12:49:55 UTC
(In reply to Marcel de Rooy from comment #13)
> #   Failed test 'NoIssuesChargeGuarantees tests'
> #   at t/db_dependent/SIP/Patron.t line 338.


Did you restart_all to make sure SIP is adjusted? All tests pass for me
Comment 15 Marcel de Rooy 2023-06-01 14:16:12 UTC
# Subtest: NoIssuesChargeGuarantees tests
    1..6
    ok 1 - Only patron's fines are reported in total
    ok 2 - Guarantor blocked
    not ok 3 - Screen message includes related fines total
    #   Failed test 'Screen message includes related fines total'
    #   at t/db_dependent/SIP/Patron.t line 331.
    #                   'Greetings from Koha. Og_K_qe88A -- Patron blocked by fines (11.11) on guaranteed accounts -- Patron owes 11.00'
    #     doesn't match '(?^u:Patron blocked by fines \(22\.22\) on guaranteed accounts)'
    ok 4 - Guarantee only fines correctly counted
    ok 5 - Guarantee not blocked by guarantor fines
    ok 6 - Screen message does not include blocked message
    # Looks like you failed 1 test of 6.
not ok 9 - NoIssuesChargeGuarantees tests

There is a difference. Only one test failed now. So progress :)
Comment 16 Marcel de Rooy 2023-06-01 14:30:55 UTC
(In reply to Marcel de Rooy from comment #15)

>     #   Failed test 'Screen message includes related fines total'
>     #   at t/db_dependent/SIP/Patron.t line 331.
>     #                   'Greetings from Koha. Og_K_qe88A -- Patron blocked
> by fines (11.11) on guaranteed accounts -- Patron owes 11.00'
>     #     doesn't match '(?^u:Patron blocked by fines \(22\.22\) on
> guaranteed accounts)'

If I debug on $patron->account->lines, I only see that 11 euro.
Do you miss a few assumptions in the mocking or so still?
Comment 17 Nick Clemens 2023-06-01 18:20:42 UTC
Created attachment 151951 [details] [review]
Bug 33411: (follow-up) Set debit type for mocked fines
Comment 18 Nick Clemens 2023-06-01 18:21:31 UTC
(In reply to Marcel de Rooy from comment #16)
> (In reply to Marcel de Rooy from comment #15)
> 
> >     #   Failed test 'Screen message includes related fines total'
> >     #   at t/db_dependent/SIP/Patron.t line 331.
> >     #                   'Greetings from Koha. Og_K_qe88A -- Patron blocked
> > by fines (11.11) on guaranteed accounts -- Patron owes 11.00'
> >     #     doesn't match '(?^u:Patron blocked by fines \(22\.22\) on
> > guaranteed accounts)'
> 
> If I debug on $patron->account->lines, I only see that 11 euro.
> Do you miss a few assumptions in the mocking or so still?

Yes, not setting the type meant it was randomly a type that was counted in non-issues-charges or not. Should be fixed now
Comment 19 Marcel de Rooy 2023-06-02 06:25:57 UTC
(In reply to Nick Clemens from comment #18)
> (In reply to Marcel de Rooy from comment #16)
> > (In reply to Marcel de Rooy from comment #15)
> > 
> > >     #   Failed test 'Screen message includes related fines total'
> > >     #   at t/db_dependent/SIP/Patron.t line 331.
> > >     #                   'Greetings from Koha. Og_K_qe88A -- Patron blocked
> > > by fines (11.11) on guaranteed accounts -- Patron owes 11.00'
> > >     #     doesn't match '(?^u:Patron blocked by fines \(22\.22\) on
> > > guaranteed accounts)'
> > 
> > If I debug on $patron->account->lines, I only see that 11 euro.
> > Do you miss a few assumptions in the mocking or so still?
> 
> Yes, not setting the type meant it was randomly a type that was counted in
> non-issues-charges or not. Should be fixed now

Great. One randomly failing test less.
Comment 20 Marcel de Rooy 2023-06-02 07:20:20 UTC
Created attachment 151970 [details] [review]
Bug 33411: Unit test update

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2023-06-02 07:20:22 UTC
Created attachment 151971 [details] [review]
Bug 33411: Don't count related guarantees fees against NoIssuesChargeGuarantees

To test:
 1 - Find an adult patron in Koha
 2 - Add two guarantees
 3 - Add $1 fine to parent, $5 fine to each guarantee
 4 - Set no issues charge guarantees to 9
 5 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 6 - Note the 64 message starts with Y's that mean patron is blocked
 7 - Repeat with each child barcode
 8 - They are blocked?
 9 - Apply patch, restart all (restart_all will restart SIP)
10 - Try again
11 - Parent blocked, children should not be

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2023-06-02 07:20:24 UTC
Created attachment 151972 [details] [review]
Bug 33411: (follow-up) Set debit type for mocked fines

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Tomás Cohen Arazi 2023-06-05 17:37:11 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 24 George Williams (NEKLS) 2023-06-08 20:17:16 UTC
Any chance this patch can be applied to 22.11 also?
Comment 25 Tomás Cohen Arazi 2023-06-09 14:07:43 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 26 Matt Blenkinsop 2023-06-12 10:15:09 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x