[Bug 27058] New: Cannot place hold to ordered item when on shelf holds are not allowed
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Bug ID: 27058 Summary: Cannot place hold to ordered item when on shelf holds are not allowed Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: joonas.kylmala@helsinki.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #1 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113820 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113820&action=edit 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113820|0 |1 is obsolete| | --- Comment #2 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113821 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113821&action=edit Bug 27058: Add test to show ordered items cannot be reserved -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #3 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113822 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113822&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113821|0 |1 is obsolete| | Attachment #113822|0 |1 is obsolete| | --- Comment #4 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113824 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113824&action=edit Bug 27058: Add test to show ordered items cannot be checked out -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #5 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113825 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113825&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=6918 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Assignee|koha-bugs@lists.koha-commun |joonas.kylmala@helsinki.fi |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- The unit test isn't failing wit the second patch applied. Should it? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113824|0 |1 is obsolete| | Attachment #113825|0 |1 is obsolete| | --- Comment #8 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 114203 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114203&action=edit Bug 27058: Add test to show ordered items cannot be checked out -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #9 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 114204 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114204&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #10 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114203|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 114210 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114210&action=edit Bug 27058: Add test to show ordered items cannot be checked out Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114204|0 |1 is obsolete| | --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 114211 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114211&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- my $biblionumber1 = $biblio2->biblionumber; Obscure way of coding -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #14 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #15 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117021 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117021&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #16 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Quite distracted by the skipholdstrap stuff close to your changes. See 27729. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> ---
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | Status|BLOCKED |Signed Off --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114210|0 |1 is obsolete| | --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 117780 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117780&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114211|0 |1 is obsolete| | --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 117781 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117781&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117021|0 |1 is obsolete| | --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 117782 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117782&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Cant block this one for the stuff mentioned in comment19. Refactor_holds++ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #25 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #26 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #27 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nugged@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #29 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 117855 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117855&action=edit Bug 27058: Add test for IsAvailableForItemLevelRequest and notforloan -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117855|0 |1 is obsolete| | --- Comment #31 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117864 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117864&action=edit Bug 27058: Add test for IsAvailableForItemLevelRequest and notforloan Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #32 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 117865 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117865&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #33 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable Version(s)|21.05.00 |21.05.00,20.11.04 released in| | --- Comment #35 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nengard@gmail.com --- Comment #36 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- *** Bug 6918 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com --- Comment #37 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org