|
Description
Colin Campbell
2019-08-05 11:04:41 UTC
Created attachment 91992 [details] [review] Patch correcting extraction of items Patch to convert start and end to zero based offsets consistently, and use end as terminus condition for the for loop using them. To test, unpatched an extra AV field is returned with the patch applied the correct number is returned (start and end are sequence numbers based on 1 and indicating the first and last item to be returned) This is more a bug than enhancement no ? We've had some people having issues retrieving fine information via SIP. Could you speak more to this or give an example message causing the issue? I'm not quite understanding the description. It looks like a pretty simple fix but unfortunately this patch no longer cleanly applies to master. Created attachment 165507 [details] [review] BUG 23426 This patch adds fine items (AV) to patron information response in SIP2 In addition the active currency we be part of the response (BH) This also fixes the number of items in the response which are specified in BP and BQ in the request to test: a) create a manual invoice for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " c) verify that response includes fields like '|AVManual fee ' Created attachment 165508 [details] [review] BUG 23426 This patch adds fine items (AV) to patron information response in SIP2 In addition the active currency we be part of the response (BH) This also fixes the number of items in the response which are specified in BP and BQ in the request to test: a) create a manual invoice for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " c) verify that response includes fields like '|AVManual fee ' Created attachment 165509 [details] [review] BUG 23426 This patch adds fine items (AV) to patron information response in SIP2 In addition the active currency we be part of the response (BH) This also fixes the number of items in the response which are specified in BP and BQ in the request to test: a) create a manual invoice for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " c) verify that response includes fields like '|AVManual fee ' Created attachment 165654 [details] [review] BUG 23426: Add fine items to patron information response in SIP2 This patch adds fine items (AV) to patron information response in SIP2 In addition the active currency we be part of the response (BH) This also fixes the number of items in the response which are specified in BP and BQ in the request to test: a) create a manual invoice for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " c) verify that no |AV field is in response d) apply patch e) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " f) verify that response includes fields like '|AVManual fee ' Created attachment 166240 [details] [review] Bug 23426: Add fine items to patron information response in SIP2 This patch adds fine items (AV) to patron information response in SIP2 In addition the active currency we be part of the response (BH) This also fixes the number of items in the response which are specified in BP and BQ in the request to test: a) create a manual invoice for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " c) verify that no |AV field is in response d) apply patch e) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " f) verify that response includes fields like '|AVManual fee ' Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD):
1. SIP response before the patch was applied:
./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y "
Attempting socket connection to 127.0.0.1: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: 6300120240507 091010 Y AOCPL|AA23529000035676|ACterm1|
READ: 64YYYY YY 00120240507 091010000000000000000100000000AOCPL|AA23529000035676|AEHenry Acevedo|BLY|BV10|CC5|BD4345 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB19580530|PCS|PIN|AFGreetings from Koha. -- Patron blocked by fines -- Patron owes 10.00|
2. SIP response after the patch was applied:
./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y "
Attempting socket connection to 127.0.0.1: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: 6300120240507 091158 Y AOCPL|AA23529000035676|ACterm1|
READ: 64YYYY YY 00120240507 091158000000000000000100000000AOCPL|AA23529000035676|AEHenry Acevedo|BLY|BHUSD|BV10|CC5|AVManual fee 10.00|BD4345 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB19580530|PCS|PIN|AFGreetings from Koha. -- Patron blocked by fines -- Patron owes 10.00|
3. To get the SIP emulator to work in KTD on main, comment out in /etc/koha/sites/kohadev/SIPconfig.xml:
<!-- <service
port="8023/tcp"
transport="telnet"
protocol="SIP/2.00"
timeout="60" />
-->
(See https://gitlab.com/koha-community/koha-testing-docker/-/issues/428)
Changed assignee. Please remove first patch? Please add a test for MsgType. Look in t/db_dependent/SIP (In reply to Marcel de Rooy from comment #12) > Please remove first patch? That was my fault - I changed the title from "BUG 23426:..." to "Bug 23426:..". I will obsolete it. > Please add a test for MsgType. Look in t/db_dependent/SIP I'll leave this to Jan. Created attachment 169272 [details] [review] Bug 23426: (follow-up) Adds test file to test: prove t/db_dependent/SIP/Message.t (In reply to Marcel de Rooy from comment #12) > Please remove first patch? > Please add a test for MsgType. Look in t/db_dependent/SIP Hi Marcel, as there were already test for that MsgType (Patron Info) I added another subtest to t/db_dependent/SIP/Message.t to test all 3 changes the original patch does: - test for fine items (AV) when requeting patron info with summary ' Y ' - test if only requested (BP until BQ) AV fields are returned - test if 3 char long currency is returne I hope this is what you meant. Thanks for the test!
A last request? For some time, we check perltidy on our patches. This is the result for this patch set:
WARN C4/SIP/Sip/MsgType.pm
WARN tidiness
The file is less tidy than before (bad/messy lines before: 173, now: 175)
WARN t/db_dependent/SIP/Message.t
WARN tidiness
The file is less tidy than before (bad/messy lines before: 531, now: 563)
Please tidy both patches. Thanks.
Created attachment 170931 [details] [review] Bug 23426: (QA follow-up) Tidy (In reply to Marcel de Rooy from comment #16) Hey Marcel, although I ran perltidy as described in the wiki, it did not change the code I submitted in t/db_dependent/SIP/Message.t so maybe your script will raise this warning again? > WARN t/db_dependent/SIP/Message.t > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 531, now: 563) (In reply to Jan Kissig from comment #18) > (In reply to Marcel de Rooy from comment #16) > > > Hey Marcel, although I ran perltidy as described in the wiki, it did not > change the code I submitted in t/db_dependent/SIP/Message.t so maybe your > script will raise this warning again? > > > WARN t/db_dependent/SIP/Message.t > > WARN tidiness > > The file is less tidy than before (bad/messy lines before: > > 531, now: 563) Sorry for the late reply. I will remove the last patch in view of: C4/SIP/ILS/Patron.pm | 325 ++++++++++++++++++++++++------------------ (In reply to Jan Kissig from comment #18) > (In reply to Marcel de Rooy from comment #16) > > > Hey Marcel, although I ran perltidy as described in the wiki, it did not > change the code I submitted in t/db_dependent/SIP/Message.t so maybe your > script will raise this warning again? > > > WARN t/db_dependent/SIP/Message.t > > WARN tidiness > > The file is less tidy than before (bad/messy lines before: > > 531, now: 563) Not sure what you did exactly. Maybe ask for some help on MM in this respect. I have tied these files now in view of time passed. Created attachment 172383 [details] [review] Bug 23426: Add fine items to patron information response in SIP2 This patch adds fine items (AV) to patron information response in SIP2 In addition the active currency we be part of the response (BH) This also fixes the number of items in the response which are specified in BP and BQ in the request to test: a) create a manual invoice for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " c) verify that no |AV field is in response d) apply patch e) in ktd call: perl /usr/share/koha/bin/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 -m patron_information -s " Y " f) verify that response includes fields like '|AVManual fee ' Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Tidied inline Created attachment 172384 [details] [review] Bug 23426: (follow-up) Add tests Test plan: [1] Prove t/db_dependent/SIP/Message.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> EDIT: Tidied inline. Pushed for 24.11! Well done everyone, thank you! I would appreciate if this is backported to other versions as well. |