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. please do not backport as this patch does not confirm to sip2 standard. If this patch gets reverted, it could be set to closed and I would put the 2 other improvements made (active currency (BH) and number of items in the response which are specified in BP and BQ) as new seperate bugs (In reply to Jan Kissig from comment #26) > If this patch gets reverted, it could be set to closed and I would put the 2 > other improvements made (active currency (BH) and number of items in the > response which are specified in BP and BQ) as new seperate bugs We can either revert or you provide a follow-up patch that just un-does what is not correct. I think I would prefer that if possible, as that would make it a bit easier for the release notes. (In reply to Ewa Gózd from comment #27) > (In reply to Jan Kissig from comment #26) > > If this patch gets reverted, it could be set to closed and I would put the 2 > > other improvements made (active currency (BH) and number of items in the > > response which are specified in BP and BQ) as new seperate bugs > > We can either revert or you provide a follow-up patch that just un-does what > is not correct. I think I would prefer that if possible, as that would make > it a bit easier for the release notes. Sorry for the identity theft, Ewa, fixed now. Above was me. Created attachment 174232 [details] [review] Bug 23426: (follow-up) Enhance sip_cli_emulator.pl for test plan This follow up enhances the sip_cli_emulator.pl to use start-item and/or end-item as params. With these new params the original test plan can be extended: Test plan: a) create several manual invoices for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) run perl 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 " --start-item=1 --end-item=2 to get fine 1 and 2 or perl 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 " --start-item=3 --end-item=3 to retrieve fine 3 Created attachment 174233 [details] [review] Bug 23426: (follow-up) This patch reintroduces the former implementation of fine items The original implementation of fine items was accidently overwritten with this patch. This follow up reverts these changes but keeps additional improvements that were also part of this patch. These are: - Returning the active currency as part of the response (BH) - Fixing the number of items in the response which are specified in BP and BQ when other items as fine items are requested. Created attachment 174234 [details] [review] Bug 23426: (follow-up) Enhance sip_cli_emulator.pl for test plan This follow up enhances the sip_cli_emulator.pl to use start-item and/or end-item as params. With these new params the original test plan can be extended: Test plan: a) create several manual invoices for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) run perl 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 " --start-item=1 --end-item=2 to get fine 1 and 2 or perl 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 " --start-item=3 --end-item=3 to retrieve fine 3 Resetting to SO to trigger QA team on the last 2 follow-up patches. Would like another set of eyes. Created attachment 174464 [details] [review] Bug 23426: (follow-up) This patch reintroduces the former implementation of fine items The original implementation of fine items was accidently overwritten with this patch. This follow up reverts these changes but keeps additional improvements that were also part of this patch. These are: - Returning the active currency as part of the response (BH) - Fixing the number of items in the response which are specified in BP and BQ when other items as fine items are requested. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 174465 [details] [review] Bug 23426: (follow-up) Enhance sip_cli_emulator.pl for test plan This follow up enhances the sip_cli_emulator.pl to use start-item and/or end-item as params. With these new params the original test plan can be extended: Test plan: a) create several manual invoices for patron 23529000035676 : http://localhost:8081/cgi-bin/koha/members/maninvoice.pl?borrowernumber=19 b) run perl 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 " --start-item=1 --end-item=2 to get fine 1 and 2 or perl 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 " --start-item=3 --end-item=3 to retrieve fine 3 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Last to patches picked for main. Pushed for 24.11! Well done everyone, thank you! Hi Jan, can you please have a look at the failing test? t_db_dependent_SIP_Message_t Subtest: Fine items, currency, start and end item response in patron info 1..6 ok 1 - At least we got a response. ok 2 - Response code fine ok 3 - Verified active currency ok 4 - Verified fine item 1 is not in response not ok 5 - Verified fine item 2 not ok 6 - Verified fine item 3 Created attachment 174479 [details] [review] Bug 23426: (follow-up) Fix failing test to send correct summary flag Backported to 24.05.x for 24.05.06 I had a hard time figuring out what the release note meant, and have had a go at amending it. Not sure that I have got this right! (In reply to David Nind from comment #40) > I had a hard time figuring out what the release note meant, and have had a > go at amending it. > > Not sure that I have got this right! Hi David, I think you discovered all! Pushed to 23.11.x for 23.11.11 Not pushed to LTS. Marked Resolved. If you feel this should be in LTS please reply with your reason. |