Bug 35461 - Renew All 66 SIP server response messages produce HASH content in replies
Summary: Renew All 66 SIP server response messages produce HASH content in replies
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-12-01 13:45 UTC by Lari Strand
Modified: 2024-02-09 20:14 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03,23.05.09


Attachments
Bug 35461: Add unit tests (4.09 KB, patch)
2024-01-02 18:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses (2.62 KB, patch)
2024-01-02 18:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35461: Tidy code (5.75 KB, patch)
2024-01-02 18:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35461: (QA follow-up) Fix unit test (1.02 KB, patch)
2024-01-02 20:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35461: Add unit tests (4.51 KB, patch)
2024-01-02 20:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses (2.62 KB, patch)
2024-01-02 20:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35461: Add unit tests (4.51 KB, patch)
2024-01-03 11:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses (2.62 KB, patch)
2024-01-03 11:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35461: Add unit tests (4.56 KB, patch)
2024-01-03 18:27 UTC, David Nind
Details | Diff | Splinter Review
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses (2.66 KB, patch)
2024-01-03 18:27 UTC, David Nind
Details | Diff | Splinter Review
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses (2.71 KB, patch)
2024-01-09 09:50 UTC, Lari Strand
Details | Diff | Splinter Review
Bug 35461: Add unit tests (4.65 KB, patch)
2024-01-26 10:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses (2.80 KB, patch)
2024-01-26 10:40 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 Lari Strand 2023-12-01 13:45:13 UTC
"Renew All" -function aka. 65-66-messages work like this: 66-return messages has renewed items in BM-fields and non-renewables go into BN-fields.

If all items were renewable, then the reply message looks like:

Reply:
13.07.2022 10:06:17:530 OUTESTI1 6610005000020220713 100616AOOUPK|BM564N15977171|BM564N09203960|BM564N23261720|BM564N25161958|BM564N23455012|C4::SIP::SIPServer=HASHC4::SIP::SIPServer=HASHAY6AZCEFE

If items include renewables and non-renewables, the response includes the renewed  items first, then a HASH mixed into the first BN-field, then rest of the renewed ones like they should show in BN-fields and after that, anohter HASH:

13.07.2022 14:41:32:610 OUTESTI1 6610003000420220713 144130AOOUPK|BM564N23455012|BM564N23261720|BM564N25161958|C4::SIP::SIPServer=HASHBN564N09203960|BN564N04191259|BN564N15977171|BN564N13788332|C4::SIP::SIPServer=HASHAY8AZC7C5

If all loans were unrenewable (this test included just one item), you first get a HASH in a BN-field, and another at the end.

13.07.2022 14:49:00:037 OUTESTI1 6610000000120220713 144858AOOUPK|C4::SIP::SIPServer=HASHBN564N19874533|C4::SIP::SIPServer=HASHAY9AZDD3B

Our self checkout/checkin machine can handle/parse these reply messages correctly, but this might not be the case with all self checkout/checkin devices.
Comment 1 Kyle M Hall 2024-01-02 18:02:21 UTC
Created attachment 160419 [details] [review]
Bug 35461: Add unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 2 Kyle M Hall 2024-01-02 18:02:26 UTC
Created attachment 160420 [details] [review]
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses

"Renew All" -function aka. 65-66-messages work like this: 66-return messages has renewed items in BM-fields and non-renewables go into BN-fields.

If all items were renewable, then the reply message looks like:

Reply:
13.07.2022 10:06:17:530 OUTESTI1 6610005000020220713 100616AOOUPK|BM564N15977171|BM564N09203960|BM564N23261720|BM564N25161958|BM564N23455012|C4::SIP::SIPServer=HASHC4::SIP::SIPServer=HASHAY6AZCEFE

If items include renewables and non-renewables, the response includes the renewed  items first, then a HASH mixed into the first BN-field, then rest of the renewed ones like they should show in BN-fields and after that, anohter HASH:

13.07.2022 14:41:32:610 OUTESTI1 6610003000420220713 144130AOOUPK|BM564N23455012|BM564N23261720|BM564N25161958|C4::SIP::SIPServer=HASHBN564N09203960|BN564N04191259|BN564N15977171|BN564N13788332|C4::SIP::SIPServer=HASHAY8AZC7C5

If all loans were unrenewable (this test included just one item), you first get a HASH in a BN-field, and another at the end.

13.07.2022 14:49:00:037 OUTESTI1 6610000000120220713 144858AOOUPK|C4::SIP::SIPServer=HASHBN564N19874533|C4::SIP::SIPServer=HASHAY9AZDD3B

Our self checkout/checkin machine can handle/parse these reply messages correctly, but this might not be the case with all self checkout/checkin devices.

Test Plan:
1) Unit test patch
2) prove t/db_dependent/SIP/Message.t
3) Note failures
4) Apply this patch
5) prove t/db_dependent/SIP/Message.t
6) Tests pass!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Kyle M Hall 2024-01-02 18:07:10 UTC
Created attachment 160421 [details] [review]
Bug 35461: Tidy code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 David Nind 2024-01-02 20:37:13 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2024-01-02 20:47:28 UTC
Created attachment 160436 [details] [review]
Bug 35461: (QA follow-up) Fix unit test
Comment 6 Kyle M Hall 2024-01-02 20:49:24 UTC
Created attachment 160437 [details] [review]
Bug 35461: Add unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2024-01-02 20:49:32 UTC
Created attachment 160438 [details] [review]
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses

"Renew All" -function aka. 65-66-messages work like this: 66-return messages has renewed items in BM-fields and non-renewables go into BN-fields.

If all items were renewable, then the reply message looks like:

Reply:
13.07.2022 10:06:17:530 OUTESTI1 6610005000020220713 100616AOOUPK|BM564N15977171|BM564N09203960|BM564N23261720|BM564N25161958|BM564N23455012|C4::SIP::SIPServer=HASHC4::SIP::SIPServer=HASHAY6AZCEFE

If items include renewables and non-renewables, the response includes the renewed  items first, then a HASH mixed into the first BN-field, then rest of the renewed ones like they should show in BN-fields and after that, anohter HASH:

13.07.2022 14:41:32:610 OUTESTI1 6610003000420220713 144130AOOUPK|BM564N23455012|BM564N23261720|BM564N25161958|C4::SIP::SIPServer=HASHBN564N09203960|BN564N04191259|BN564N15977171|BN564N13788332|C4::SIP::SIPServer=HASHAY8AZC7C5

If all loans were unrenewable (this test included just one item), you first get a HASH in a BN-field, and another at the end.

13.07.2022 14:49:00:037 OUTESTI1 6610000000120220713 144858AOOUPK|C4::SIP::SIPServer=HASHBN564N19874533|C4::SIP::SIPServer=HASHAY9AZDD3B

Our self checkout/checkin machine can handle/parse these reply messages correctly, but this might not be the case with all self checkout/checkin devices.

Test Plan:
1) Unit test patch
2) prove t/db_dependent/SIP/Message.t
3) Note failures
4) Apply this patch
5) prove t/db_dependent/SIP/Message.t
6) Tests pass!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2024-01-02 20:50:14 UTC
(In reply to David Nind from comment #4)
> The tests pass for me with only the 'Add unit tests' patch.
> 
> The test plan says that it shouldn't.
> 
> Is this an issue for sign off (they also pass after applying all the
> patches)?

Fixed the bad test and squashed those changes into the original patch set!
Comment 9 David Nind 2024-01-02 21:37:22 UTC
I hope I haven't confused things....

With the unit tests patch, the tests fail (as expected):

prove t/db_dependent/SIP/Message.t
t/db_dependent/SIP/Message.t .. 5/18 
    #   Failed test at t/db_dependent/SIP/Message.t line 1399.
    #          got: '61'
    #     expected: 'String 'HASH(' not found in reponse ( Bug 35461 )'
    # Looks like you failed 1 test of 3.

#   Failed test 'Test renew desensitize'
#   at t/db_dependent/SIP/Message.t line 106.
t/db_dependent/SIP/Message.t .. 18/18 # Looks like you failed 1 test of 18.
t/db_dependent/SIP/Message.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/18 subtests 

Test Summary Report
-------------------
t/db_dependent/SIP/Message.t (Wstat: 256 Tests: 18 Failed: 1)
  Failed test:  6
  Non-zero exit status: 1
Files=1, Tests=18,  7 wallclock secs ( 0.04 usr  0.00 sys +  5.08 cusr  1.11 csys =  6.23 CPU)
Result: FAIL


After applying the main patch, I'm still getting test failures:


prove t/db_dependent/SIP/Message.t
t/db_dependent/SIP/Message.t .. 5/18 
    #   Failed test at t/db_dependent/SIP/Message.t line 1399.
    #          got: '-2'
    #     expected: 'String 'HASH(' not found in reponse ( Bug 35461 )'
    # Looks like you failed 1 test of 3.

#   Failed test 'Test renew desensitize'
#   at t/db_dependent/SIP/Message.t line 106.
t/db_dependent/SIP/Message.t .. 17/18 # Looks like you failed 1 test of 18.
t/db_dependent/SIP/Message.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/18 subtests 

Test Summary Report
-------------------
t/db_dependent/SIP/Message.t (Wstat: 256 Tests: 18 Failed: 1)
  Failed test:  6
  Non-zero exit status: 1
Files=1, Tests=18,  8 wallclock secs ( 0.03 usr  0.01 sys +  5.44 cusr  1.06 csys =  6.54 CPU)
Result: FAIL
Comment 10 Kyle M Hall 2024-01-03 11:27:49 UTC
Created attachment 160453 [details] [review]
Bug 35461: Add unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2024-01-03 11:27:57 UTC
Created attachment 160454 [details] [review]
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses

"Renew All" -function aka. 65-66-messages work like this: 66-return messages has renewed items in BM-fields and non-renewables go into BN-fields.

If all items were renewable, then the reply message looks like:

Reply:
13.07.2022 10:06:17:530 OUTESTI1 6610005000020220713 100616AOOUPK|BM564N15977171|BM564N09203960|BM564N23261720|BM564N25161958|BM564N23455012|C4::SIP::SIPServer=HASHC4::SIP::SIPServer=HASHAY6AZCEFE

If items include renewables and non-renewables, the response includes the renewed  items first, then a HASH mixed into the first BN-field, then rest of the renewed ones like they should show in BN-fields and after that, anohter HASH:

13.07.2022 14:41:32:610 OUTESTI1 6610003000420220713 144130AOOUPK|BM564N23455012|BM564N23261720|BM564N25161958|C4::SIP::SIPServer=HASHBN564N09203960|BN564N04191259|BN564N15977171|BN564N13788332|C4::SIP::SIPServer=HASHAY8AZC7C5

If all loans were unrenewable (this test included just one item), you first get a HASH in a BN-field, and another at the end.

13.07.2022 14:49:00:037 OUTESTI1 6610000000120220713 144858AOOUPK|C4::SIP::SIPServer=HASHBN564N19874533|C4::SIP::SIPServer=HASHAY9AZDD3B

Our self checkout/checkin machine can handle/parse these reply messages correctly, but this might not be the case with all self checkout/checkin devices.

Test Plan:
1) Unit test patch
2) prove t/db_dependent/SIP/Message.t
3) Note failures
4) Apply this patch
5) prove t/db_dependent/SIP/Message.t
6) Tests pass!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2024-01-03 11:30:23 UTC
(In reply to David Nind from comment #9)
> I hope I haven't confused things....
>

Nope, the unit test was missing a comma causing it to fail all the time and in my haste I didn't re-test the "fixed" version against the patch to ensure it passed afterward! It's ready to test again!
Comment 13 David Nind 2024-01-03 18:27:40 UTC
Created attachment 160496 [details] [review]
Bug 35461: Add unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2024-01-03 18:27:43 UTC
Created attachment 160497 [details] [review]
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses

"Renew All" -function aka. 65-66-messages work like this: 66-return messages has renewed items in BM-fields and non-renewables go into BN-fields.

If all items were renewable, then the reply message looks like:

Reply:
13.07.2022 10:06:17:530 OUTESTI1 6610005000020220713 100616AOOUPK|BM564N15977171|BM564N09203960|BM564N23261720|BM564N25161958|BM564N23455012|C4::SIP::SIPServer=HASHC4::SIP::SIPServer=HASHAY6AZCEFE

If items include renewables and non-renewables, the response includes the renewed  items first, then a HASH mixed into the first BN-field, then rest of the renewed ones like they should show in BN-fields and after that, anohter HASH:

13.07.2022 14:41:32:610 OUTESTI1 6610003000420220713 144130AOOUPK|BM564N23455012|BM564N23261720|BM564N25161958|C4::SIP::SIPServer=HASHBN564N09203960|BN564N04191259|BN564N15977171|BN564N13788332|C4::SIP::SIPServer=HASHAY8AZC7C5

If all loans were unrenewable (this test included just one item), you first get a HASH in a BN-field, and another at the end.

13.07.2022 14:49:00:037 OUTESTI1 6610000000120220713 144858AOOUPK|C4::SIP::SIPServer=HASHBN564N19874533|C4::SIP::SIPServer=HASHAY9AZDD3B

Our self checkout/checkin machine can handle/parse these reply messages correctly, but this might not be the case with all self checkout/checkin devices.

Test Plan:
1) Unit test patch
2) prove t/db_dependent/SIP/Message.t
3) Note failures
4) Apply this patch
5) prove t/db_dependent/SIP/Message.t
6) Tests pass!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2024-01-03 18:35:25 UTC
(In reply to Kyle M Hall from comment #12)

> Nope, the unit test was missing a comma causing it to fail all the time and
> in my haste I didn't re-test the "fixed" version against the patch to ensure
> it passed afterward! It's ready to test again!

Those pesky commas! 8-; Thanks Kyle. Now signed off!
Comment 16 Lari Strand 2024-01-09 09:50:29 UTC
Created attachment 160685 [details] [review]
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses

"Renew All" -function aka. 65-66-messages work like this: 66-return messages has renewed items in BM-fields and non-renewables go into BN-fields.

If all items were renewable, then the reply message looks like:

Reply:
13.07.2022 10:06:17:530 OUTESTI1 6610005000020220713 100616AOOUPK|BM564N15977171|BM564N09203960|BM564N23261720|BM564N25161958|BM564N23455012|C4::SIP::SIPServer=HASHC4::SIP::SIPServer=HASHAY6AZCEFE

If items include renewables and non-renewables, the response includes the renewed  items first, then a HASH mixed into the first BN-field, then rest of the renewed ones like they should show in BN-fields and after that, anohter HASH:

13.07.2022 14:41:32:610 OUTESTI1 6610003000420220713 144130AOOUPK|BM564N23455012|BM564N23261720|BM564N25161958|C4::SIP::SIPServer=HASHBN564N09203960|BN564N04191259|BN564N15977171|BN564N13788332|C4::SIP::SIPServer=HASHAY8AZC7C5

If all loans were unrenewable (this test included just one item), you first get a HASH in a BN-field, and another at the end.

13.07.2022 14:49:00:037 OUTESTI1 6610000000120220713 144858AOOUPK|C4::SIP::SIPServer=HASHBN564N19874533|C4::SIP::SIPServer=HASHAY9AZDD3B

Our self checkout/checkin machine can handle/parse these reply messages correctly, but this might not be the case with all self checkout/checkin devices.

Test Plan:
1) Unit test patch
2) prove t/db_dependent/SIP/Message.t
3) Note failures
4) Apply this patch
5) prove t/db_dependent/SIP/Message.t
6) Tests pass!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: lmstrand <lmstrand@gmail.com>
Comment 17 Katrin Fischer 2024-01-09 09:53:20 UTC
Love to see the sign-offs - Kyle, can we count you for QA on this one?
Comment 18 Lari Strand 2024-01-09 10:00:01 UTC
Also tested with a Lyngsoe Systems self checkout/service device and it was happy.
Comment 19 Kyle M Hall 2024-01-09 14:49:16 UTC
(In reply to Katrin Fischer from comment #17)
> Love to see the sign-offs - Kyle, can we count you for QA on this one?

I wrote the patches so not unless you think it's sufficient :)
Comment 20 Katrin Fischer 2024-01-12 08:10:34 UTC
(In reply to Kyle M Hall from comment #19)
> (In reply to Katrin Fischer from comment #17)
> > Love to see the sign-offs - Kyle, can we count you for QA on this one?
> 
> I wrote the patches so not unless you think it's sufficient :)

Good point :D I was only looking at the sign-off lines.
Comment 21 Marcel de Rooy 2024-01-26 10:40:09 UTC
Created attachment 161501 [details] [review]
Bug 35461: Add unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2024-01-26 10:40:12 UTC
Created attachment 161502 [details] [review]
Bug 35461: Fix function call parameters causing the HASH to show in the renew all responses

"Renew All" -function aka. 65-66-messages work like this: 66-return messages has renewed items in BM-fields and non-renewables go into BN-fields.

If all items were renewable, then the reply message looks like:

Reply:
13.07.2022 10:06:17:530 OUTESTI1 6610005000020220713 100616AOOUPK|BM564N15977171|BM564N09203960|BM564N23261720|BM564N25161958|BM564N23455012|C4::SIP::SIPServer=HASHC4::SIP::SIPServer=HASHAY6AZCEFE

If items include renewables and non-renewables, the response includes the renewed  items first, then a HASH mixed into the first BN-field, then rest of the renewed ones like they should show in BN-fields and after that, anohter HASH:

13.07.2022 14:41:32:610 OUTESTI1 6610003000420220713 144130AOOUPK|BM564N23455012|BM564N23261720|BM564N25161958|C4::SIP::SIPServer=HASHBN564N09203960|BN564N04191259|BN564N15977171|BN564N13788332|C4::SIP::SIPServer=HASHAY8AZC7C5

If all loans were unrenewable (this test included just one item), you first get a HASH in a BN-field, and another at the end.

13.07.2022 14:49:00:037 OUTESTI1 6610000000120220713 144858AOOUPK|C4::SIP::SIPServer=HASHBN564N19874533|C4::SIP::SIPServer=HASHAY9AZDD3B

Our self checkout/checkin machine can handle/parse these reply messages correctly, but this might not be the case with all self checkout/checkin devices.

Test Plan:
1) Unit test patch
2) prove t/db_dependent/SIP/Message.t
3) Note failures
4) Apply this patch
5) prove t/db_dependent/SIP/Message.t
6) Tests pass!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: lmstrand <lmstrand@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Katrin Fischer 2024-01-26 13:36:11 UTC
Please fix!

 OK	C4/SIP/Sip/MsgType.pm

 FAIL	t/db_dependent/SIP/Message.t
   FAIL	  spelling
		 reponse ==> response
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 531, now: 532)
Comment 24 Katrin Fischer 2024-01-26 13:39:49 UTC
Never mind, I fixed it inline.
Comment 25 Katrin Fischer 2024-01-26 14:17:52 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 26 Fridolin Somers 2024-01-31 13:28:02 UTC
Pushed to 23.11.x for 23.11.03
Comment 27 Lucas Gass 2024-02-09 20:14:39 UTC
Backported to 23.05.x for upcoming 23.05.09