Description
Joonas Kylmälä
2020-11-19 14:55:06 UTC
Created attachment 113820 [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. Created attachment 113821 [details] [review] Bug 27058: Add test to show ordered items cannot be reserved Created attachment 113822 [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 Created attachment 113824 [details] [review] Bug 27058: Add test to show ordered items cannot be checked out 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 The unit test isn't failing wit the second patch applied. Should it? (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. Created attachment 114203 [details] [review] Bug 27058: Add test to show ordered items cannot be checked out 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 (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. 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> 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> my $biblionumber1 = $biblio2->biblionumber; Obscure way of coding (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. 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 (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. (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 Quite distracted by the skipholdstrap stuff close to your changes. See 27729. 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. 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. 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> 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> 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> Cant block this one for the stuff mentioned in comment19. Refactor_holds++ (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. 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. (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). 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 (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. Created attachment 117855 [details] [review] Bug 27058: Add test for IsAvailableForItemLevelRequest and notforloan Created attachment 117864 [details] [review] Bug 27058: Add test for IsAvailableForItemLevelRequest and notforloan Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> 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) 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. Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.04 *** Bug 6918 has been marked as a duplicate of this bug. *** 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. |