Bug 34153 - Add ability to allow items with additional materials notes to be checked out via SIP
Summary: Add ability to allow items with additional materials notes to be checked out ...
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-29 12:06 UTC by Kyle M Hall
Modified: 2023-10-14 12:20 UTC (History)
5 users (show)

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


Attachments
Bug 34153: Add ability to allow items with additional materials notes to be checked out via SIP (6.81 KB, patch)
2023-06-29 12:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34153: Add ability to allow items with additional materials notes to be checked out via SIP (6.86 KB, patch)
2023-09-17 11:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 34153: Add tests (3.64 KB, patch)
2023-09-20 06:57 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34153: Add ability to allow items with additional materials notes to be checked out via SIP (3.48 KB, patch)
2023-09-20 06:57 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34153: (QA follow-up) Tidy code (11.71 KB, patch)
2023-09-20 10:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34153: Add tests (3.73 KB, patch)
2023-09-27 06:06 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34153: Add ability to allow items with additional materials notes to be checked out via SIP (3.60 KB, patch)
2023-09-27 06:06 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34153: (QA follow-up) Tidy code (11.85 KB, patch)
2023-09-27 06:06 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-06-29 12:06:11 UTC
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.
Comment 1 Kyle M Hall 2023-06-29 12:10:33 UTC
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
Comment 2 Nick Clemens (kidclamp) 2023-06-29 13:05:07 UTC
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
Comment 3 Liz Rea 2023-06-29 14:00:57 UTC
The syspref override is the usual way to handle this, however if they still want a screen message that won't be sufficient.
Comment 4 Kyle M Hall 2023-06-29 15:02:56 UTC
(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.
Comment 5 David Nind 2023-09-17 11:47:41 UTC
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>
Comment 6 David Nind 2023-09-17 11:56:03 UTC
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|
Comment 7 Victor Grousset/tuxayo 2023-09-20 06:57:12 UTC
Created attachment 155920 [details] [review]
Bug 34153: Add tests
Comment 8 Victor Grousset/tuxayo 2023-09-20 06:57:14 UTC
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>
Comment 9 Victor Grousset/tuxayo 2023-09-20 07:03:31 UTC
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)
Comment 10 Kyle M Hall 2023-09-20 10:25:46 UTC
Created attachment 155927 [details] [review]
Bug 34153: (QA follow-up) Tidy code
Comment 11 Victor Grousset/tuxayo 2023-09-23 21:27:03 UTC
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 🙂
> 
> 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 ^_^
Comment 12 Kyle M Hall 2023-09-25 13:55:11 UTC
(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!
Comment 13 Victor Grousset/tuxayo 2023-09-27 06:06:43 UTC
Created attachment 156250 [details] [review]
Bug 34153: Add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 14 Victor Grousset/tuxayo 2023-09-27 06:06:46 UTC
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>
Comment 15 Victor Grousset/tuxayo 2023-09-27 06:06:48 UTC
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>
Comment 16 Victor Grousset/tuxayo 2023-09-27 06:11:38 UTC
(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 :)
Comment 17 Tomás Cohen Arazi 2023-10-04 20:43:58 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 18 Fridolin Somers 2023-10-06 00:58:57 UTC
Great, but more a enhancement.

Not pushed to 23.05.x