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
Created attachment 149289 [details] [review] Bug 33411: Unit test update
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
Created attachment 149292 [details] [review] Bug 33411: Unit test update Signed-off-by: David Nind <david@davidnind.com>
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>
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. |
> 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)
Moving off the QA queue pending clarification of the test plan and holding off new test attempt.
Created attachment 151271 [details] [review] Bug 33411: Unit test update Signed-off-by: David Nind <david@davidnind.com>
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
(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
Created attachment 151278 [details] [review] Bug 33411: Unit test update Signed-off-by: David Nind <david@davidnind.com>
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>
# 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.
(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
# 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 :)
(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?
Created attachment 151951 [details] [review] Bug 33411: (follow-up) Set debit type for mocked fines
(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
(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.
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>
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>
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>
Pushed to master for 23.11. Nice work everyone, thanks!
Any chance this patch can be applied to 22.11 also?
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work everyone! Pushed to oldstable for 22.11.x