Bug 23548 - AQ field required in checkin response
Summary: AQ field required in checkin response
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Per Larsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-05 17:44 UTC by Stefan Berndtsson
Modified: 2023-12-28 20:47 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes SIP return messages so that there is an "AQ|" field, even if it is empty (this is a required field according to the specification, and some machines (such as PV-SUPA) crash if it is not present).
Version(s) released in:
23.11.00,23.05.04,22.11.10


Attachments
Bug 23548: Set AQ to empty when item not exist (830 bytes, patch)
2021-12-17 12:32 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 23548: Set AQ to empty when item not exist (878 bytes, patch)
2022-06-01 10:25 UTC, David Nind
Details | Diff | Splinter Review
Bug 23548: Set AQ to empty when item not exist (943 bytes, patch)
2022-06-06 15:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23548: Unit test - Set AQ to empty when item not exist (1.29 KB, patch)
2023-08-17 12:27 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 23548: Unit test - Set AQ to empty when item not exist (1.35 KB, patch)
2023-08-31 20:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23548: Set AQ to empty when item not exist (1001 bytes, patch)
2023-08-31 20:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23548: (QA follow-up) tidy up code (1.04 KB, patch)
2023-08-31 21:03 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23548: Unit test - Set AQ to empty when item not exist (1.40 KB, patch)
2023-09-01 16:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23548: Unit test - Set AQ to empty when item not exist (1.40 KB, patch)
2023-09-01 16:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23548: Set AQ to empty when item not exist (1.03 KB, patch)
2023-09-01 16:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23548: (QA follow-up) tidy up code (1.09 KB, patch)
2023-09-01 16:44 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Berndtsson 2019-09-05 17:44:35 UTC
When doing checkin through SIP, the AQ field is a required field in the response (code 10), however it is only provided when the item checked in is found.

There should be an empty "AQ|" in the return message when the item is not found to not violate the required field status.

For context, this is an actual issue with the checkin stations from PV-SUPA when you want them to accept any book, even if they're not found by Koha. It gives an error, and refuses to handle the checkin further.
Comment 1 David Gustafsson 2021-12-17 12:32:32 UTC
Created attachment 128657 [details] [review]
Bug 23548: Set AQ to empty when item not exist
Comment 2 David Nind 2022-06-01 01:28:55 UTC
I'm not familiar with SIP2, but I would like to test.

Are you able to provide a test plan similar to bug 26871 or bug 20517? Ideally with the commands to give, and the responses expected.

Thanks!
Comment 3 Stefan Berndtsson 2022-06-01 07:13:19 UTC
(In reply to David Nind from comment #2)
> I'm not familiar with SIP2, but I would like to test.

I don't have an instance without the patch near me at the moment that can run SIP commands, but it should be something like (tweak credentials and such as needed):

Barcode 123123123 is a barcode that is not in the database.

1. Have SIP-server running
2. ./sip_cli_emulator.pl -m checkin -a localhost -p 6001 -l 123 -su sipuser -sp sippass -t CR --item 123123123
3. See that AQ is missing completely.
4. Apply patch
5. Run same command again.
6. See that AQ| is present.

That is the expected situation, but as mentioned, I haven't tested the non-patched version.
Comment 4 David Nind 2022-06-01 10:25:19 UTC
Created attachment 135533 [details] [review]
Bug 23548: Set AQ to empty when item not exist

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2022-06-01 10:29:44 UTC
Thanks Stefan!

With koha-testing-docker, SIP2 is already configured. SIP user and password is term1.

Changed step 2 from comment #3 to ./misc/sip_cli_emulator.pl -m checkin -a localhost -p 6001 -l 123 -su term1 -sp term1 -t CR --item 123123123

Output before patch applied:

Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at ./misc/sip_cli_emulator.pl line 120.
SEND: 9300CNterm1|COterm1|CP123|
READ: 941
Trying 'checkin'
SEND: 09N20220601    10213620220601    102136AP123|AO123|AB123123123|ACterm1|BIN|
READ: 
100NUY20220601    102136AO123|AB123123123|CV99|AFInvalid Item|

Output after patch applied:

Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at ./misc/sip_cli_emulator.pl line 120.
SEND: 9300CNterm1|COterm1|CP123|
READ: 941
Trying 'checkin'
SEND: 09N20220601    10232820220601    102328AP123|AO123|AB123123123|ACterm1|BIN|
READ: 
100NUY20220601    102329AO123|AB123123123|AQ|CV99|AFInvalid Item|

Last line now as |AQ|
Comment 6 Martin Renvoize 2022-06-06 15:43:38 UTC
Created attachment 135732 [details] [review]
Bug 23548: Set AQ to empty when item not exist

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2022-06-06 15:45:24 UTC
This works as described, but I'm not sure I follow the workflow.. how can you check in an item that Koha can't find?

There's clearly a need, even if I don't fully understand the real-life situation this can happen.

I'd love to see a unit test cover this change however, so right now it's failing on that front.. please provide a unit test and then we can pass.
Comment 8 Stefan Berndtsson 2022-06-07 07:56:29 UTC
(In reply to Martin Renvoize from comment #7)
> This works as described, but I'm not sure I follow the workflow.. how can
> you check in an item that Koha can't find?

The point is that you can't, but the resulting reply (failed to find the item), is required to have the AQ field present (empty) according to the specification. Machines from PV-SUPA crashes when the reply does not contain that required field.
Comment 9 Martin Renvoize 2022-06-07 21:35:54 UTC
I'm convinced, and looked up the specs to confirm. Now we just need a unit test to protect us from later regressions ;-).

Are you able to submit that, of should I add it to my list to try and get to soon?
Comment 10 Stefan Berndtsson 2022-06-08 07:25:50 UTC
> Are you able to submit that, of should I add it to my list to try and get to
> soon?

Unfortunately I'm currently tied up with other urgent things here before my vacation, so I don't see myself having time within a 6-8 weeks right now.
Comment 11 Stefan Berndtsson 2022-10-13 11:44:34 UTC
(In reply to Martin Renvoize from comment #9)
> I'm convinced, and looked up the specs to confirm. Now we just need a unit
> test to protect us from later regressions ;-).
> 
> Are you able to submit that, of should I add it to my list to try and get to
> soon?

I'm unable to run the existing tests for two reasons.

1. I get:
  DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_koha`.`checkout_renewals`, CONSTRAINT `renewals_renewer_id` FOREIGN KEY (`renewer_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE)


2. test_checkout_v2 assumes that circulation rules are set in some specific way in the database and does not mock those values in the test. (This may apply to other tests as well).
Comment 12 Katrin Fischer 2022-11-14 22:18:03 UTC
Some of the tests are not as independent as they should be. Ususally we assume all tests need to pass on a standard sample database as provided by ktd or kohadevbox. You could also probably download the SQL with the data for testing.
Comment 13 Matthias Meusburger 2023-08-17 12:27:54 UTC
Created attachment 154557 [details] [review]
Bug 23548: Unit test - Set AQ to empty when item not exist
Comment 14 Matthias Meusburger 2023-08-17 12:29:26 UTC
Hello! I've added a unit test for this patch.
Comment 15 Victor Grousset/tuxayo 2023-08-31 20:54:09 UTC
Created attachment 155046 [details] [review]
Bug 23548: Unit test - Set AQ to empty when item not exist

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 16 Victor Grousset/tuxayo 2023-08-31 20:54:12 UTC
Created attachment 155047 [details] [review]
Bug 23548: Set AQ to empty when item not exist

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 17 Victor Grousset/tuxayo 2023-08-31 21:03:20 UTC
Created attachment 155048 [details] [review]
Bug 23548: (QA follow-up) tidy up code
Comment 18 Victor Grousset/tuxayo 2023-08-31 21:16:07 UTC
Thanks Matthias for the tests :)

Martin had validated almost everything, so I'm just completing the missing stuff and doing some double checking:

Test code ok and it works as expected to force having the implementation do it's job.
Implementation code looks good.
QA script happy with the follow-up.

passing QA, great teamwork, thanks all! :)
Comment 19 Kyle M Hall 2023-09-01 16:38:29 UTC
Created attachment 155131 [details] [review]
Bug 23548: Unit test - Set AQ to empty when item not exist

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2023-09-01 16:44:16 UTC
Created attachment 155132 [details] [review]
Bug 23548: Unit test - Set AQ to empty when item not exist

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2023-09-01 16:44:22 UTC
Created attachment 155133 [details] [review]
Bug 23548: Set AQ to empty when item not exist

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2023-09-01 16:44:25 UTC
Created attachment 155134 [details] [review]
Bug 23548: (QA follow-up) tidy up code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Tomás Cohen Arazi 2023-09-06 12:57:52 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 24 Fridolin Somers 2023-09-08 19:18:00 UTC
Pushed to 23.05.x for 23.05.04
Comment 25 Matt Blenkinsop 2023-09-15 09:50:52 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x