|
Description
Stefan Berndtsson
2019-09-05 17:44:35 UTC
Created attachment 128657 [details] [review] Bug 23548: Set AQ to empty when item not exist 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! (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. Created attachment 135533 [details] [review] Bug 23548: Set AQ to empty when item not exist Signed-off-by: David Nind <david@davidnind.com> 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| 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> 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. (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. 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? > 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.
(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). 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. Created attachment 154557 [details] [review] Bug 23548: Unit test - Set AQ to empty when item not exist Hello! I've added a unit test for this patch. 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> 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> Created attachment 155048 [details] [review] Bug 23548: (QA follow-up) tidy up code 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! :) 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> 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> 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> Created attachment 155134 [details] [review] Bug 23548: (QA follow-up) tidy up code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed to master for 23.11. Nice work everyone, thanks! Pushed to 23.05.x for 23.05.04 Nice work everyone! Pushed to oldstable for 22.11.x |