|
Description
Nick Clemens (kidclamp)
2023-01-23 16:48:45 UTC
Created attachment 145590 [details] [review] 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 This needs unit tests, and maybe a more thorough plan - posting for opinions 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?
Created attachment 149762 [details] [review] 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 Created attachment 149763 [details] [review] Bug 32702: Unit tests (In reply to Katrin Fischer from comment #3) > I haven't had time to look at the methods at a whole, but do we really want > to remove the checks from IsAvailableForItemLevelRequest? Holdability is determined by calling both routines, and we call 'CanItemBeReserved' first - moving these checks to the start of the routine saves time calculating values that will be ignored later (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. Created attachment 157133 [details] [review] 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 Created attachment 157134 [details] [review] Bug 32702: Unit tests (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 > > 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. Added values to the template to handle new values (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! Created attachment 161725 [details] [review] 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> Created attachment 161726 [details] [review] Bug 32702: Unit tests Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> 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' }`
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) (In reply to Julian Maurice from comment #15) > 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 ?) Indeed, I just added those checks on bug 34886 > 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) See comment 1 - I think that's the ultimate goal 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); (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. Created attachment 170156 [details] [review] 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> Created attachment 170157 [details] [review] Bug 32702: Unit tests Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> Created attachment 170158 [details] [review] Bug 32702: (follow-up) Add CanItemBeReserved checks to opac scripts comment 14 is still valid, the patch is missing the use of the cache mechanism on return. |