Bug 23426 - Empty AV field returned in Patron Info in addition to those requested
Summary: Empty AV field returned in Patron Info in addition to those requested
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Jan Kissig
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-05 11:04 UTC by Colin Campbell
Modified: 2024-09-04 08:46 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Patch correcting extraction of items (2.08 KB, patch)
2019-08-05 13:22 UTC, Colin Campbell
Details | Diff | Splinter 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 (5.78 KB, patch)
2024-04-25 09:44 UTC, Jan Kissig
Details | Diff | Splinter 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 (5.78 KB, patch)
2024-04-25 09:46 UTC, Jan Kissig
Details | Diff | Splinter 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 (5.78 KB, patch)
2024-04-25 09:47 UTC, Jan Kissig
Details | Diff | Splinter Review
BUG 23426: Add fine items to patron information response in SIP2 (6.06 KB, patch)
2024-04-26 12:46 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 23426: Add fine items to patron information response in SIP2 (6.11 KB, patch)
2024-05-07 09:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 23426: (follow-up) Adds test file (3.62 KB, patch)
2024-07-22 10:47 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 23426: (QA follow-up) Tidy (22.43 KB, patch)
2024-09-02 07:43 UTC, Jan Kissig
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2019-08-05 11:04:41 UTC
If fine items are requested in a patron info request, they are returned but an additional, always empty AV field is also returned.
e.g. request is for fine items with start = 1 and stop = 1
instead of returning AVFirst items description|
client receives AVFirst items description|AV|

cause an off by one error in fine items - patch to follow
Comment 1 Colin Campbell 2019-08-05 13:22:23 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)
Comment 2 Fridolin Somers 2020-09-29 12:22:09 UTC
This is more a bug than enhancement no ?
Comment 3 David Cook 2020-09-30 07:13:01 UTC
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.
Comment 4 Lucas Gass 2022-04-22 22:05:10 UTC
It looks like a pretty simple fix but unfortunately this patch no longer cleanly applies to master.
Comment 5 Jan Kissig 2024-04-25 09:44:22 UTC
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 '
Comment 6 Jan Kissig 2024-04-25 09:46:29 UTC
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 '
Comment 7 Jan Kissig 2024-04-25 09:47:25 UTC
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 '
Comment 8 Jan Kissig 2024-04-26 12:46:34 UTC
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 '
Comment 9 David Nind 2024-05-07 09:14:44 UTC
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>
Comment 10 David Nind 2024-05-07 09:19:01 UTC
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)
Comment 11 David Nind 2024-05-07 09:23:11 UTC
Changed assignee.
Comment 12 Marcel de Rooy 2024-07-19 09:20:14 UTC
Please remove first patch?
Please add a test for MsgType. Look in t/db_dependent/SIP
Comment 13 David Nind 2024-07-22 02:03:09 UTC
(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.
Comment 14 Jan Kissig 2024-07-22 10:47:16 UTC
Created attachment 169272 [details] [review]
Bug 23426: (follow-up) Adds test file

to test:
  prove t/db_dependent/SIP/Message.t
Comment 15 Jan Kissig 2024-07-22 10:55:45 UTC
(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.
Comment 16 Marcel de Rooy 2024-08-16 08:57:23 UTC
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.
Comment 17 Jan Kissig 2024-09-02 07:43:37 UTC
Created attachment 170931 [details] [review]
Bug 23426: (QA follow-up) Tidy
Comment 18 Jan Kissig 2024-09-04 08:46:10 UTC
(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)