Bug 21572 - Allow checkout an item with reserve that is not yet in waiting state
Summary: Allow checkout an item with reserve that is not yet in waiting state
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Joonas Kylmälä
QA Contact: Testopia
URL:
Keywords:
Depends on: 23233
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-15 13:16 UTC by Joonas Kylmälä
Modified: 2021-03-17 11:38 UTC (History)
7 users (show)

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


Attachments
Bug 21572: Unify checkout policy of reserved items in SIP, SCO, and intranet (20.42 KB, patch)
2018-10-17 14:27 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 21572: Remove unnecessary SIP checkout code regarding holds (4.11 KB, patch)
2018-10-17 14:27 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2018-10-15 13:16:24 UTC
Currently if a librarian checkouts an item to a patron that is in the "Reserved" state the librarian will get a prompt saying "Item Whinney pooh / (1111111111 has been on hold for Joe (1111111111) at My Library since 15.10.2018". If there is little staff the holds might pile up and then patrons who actually went to the library cannot checkout those items. Some libraries prefer to give the first to the patrons who went actually to the physical library building instead of just reserved it online. Koha actually used to work like this with the SIP server before commit "Bug 15438 - Checking out an on-hold item sends holder's borrowernumber in AF (screen message) field."
Comment 1 Joonas Kylmälä 2018-10-15 13:20:10 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.
Comment 2 Katrin Fischer 2018-10-15 16:46:42 UTC
I think this should probably be controlled by the AllowItemsOnHoldCheckout system preference?
Comment 3 Joonas Kylmälä 2018-10-16 08:53:56 UTC
(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
Comment 4 David Cook 2018-10-17 02:53:44 UTC
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...
Comment 5 David Cook 2018-10-17 04:03:59 UTC
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?
Comment 6 Joonas Kylmälä 2018-10-17 06:31:30 UTC
(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!
Comment 7 Joonas Kylmälä 2018-10-17 06:36:05 UTC
(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).
Comment 8 David Cook 2018-10-17 07:28:23 UTC
(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 :/.
Comment 9 Joonas Kylmälä 2018-10-17 08:38:40 UTC
(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.
Comment 10 Joonas Kylmälä 2018-10-17 14:27:03 UTC
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)
Comment 11 Joonas Kylmälä 2018-10-17 14:27:07 UTC
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."
Comment 12 Joonas Kylmälä 2018-10-17 14:36:48 UTC
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.
Comment 13 Katrin Fischer 2018-12-20 21:30:23 UTC
Hi Joonas, please don't forget to assign the bug to yourself when providing a patch!
Comment 14 Marie-Luce Laflamme 2019-05-03 20:37:03 UTC
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
Comment 15 Kyle M Hall 2019-06-28 16:54:29 UTC
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?
Comment 16 Joonas Kylmälä 2019-07-01 07:01:51 UTC
(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.
Comment 17 Katrin Fischer 2019-07-01 08:12:13 UTC
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)
Comment 18 Joonas Kylmälä 2019-07-01 09:24:04 UTC
(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?
Comment 19 Katrin Fischer 2019-07-01 09:27:49 UTC
(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.
Comment 20 Joonas Kylmälä 2019-07-01 09:36:08 UTC
(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.
Comment 21 Kyle M Hall 2019-07-01 11:42:25 UTC
> 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!
Comment 22 Joonas Kylmälä 2019-07-01 11:44:45 UTC
(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".
Comment 23 Joonas Kylmälä 2019-07-01 11:45:30 UTC
(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"
Comment 24 Kyle M Hall 2019-07-01 13:11:04 UTC
(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 ;)
Comment 25 Joonas Kylmälä 2019-10-28 10:08:33 UTC
(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.
Comment 26 Joonas Kylmälä 2019-10-28 10:38:30 UTC
(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)
Comment 27 Joonas Kylmälä 2019-12-18 13:55:08 UTC
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.