Bug 29754 - Patron fines counted twice for SIP when NoIssuesChargeGuarantorsWithGuarantees is enabled
Summary: Patron fines counted twice for SIP when NoIssuesChargeGuarantorsWithGuarantee...
Status: CLOSED FIXED
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: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-12-22 15:23 UTC by Nick Clemens
Modified: 2023-12-28 20:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.03,21.05.11,20.11.17


Attachments
Bug 29754: Unit test (5.29 KB, patch)
2021-12-22 15:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29754: Don't include patron when fetching guarantees fines (2.18 KB, patch)
2021-12-22 15:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29754: Unit test (5.35 KB, patch)
2022-01-24 17:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29754: Don't include patron when fetching guarantees fines (2.24 KB, patch)
2022-01-24 17:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29754: Unit test (5.40 KB, patch)
2022-02-07 12:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29754: Don't include patron when fetching guarantees fines (2.30 KB, patch)
2022-02-07 12:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29754: (QA follow-up) Minor tests tweaks (2.96 KB, patch)
2022-02-07 12:03 UTC, Tomás Cohen Arazi
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:23:02 UTC
The check for fines amount when using this pref fetches the patrons fines, then adds the total fines with guarantors, including this borrowers fines. This means the patrons own fines are doubled
Comment 1 Nick Clemens 2021-12-22 15:56:24 UTC
Created attachment 128863 [details] [review]
Bug 29754: Unit test

This adds a unit test to the SIP code

It also adds some spacing and dividers to make the tests for 'relationships_debt' easier to read

To test:
prove -v t/db_dependent/Koha/Patron.t - passes and is readable
prove -v t/db_dependent/SIP/Patron.t - fails
Comment 2 Nick Clemens 2021-12-22 15:56:27 UTC
Created attachment 128864 [details] [review]
Bug 29754: Don't include patron when fetching guarantees fines

This patch removes the flag to fetch the patrons own debts as we have already
calculated them

This test plan highlights another issue - the value of NoIssuesChargeGuarantorsWithGuarantees
is not checked in SIP code - we merely get the total owed including guarantees and compares to
noissuescharge
See bug 29755

To test:
 1 - Apply first patch only
 2 - prove t/db_dependent/SIP/Patron.t
 3 - It fails
 4 - Enable NoIssuesChargeGuarantorsWithGuarantees, you can set to 1
 5 - Create/find a patron
 6 - Add a $10 charge to parons account
 7 - Add a guarantee to patron
 8 - Add a $5 charge to guarantee
 9 - Set noissuescharge to 15.01
10 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
11 - Note the 64 message starts with Y's that mean patron is blocked
12 - Patron shouldn't be blocked as under 15.01
13 - Apply patch
14 - Restart all
15 - repeat 10
16 - Patron is no longer blocked
Comment 3 Nick Clemens 2021-12-22 16:00:21 UTC
Ah, simpler to check correct total, 'BV' field in SIP response will be 25 before patch, 15 after
Comment 4 Martin Renvoize 2022-01-24 17:01:24 UTC
Created attachment 129732 [details] [review]
Bug 29754: Unit test

This adds a unit test to the SIP code

It also adds some spacing and dividers to make the tests for 'relationships_debt' easier to read

To test:
prove -v t/db_dependent/Koha/Patron.t - passes and is readable
prove -v t/db_dependent/SIP/Patron.t - fails

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2022-01-24 17:01:27 UTC
Created attachment 129733 [details] [review]
Bug 29754: Don't include patron when fetching guarantees fines

This patch removes the flag to fetch the patrons own debts as we have already
calculated them

This test plan highlights another issue - the value of NoIssuesChargeGuarantorsWithGuarantees
is not checked in SIP code - we merely get the total owed including guarantees and compares to
noissuescharge
See bug 29755

To test:
 1 - Apply first patch only
 2 - prove t/db_dependent/SIP/Patron.t
 3 - It fails
 4 - Enable NoIssuesChargeGuarantorsWithGuarantees, you can set to 1
 5 - Create/find a patron
 6 - Add a $10 charge to parons account
 7 - Add a guarantee to patron
 8 - Add a $5 charge to guarantee
 9 - Set noissuescharge to 15.01
10 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
11 - Note the 64 message starts with Y's that mean patron is blocked
12 - Patron shouldn't be blocked as under 15.01
13 - Apply patch
14 - Restart all
15 - repeat 10
16 - Patron is no longer blocked

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-01-24 17:03:07 UTC
Logic follows, this works as expected and includes the requisite unit tests.

Signing off.
Comment 7 Tomás Cohen Arazi 2022-02-07 12:03:38 UTC
Created attachment 130213 [details] [review]
Bug 29754: Unit test

This adds a unit test to the SIP code

It also adds some spacing and dividers to make the tests for 'relationships_debt' easier to read

To test:
prove -v t/db_dependent/Koha/Patron.t - passes and is readable
prove -v t/db_dependent/SIP/Patron.t - fails

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2022-02-07 12:03:43 UTC
Created attachment 130214 [details] [review]
Bug 29754: Don't include patron when fetching guarantees fines

This patch removes the flag to fetch the patrons own debts as we have already
calculated them

This test plan highlights another issue - the value of NoIssuesChargeGuarantorsWithGuarantees
is not checked in SIP code - we merely get the total owed including guarantees and compares to
noissuescharge
See bug 29755

To test:
 1 - Apply first patch only
 2 - prove t/db_dependent/SIP/Patron.t
 3 - It fails
 4 - Enable NoIssuesChargeGuarantorsWithGuarantees, you can set to 1
 5 - Create/find a patron
 6 - Add a $10 charge to parons account
 7 - Add a guarantee to patron
 8 - Add a $5 charge to guarantee
 9 - Set noissuescharge to 15.01
10 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL -m patron_information --patron BARCODE
11 - Note the 64 message starts with Y's that mean patron is blocked
12 - Patron shouldn't be blocked as under 15.01
13 - Apply patch
14 - Restart all
15 - repeat 10
16 - Patron is no longer blocked

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-02-07 12:03:47 UTC
Created attachment 130215 [details] [review]
Bug 29754: (QA follow-up) Minor tests tweaks

This patch does two things:

- Wraps the new SIP tests in a single transaction. We do that to avoid
  sub-test interference, specially when test files get big. The provided
  tests are correct and don't require previous tests data.
- Migrated from the introduced 'diag' calls to 'note', as I just learned
  they are the same, but 'note' is only displayed when in verbose mode.
  Which is great to avoid unnecessary noise in Jenkins.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Fridolin Somers 2022-02-10 08:03:16 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 11 Kyle M Hall 2022-02-11 12:01:34 UTC
Pushed to 21.11.x for 21.11.03
Comment 12 Andrew Fuerste-Henry 2022-02-21 16:57:32 UTC
Pushed to 21.05.x for 21.05.11
Comment 13 Victor Grousset/tuxayo 2022-03-18 00:01:20 UTC
Backported: Pushed to 20.11.x branch for 20.11.17
Comment 14 wainuiwitikapark 2022-03-21 00:38:23 UTC
Does this need to be backported to 19.11.x?