Bug 29755 - SIP2 code does not correctly handle NoIssuesChargeGuarantees or NoIssuesChargeGuarantorsWithGuarantees
Summary: SIP2 code does not correctly handle NoIssuesChargeGuarantees or NoIssuesCha...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords: rel_21_11_candidate
Depends on:
Blocks:
 
Reported: 2021-12-22 15:55 UTC by Nick Clemens
Modified: 2022-11-07 14:51 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes SIP2 so that it correctly determines if issues should be blocked for patrons when the NoIssuesChargeGuarantees and NoIssuesChargeGuarantorsWithGuarantees system preferences are set. Currently, it only checks the noissuescharge system preference as the limit for charges, and not the other 'No Issues charge' system preferences.
Version(s) released in:
22.05.00


Attachments
Bug 29755: Check each NoIssuesCharge separately (6.13 KB, patch)
2022-02-10 14:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29755: Check each NoIssuesCharge separately (6.18 KB, patch)
2022-04-27 23:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 29755: Check each NoIssuesCharge separately (6.18 KB, patch)
2022-04-28 11:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29755: (follow-up) Mock NoIssuesChargeGuarantorsWithGuarantees in test (885 bytes, patch)
2022-04-28 11:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29755: Check each NoIssuesCharge separately (6.23 KB, patch)
2022-04-28 18:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 29755: (follow-up) Mock NoIssuesChargeGuarantorsWithGuarantees in test (933 bytes, patch)
2022-04-28 18:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 29755: Check each NoIssuesCharge separately (6.24 KB, patch)
2022-05-06 12:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29755: (follow-up) Mock NoIssuesChargeGuarantorsWithGuarantees in test (998 bytes, patch)
2022-05-06 12:01 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-12-22 15:55:22 UTC
The sip code always checks noissuescharge as the limit for fines, never the other possible limits
Comment 1 Nick Clemens 2022-02-10 14:37:59 UTC
Created attachment 130449 [details] [review]
Bug 29755: Check each NoIssuesCharge separately

This patch updates SIP patron code to use account methods to calculate balances
over the patronflags returns. It also checks if patron should be blocked for each
'No Issues charge' preference

Tests are added for NoIssuesChargeGuarantees

To test:
 1 - Set noissuescharge preference to 5
 2 - Add a $10 charge to a patron
 3 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 4 - Note the 64 message starts with Y's that mean patron is blocked
 5 - Set noissuescharge to 11
 6 - Repeat 3, patron is no longer blocked
 7 - Set NoIssuesChargeGuarantees to 8
 8 - Repeat 3, patron is blocked
 9 - Pay $3 on patron so they owe 7
10 - Repeat 3, patron is not blocked
11 - Add a child account with patron as guarantor
12 - Repeat 3, patron is not blocked
13 - Add a $4 charge to child
14 - Repeat 3, patron is blocked
15 - Repeat 3, but with child barcode, child is not blocked
16 - Set NoIssuesChargeGuarantorsWithGuarantees to 11
17 - Repeat 3, patron is blocked
18 - Repeat 3 with child barcode, child is blocked
Comment 2 David Nind 2022-04-27 23:40:16 UTC
Created attachment 134196 [details] [review]
Bug 29755: Check each NoIssuesCharge separately

This patch updates SIP patron code to use account methods to calculate balances
over the patronflags returns. It also checks if patron should be blocked for each
'No Issues charge' preference

Tests are added for NoIssuesChargeGuarantees

To test:
 1 - Set noissuescharge preference to 5
 2 - Add a $10 charge to a patron
 3 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 4 - Note the 64 message starts with Y's that mean patron is blocked
 5 - Set noissuescharge to 11
 6 - Repeat 3, patron is no longer blocked
 7 - Set NoIssuesChargeGuarantees to 8
 8 - Repeat 3, patron is blocked
 9 - Pay $3 on patron so they owe 7
10 - Repeat 3, patron is not blocked
11 - Add a child account with patron as guarantor
12 - Repeat 3, patron is not blocked
13 - Add a $4 charge to child
14 - Repeat 3, patron is blocked
15 - Repeat 3, but with child barcode, child is not blocked
16 - Set NoIssuesChargeGuarantorsWithGuarantees to 11
17 - Repeat 3, patron is blocked
18 - Repeat 3 with child barcode, child is blocked

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2022-04-27 23:47:52 UTC
After signing off, I ran the tests and they failed:

prove t/db_dependent/SIP/Patron.t 
t/db_dependent/SIP/Patron.t .. 8/10 
    #   Failed test 'Guarantee only fines correctly counted'
    #   at t/db_dependent/SIP/Patron.t line 318.
    #          got: '11.11'
    #     expected: '0.11'

    #   Failed test 'Guarantee not blocked by guarantor fines'
    #   at t/db_dependent/SIP/Patron.t line 319.
    # Looks like you failed 2 tests of 4.
t/db_dependent/SIP/Patron.t .. 9/10 
#   Failed test 'NoIssuesChargeGuarantees tests'
#   at t/db_dependent/SIP/Patron.t line 322.
t/db_dependent/SIP/Patron.t .. 10/10 # Looks like you failed 1 test of 10.
t/db_dependent/SIP/Patron.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/10 subtests 

Test Summary Report
-------------------
t/db_dependent/SIP/Patron.t (Wstat: 256 Tests: 10 Failed: 1)
  Failed test:  9
  Non-zero exit status: 1
Files=1, Tests=10,  9 wallclock secs ( 0.04 usr  0.01 sys +  7.56 cusr  1.12 csys =  8.73 CPU)
Result: FAIL

Also, for steps 16-18 of the test plan, I think step 16 needs changing to "16 - Set NoIssuesChargeGuarantorsWithGuarantees to 10", rather than 11. 
- If 11 is used, the patrons and the child are not blocked.
- The wording for the system preference is "Prevent a patron from checking out if the patron has guarantors and those guarantor's guarantees owing in total more than X USD in fines."
- The key part is "MORE THAN X USD". In the test, the total is 11.
Comment 4 Nick Clemens 2022-04-28 11:09:38 UTC
Created attachment 134221 [details] [review]
Bug 29755: Check each NoIssuesCharge separately

This patch updates SIP patron code to use account methods to calculate balances
over the patronflags returns. It also checks if patron should be blocked for each
'No Issues charge' preference

Tests are added for NoIssuesChargeGuarantees

To test:
 1 - Set noissuescharge preference to 5
 2 - Add a $10 charge to a patron
 3 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 4 - Note the 64 message starts with Y's that mean patron is blocked
 5 - Set noissuescharge to 11
 6 - Repeat 3, patron is no longer blocked
 7 - Set NoIssuesChargeGuarantees to 8
 8 - Repeat 3, patron is blocked
 9 - Pay $3 on patron so they owe 7
10 - Repeat 3, patron is not blocked
11 - Add a child account with patron as guarantor
12 - Repeat 3, patron is not blocked
13 - Add a $4 charge to child
14 - Repeat 3, patron is blocked
15 - Repeat 3, but with child barcode, child is not blocked
16 - Set NoIssuesChargeGuarantorsWithGuarantees to 10
17 - Repeat 3, patron is blocked
18 - Repeat 3 with child barcode, child is blocked

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Nick Clemens 2022-04-28 11:09:42 UTC
Created attachment 134222 [details] [review]
Bug 29755: (follow-up) Mock NoIssuesChargeGuarantorsWithGuarantees in test
Comment 6 David Nind 2022-04-28 18:31:53 UTC
Created attachment 134297 [details] [review]
Bug 29755: Check each NoIssuesCharge separately

This patch updates SIP patron code to use account methods to calculate balances
over the patronflags returns. It also checks if patron should be blocked for each
'No Issues charge' preference

Tests are added for NoIssuesChargeGuarantees

To test:
 1 - Set noissuescharge preference to 5
 2 - Add a $10 charge to a patron
 3 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 4 - Note the 64 message starts with Y's that mean patron is blocked
 5 - Set noissuescharge to 11
 6 - Repeat 3, patron is no longer blocked
 7 - Set NoIssuesChargeGuarantees to 8
 8 - Repeat 3, patron is blocked
 9 - Pay $3 on patron so they owe 7
10 - Repeat 3, patron is not blocked
11 - Add a child account with patron as guarantor
12 - Repeat 3, patron is not blocked
13 - Add a $4 charge to child
14 - Repeat 3, patron is blocked
15 - Repeat 3, but with child barcode, child is not blocked
16 - Set NoIssuesChargeGuarantorsWithGuarantees to 10
17 - Repeat 3, patron is blocked
18 - Repeat 3 with child barcode, child is blocked

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-04-28 18:31:58 UTC
Created attachment 134299 [details] [review]
Bug 29755: (follow-up) Mock NoIssuesChargeGuarantorsWithGuarantees in test

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Martin Renvoize 2022-05-06 12:01:47 UTC
Created attachment 134725 [details] [review]
Bug 29755: Check each NoIssuesCharge separately

This patch updates SIP patron code to use account methods to calculate balances
over the patronflags returns. It also checks if patron should be blocked for each
'No Issues charge' preference

Tests are added for NoIssuesChargeGuarantees

To test:
 1 - Set noissuescharge preference to 5
 2 - Add a $10 charge to a patron
 3 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
 4 - Note the 64 message starts with Y's that mean patron is blocked
 5 - Set noissuescharge to 11
 6 - Repeat 3, patron is no longer blocked
 7 - Set NoIssuesChargeGuarantees to 8
 8 - Repeat 3, patron is blocked
 9 - Pay $3 on patron so they owe 7
10 - Repeat 3, patron is not blocked
11 - Add a child account with patron as guarantor
12 - Repeat 3, patron is not blocked
13 - Add a $4 charge to child
14 - Repeat 3, patron is blocked
15 - Repeat 3, but with child barcode, child is not blocked
16 - Set NoIssuesChargeGuarantorsWithGuarantees to 10
17 - Repeat 3, patron is blocked
18 - Repeat 3 with child barcode, child is blocked

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-05-06 12:01:51 UTC
Created attachment 134726 [details] [review]
Bug 29755: (follow-up) Mock NoIssuesChargeGuarantorsWithGuarantees in test

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2022-05-06 12:02:32 UTC
Nice to see this migration to using Koha::Patron objects and methods.. works as expected and no regressions found.

Tests all passing and QA script is happy

PQA
Comment 11 Fridolin Somers 2022-05-07 01:59:02 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄