Bug 22037 - regression: guarantor no longer blocked (debarred) if child is over limit, when checking out via SIP.
Summary: regression: guarantor no longer blocked (debarred) if child is over limit, wh...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-20 22:02 UTC by Barton Chittenden
Modified: 2019-10-17 19:16 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.04


Attachments
Bug 22037: Block SIP checkout if guarantees have debt (989 bytes, patch)
2018-12-20 22:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22037: Block SIP checkout if guarantees have debt (1.04 KB, patch)
2019-07-03 14:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22037 [QA follow-up]: Implement use of CHARGES_GUARANTEES (1.20 KB, patch)
2019-08-22 13:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22037: Block SIP checkout if guarantees have debt (1.12 KB, patch)
2019-09-13 07:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22037: (QA follow-up) Implement use of CHARGES_GUARANTEES (1.28 KB, patch)
2019-09-13 07:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22037: (QA follow-up) Correct misleading comment (1.26 KB, patch)
2019-09-13 07:39 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 Barton Chittenden 2018-12-20 22:02:18 UTC
Currently on 18.05.06.

I have two sample patrons set up.  XXXXXXXX (an adult patron) and
YYYYYYYYYY (a child patron).  XXXXXXXX is the guarantor for YYYYYYYYYY
and I have put a 12.00 fee on YYYYYYYYYY's account.  Because of our
system settings, a fee of 10.01 or over on a child's account should
block checkouts by the adult guarantor and, in this case it does.  If I
go to XXXXXXXX's account in Koha I get the "Fees & Charges: Patron's
guarantees collectively owe 12.00. Checkouts are BLOCKED because fine
balance is OVER THE LIMIT." message that I expect to see and the patron
is blocked from checkout in the staff client.

In a SIP2 transaction, however, XXXXXXXX is not blocked.

Prior to our upgrade this scenario would cause XXXXXXXX's account to send 
a SIP2 PI response of 'N.'  Since the upgrade, the guarantor account with
an over-limit guarantee is not triggering an 'N' response in the PI field
- it's coming through as PIY as seen in the sample SIP response below:

--> 6300120181220    150631          AOLEAVENWRTH|AAXXXXXXXX|AD1234|BP1|BQ5|AY2AZEE0C
<-- 64              00120181220    150632000000000000000000000000AOLEAVENWRTH|AAXXXXXXXX|AESIP Test|BLY|CQY|CC10|PB19000101|PCADULT|PIY|AFGreetings from Koha. |AFLEAVENWRTH|AY2AZD277
Comment 1 Jonathan Druart 2018-12-20 22:39:30 UTC
Certainly caused by
  commit bddfed75913a2302c19c00b00a6fe86538af6f9f
  Bug 19933: Remove patronflags - tricky ones

Which wrongly said "This patch only deal with the circulation code as I am not convident enough with SIP code"... hum...
Comment 2 Jonathan Druart 2018-12-20 22:42:45 UTC
Created attachment 83436 [details] [review]
Bug 22037: Block SIP checkout if guarantees have debt
Comment 3 Jonathan Druart 2018-12-20 22:43:09 UTC
Could you test this patch Barton?
Comment 4 Barton Chittenden 2018-12-20 23:37:23 UTC
Jonathan,

I'm in the proccess of spinning up a new kohadevbox, I'll test tomorrow. In the mean time, do you mind writing up a test plan?

Oh, and just for the sake of completeness, here's how to replicate, as far as I can tell:

1) Set up patrons GUARANTOR and CHILD, set GUARANTOR as the guarantor for CHILD.
2) Set sysprefs: noissuescharge = 10, NoIssuesChargeGuarantees = 10
3) Charge CHILD a fee of 12.00
4) Send a SIP 63 message for GUARANTOR
5) The SIP 64 reply will contain PIY rather than PIN.
Comment 5 Barton Chittenden 2019-02-12 12:02:31 UTC
Forwarded from George Williamson (NEKLS):

The patch does not fix the problem on our production server.

I've still got the same test patrons set up with the same parameters as in the original message.  Running the same test now, the adult patron still shows PI = Y.  Prior to Koha 18.05 the message would have been PI = N.

This is the result of the test I just ran a few moments ago:

--> 6300120190211    092532          AOLEAVENWRTH|AA89898971|ADXXXX|BP1|BQ5|AY1AZEE08
<-- 64              00120190211    092532000000000000000000000000AOLEAVENWRTH|AA89898971|AESIP Test|BLY|CQY|CC10|PB19000101|PCADULT|PIY|AFGreetings from Koha. |AFLEAVENWRTH|AY1AZD274
Comment 6 Jonathan Druart 2019-02-22 13:34:22 UTC
I can see the problem, it is obvious, we should test CHARGES_GUARANTEES, not CHARGES.
But I do not understand how it is a regression, regarding git log it sounds more like a long standing bug.

Can you give me an exact Koha version where the bug happens?

Tested on 17.11.x (HEAD is 71565cb9bc3bc73cd4114217ea4206879a4f2c23) and it seems that the output is wrong as well.
Comment 7 Jonathan Druart 2019-02-22 13:40:24 UTC
On master, as well as 17.11.x, I get:

misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information               1 ↵ [10:36:30]
Attempting socket connection to localhost:6001...connected!
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'patron_information'
SEND: 6300120190222    103634          AOCPL|AA23529000035676|ACterm1|
READ: 64              00120190222    103635000000000000000000000000AOCPL|AA23529000035676|AEHenry Acevedo|BLY|CC10|BD4345 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB19580530|PCS|PIY|AFGreetings from Koha. |


Last test was on 17.11.x with:

MariaDB [koha_kohadev]> select borrowernumber, cardnumber, guarantorid from borrowers where guarantorid is not null;                                                                                               
+----------------+----------------+-------------+
| borrowernumber | cardnumber     | guarantorid |
+----------------+----------------+-------------+
|             37 | 23529000197047 |          19 |
+----------------+----------------+-------------+
1 row in set (0.00 sec)

MariaDB [koha_kohadev]> select borrowernumber, cardnumber from borrowers where borrowernumber=19;
+----------------+----------------+
| borrowernumber | cardnumber     |
+----------------+----------------+
|             19 | 23529000035676 |
+----------------+----------------+


MariaDB [koha_kohadev]> select amount from accountlines where borrowernumber=37;
+-----------+
| amount    |
+-----------+
| 12.000000 |
+-----------+


MariaDB [koha_kohadev]> select variable, value from systempreferences where variable like "noissuescharge%";
+--------------------------+-------+
| variable                 | value |
+--------------------------+-------+
| noissuescharge           | 10    |
| NoIssuesChargeGuarantees | 10    |
+--------------------------+-------+
2 rows in set (0.00 sec)


I do see "Fees & Charges: Patron's guarantees collectively owe 12.00. Checkouts are BLOCKED because fine balance is OVER THE LIMIT." on the guarantor account.
Comment 8 Kyle M Hall 2019-06-03 10:33:42 UTC
Jonathan,

First, does the patch you've attached here fix a bug? If so, we should probably get it onto a fresh bug report.

Second, since the behavior we want *is* new, we need to add a sip2 parameter for it. Do you agree?
Comment 9 Jonathan Druart 2019-06-04 23:39:52 UTC
Hi Kyle,
This patch is supposed to fix the regression caused by:
  Bug 19933: Remove patronflags - tricky ones
Comment 10 Kyle M Hall 2019-06-11 19:48:56 UTC
(In reply to Jonathan Druart from comment #9)
> Hi Kyle,
> This patch is supposed to fix the regression caused by:
>   Bug 19933: Remove patronflags - tricky ones

Jonathan, what do you think needs to happen to get this bug solved?
Comment 11 Jonathan Druart 2019-06-12 13:46:42 UTC
Either there is a regression and we can fix it with the submitted patch. Or we want to fix a long standing bug and in that case more work is needed.
Comment 12 Kyle M Hall 2019-07-03 14:22:15 UTC
Created attachment 91229 [details] [review]
Bug 22037: Block SIP checkout if guarantees have debt

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Marcel de Rooy 2019-08-16 08:13:30 UTC
Jonathan:
I understand the move back to $flags->{CHARGES}->{amount} in view of the other report.
But when looking at patronflags and guarantors, I actually do not see where $flags{'CHARGES_GUARANTEES'} is being used. A git grep on CHARGES_GUARANTEES only shows the line creating this key in patronflags itself..

So please explain this comment:
# This "amount" is the negative balance or the one of the guarantees
Where is the balance of the guarantees stored into CHARGES ?

Changing status to reflect need for feedback.
Comment 14 Kyle M Hall 2019-08-22 13:27:07 UTC
Created attachment 92434 [details] [review]
Bug 22037 [QA follow-up]: Implement use of CHARGES_GUARANTEES
Comment 15 Marcel de Rooy 2019-08-23 07:53:46 UTC
Just fyi from qa tools:
        * Commit title does not start with 'Bug XXXXX: ' - b329f91073
        * Commit title does not contain 'follow-up' correctly spelt - b329f91073
        * Commit title does not contain '(QA follow-up)' correctly spelt - b329f91073

Using the test plan from above with a patron status message (63):
63engYYYYMMDDZZZZHHMMSSXXXXXXXXXX|AOCPL|AA19|ADpassword|
64YYYY      YY  eng20190823    074640000000000000000100000001AOCPL|AA19|AEHenry Acevedo|BLY|CQN|BV12|CC6|BD4345 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB20090530|PCK|PIN|AFGreetings from Koha.  -- Patron owes 12.00 -- Invalid password|
63engYYYYMMDDZZZZHHMMSSXXXXXXXXXX|AOCPL|AA51|ADpassword|
64YYYY      YY  eng20190823    074653000000000000000100000000AOCPL|AA51|AEMarcel Dev|BLY|CQN|BV17|CC6|BDadres_new|BEemail|PCS|PIY|AFGreetings from Koha.  -- Patron owes 5.00 -- Invalid password|

Even for NoIssuesChargeGuarantees == 4
Comment 16 Marcel de Rooy 2019-09-13 07:05:31 UTC
QA: Returning here now
Comment 17 Marcel de Rooy 2019-09-13 07:29:29 UTC
Barton: I have been looking a bit longer to the PI field as you mentioned initially in your test plan.
As far as I can see, you should not judge this patch set on that field:

C4/SIP/Sip/MsgType.pm:        $resp .= maybe_add( FID_INET_PROFILE, $patron->inet_privileges, $server );
$patron->inet_privileges
Depends on $self->{inet}
inet            => ( !$debarred && !$expired ),
inet                    => 0,   # EnvisionWare extension

Although blocking a user results in blocking inet_privileges, it should not be the main field to look at. As above, it is called an 'extension'.
I would rather look at the 14-char patron status string and the BV (Fee amount) field.
While testing this with a patron having a guarantee with too much fines, I saw:
64YYYY      YY  
The first Y is the charge denied position, meaning here: yes, it is blocked. Same for the three following Y's for renew, recall and hold.
The last two Y's are for excessive fines and fees.

After paying the fines for the guarantee, the six Y's were all cleared. No amount in BV. Which is the expected result.
Comment 18 Marcel de Rooy 2019-09-13 07:39:29 UTC
Created attachment 92770 [details] [review]
Bug 22037: Block SIP checkout if guarantees have debt

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2019-09-13 07:39:34 UTC
Created attachment 92771 [details] [review]
Bug 22037: (QA follow-up) Implement use of CHARGES_GUARANTEES

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2019-09-13 07:39:38 UTC
Created attachment 92772 [details] [review]
Bug 22037: (QA follow-up) Correct misleading comment

Comment needs to be corrected, and now applies to the whole code
paragraph following it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Martin Renvoize 2019-09-17 11:40:46 UTC
Nice work!

Pushed to master for 19.11.00
Comment 22 Fridolin SOMERS 2019-09-19 15:44:44 UTC
Pushed to 19.05.x for 19.05.04
Comment 23 Lucas Gass 2019-10-17 19:16:34 UTC
backported to 18.11.x for 18.11.11