Bug 24160

Summary: Short loan collection holds
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fiona.borthwick, gmcharlt, janet.mcgowan, joonas.kylmala, kyle.m.hall, mark.gavillet, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24683
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 24160: Fix OPACHoldsIfAvailableAtPickup to follow holds policy
Bug 24160: Fix OPACHoldsIfAvailableAtPickup to follow holds policy

Description Jonathan Druart 2019-12-03 19:40:12 UTC
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.
Comment 1 Jonathan Druart 2019-12-13 16:21:28 UTC
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
Comment 2 PTFS Europe Sandboxes 2020-02-26 11:30:43 UTC
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>
Comment 3 Joonas Kylmälä 2020-07-15 15:18:10 UTC
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.
Comment 4 Katrin Fischer 2020-07-18 16:11:49 UTC
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?
Comment 5 Jonathan Druart 2020-07-20 07:00:48 UTC
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.
Comment 6 Joonas Kylmälä 2020-07-20 07:45:58 UTC
(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.
Comment 7 Joonas Kylmälä 2020-07-20 08:04:38 UTC
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...
Comment 8 Nick Clemens 2020-07-31 11:37:48 UTC
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