Summary: | Improve flexibility and consistency of AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO | ||
---|---|---|---|
Product: | Koha | Reporter: | Joonas Kylmälä <joonas.kylmala> |
Component: | Circulation | Assignee: | Emily Lamancusa (emlam) <emily.lamancusa> |
Status: | Needs Signoff --- | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | ajohnson, dcook, edward.m.veal, emily.lamancusa, gmcharlt, kyle, magnus, marie-luce.laflamme, sally.healey, ssmith3 |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23909 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27936 |
||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Medium patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: |
This changes the system preferences AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO to allow multi-select, so that libraries can specify exactly which hold statuses should allow or block another patron from checking out an item using SIP-based self-checkout and/or the Koha SCO module.
|
|
Version(s) released in: | Circulation function: | ||
Bug Depends on: | 40671, 23233 | ||
Bug Blocks: | |||
Attachments: |
Bug 21572: Unify checkout policy of reserved items in SIP, SCO, and intranet
Bug 21572: Remove unnecessary SIP checkout code regarding holds Bug 21572: Update CanBookBeIssued to ignore each hold status separately Bug 21572: Dbrev - multi-selection for AllowItemsOnHoldCheckout sysprefs Bug 21572: Adjust SIP and SCO checkout logic Bug 21572: Adjust SIP and SCO checkout logic Bug 21572: Update CanBookBeIssued to ignore each hold status separately Bug 21572: Dbrev - multi-selection for AllowItemsOnHoldCheckout sysprefs Bug 21572: Adjust SIP and SCO checkout logic |
Description
Joonas Kylmälä
2018-10-15 13:16:24 UTC
Just to add: Koha also used to, and still does, and should continue to prevent checking out with SIP server if the item was in Waiting state, i.e. in the shelf/book pile where reserves waiting for pickup are. I think this should probably be controlled by the AllowItemsOnHoldCheckout system preference? (In reply to Katrin Fischer from comment #2) > I think this should probably be controlled by the AllowItemsOnHoldCheckout > system preference? I hope it could be because then Koha would not become so complex to configure for the user. Currently all the different ways of of checking out are a bit different (SCO, SIP, intranet) and it would be good to unify them. I think we should merge AllowItemsOnHoldCheckoutSCO syspref to AllowItemsOnHoldCheckout. Currently SCO allows to check out items with Waiting state if AllowItemsOnHoldCheckoutSCO is enabled but SIP doesn't with AllowItemsOnHoldCheckout enabled. I think how SCO works with Waiting state holds is bad, so here is my proposal: 1. We have only one syspref: AllowItemsOnHoldCheckout 2. AllowItemsOnHoldCheckout would only allow checking out holds in the Reserved state, i.e. holds that are *not* yet processed by the library's staff – this way we don't have items on the shelfs that cannot be actually checked out => confused patrons. Holds in the Waiting state (i.e. the ones that are processed by staff) could not be checked out except from the intranet if the librarian wants to override this policy. If just one syspref is not possible because some libraries want to allow reserves in waiting state to be checked out by other patrons (but why would anyone, because then "holds" lose their meaning entirely) then I suggest the following two sysprefs instead of AllowItemsOnHoldCheckout: AllowCheckoutIfReserveNotWaiting AllowCheckoutIfReserveWaiting I'm bumping into this issue as well, which is why I'm scouring Bugzilla for more information. I do find it weird that the intranet, SCO, and SIP have different ways of handling checkouts/renewals for items with holds... Actually, I just noticed that while you cannot renew an item that has a hold, you can check it in, ignore the hold, try to check it out again, and override the errors/warnings and check it out again to the original patron. So it looks like maybe you can already do what you want? (In reply to David Cook from comment #5) > Actually, I just noticed that while you cannot renew an item that has a > hold, you can check it in, ignore the hold, try to check it out again, and > override the errors/warnings and check it out again to the original patron. One can renew item with unfilled hold by using AllowRenewalIfOtherItemsAvailable. At least that works when one clicks those checkboxes for renewal in the intranet's checkouts tab for patron. > So it looks like maybe you can already do what you want? No, the problem I personally have is that it is annoying the confirmation box comes for an item with unprocessed holds (this should be configurable with AllowItemsOnHoldCheckout) and also that SCO will let you check out items that have holds in the Waiting state (i.e. they have been processed and when patron shows up they expect them to still be there waiting for them – otherwise there is no point of holds). I'm looking to unify all these three different ways of checking out an item. I started to work on a patch yesterday, hopefully can share something working with you today! (In reply to Joonas Kylmälä from comment #6) > (In reply to David Cook from comment #5) > > Actually, I just noticed that while you cannot renew an item that has a > > hold, you can check it in, ignore the hold, try to check it out again, and > > override the errors/warnings and check it out again to the original patron. > > One can renew item with unfilled hold by using > AllowRenewalIfOtherItemsAvailable. At least that works when one clicks those > checkboxes for renewal in the intranet's checkouts tab for patron. Ah sorry, that syspref is only when there are other items available that can fill the hold. So I guess we are both having the same issue in the end but it just appears now in many different forms. Btw, I think your problem doesn't exists with SIP server (you need to have AllowItemsOnHoldCheckout enabled). (In reply to Joonas Kylmälä from comment #7) > (In reply to Joonas Kylmälä from comment #6) > > (In reply to David Cook from comment #5) > > > Actually, I just noticed that while you cannot renew an item that has a > > > hold, you can check it in, ignore the hold, try to check it out again, and > > > override the errors/warnings and check it out again to the original patron. > > > > One can renew item with unfilled hold by using > > AllowRenewalIfOtherItemsAvailable. At least that works when one clicks those > > checkboxes for renewal in the intranet's checkouts tab for patron. > > Ah sorry, that syspref is only when there are other items available that can > fill the hold. So I guess we are both having the same issue in the end but > it just appears now in many different forms. Btw, I think your problem > doesn't exists with SIP server (you need to have AllowItemsOnHoldCheckout > enabled). I have AllowItemsOnHoldCheckout activated but I'm trying to use the intranet for renewals while holds are waiting and that doesn't work :/. (In reply to David Cook from comment #8) > I have AllowItemsOnHoldCheckout activated but I'm trying to use the intranet > for renewals while holds are waiting and that doesn't work :/. Is AllowRenewalIfOtherItemsAvailable enabled? And since you brought the topic up, I don't think AllowItemsOnHoldCheckout should be used for renewals because libraries at least in Finland, and I would guess elsewhere, want the item to be returned as quickly as possible if there are holds for it (that cannot be satisfied by other items), and not when the maximum amount of renewals has been reached. The librarian can override this restriction in intranet with the way you described earlier. Created attachment 80714 [details] [review] Bug 21572: Unify checkout policy of reserved items in SIP, SCO, and intranet Instead of having two different system preferences, AllowItemsOnHoldCheckout and AllowItemsOnHoldCheckoutSCO, now we have only the former one. The preference is also moved now in the C4::Circulation::CanBookBeIssued subroutine so that we have only one single truth about whether the book can be checked out. This patch changes the current checkout policy a bit. Without this patch patron could check out item in Waiting state with the SCO module but after this patch it is not possible anymore. Only checking out items in Reserved state is possible. This is how the SIP server already currently works so now these two would be unified. Allowing to checkout items in the Waiting state doesn't make any sense because it would defeat the purpose of holds, therefore the SIP server approach on only allowing checking out Reserved state items was chosen. The unwanted message "Item XXXXX / (yyyy) has been on hold for Joe (yyyy) at My Library since 15.10.2018" on intranet will be gone now after this patch if AllowItemsOnHoldCheckout is enabled. To test: Testing in SCO, SIP, Intranet with AllowItemsOnHoldCheckout set 'Don't allow': 1. Apply patch 2. Have a biblio record with one item A 3. Make a hold on item A for patron Y (bib level or item, doesn't matter) 4. Try to checkout item A for patron X 5. It should fail (in case of intranet a warning/confirmation dialog should come) Testing in SCO, SIP, Intranet with AllowItemsOnHoldCheckout set 'Allow': 1. Apply patch 2. Have a biblio record with one item A 3. Make a hold on item A for patron Y (bib level or item, doesn't matter) 4. Try to checkout item A for patron X 5. It should succeed (in case of intranet there should be no confirmation dialog) Testing in SCO, SIP, Intranet with AllowItemsOnHoldCheckout set 'Allow' or 'Don't allow': 1. Apply patch 2. Have a biblio record with one item A 3. Make a hold on item A for patron Y (bib level or item, doesn't matter) 4. Check in item A in intranet so that it goes to Waiting status 5. Try to checkout item A for patron X 6. It should fail (in case of intranet a warning/confirmation dialog should come) Created attachment 80715 [details] [review] Bug 21572: Remove unnecessary SIP checkout code regarding holds The loop that checks whether "Item is on hold shelf for another patron" is already done in C4::Circulation::CanBookBeIssued() so we don't have to do it again but just trust it to tell us the confirmation message "RESERVE_WAITING" when it is someone else's hold that the patron tries to checkout. This removes also the message "Item was reserved for you." since it could have have never appeared since RESERVED confirmation message comes only when the item is reserved to another patron. To test: 1. Apply patch 2. Have a biblio record with one item A 3. Make a hold on item A for patron Y (bib level or item, doesn't matter) 4. Check in item A in intranet so that it goes to Waiting status 5. Try to checkout item A for patron X with the SIP server 6. Checkout should fail with message "Item is on hold shelf for another patron." 1. Apply patch 2. Have a biblio record with two items A and B 3. Make a hold on item A for patron Y (bib level or item, doesn't matter) 4. Check in item A in intranet so that it goes to Waiting status 5. Try to checkout item B for patron X with the SIP server 6. Checkout should succeed 1. Apply patch 2. Make sure AllowItemsOnHoldCheckout is set "Don't allow" 3. Have a biblio record with one item A 4. Make a hold on item A for patron Y (bib level or item, doesn't matter) 5. Try to checkout item A for patron X with the SIP server 6. Checkout should fail with message "Item is reserved for another patron upon return." Please review the patches I sent. I have only tested thoroughly the SIP code change, i.e. "Bug 21572: Remove unnecessary SIP checkout code regarding holds" and done its test plan. The first patch "Bug 21572: Unify checkout policy of reserved items in SIP, SCO, and intranet" I have not tested so much and was hoping if you could and I didn't have time to go through the provided test plan because there are 9 (or 12) different plans to go through and I have other things I need to focus on. Katrin or David, would you able to test? Maybe just for one of the three checkout ways (SIP, SCO, intra) if testing everything is too much. Hi Joonas, please don't forget to assign the bug to yourself when providing a patch! patch does not apply CONFLICT (content): Merge conflict in C4/Circulation.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 21572: Unify checkout policy of reserved items in SIP, SCO, and intranet Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-21572-Unify-checkout-policy-of-reserved-items--bkchx6.patch Ignoring the fact that AllowItemsOnHoldCheckout appears to be broken right now, this seems like it is just *removing* flexibility from Koha, right? Why would we want less flexibility instead of more? (In reply to Kyle M Hall from comment #15) > Ignoring the fact that AllowItemsOnHoldCheckout appears to be broken right > now, this seems like it is just *removing* flexibility from Koha, right? Why > would we want less flexibility instead of more? Thanks for this comment, for now I think we should keep both of these sysprefs as discussed in bug #15131. I will make a new version of the patch where this setting is available again and also applies to SIP checkouts instead of just SCO module. Later on we could probably address #15131 in some better way like making a possibility to authorize another patron to pickup their items. I'm moving this bug back to ASSIGNED state. Hi Joonas and Kyle, please also take a look at: bug 23233 bug 23223 and bug 7090 (which suggests that this was for SIP initially) (In reply to Katrin Fischer from comment #17) > please also take a look at: > bug 23233 > bug 23223 > and bug 7090 (which suggests that this was for SIP initially) Mhm, okay so in #23233 Kyle says the AllowItemsOnHoldCheckout affects only SIP based checkouts which is almost true but not quite there because of one additional usage in the "has_pending_hold" function. If we get rid of the usage in "has_pending_hold" function then IMHO it would okay to merge AllowItemsOnHoldCheckout and AllowItemsOnHoldCheckoutSCO together since they are both self-checkouts. That lead me to the question why in #15131 this new syspref AllowItemsOnHoldCheckoutSCO was even introduced? Didn't the bug reporter just know because of the wrong documentation (as Jonathan points out in the report) that the AllowItemsOnHoldCheckout would have worked also in the situation described in the report? (In reply to Joonas Kylmälä from comment #18) > (In reply to Katrin Fischer from comment #17) > > please also take a look at: > > bug 23233 > > bug 23223 > > and bug 7090 (which suggests that this was for SIP initially) > > Mhm, okay so in #23233 Kyle says the AllowItemsOnHoldCheckout affects only > SIP based checkouts which is almost true but not quite there because of one > additional usage in the "has_pending_hold" function. If we get rid of the > usage in "has_pending_hold" function then IMHO it would okay to merge > AllowItemsOnHoldCheckout and AllowItemsOnHoldCheckoutSCO together since they > are both self-checkouts. That lead me to the question why in #15131 this new > syspref AllowItemsOnHoldCheckoutSCO was even introduced? Didn't the bug > reporter just know because of the wrong documentation (as Jonathan points > out in the report) that the AllowItemsOnHoldCheckout would have worked also > in the situation described in the report? I think better to keep them separate - both are self checks, but... libraries. There might be a use case that we are not aware of right now. (In reply to Katrin Fischer from comment #19) > I think better to keep them separate - both are self checks, but... > libraries. There might be a use case that we are not aware of right now. Okay, fine by me, but maybe we could also announce somewhere in the main page that we are deprecating it? Or better, in the syspref setttings page. This way we don't have to carry it with us just because theoretically somebody might want to use it. And so if somebody complains then we remove the deprecation. I'm gonna make this bug depend on #23233 (the renaming of AllowItemsOnHoldCheckout to AllowItemsOnHoldCheckoutSIP) since that sounds easy to fix now and would conflict with this patch. > Okay, fine by me, but maybe we could also announce somewhere in the main
> page that we are deprecating it? Or better, in the syspref setttings page.
> This way we don't have to carry it with us just because theoretically
> somebody might want to use it. And so if somebody complains then we remove
> the deprecation.
I'm not sure I understand what you are proposing we deprecate. Can you clarify that for me? Thanks!
(In reply to Kyle M Hall from comment #21) > > Okay, fine by me, but maybe we could also announce somewhere in the main > > page that we are deprecating it? Or better, in the syspref setttings page. > > This way we don't have to carry it with us just because theoretically > > somebody might want to use it. And so if somebody complains then we remove > > the deprecation. > > I'm not sure I understand what you are proposing we deprecate. Can you > clarify that for me? Thanks! To deprecate AllowItemsOnHoldCheckoutSCO and just have one syspref, e.g. "AllowItemsOnHoldCheckoutSCO". (In reply to Joonas Kylmälä from comment #22) > > I'm not sure I understand what you are proposing we deprecate. Can you > > clarify that for me? Thanks! > > To deprecate AllowItemsOnHoldCheckoutSCO and just have one syspref, e.g. > "AllowItemsOnHoldCheckoutSCO". Sorry, I meant just to have for example "AllowItemsOnHoldCheckout" (In reply to Joonas Kylmälä from comment #23) > (In reply to Joonas Kylmälä from comment #22) > > > I'm not sure I understand what you are proposing we deprecate. Can you > > > clarify that for me? Thanks! > > > > To deprecate AllowItemsOnHoldCheckoutSCO and just have one syspref, e.g. > > "AllowItemsOnHoldCheckoutSCO". > > Sorry, I meant just to have for example "AllowItemsOnHoldCheckout" Thanks! I tend to agree with Katrin. For instance, it may be that Koha SCO is being used on site and librarians want to prevent patrons from grabbing books from the hold shelf and SIP is being used off site and if the "patron has possession" of the item they need to be able to check it out. This scenario is completely contrived but I think it's illustrative ;) (In reply to Kyle M Hall from comment #24) > Thanks! I tend to agree with Katrin. For instance, it may be that Koha SCO > is being used on site and librarians want to prevent patrons from grabbing > books from the hold shelf and SIP is being used off site and if the "patron > has possession" of the item they need to be able to check it out. This > scenario is completely contrived but I think it's illustrative ;) If this is the only scenario that could cause problem theoretically to someone then I suggest we still would unify the sysprefs and fix this by making an option in the branch level to allow checkouts of reserved items. In my opinion SIP and SCO should be both treated the same or libraries will start abusing these features like by using the SCO module in one branch because it can do the job even though SIP would be better because you don't have to maintain a different kinds of self checkout machine. (In reply to Joonas Kylmälä from comment #25) > (In reply to Kyle M Hall from comment #24) > > Thanks! I tend to agree with Katrin. For instance, it may be that Koha SCO > > is being used on site and librarians want to prevent patrons from grabbing > > books from the hold shelf and SIP is being used off site and if the "patron > > has possession" of the item they need to be able to check it out. This > > scenario is completely contrived but I think it's illustrative ;) > > If this is the only scenario that could cause problem theoretically to > someone then I suggest we still would unify the sysprefs and fix this by > making an option in the branch level to allow checkouts of reserved items. > In my opinion SIP and SCO should be both treated the same or libraries will > start abusing these features like by using the SCO module in one branch > because it can do the job even though SIP would be better because you don't > have to maintain a different kinds of self checkout machine. To clarify, with reserved item I mean reserved and in waiting state (as the original patch is about removing the possibility of checking out waiting reserved items in SCO module) I will start reworking the patches so that they show more clearly the logical differences between SIP and SCO checkout and that the code is the all in the CanBookBeIssued subroutine but now it can be just controlled with parameters. This way the deprecation discussion can be had separately. Nudging this bug and wondering where it's at. Our library system would like to have the patron who pulled it off the shelf be able to check the item out via SCO, even if there is a pending hold for it somewhere in the system. This has been a major pain point during COVID as most checkouts have been via SCO. I know this has been inactive for a while, but this is still a regular pain point for us. We are almost completely self checkout and our pending list is very active . For example, a patron can pull a book of the shelf to checkout, but a hold could be placed while they continue to browse. Now they require staff assistance to override this and they cannot fully use self checkout. Making an attempt to revive this one... Past attempts to adjust this behavior have mostly gotten held up by concerns about the behavior change removing flexibility or disrupting an existing workflow. In that case, why not add flexibility instead by changing these sysprefs to multi-select, and allowing libraries to specify exactly which hold statuses should block checkout in each case? That would allow libraries to preserve any aspects of the existing behavior that they find desirable, without introducing any new sysprefs or added complexity. It would also make it a lot clearer exactly what the sysprefs will do. Created attachment 185328 [details] [review] Bug 21572: Update CanBookBeIssued to ignore each hold status separately Adjust CanBookBeIssued parameter $ignore_reserves from a scalar to a hashref, to allow granular override of each hold status. To test: prove t/db_dependent/Circulation.t Created attachment 185330 [details] [review] Bug 21572: Dbrev - multi-selection for AllowItemsOnHoldCheckout sysprefs Update AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO to allow multiple selection, so that libraries can specify whether patrons should be allowed to check out items with found holds granularly according to hold status. The existing behavior for AllowItemsOnHoldCheckoutSIP is: Allow - allow items with pending holds (but no other hold status) to be checked out Don't allow - don't allow items to be checked out if any hold status is found (including pending/unassigned holds) The existing behavior for AllowItemsOnHoldCheckoutSCO is: Allow - allow items with holds in any status to be checked out Don't allow - don't allow items to be checked out if any hold status is found (including pending/unassigned holds) This patch retains existing behavior while giving libraries the opportunity to further customize which types of checkouts should be allowed in each case. This also resolves the behavior inconsistency between the SCO module and SIP-based self-checkouts. To test: 1. Before applying patch, go to Administration module and search for "AllowItemsOnHoldCheckout" in the system preference search 2. Set both preferences to Allow 3. Apply patch and updatedatabase 4. Search for "AllowItemsOnHoldCheckout" in the system preference search again (to reload the page without resubmitting) --> Confirm that both preferences now allow multiple selection --> Confirm that the system preferences have the following settings, to retain existing behavior: AllowItemsOnHoldCheckoutSIP - only Pending is checked AllowItemsOnHoldCheckoutSCO - all hold statuses are checked Bonus testing: Make some changes to the settings and run updatedatabase again. Confirm that your settings persist after the database update. Created attachment 185332 [details] [review] Bug 21572: Adjust SIP and SCO checkout logic Handle the new multi-select values for AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO To test: 1. With patch applied, edit AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO to clear all options 2. Set HoldsNeedProcessingSIP to "Don't fulfill" 3. Place holds on 4 items: a. Hold A - place hold and leave as-is (pending status) b. Hold B - place hold for pickup at logged-in branch. Check in the item and confirm the hold (waiting status) c. Hold C - place hold for pickup at another branch. Check in the item and confirm the transfer (transferred status) d. Hold D - place hold for pickupt at the logged-in branch. 4. Use the SIP CLI emulator to check in the item from Hold D (this will set it to processing status) : misc/sip_cli_emulator.pl -a localhost -p 6001 -l <logged-in library branchcode> -su term1 -sp term1 -m checkin --item <barcode> 5. Use the SIP CLI emulator to attempt to check out each of the four items: misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m checkout --patron koha --item <barcode> --> Confirm that all checkouts are refused 6. Navigate to the SCO module and log in: http://localhost:8080/cgi-bin/koha/sco/sco-main.pl 7. Attempt to check out each of the four items --> Confirm that all checkouts are refused 8. Edit AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO and check all options 9. Attempt to check out each of the four items --> Confirm that all checkouts are now allowed 10. Set up four new holds as in step 3-4 with new items 11. Attempt to check out each of the four new items using the SIP CLI emulator as in step 5 --> Confirm that all checkouts are now allowed 12. prove t/db_dependent/SIP/Transaction.t Note: With these patches, when an item assigned to a hold is checked out, the hold status is not reverted - the item remains both checked out and on hold. I'd consider this a bug, but it's the existing behavior on main when AllowItemsOnHoldCheckoutSCO is set to allow, and outside the scope of this bug. If there's agreement on the approach for this bug, I'll submit patches on a follow-up bug to revert the hold status when checking out an item assigned to a hold. Thanks for the new patches, I will test them and see if I can sign off. But I also wanted to raise a question if there is some more basic flaw in the logic here: Say you have a record with 5 items and one record level hold (on the shelf hold). If you try to check out any item connected with this record in SCO you are denied, with this message: Sorry, this item cannot be checked out at this station. This item is on hold for another patron. Please see a member of the library staff. This happens even if there are four items left on the shelf, that can be used to fulfill the one hold on the record. And it says "This item is on hold", even if there is no item level hold, just a record level hold. Looking at the code, it looks like there is a shift from "check if there are any holds on this particular item" to "check if there are any holds on the record it belongs to". I wonder if this is meant to happen or if it is a bug. Do any libraries actually want this functionality? opac/sco/sco-main.pl calls C4::Circulation::CanBookBeIssued like this: ( $impossible, $needconfirm ) = CanBookBeIssued( $patron, $barcode, undef, 0, C4::Context->preference("AllowItemsOnHoldCheckoutSCO") ); https://git.koha-community.org/Koha-community/Koha/src/branch/main/opac/sco/sco-main.pl#L196 CanBookBeIssued says (note the singular "item" in the comment): # See if the item is on reserve. my ( $restype, $res ) = C4::Reserves::CheckReserves($item_object); https://git.koha-community.org/Koha-community/Koha/src/branch/main/C4/Circulation.pm#L1216 The POD for C4::Reserves::CheckReserves says: As I understand it, C<&CheckReserves> looks for the given item in the reserves. If it is found, that's a match, and C<$status> is set to C<Waiting>. Otherwise, it finds the most important item in the reserves with the same biblio number as this book (I'm not clear on this) and returns it with C<$status> set to C<Reserved>. https://git.koha-community.org/Koha-community/Koha/src/branch/main/C4/Reserves.pm#L833 I suspect there is a problem in the shift that happens here, from looking for reserves connected directly to the item, to looking for any reserves connected to the record. Not sure if this is directly relevant to the patches on this bug, or if it should be a sperate bug. I tested this in KTD, initially with the scanrio I described above: - 1 record with 5 items - Place a record level hold on the record for Edna Acosta - Select all the options for AllowItemsOnHoldCheckoutSCO - Log into SCO with the koha/koha user - Try to issue on of the items from the record to myself I get this error: Sorry, this item cannot be checked out at this station. This item is on hold for another patron. Please see a member of the library staff. If I remove the record level hold I can check the item out to myself. If I add the hold again, I can not check out. Created attachment 185394 [details] [review] Bug 21572: Adjust SIP and SCO checkout logic Handle the new multi-select values for AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO To test: 1. With patch applied, edit AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO to clear all options 2. Set HoldsNeedProcessingSIP to "Don't fulfill" 3. Place holds on 4 items: a. Hold A - place hold and leave as-is (pending status) b. Hold B - place hold for pickup at logged-in branch. Check in the item and confirm the hold (waiting status) c. Hold C - place hold for pickup at another branch. Check in the item and confirm the transfer (transferred status) d. Hold D - place hold for pickupt at the logged-in branch. 4. Use the SIP CLI emulator to check in the item from Hold D (this will set it to processing status) : misc/sip_cli_emulator.pl -a localhost -p 6001 -l <logged-in library branchcode> -su term1 -sp term1 -m checkin --item <barcode> 5. Use the SIP CLI emulator to attempt to check out each of the four items: misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m checkout --patron koha --item <barcode> --> Confirm that all checkouts are refused 6. Navigate to the SCO module and log in: http://localhost:8080/cgi-bin/koha/sco/sco-main.pl 7. Attempt to check out each of the four items --> Confirm that all checkouts are refused 8. Edit AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO and check all options 9. Attempt to check out each of the four items --> Confirm that all checkouts are now allowed 10. Set up four new holds as in step 3-4 with new items 11. Attempt to check out each of the four new items using the SIP CLI emulator as in step 5 --> Confirm that all checkouts are now allowed 12. prove t/db_dependent/SIP/Transaction.t Thanks for testing, Mangus! I wondered about that behavior for record-level holds myself - it's present in main, and was actually one of my motivating factors in writing this patchset. I didn't think about the use case where a library might want to prevent checkout if there is a pending item-level hold but not a pending record-level hold, but I agree that would be a sensible behavior, and is worth adding a follow-up patch for - stay tuned! The testing error was a copy/paste error - the SCO was checking the SIP preference instead of the SCO preference. Good catch! (In reply to Emily Lamancusa (emlam) from comment #38) > I didn't think about the use case where a > library might want to prevent checkout if there is a pending item-level hold > but not a pending record-level hold, but I agree that would be a sensible > behavior, and is worth adding a follow-up patch for - stay tuned! On second thought, I think it would make more sense to address that use case in a separate bug after all. I'm not sure any libraries would want bib-level holds to block checkouts if there are other items available, but I think some would want to block checkouts if there are no other items that could fill the hold, and that's a bigger patch that would be out of scope here. In theory I could make "pending" in this patchset refer to pending item-level holds only and leave the behavior of bib-level holds completely for a follow-up bug, but I'd rather not make any changes here that don't allow the existing behavior to be exactly perserved, since that kind of thing has held up this bug for 7 years already! I uploaded a revised patch to fix the error from comment 36, so setting this back to NSO. Sorry: $ git bz apply 21572 Bug 21572 - Improve flexibility and consistency of AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO 185328 - Bug 21572: Update CanBookBeIssued to ignore each hold status separately 185330 - Bug 21572: Dbrev - multi-selection for AllowItemsOnHoldCheckout sysprefs 185394 - Bug 21572: Adjust SIP and SCO checkout logic Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 21572: Update CanBookBeIssued to ignore each hold status separately Applying: Bug 21572: Dbrev - multi-selection for AllowItemsOnHoldCheckout sysprefs Applying: Bug 21572: Adjust SIP and SCO checkout logic Using index info to reconstruct a base tree... M t/db_dependent/SIP/Transaction.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/SIP/Transaction.t CONFLICT (content): Merge conflict in t/db_dependent/SIP/Transaction.t error: Failed to merge in the changes. Patch failed at 0001 Bug 21572: Adjust SIP and SCO checkout logic hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-21572-Adjust-SIP-and-SCO-checkout-logic-_z0p7w87.patch Ah, it conflicts with the test change on bug 40058. Rebasing on top of 40058's follow-up, 40671, so we don't conflict again. Created attachment 185564 [details] [review] Bug 21572: Update CanBookBeIssued to ignore each hold status separately Adjust CanBookBeIssued parameter $ignore_reserves from a scalar to a hashref, to allow granular override of each hold status. To test: prove t/db_dependent/Circulation.t Created attachment 185565 [details] [review] Bug 21572: Dbrev - multi-selection for AllowItemsOnHoldCheckout sysprefs Update AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO to allow multiple selection, so that libraries can specify whether patrons should be allowed to check out items with found holds granularly according to hold status. The existing behavior for AllowItemsOnHoldCheckoutSIP is: Allow - allow items with pending holds (but no other hold status) to be checked out Don't allow - don't allow items to be checked out if any hold status is found (including pending/unassigned holds) The existing behavior for AllowItemsOnHoldCheckoutSCO is: Allow - allow items with holds in any status to be checked out Don't allow - don't allow items to be checked out if any hold status is found (including pending/unassigned holds) This patch retains existing behavior while giving libraries the opportunity to further customize which types of checkouts should be allowed in each case. This also resolves the behavior inconsistency between the SCO module and SIP-based self-checkouts. To test: 1. Before applying patch, go to Administration module and search for "AllowItemsOnHoldCheckout" in the system preference search 2. Set both preferences to Allow 3. Apply patch and updatedatabase 4. Search for "AllowItemsOnHoldCheckout" in the system preference search again (to reload the page without resubmitting) --> Confirm that both preferences now allow multiple selection --> Confirm that the system preferences have the following settings, to retain existing behavior: AllowItemsOnHoldCheckoutSIP - only Pending is checked AllowItemsOnHoldCheckoutSCO - all hold statuses are checked Bonus testing: Make some changes to the settings and run updatedatabase again. Confirm that your settings persist after the database update. Created attachment 185566 [details] [review] Bug 21572: Adjust SIP and SCO checkout logic Handle the new multi-select values for AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO To test: 1. With patch applied, edit AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO to clear all options 2. Set HoldsNeedProcessingSIP to "Don't fulfill" 3. Place holds on 4 items: a. Hold A - place hold and leave as-is (pending status) b. Hold B - place hold for pickup at logged-in branch. Check in the item and confirm the hold (waiting status) c. Hold C - place hold for pickup at another branch. Check in the item and confirm the transfer (transferred status) d. Hold D - place hold for pickupt at the logged-in branch. 4. Use the SIP CLI emulator to check in the item from Hold D (this will set it to processing status) : misc/sip_cli_emulator.pl -a localhost -p 6001 -l <logged-in library branchcode> -su term1 -sp term1 -m checkin --item <barcode> 5. Use the SIP CLI emulator to attempt to check out each of the four items: misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m checkout --patron koha --item <barcode> --> Confirm that all checkouts are refused 6. Navigate to the SCO module and log in: http://localhost:8080/cgi-bin/koha/sco/sco-main.pl 7. Attempt to check out each of the four items --> Confirm that all checkouts are refused 8. Edit AllowItemsOnHoldCheckoutSIP and AllowItemsOnHoldCheckoutSCO and check all options 9. Attempt to check out each of the four items --> Confirm that all checkouts are now allowed 10. Set up four new holds as in step 3-4 with new items 11. Attempt to check out each of the four new items using the SIP CLI emulator as in step 5 --> Confirm that all checkouts are now allowed 12. prove t/db_dependent/SIP/Transaction.t |