Bug 23223

Summary: Allow requested items to be checked out via SIP2
Product: Koha Reporter: Joy Nelson <1joynelson>
Component: SIP2Assignee: Agustín Moyano <agustinmoyano>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: colin.campbell, jonathan.druart, katrin.fischer, kyle, martin.renvoize, mcarrillo, sally.healey, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24761
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 23640    
Bug Blocks:    
Attachments: Bug 23223: Add AllowItemsOnHoldCheckoutSIP preference
Bug 23223: Disable syspref cache in SIPServer
Bug 23223: Add AllowItemsOnHoldCheckoutSIP preference

Description Joy Nelson 2019-06-26 21:00:58 UTC
Currently patrons are being blocked at self-check when they find an item on the shelf that they want and that item could also fill a hold.  
If the item is brought to the circ desk, the staff can override the 'hold' and check it out to the patron.  The SIP2 devices do not allow for the override.

We propose:
1) Add new SIP2 option or system preference ( or 2nd option to AllowItemsOnHoldCheckout ) to allow allow non-waiting items on hold to be checked out
2) Implement business logic in ILS::Transaction::Checkout or C4::Circulation
3) Add unit tests
Comment 1 Katrin Fischer 2019-06-27 06:05:42 UTC
I digged out the original bug report for Adding AllowItemsOnHoldCheckout and it looked like it was supposed to cover SIP2 as well: bug 7090 

So maybe this is a bug/regression?
Comment 2 Sally 2019-10-15 17:09:16 UTC
(In reply to Katrin Fischer from comment #1)
> I digged out the original bug report for Adding AllowItemsOnHoldCheckout and
> it looked like it was supposed to cover SIP2 as well: bug 7090 
> 
> So maybe this is a bug/regression?

Yes, I'd say it was a bug/regression.  

If it helps to narrow it down, we have a Koha installation on 16.11 with AllowItemsOnHoldCheckout set to Allow, and items on hold (not waiting) can be checked out via SIP2.
Comment 3 Agustín Moyano 2019-12-13 01:13:27 UTC
Created attachment 96234 [details] [review]
Bug 23223: Add AllowItemsOnHoldCheckoutSIP preference

This patch adds a new systempreference for behavior for AllowItemsOnHoldCheckout may difer between intranet, SCO and SIP

To test:
1. apply this patch
2. perl installer/data/mysql/updatedatabase.pl
SUCCESS => There is a new syspref called AllowItemsOnHoldCheckoutSIP
3. configure and start SIPServer
4. with misc/sip_cli_emulator.pl try to checkout an item with a hold
CHECK => checkout is not allowed
5. set AllowItemsOnHoldCheckoutSIP to Allow
6. restart SIPServer
7. repeat step 4
SUCCESS => checkout is allowed
8. Sign off
Comment 4 Agustín Moyano 2019-12-13 01:13:31 UTC
Created attachment 96235 [details] [review]
Bug 23223: Disable syspref cache in SIPServer

When a syspref is modified, SIPServer doesn't update it's cached value until restarted. This patch disables syspref cache for SIPServer.

To test:
1. set AllowItemsOnHoldCheckoutSIP preference to Don't allow
2. configure and start sipserver
3. with misc/sip_cli_emulator.pl try to checkout an item that has a hold
CHECK => checkout is not allowed
4. set AllowItemsOnHoldCheckoutSIP preference to Allow
5. repeat step 3
CHECK => checkout is still not allowed
6. apply this patch and restart_all
7. repeat steps 1 to 5
SUCCESS => checkout is allowed / disallowed according to AllowItemsOnHoldCheckoutSIP preference value and no SIPServer restart is needed
8. Sign off
Comment 5 Kyle M Hall 2020-02-28 12:11:56 UTC
Agustin, we typically avoid controlling SIP behavior at the syspref level. Instead, it is always set at the SIP config account level. Some good examples of this are:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22016
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13411
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17826

Would you be able to rewrite your feature to be controlled by a new attribute in the SIP config, on a per-account basis?
Comment 6 Kyle M Hall 2020-02-28 12:21:47 UTC
Considering this patch is splitting the use of an existing system preference, I don't think my previous comment is a blocker ( though adding a config level override would definitely have merit ).
Comment 7 Kyle M Hall 2020-02-28 12:25:05 UTC
(In reply to Kyle M Hall from comment #6)
> Considering this patch is splitting the use of an existing system
> preference, I don't think my previous comment is a blocker ( though adding a
> config level override would definitely have merit ).

I am concerned about your followup disabling syspref caching for the SIP server. I'm starting to wonder if my first instinct was correct, and we should simply not have this behavior controlled by a syspref ( or have a conf setting take precedence over the syspref if defined in the config ).
Comment 8 Martin Renvoize 2020-02-28 12:34:11 UTC
Yikes, I agree.. I really don't like the sledgehammer approach of disabling cache entirely... we should discus a broad change like that on it's own bug.

I presume you're doing it because your finding that caches are not properly flushed when sysprefs change.. I'd much rather see work carried out to try and improve that situation from that direction.

As for Syspref vs Config file.. I generally lean towards sysprefs.. they're accessible to the librarian setting it up and as such clearer that there's a difference between SIP and other operations... I personally think only really the server level stuff belongs in the config files and functionality things like this belong in preferences.
Comment 9 Tomás Cohen Arazi 2020-02-28 12:50:57 UTC
(In reply to Martin Renvoize from comment #8)
> Yikes, I agree.. I really don't like the sledgehammer approach of disabling
> cache entirely... we should discus a broad change like that on it's own bug.
> 
> I presume you're doing it because your finding that caches are not properly
> flushed when sysprefs change.. I'd much rather see work carried out to try
> and improve that situation from that direction.

Take a look at bug 16758.

> As for Syspref vs Config file.. I generally lean towards sysprefs.. they're
> accessible to the librarian setting it up and as such clearer that there's a
> difference between SIP and other operations... I personally think only
> really the server level stuff belongs in the config files and functionality
> things like this belong in preferences.

I agree the idea behind putting configs in the SIPspecific file allows controlling the SIP server behaviour to match different vendors. That way we can have more than one server, with different behaviours depending on the vendor that is using it. BUT in this case is about circulation rules and it feels correct to keep it as a syspref. Unless there's a case for having different vendors different circulation rules for on hold items.
Comment 10 Kyle M Hall 2020-02-28 13:14:53 UTC
I think the best course of action is to move the followup to its' own bug report at this point. I have no problem with the main patch as it stands.
Comment 11 Tomás Cohen Arazi 2020-02-28 14:26:55 UTC
I filed bug 24761 to handle the caching part of this.
Comment 12 Tomás Cohen Arazi 2020-02-28 17:10:56 UTC
Comment on attachment 96235 [details] [review]
Bug 23223: Disable syspref cache in SIPServer

I'm obsoleting the sysprefs caching handling patch here, as I wasn't aware of bug 23640. Thanks Jonathan for pointing that out.
Comment 13 Kyle M Hall 2020-03-02 17:51:36 UTC
Created attachment 99957 [details] [review]
Bug 23223: Add AllowItemsOnHoldCheckoutSIP preference

This patch adds a new systempreference for behavior for AllowItemsOnHoldCheckout may difer between intranet, SCO and SIP

To test:
1. apply this patch
2. perl installer/data/mysql/updatedatabase.pl
SUCCESS => There is a new syspref called AllowItemsOnHoldCheckoutSIP
3. configure and start SIPServer
4. with misc/sip_cli_emulator.pl try to checkout an item with a hold
CHECK => checkout is not allowed
5. set AllowItemsOnHoldCheckoutSIP to Allow
6. restart SIPServer
7. repeat step 4
SUCCESS => checkout is allowed
8. Sign off

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Jonathan Druart 2020-04-14 14:11:33 UTC
Hum? What are we doing here? What is suppose to do this patch?

It adds a syspref that already exists...
Comment 15 Tomás Cohen Arazi 2020-05-27 11:47:29 UTC
(In reply to Jonathan Druart from comment #14)
> Hum? What are we doing here? What is suppose to do this patch?

We obsoleted the patch that dealt with caching as it overlapped with bug 23640, but forgot to extract from it the portion that makes SIP use the new syspref.

> It adds a syspref that already exists...

The new syspref does not yet exist, this patch adds the ability to have different behaviours for SIP than (say) SCO. It is just we missed the fact that the obsolete patch introduced the use of it.
Comment 16 Tomás Cohen Arazi 2020-05-27 11:55:54 UTC
I didn't know about bug 23233, which got pushed in December. So you are right, Jonathan.

The original bug got solved by two different reports:
- bug 23640
- bug 23233

Closing as duplicate

*** This bug has been marked as a duplicate of bug 23233 ***