Scenario: * The library has 3 items - two with item type "general shelves", one with type "desk reserve". * Circ rules are set to not allow holds on "desk reserve" items in default holds policy by item type. * OPACHoldsIfAvailableAtPickup is set to "Don't allow" to prevent patrons reserving an item which is available at the specified pickup library. A patron wants to place a hold on one of the “general shelves” items which are both on loan. If this is done by a member of staff in the staff client there is no problem. If the patron tries to place a hold in the opac it is denied because the "desk reserve" item status is available, even though circ rules specify "desk reserve" item types are not holdable.
Created attachment 96265 [details] [review] Bug 24160: Fix OPACHoldsIfAvailableAtPickup to follow holds policy Bug 17453 added the ability to restrict the pick up location for items, if at least one item was available at this location. It assumed that an available item was an item that was not checked out, not lost and not damaged. But actually we should follow holds policy, defined in the circulation rules. An item that is not checked out but on which no hold can be placed on, it should be considered as not available. Test plan: 0/ Setting up the problematic situation Considering a bibliographic record with 3 items I1, I2, and I3: I1, I2 and I3 have the same location (say Centerville) I1 and I2 have the item type 'Book' and are checked out I3 is a "Computer Files" "Computer Files" has a "No holds allowed" hold policy (bottom of the circulation rules view) Turn on off (Don't allow) OPACHoldsIfAvailableAtPickup 1/ At the OPAC place a hold on this bibliographic record (biblio or item-level hold), open the "Pick up location" dropdown list => Without this patch you cannot select "Centerville" => With this patch applied you can select it 2/ Confirm the hold => The hold as been correctly placed on the item and the pickup location is correct. 3/ Cancel the hold 4/ Check I1 in 5/ Repeat 1/ => Without and with this patch you cannot select "Centerville" 6/ Open the HTML inspector in the browser, select the dropdown list and remove the 'disabled="disabled"' attribute of the CPL option 7/ Select (force) Centerville 8/ Confirm the hold => Nothing happened! You tried to cheat and place a hold to pickup at an invalid location. Note for QA: I have tried to move that code to a module but it's not trivial at all. The availability needs to be indexed in the search engine to make all of this much more easy. Sponsored-by: Educampus
Created attachment 99640 [details] [review] Bug 24160: Fix OPACHoldsIfAvailableAtPickup to follow holds policy Bug 17453 added the ability to restrict the pick up location for items, if at least one item was available at this location. It assumed that an available item was an item that was not checked out, not lost and not damaged. But actually we should follow holds policy, defined in the circulation rules. An item that is not checked out but on which no hold can be placed on, it should be considered as not available. Test plan: 0/ Setting up the problematic situation Considering a bibliographic record with 3 items I1, I2, and I3: I1, I2 and I3 have the same location (say Centerville) I1 and I2 have the item type 'Book' and are checked out I3 is a "Computer Files" "Computer Files" has a "No holds allowed" hold policy (bottom of the circulation rules view) Turn on off (Don't allow) OPACHoldsIfAvailableAtPickup 1/ At the OPAC place a hold on this bibliographic record (biblio or item-level hold), open the "Pick up location" dropdown list => Without this patch you cannot select "Centerville" => With this patch applied you can select it 2/ Confirm the hold => The hold as been correctly placed on the item and the pickup location is correct. 3/ Cancel the hold 4/ Check I1 in 5/ Repeat 1/ => Without and with this patch you cannot select "Centerville" 6/ Open the HTML inspector in the browser, select the dropdown list and remove the 'disabled="disabled"' attribute of the CPL option 7/ Select (force) Centerville 8/ Confirm the hold => Nothing happened! You tried to cheat and place a hold to pickup at an invalid location. Note for QA: I have tried to move that code to a module but it's not trivial at all. The availability needs to be indexed in the search engine to make all of this much more easy. Sponsored-by: Educampus Signed-off-by: Simon Perry <simon.perry@itcarlow.ie>
The underlying issue as far as I can tell is that opac-reserve.pl is doing things in its own way and not using Koha/C4 modules to do the checking/processing. I would prefer instead a refactoring commit to use subroutines from C4 or Koha and then a small patch on top of that that fixes this issue. It's a bit more work but would be really helpful since it can be then tested and since the proposed patch here introduces more logic it might also have bugs, and the question arises whether the holdability is the same now in intranet and opac. At the moment I'm not confident/willing enough to QA this patch but if someone else wants to do it I don't prevent.
Hm, I think Joonas makes a good point here - also if we fixed it on module level, this could also work for ILS-DI/REST API. On the other side there appears already to be a difference between staff and OPAC behavior here? I also think this qualifies as a bug fix. Agreed?
As I wrote in the commit message, moving this code to a module is not trivial and is, imo, out of the scope of this bug report. This is a bugfix for a code that existed already in the controller. I am the one to blame for the original code (bug 17453), but at that time it was already tricky to have the code in a module, the whole script needs to be rewritten/redesigned.
(In reply to Jonathan Druart from comment #5) > As I wrote in the commit message, moving this code to a module is not > trivial and is, imo, out of the scope of this bug report. > This is a bugfix for a code that existed already in the controller. I am the > one to blame for the original code (bug 17453), but at that time it was > already tricky to have the code in a module, the whole script needs to be > rewritten/redesigned. Are you sure? Please see in C4::Reserves IsAvailableForItemLevelRequest(). You can pass pickup branch there too and it follows the circ rules. So now that I studied the problem description here more, I think removing bunch of code from the opac-reserve.pl and adding in the already existing call to IsAvailableForItemLevelRequest() just the the pickup branch parameter fixes this. This way even the syspref OPACHoldsIfAvailableAtPickup can be dropped since it has no effect. There might be a gotcha here but I have not come across any yet so my stance on not approving this patch as-it-is still stands.
I think ItemsAnyAvailableForHold should be extended with pickup library too (easy), but then that's it. If we refactor this now before adding more logic to opac-reserve.pl I think it will be easier than refactor it later, because otherwise we might add some new logic that is not following the circ rules and then libraries will start depending on that...
I am not sure this behaviour is correct. In the scenario you describe it makes sense, however, consider: 1 - Library has 'New books' 2 - They are not holdable, but they do circulate 3 - Patron should not be able to place the hold for pickup at this library because they can come pickup the book Current behaviour works like this and makes sense - the staff can force the hold, but not the patron