https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 145590 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145590&action=edit Bug 32702: Move item status checks to CanItemBeReserved This patch moves item status checks from IsAvailableForItemLevelRequest to CanItemBeReserved. Changes to existing calls highlighted below. Existing Calls for CanItemBeReserved/CanBookBeReserved: Call in Circulation.pm checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in ILSDI/Services.pm HoldItem - would have allowed placing holds on damaged/notforloan/etc items C4/Reserves: What is ItemsAnyAvailableAndNotRestricted used for C4/SIP/ILS/Transaction/Hold.pm: do_hold - would have allowed hold on lost/damaged/etc. Koha/Club/Hold/pm: add - would have allowed etc. Koha/REST/V1/Holds.pm: add - would have allowed ... opac/opac-reserve.pl: Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest reserve/placerequest: Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest In this case and opac, I believe before this a hold could have been forced with the right parameters Existing Calls for IsAvailableForItemLevelRequest C4/Circulation - see above Call in ILSDI/Services.pm: GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in opac/opac-reserve.pl: Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in reserve/request.pl: Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest To test: 1 - Setup a record with items to have: 1 available item 1 lost item 1 item with positive not for loan status 1 item with negative not for loan status 1 item withdrawn 1 item damaged 1 item not for loan by itemtype 2 - Attempt to place hold on staff and opac and note the statuses 3 - Apply patch 4 - Confirm the statuses have not changed 5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API 6 - You should not be able to place the hold 7 - Attempt to place club holds on these items - it should not be possible 8 - Attempt to place holds via ILSDI - it should not be possible -- 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=32702 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |caroline.cyr-la-rose@inlibr | |o.com, | |jonathan.druart+koha@gmail. | |com, | |katrin.fischer@bsz-bw.de, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 --- Comment #3 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- In general I think we should stick with the well tested behavior from opac/intranet requests and make sure that the APIs use the same logic. So if we already had the stricter behavior in GUI, I think that's usually the right choice. In your documentation, there is one inconsistency: Call in ILSDI/Services.pm HoldItem - would have allowed placing holds on damaged/notforloan/etc items Call in ILSDI/Services.pm: GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest I believe you caused an unwanted change in behaviour, negative nfl-values are in fact holdable, but won't be no longer with the change: - $item->notforloan > 0 || # item with negative or zero notforloan value is holdable vs. + return { status => 'notforloan' } if ( $item->notforloan || $notforloan_per_itemtype ); I haven't had time to look at the methods at a whole, but do we really want to remove the checks from IsAvailableForItemLevelRequest? -- 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=32702 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145590|0 |1 is obsolete| | --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149762 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149762&action=edit Bug 32702: Move item status checks to CanItemBeReserved This patch moves item status checks from IsAvailableForItemLevelRequest to CanItemBeReserved. Changes to existing calls highlighted below. Existing Calls for CanItemBeReserved/CanBookBeReserved: Call in Circulation.pm checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in ILSDI/Services.pm HoldItem - would have allowed placing holds on damaged/notforloan/etc items GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest C4/Reserves: What is ItemsAnyAvailableAndNotRestricted used for C4/SIP/ILS/Transaction/Hold.pm: do_hold - would have allowed hold on lost/damaged/etc. Koha/Club/Hold/pm: add - would have allowed etc. Koha/REST/V1/Holds.pm: add - would have allowed ... opac/opac-reserve.pl: Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest reserve/placerequest: Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest In this case and opac, I believe before this a hold could have been forced with the right parameters Existing Calls for IsAvailableForItemLevelRequest C4/Circulation - see above Call in opac/opac-reserve.pl: Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in reserve/request.pl: Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest To test: 1 - Setup a record with items to have: 1 available item 1 lost item 1 item with positive not for loan status 1 item with negative not for loan status 1 item withdrawn 1 item damaged 1 item not for loan by itemtype 2 - Attempt to place hold on staff and opac and note the statuses 3 - Apply patch 4 - Confirm the statuses have not changed 5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API 6 - You should not be able to place the hold 7 - Attempt to place club holds on these items - it should not be possible 8 - Attempt to place holds via ILSDI - it should not be possible -- 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=32702 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | CC| |lucas@bywatersolutions.com -- 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=32702 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |andrewfh@dubcolib.org --- Comment #7 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- (In reply to Nick Clemens from comment #4)

To test: 1 - Setup a record with items to have: 1 available item 1 lost item 1 item with positive not for loan status 1 item with negative not for loan status 1 item withdrawn 1 item damaged 1 item not for loan by itemtype 2 - Attempt to place hold on staff and opac and note the statuses 3 - Apply patch 4 - Confirm the statuses have not changed 5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API 6 - You should not be able to place the hold 7 - Attempt to place club holds on these items - it should not be possible 8 - Attempt to place holds via ILSDI - it should not be possible

With these patches applied, I was able to create holds via API for all of these items, despite their statuses. I did get blocked from placing holds beyond the holds-per-record limit in the circ rules, but otherwise everything was permitted. Also, after the patches, in the staff client the lost and withdrawn items are showing new wording in the first column of the item selection table when placing a hold. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 157134 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157134&action=edit Bug 32702: Unit tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Nick Clemens from comment #10)

(In reply to Andrew Fuerste-Henry from comment #7)

...

(In reply to Nick Clemens from comment #4) With these patches applied, I was able to create holds via API for all of these items, despite their statuses. I did get blocked from placing holds beyond the holds-per-record limit in the circ rules, but otherwise everything was permitted.

See bug 35053 - if you only pas the item_id to the API it should work correctly

Submitted a fix on that one. Please test! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157133|0 |1 is obsolete| | --- Comment #12 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- Created attachment 161725 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161725&action=edit Bug 32702: Move item status checks to CanItemBeReserved This patch moves item status checks from IsAvailableForItemLevelRequest to CanItemBeReserved. Changes to existing calls highlighted below. Existing Calls for CanItemBeReserved/CanBookBeReserved: Call in Circulation.pm checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in ILSDI/Services.pm HoldItem - would have allowed placing holds on damaged/notforloan/etc items GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest C4/Reserves: What is ItemsAnyAvailableAndNotRestricted used for C4/SIP/ILS/Transaction/Hold.pm: do_hold - would have allowed hold on lost/damaged/etc. Koha/Club/Hold/pm: add - would have allowed etc. Koha/REST/V1/Holds.pm: add - would have allowed ... opac/opac-reserve.pl: Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest reserve/placerequest: Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest In this case and opac, I believe before this a hold could have been forced with the right parameters Existing Calls for IsAvailableForItemLevelRequest C4/Circulation - see above Call in opac/opac-reserve.pl: Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in reserve/request.pl: Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest To test: 1 - Setup a record with items to have: 1 available item 1 lost item 1 item with positive not for loan status 1 item with negative not for loan status 1 item withdrawn 1 item damaged 1 item not for loan by itemtype 2 - Attempt to place hold on staff and opac and note the statuses 3 - Apply patch 4 - Confirm the statuses have not changed 5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API 6 - You should not be able to place the hold 7 - Attempt to place club holds on these items - it should not be possible 8 - Attempt to place holds via ILSDI - it should not be possible Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |victor@tuxayo.net --- Comment #14 from Victor Grousset/tuxayo <victor@tuxayo.net> --- In CanItemBeReserved, every current return uses the cache that is setup for this sub. In this patch, the moved returns are just like `return { status => 'notforloan' }` Pretty sure they should be like `return _cache { status => 'notforloan' }` -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #15 from Julian Maurice <julian.maurice@biblibre.com> --- There is at least 3 places where IsAvailableForItemLevelRequest is called and CanItemBeReserved is not: * opac/opac-ISBDdetail.pl * opac/opac-MARCdetail.pl * opac/opac-detail.pl (side note: it's the same code in those 3 scripts and it's used to set a template variable used only in opac-detail-sidebar.inc; should be moved into a TT plugin ?) Removing checks from IsAvailableForItemLevelRequest means that those checks won't be done in these places. Instead, do you think it's possible to call IsAvailableForItemLevelRequest from CanItemBeReserved ? (and remove calls to IsAvailableForItemLevelRequest whenever CanItemBeReserved is called) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 --- Comment #17 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Nick Clemens from comment #16)

Would you be willing to accept a smaller change in those scripts for now? $can_holds_be_placed = IsAvailableForItemLevelRequest($item, $patron, undef) && CanItemBeReserved($patron, $item, undef);

Yes, that should work. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 --- Comment #19 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 170157 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170157&action=edit Bug 32702: Unit tests Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Michaela Sieber <michaela.sieber@kit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |michaela.sieber@kit.edu -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emily.lamancusa@montgomeryc | |ountymd.gov -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #170156|0 |1 is obsolete| | Attachment #170157|0 |1 is obsolete| | Attachment #170158|0 |1 is obsolete| | --- Comment #22 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 179474 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179474&action=edit Bug 32702: Move item status checks to CanItemBeReserved This patch moves item status checks from IsAvailableForItemLevelRequest to CanItemBeReserved. Changes to existing calls highlighted below. Existing Calls for CanItemBeReserved/CanBookBeReserved: Call in Circulation.pm checkHighHolds - previously would have allowed damaged, notforloan, etc items to fill holds CanBookBeRenewed - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in ILSDI/Services.pm HoldItem - would have allowed placing holds on damaged/notforloan/etc items GetServices - Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest C4/Reserves: What is ItemsAnyAvailableAndNotRestricted used for C4/SIP/ILS/Transaction/Hold.pm: do_hold - would have allowed hold on lost/damaged/etc. Koha/Club/Hold/pm: add - would have allowed etc. Koha/REST/V1/Holds.pm: add - would have allowed ... opac/opac-reserve.pl: Was already combo of CanItemBeReserved and IsAvailableForItemLevelRequest reserve/placerequest: Items would have been filtered on reserve/request.pl by IsAvailableForItemLevelRequest In this case and opac, I believe before this a hold could have been forced with the right parameters Existing Calls for IsAvailableForItemLevelRequest C4/Circulation - see above Call in opac/opac-reserve.pl: Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest Call in reserve/request.pl: Already a combination of CanItemBeReserved and IsAvailableForItemLevelRequest To test: 1 - Setup a record with items to have: 1 available item 1 lost item 1 item with positive not for loan status 1 item with negative not for loan status 1 item withdrawn 1 item damaged 1 item not for loan by itemtype 2 - Attempt to place hold on staff and opac and note the statuses 3 - Apply patch 4 - Confirm the statuses have not changed 5 - Attempt to hold lost/damaged/withdrawn/notforloan items via API 6 - You should not be able to place the hold 7 - Attempt to place club holds on these items - it should not be possible 8 - Attempt to place holds via ILSDI - it should not be possible Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 --- Comment #24 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 179476 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179476&action=edit Bug 32702: (follow-up) Add CanItemBeReserved checks to opac scripts -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |arthur.suzuki@biblibre.com --- Comment #26 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- rebased on current master + made a follow-up to use the cache mechanism. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- This feels like an enh ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=33801 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |20985 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20985 [Bug 20985] CanBookBeReserved and CanItemBeReserved should check 'On shelf holds allowed' policy -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 --- Comment #30 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Also CanItemBeReserved have a much better exploitable return with a "status" which will help debug any cause of holdability failure/issue. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #32 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- I made a new patch to completely remove "isAvailableForItemLevelRequest" and move the checks to "CanItemBeReserved". Some of those checks were duplicated. Also I've changed the perl scripts and any other file using isAvailableForItemLevelRequest to use CanItemBeReserved instead. I hope it will make Koha and holds easier to maintain and debug. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|25408 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 [Bug 25408] CanBookBeReserved & CanItemBeReserved should check "opacitemholds" policy -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 --- Comment #34 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- (In reply to David Nind from comment #33)

Is the test plan still valid?

Yes :)

If it is, I am happy to attempt to test it.

I'd be very grateful!

Asking, as there are a lot of changes and follow-ups, and it seems to be a tricky area to test.

Indeed, holds are quite tricky. Also this patch depends on 20985 (already signed-off but need QA after rebase). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32702 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.