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
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...
Created attachment 83436 [details] [review] Bug 22037: Block SIP checkout if guarantees have debt
Could you test this patch Barton?
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.
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
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.
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.
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?
Hi Kyle, This patch is supposed to fix the regression caused by: Bug 19933: Remove patronflags - tricky ones
(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?
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.
Created attachment 91229 [details] [review] Bug 22037: Block SIP checkout if guarantees have debt Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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.
Created attachment 92434 [details] [review] Bug 22037 [QA follow-up]: Implement use of CHARGES_GUARANTEES
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
QA: Returning here now
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.
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>
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>
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>
Nice work! Pushed to master for 19.11.00
Pushed to 19.05.x for 19.05.04
backported to 18.11.x for 18.11.11