At this time, any item with an additional materials date is blocked from checkout via SIP with a screen message to take the item to a circulation desk for checkout. Some libraries wish to allow patrons to check out items via SIP even if the item has additional materials.
Created attachment 152864 [details] [review] Bug 34153: Add ability to allow items with additional materials notes to be checked out via SIP At this time, any item with an additional materials date is blocked from checkout via SIP with a screen message to take the item to a circulation desk for checkout. Some libraries wish to allow patrons to check out items via SIP even if the item has additional materials. Test Plan: 1) Create an item with an additional materials note 2) Attempt to check it out via SIP 3) Note the failure and message 4) Enable the new SIP account option "allow_additional_materials_checkout" 5) Restart the SIP server 6) Attempt the checkout again 7) Note the checkout success and new AF field message! 8) prove t/db_dependent/SIP/Message.t
Why not use the existing syspref_overrides setting here? With these patches, you get the additional materials note only if both CircConfirmItemParts system preference is enabled and allow_additional_materials_checkout is in the SIP config. I think we should always be adding the additional materials note to the screen msg You can access the Koha item object in the SIP item object - so I don't think we should add a fetch. Possibly we should just add the additional_materials field to the SIP object
The syspref override is the usual way to handle this, however if they still want a screen message that won't be sufficient.
(In reply to Liz Rea from comment #3) > The syspref override is the usual way to handle this, however if they still > want a screen message that won't be sufficient. ^ This. It provides a screen message.
Created attachment 155740 [details] [review] Bug 34153: Add ability to allow items with additional materials notes to be checked out via SIP At this time, any item with an additional materials date is blocked from checkout via SIP with a screen message to take the item to a circulation desk for checkout. Some libraries wish to allow patrons to check out items via SIP even if the item has additional materials. Test Plan: 1) Create an item with an additional materials note 2) Attempt to check it out via SIP 3) Note the failure and message 4) Enable the new SIP account option "allow_additional_materials_checkout" 5) Restart the SIP server 6) Attempt the checkout again 7) Note the checkout success and new AF field message! 8) prove t/db_dependent/SIP/Message.t Signed-off-by: David Nind <david@davidnind.com>
Testing notes (using KTD): 1. Edit an item for a record and add some text to 952$3 (Materials specified (bound volume or other part)) - I added a note to Programming Perl (record #262, barcode 39999000011418). 2. Attempt to check out the item using SIP - item is successfully issued and no message: ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron koha -m checkout --item 39999000011418 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 'checkout' SEND: 11YN20230917 05411420230917 054114AOCPL|AAkoha|AB39999000011418|ACterm1|BIN| READ: 121NNY20230917 054114AOCPL|AAkoha|AB39999000011418|AJProgramming Perl /|AH20230922 235900 3. Check in the item. 4. Enable system preference CircConfirmItemParts (set to 'Require'). 5. Attempt to check out the item using SIP - note that the message is 'Item must be checked out at a circulation desk' and the item is not checked out: ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron koha -m checkout --item 39999000011418 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 'checkout' SEND: 11YN20230917 06184820230917 061848AOCPL|AAkoha|AB39999000011418|ACterm1|BIN| READ: 120NUN20230917 061848AOCPL|AAkoha|AB39999000011418|AJProgramming Perl /|AH|AFItem must be checked out at a circulation desk|BLY| 6. Apply the patch. 7. Edit /etc/koha/sites/kohadev/SIPconfig.xml and add allow_additional_materials_checkout="1" - line 47 should read: <login id="term1" password="term1" delimiter="|" error-detect="enabled" institution="CPL" encoding="ascii" checked_in_ok="1" allow_additional_materials_checkout="1"/> 8. restart_all 9. Repeat step 5 - should now get a different message 'Item has additional materials: ...', and the item is checked out. ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 -l CPL --patron koha -m checkout --item 39999000011418 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 'checkout' SEND: 11YN20230917 11150920230917 111509AOCPL|AAkoha|AB39999000011418|ACterm1|BIN| READ: 121NNY20230917 111509AOCPL|AAkoha|AB39999000011418|AJProgramming Perl /|AH20230922 235900|AFItem has additional materials: Text added to item record|
Created attachment 155920 [details] [review] Bug 34153: Add tests
Created attachment 155921 [details] [review] Bug 34153: Add ability to allow items with additional materials notes to be checked out via SIP At this time, any item with an additional materials date is blocked from checkout via SIP with a screen message to take the item to a circulation desk for checkout. Some libraries wish to allow patrons to check out items via SIP even if the item has additional materials. Test Plan: 1) Create an item with an additional materials note 2) Attempt to check it out via SIP 3) Note the failure and message 4) Enable the new SIP account option "allow_additional_materials_checkout" 5) Restart the SIP server 6) Attempt the checkout again 7) Note the checkout success and new AF field message! 8) prove t/db_dependent/SIP/Message.t Signed-off-by: David Nind <david@davidnind.com>
Split tests in a separate commit to confirm that it indeed fails with the implementation. Code looks good (except tidying work to do), will manually test ASAP Meanwhile there is this to look at and fix: WARN C4/SIP/ILS/Transaction/Checkout.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 58, now: 60) WARN t/db_dependent/SIP/Message.t WARN tidiness The file is less tidy than before (bad/messy lines before: 521, now: 561)
Created attachment 155927 [details] [review] Bug 34153: (QA follow-up) Tidy code
Thanks for the tidying :) Lol perltidy puts tons of spaces to align $response with 'equals'. > check_field( > $respcode, $response, FID_SCREEN_MSG, 'Item must be checked out at a circulation desk', > 'Check screen msg', 'equals' > ); Should we care? Maybe not, let's continue with the workflow and if someone has more on the .perltidyrc matter, let's create a ticket or email the koha-dev list or something. Last point to settle is whether allow_additional_materials_checkout should be on by default. I don't have an educated enough opinion on this. I sent an email to the rest of the QA team, let's see what comes out. > Hi [U+1F642] > > The only thing I need to pass this ticket is to settle on this question. > To eventually make a follow-up changing the default config file. > (it's currently allow_additional_materials_checkout="1") > > I'm leaning a bit towards having it off by default. I suppose libraries > would prefer to have to make the conscious decision to trust the patrons to > self manage all items having additional materials. Otherwise it might feel like a loophole. > Plus the 952$3 needs to be written for the general public instead of for > librarians when having allow_additional_materials_checkout=1. > Or maybe these two points aren't really important, hence reaching for opinion here ^_^
(In reply to Victor Grousset/tuxayo from comment #11) > Lol perltidy puts tons of spaces to align $response with 'equals'. > > check_field( > > $respcode, $response, FID_SCREEN_MSG, 'Item must be checked out at a circulation desk', > > 'Check screen msg', 'equals' > > ); I agree it's an interesting choice, but we have a community .perltidyrc as of bug 30002 and it's just doing what it's told to. If we don't like those results we should probably have a followup to bug 30002. > Last point to settle is whether allow_additional_materials_checkout should > be on by default. I don't have an educated enough opinion on this. I did not add the option to the debian SIPconfig.xml template, so it is off by default ( as it should be to retain current behavior ). The file etc/SIPconfig.xml is just an example file to demonstrate all the possible options in the SIP config, so it's fine to have it "enabled" there!
Created attachment 156250 [details] [review] Bug 34153: Add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 156251 [details] [review] Bug 34153: Add ability to allow items with additional materials notes to be checked out via SIP At this time, any item with an additional materials date is blocked from checkout via SIP with a screen message to take the item to a circulation desk for checkout. Some libraries wish to allow patrons to check out items via SIP even if the item has additional materials. Test Plan: 1) Create an item with an additional materials note 2) Attempt to check it out via SIP 3) Note the failure and message 4) Enable the new SIP account option "allow_additional_materials_checkout" 5) Restart the SIP server 6) Attempt the checkout again 7) Note the checkout success and new AF field message! 8) prove t/db_dependent/SIP/Message.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 156252 [details] [review] Bug 34153: (QA follow-up) Tidy code Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
(In reply to Kyle M Hall from comment #12) > I agree it's an interesting choice, but we have a community .perltidyrc as > of bug 30002 and it's just doing what it's told to. If we don't like those > results we should probably have a followup to bug 30002. I know, that was the question, does it look like something worth tweaking our .perltidyrc? > The file etc/SIPconfig.xml is just an example file > to demonstrate all the possible options in the SIP config aaaaaah, ok all good then :D Works, makes sense, QA script happy, code looks good, passing QA :)
Pushed to master for 23.11. Nice work everyone, thanks!
Great, but more a enhancement. Not pushed to 23.05.x