Bug 27058

Summary: Cannot place hold to ordered item when on shelf holds are not allowed
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: CirculationAssignee: Joonas Kylmälä <joonas.kylmala>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: andrewfh, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, kyle, m.de.rooy, nengard, nugged
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6918
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04
Attachments: Bug 27058: Make checkout availability use all notforloan reasons
Bug 27058: Add test to show ordered items cannot be reserved
Bug 27058: Make checkout availability use all notforloan reasons
Bug 27058: Add test to show ordered items cannot be checked out
Bug 27058: Make checkout availability use all notforloan reasons
Bug 27058: Add test to show ordered items cannot be checked out
Bug 27058: Make checkout availability use all notforloan reasons
Bug 27058: Add test to show ordered items cannot be checked out
Bug 27058: Make checkout availability use all notforloan reasons
Bug 27058: Remove confusing and unnecessary $biblionumber1 variable
Bug 27058: Add test to show ordered items cannot be checked out
Bug 27058: Make checkout availability use all notforloan reasons
Bug 27058: Remove confusing and unnecessary $biblionumber1 variable
Bug 27058: Add test for IsAvailableForItemLevelRequest and notforloan
Bug 27058: Add test for IsAvailableForItemLevelRequest and notforloan
Bug 27058: (follow-up) Clarify notforloan checks with a comment

Description Joonas Kylmälä 2020-11-19 14:55:06 UTC
If you have on shelf holds with "If all unavailable" option in circulation rules  one cannot place holds on biblio where there is only ordered items (notforloan = -1). This should be possible since "If all unavailable" means that all items should be unavailable for checkout for the customer before a hold can be placed and ordered items are unavailable for checkout.

I'm attaching a trivial patch to fix this but we still need unit test.
Comment 1 Joonas Kylmälä 2020-11-19 15:00:30 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2020-11-19 15:33:00 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2020-11-19 15:33:03 UTC Comment hidden (obsolete)
Comment 4 Joonas Kylmälä 2020-11-19 15:59:12 UTC
Created attachment 113824 [details] [review]
Bug 27058: Add test to show ordered items cannot be checked out
Comment 5 Joonas Kylmälä 2020-11-19 15:59:15 UTC
Created attachment 113825 [details] [review]
Bug 27058: Make checkout availability use all notforloan reasons

The ItemsAnyAvailableAndNotRestricted function is checking whether an
item can be checked out and it incorrectly only checks the positive
notforloan values when there can be also negative notforloan
values. If notforloan value is not 0 then it means the item cannot be
checked out. In the case of ordered items the value is negative (-1)
and thus before this change the checkout availability was reported
incorrectly.

To test:
 1) Run prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
    Notice it fails
 2) Apply this patch
 3) Run prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
    Notice it passes now.

To test via Koha sandbox (Alternatively):
 1) Create circ rule with If all unavailable
 2) Create new biblio
 3) Order a new item to the biblio via acquisitions and set the not
    for loan value to Ordered / -1
 4) Notice you cannot place a hold to the biblio
 5) Apply patch
 6) Notice you can now place a hold to the biblio
Comment 6 Kyle M Hall 2020-12-04 14:20:27 UTC
The unit test isn't failing wit the second patch applied. Should it?
Comment 7 Kyle M Hall 2020-12-04 14:20:48 UTC
(In reply to Kyle M Hall from comment #6)
> The unit test isn't failing wit the second patch applied. Should it?

With the second patch *not* applied, that is.
Comment 8 Joonas Kylmälä 2020-12-04 14:56:41 UTC
Created attachment 114203 [details] [review]
Bug 27058: Add test to show ordered items cannot be checked out
Comment 9 Joonas Kylmälä 2020-12-04 14:56:44 UTC
Created attachment 114204 [details] [review]
Bug 27058: Make checkout availability use all notforloan reasons

The ItemsAnyAvailableAndNotRestricted function is checking whether an
item can be checked out and it incorrectly only checks the positive
notforloan values when there can be also negative notforloan
values. If notforloan value is not 0 then it means the item cannot be
checked out. In the case of ordered items the value is negative (-1)
and thus before this change the checkout availability was reported
incorrectly.

To test:
 1) Run prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
    Notice it fails
 2) Apply this patch
 3) Run prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
    Notice it passes now.

To test via Koha sandbox (Alternatively):
 1) Create circ rule with If all unavailable
 2) Create new biblio
 3) Order a new item to the biblio via acquisitions and set the not
    for loan value to Ordered / -1
 4) Notice you cannot place a hold to the biblio
 5) Apply patch
 6) Notice you can now place a hold to the biblio
Comment 10 Joonas Kylmälä 2020-12-04 14:59:40 UTC
(In reply to Kyle M Hall from comment #6)
> The unit test isn't failing wit the second patch applied. Should it?

yes, it should have failed. I fixed the test now – the circulation rule in the scenario was made for $itemtype itemtype when the ordered item had $itemtype2 itemtype. Not sure why I didn't spot this on my testing, maybe the test database I had was different for me earlier? Anyway, ready for review now.
Comment 11 Kyle M Hall 2020-12-04 18:30:08 UTC
Created attachment 114210 [details] [review]
Bug 27058: Add test to show ordered items cannot be checked out

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2020-12-04 18:30:20 UTC
Created attachment 114211 [details] [review]
Bug 27058: Make checkout availability use all notforloan reasons

The ItemsAnyAvailableAndNotRestricted function is checking whether an
item can be checked out and it incorrectly only checks the positive
notforloan values when there can be also negative notforloan
values. If notforloan value is not 0 then it means the item cannot be
checked out. In the case of ordered items the value is negative (-1)
and thus before this change the checkout availability was reported
incorrectly.

To test:
 1) Run prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
    Notice it fails
 2) Apply this patch
 3) Run prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
    Notice it passes now.

To test via Koha sandbox (Alternatively):
 1) Create circ rule with If all unavailable
 2) Create new biblio
 3) Order a new item to the biblio via acquisitions and set the not
    for loan value to Ordered / -1
 4) Notice you cannot place a hold to the biblio
 5) Apply patch
 6) Notice you can now place a hold to the biblio

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Marcel de Rooy 2021-02-19 07:36:12 UTC
 my $biblionumber1 = $biblio2->biblionumber;

Obscure way of coding
Comment 14 Joonas Kylmälä 2021-02-19 07:45:05 UTC
(In reply to Marcel de Rooy from comment #13)
>  my $biblionumber1 = $biblio2->biblionumber;
> 
> Obscure way of coding

Thanks for spotting. There is a similarly named global variable so I probably just wanted to get rid of those conflicts... Follow-up coming in 5.
Comment 15 Joonas Kylmälä 2021-02-19 07:53:56 UTC
Created attachment 117021 [details] [review]
Bug 27058: Remove confusing and unnecessary $biblionumber1 variable

The code is much more understandable now because with $biblio2 and
$biblionumber1 variables use to point to the same biblio one might
have thought $biblionumber1 points to $biblio1 which in this case is
not true. Let's just drop the extra variable because the object
notation of accessing is just as simple.

To test:
 1) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t => passes
Comment 16 Joonas Kylmälä 2021-02-19 07:55:07 UTC
(In reply to Joonas Kylmälä from comment #14)
> (In reply to Marcel de Rooy from comment #13)
> >  my $biblionumber1 = $biblio2->biblionumber;
> > 
> > Obscure way of coding
> 
> Thanks for spotting. There is a similarly named global variable so I
> probably just wanted to get rid of those conflicts... Follow-up coming in 5.

Done. I fixed the other instance of same problem in the file as well while I was it.
Comment 17 Marcel de Rooy 2021-02-19 08:06:19 UTC
(In reply to Joonas Kylmälä from comment #16)
> (In reply to Joonas Kylmälä from comment #14)
> > (In reply to Marcel de Rooy from comment #13)
> > >  my $biblionumber1 = $biblio2->biblionumber;
> > > 
> > > Obscure way of coding
> > 
> > Thanks for spotting. There is a similarly named global variable so I
> > probably just wanted to get rid of those conflicts... Follow-up coming in 5.
> 
> Done. I fixed the other instance of same problem in the file as well while I
> was it.

Thx for fast response. Will have a look.

QAing
Comment 18 Marcel de Rooy 2021-02-19 10:24:12 UTC
Quite distracted by the skipholdstrap stuff close to your changes. See 27729.
Comment 19 Marcel de Rooy 2021-02-19 11:02:26 UTC
From CanBookBeRenewed:
                    next unless IsAvailableForItemLevelRequest($item, $patron);
                    next unless CanItemBeReserved($borrowernumber,$itemnumber);
Why didnt we add a parameter to CanItemBeReserved if we need to specifically check for item level request?
At least some checks are done twice.

Same in opac-reserve.pl and request.pl
$policy_holdallowed &&=
            IsAvailableForItemLevelRequest($item, $patron) &&
            CanItemBeReserved( $borrowernumber, $itemNum )->{status} eq 'OK';

                    && $can_item_be_reserved eq 'OK'
                    && IsAvailableForItemLevelRequest($item_object, $patron, undef, $items_any_available)

So, in conclusion: The ItemLevelRequest is often used together with a CanItemBeReserved.
We are changing here ItemsAnyAvailableAndNotRestricted. Which is called by ItemLevelRequest. And which - how funny - even calls (sometimes) CanItemBeReserved.
Comment 20 Marcel de Rooy 2021-02-19 11:04:16 UTC
Sorry, I cant finish QA for now. Your patch is quite small but the topic of Reserving ordered items has some angles and the Holds code is a mess as illustrated by the former comment.
Comment 21 Marcel de Rooy 2021-03-05 07:35:12 UTC
Created attachment 117780 [details] [review]
Bug 27058: Add test to show ordered items cannot be checked out

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2021-03-05 07:35:16 UTC
Created attachment 117781 [details] [review]
Bug 27058: Make checkout availability use all notforloan reasons

The ItemsAnyAvailableAndNotRestricted function is checking whether an
item can be checked out and it incorrectly only checks the positive
notforloan values when there can be also negative notforloan
values. If notforloan value is not 0 then it means the item cannot be
checked out. In the case of ordered items the value is negative (-1)
and thus before this change the checkout availability was reported
incorrectly.

To test:
 1) Run prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
    Notice it fails
 2) Apply this patch
 3) Run prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
    Notice it passes now.

To test via Koha sandbox (Alternatively):
 1) Create circ rule with If all unavailable
 2) Create new biblio
 3) Order a new item to the biblio via acquisitions and set the not
    for loan value to Ordered / -1
 4) Notice you cannot place a hold to the biblio
 5) Apply patch
 6) Notice you can now place a hold to the biblio

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2021-03-05 07:35:19 UTC
Created attachment 117782 [details] [review]
Bug 27058: Remove confusing and unnecessary $biblionumber1 variable

The code is much more understandable now because with $biblio2 and
$biblionumber1 variables use to point to the same biblio one might
have thought $biblionumber1 points to $biblio1 which in this case is
not true. Let's just drop the extra variable because the object
notation of accessing is just as simple.

To test:
 1) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t => passes

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2021-03-05 07:36:46 UTC
Cant block this one for the stuff mentioned in comment19. Refactor_holds++
Comment 25 Joonas Kylmälä 2021-03-05 07:59:33 UTC
(In reply to Marcel de Rooy from comment #24)
> Cant block this one for the stuff mentioned in comment19. Refactor_holds++

Regarding the refactoring, I have already opened some bug reports, e.g. bug 27032.
Comment 26 Jonathan Druart 2021-03-05 12:38:37 UTC
Joonas, can you add a comment next to this notforloan test to explain why we did this change? It's not obvious when we read the code at first to understand why we need it in IsAvailableForItemLevelRequest but ItemsAnyAvailableAndNotRestricted.
Comment 27 Joonas Kylmälä 2021-03-05 12:51:42 UTC
(In reply to Jonathan Druart from comment #26)
> Joonas, can you add a comment next to this notforloan test to explain why we
> did this change? It's not obvious when we read the code at first to
> understand why we need it in IsAvailableForItemLevelRequest but
> ItemsAnyAvailableAndNotRestricted.

I'm not sure additional comment would help much more than the one above "[...] if at least one item available for loan/check out[...]". I don't see this test being any different from the other ones:

$i->itemlost
$i->withdrawn
$i->onloan

etc.

Somebody just probably mistakenly thought we did holdability test instead of checkoutability test, thus the bug here. I would not add additional comment because the comment above is already explaining this but instead what we should do is split the function to two and give them better names (the any available part and restricted parts).
Comment 28 Jonathan Druart 2021-03-05 13:16:47 UTC
1361 sub ItemsAnyAvailableAndNotRestricted {

1373         # we can return (end the loop) when first one found:
1374         return 1
1375             unless $i->itemlost
1376             || $i->notforloan
1377             || $i->withdrawn
1378             || $i->onloan


1296 sub IsAvailableForItemLevelRequest {
1312     return 0 if
1313         $notforloan_per_itemtype ||
1314         $item->itemlost        ||
1315         $item->notforloan > 0  ||
1316         $item->withdrawn        ||

The different between the 2 $item->notforloan tests are obvious here?



Especially with that in the history:

commit 87a8103c488961a8508f9b5b32fa165bc47301e5
Date:   Thu Jul 10 16:38:28 2008 -0400
    Further update to allow notforloan < 0 items to be placed on hold. This is a workaround for the lack of a notforhold flag.

-                               $item->{notforloan} or
+                               ( $item->{notforloan} > 0 ) or
Comment 29 Joonas Kylmälä 2021-03-05 13:22:32 UTC
(In reply to Jonathan Druart from comment #28)
> 1361 sub ItemsAnyAvailableAndNotRestricted {
> 
> 1373         # we can return (end the loop) when first one found:
> 1374         return 1
> 1375             unless $i->itemlost
> 1376             || $i->notforloan
> 1377             || $i->withdrawn
> 1378             || $i->onloan
> 
> 
> 1296 sub IsAvailableForItemLevelRequest {
> 1312     return 0 if
> 1313         $notforloan_per_itemtype ||
> 1314         $item->itemlost        ||
> 1315         $item->notforloan > 0  ||
> 1316         $item->withdrawn        ||
> 
> The different between the 2 $item->notforloan tests are obvious here?

It clear by reading the comments, in IsAvailableForItemLevelRequest it states

"Checks whether a given item record is *available for an
item-level hold* request."

For the other one, ItemsAnyAvailableAndNotRestricted, the comment states "item available for loan/check out".

To make the situation more clear the function splitting renaming is needed yes, but the comments explaining this are already there.
Comment 30 Jonathan Druart 2021-03-05 13:49:36 UTC
Created attachment 117855 [details] [review]
Bug 27058: Add test for IsAvailableForItemLevelRequest and notforloan
Comment 31 Joonas Kylmälä 2021-03-05 15:31:26 UTC
Created attachment 117864 [details] [review]
Bug 27058: Add test for IsAvailableForItemLevelRequest and notforloan

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 32 Joonas Kylmälä 2021-03-05 15:31:30 UTC
Created attachment 117865 [details] [review]
Bug 27058: (follow-up) Clarify notforloan checks with a comment

In IsAvailableForItemLevelRequest the check is for holdability and in
ItemsAnyAvailableAndNotRestricted the check is for
checkoutability. These comments should make it more clear because the
notforloan value is used for these two different purposes and is a bit
confusing (we might want to add a new field "notforhold" in future to
make the code self documenting)
Comment 33 Joonas Kylmälä 2021-03-05 15:33:04 UTC
As per discussion on IRC with Jonathan, added the comment to now make it super clear what is going on, and signed-off the tests from Jonathan.
Comment 34 Jonathan Druart 2021-03-08 14:17:35 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 35 Fridolin Somers 2021-03-12 08:06:39 UTC
Pushed to 20.11.x for 20.11.04
Comment 36 Andrew Fuerste-Henry 2021-03-14 20:04:30 UTC
*** Bug 6918 has been marked as a duplicate of this bug. ***
Comment 37 Andrew Fuerste-Henry 2021-03-14 20:06:08 UTC
I understand that this is technically a bug rather than an enhancement, but it's a change to circ rule behavior that has existed for over 9 years. I'm incredibly hesitant to backport it.