Bug 11512 - Only allow to override maximum number of holds from staff as other overrides would never be filled
Summary: Only allow to override maximum number of holds from staff as other overrides ...
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 8859 14021 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-01-09 20:15 UTC by Kyle M Hall
Modified: 2018-05-25 11:04 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 11512 - Forced holds that violate issuing rules will never be filled (1.46 KB, patch)
2017-10-26 11:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11512 - Forced holds that violate issuing rules will never be filled (1.46 KB, patch)
2017-10-26 11:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11512 - Forced holds that violate issuing rules will never be filled (1.52 KB, patch)
2018-03-12 14:16 UTC, Jesse Maseto
Details | Diff | Splinter Review
Bug 11512: Update syspref description (1.09 KB, patch)
2018-03-23 11:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11512: Update syspref description (1.09 KB, patch)
2018-03-23 11:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11512 - Forced holds that violate issuing rules will never be filled (1.61 KB, patch)
2018-04-06 06:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11512: Update syspref description (1.17 KB, patch)
2018-04-06 06:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-01-09 20:15:10 UTC
Currently in Koha, if you choose to force a hold from the staff side that would contravened the current issuing rules, that hold will never be filled, as it is always skipped over by CheckReserves.

I think the only way to resolve this is to add a flag to the reserves table to alert the system that despite the fact that this hold should usually be skipped over, in this case, it should not.
Comment 1 Katrin Fischer 2015-06-07 00:28:18 UTC
*** Bug 8859 has been marked as a duplicate of this bug. ***
Comment 2 Katrin Fischer 2015-06-09 01:26:02 UTC
*** Bug 14021 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2015-07-22 10:07:54 UTC
I have another report from a library about this - it's really annoying that we offer to override holds, but then they just never get filled... hard to explain.
Comment 4 Kyle M Hall 2017-10-26 11:42:08 UTC
Created attachment 68594 [details] [review]
Bug 11512 - Forced holds that violate issuing rules will never be filled

Currently in Koha, if you choose to force a hold from the staff side that would contravened the current issuing rules, that hold will never be filled, as it is always skipped over by CheckReserves.

This patch disallows overrideing except for tooManyReserves which are the only overridden holds that will be trapped.

Test Plan:
1) Apply this patch
2) Attempt to override hold placement, only placements where the patron has too many holds already should be allowed
Comment 5 Kyle M Hall 2017-10-26 11:43:00 UTC
Created attachment 68595 [details] [review]
Bug 11512 - Forced holds that violate issuing rules will never be filled

Currently in Koha, if you choose to force a hold from the staff side that would contravened the current issuing rules, that hold will never be filled, as it is always skipped over by CheckReserves.

This patch disallows overrideing except for tooManyReserves which are the only overridden holds that will be trapped.

Test Plan:
1) Apply this patch
2) Attempt to override hold placement, only placements where the patron has too many holds already should be allowed
Comment 6 Jesse Maseto 2018-03-12 14:16:26 UTC
Created attachment 72689 [details] [review]
Bug 11512 - Forced holds that violate issuing rules will never be filled

Currently in Koha, if you choose to force a hold from the staff side that would contravened the current issuing rules, that hold will never be filled, as it is always skipped over by CheckReserves.

This patch disallows overrideing except for tooManyReserves which are the only overridden holds that will be trapped.

Test Plan:
1) Apply this patch
2) Attempt to override hold placement, only placements where the patron has too many holds already should be allowed

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 7 Marcel de Rooy 2018-03-23 09:02:16 UTC
QA: Looking here
Comment 8 Marcel de Rooy 2018-03-23 09:12:06 UTC
It seems that we are restricting AllowHoldPolicyOverride here at the only code location where it is really relevant.
So this pref really does not do what it implicitly advertises.
I would suggest to rename the pref or at least add a disclaimer at the preference form and help page etc. before passing QA on it.
Comment 9 Kyle M Hall 2018-03-23 11:29:02 UTC
Created attachment 73172 [details] [review]
Bug 11512: Update syspref description
Comment 10 Kyle M Hall 2018-03-23 11:32:27 UTC
Created attachment 73173 [details] [review]
Bug 11512: Update syspref description
Comment 11 Marcel de Rooy 2018-04-06 06:27:33 UTC
Created attachment 73734 [details] [review]
Bug 11512 - Forced holds that violate issuing rules will never be filled

Currently in Koha, if you choose to force a hold from the staff side that would contravened the current issuing rules, that hold will never be filled, as it is always skipped over by CheckReserves.

This patch disallows overrideing except for tooManyReserves which are the only overridden holds that will be trapped.

Test Plan:
1) Apply this patch
2) Attempt to override hold placement, only placements where the patron has too many holds already should be allowed

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2018-04-06 06:27:39 UTC
Created attachment 73735 [details] [review]
Bug 11512: Update syspref description

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Jonathan Druart 2018-04-06 17:53:16 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 14 Stefan Berndtsson 2018-05-25 11:04:06 UTC
(In reply to Marcel de Rooy from comment #11)
> Currently in Koha, if you choose to force a hold from the staff side that
> would contravened the current issuing rules, that hold will never be filled,
> as it is always skipped over by CheckReserves.
> 
> This patch disallows overrideing except for tooManyReserves which are the
> only overridden holds that will be trapped.

From what I can see in the code, this isn't actually true. CheckReserves does not include neither LOST nor WITHDRAWN in its checks. In fact, we use the LOST part here.

When longoverdue.pl marks a book as LOST, our staff adds a reserve to that item, so that they will be notified if it is returned. With the code prior to this patch,
this works perfectly fine since we have the AllowHoldPolicyOverride set and returning a LOST item fills the hold.

We've also used a similiar thing with withdrawn, but that's not as critical, so
I won't argue against that one.

The assumption that this is always skipped is not valid, unless I've missed something.

We can patch things temporarily, but some sort of option for better override selectability would probably be useful here.