The changes made by Bug 11512 causes regression in functionality. That change assumes it is safe to override only tooManyReserves because other holds will not be filled anyway. [The rest is copied from Comment 14 in the bug, explaining our usecase] 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 in Gothenburg. 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. The same logic applies to WITHDRAWN.
Created attachment 82359 [details] [review] Bug 21495 - Regression in hold override functionality The changes caused by the patches for bug 21495 have broken existing workflows for many libraries and are widely considered to be a bad move. We should revert this behavior.
Created attachment 82361 [details] [review] Bug 21495: Regression in hold override functionality The changes caused by the patches for bug 21495 have broken existing workflows for many libraries and are widely considered to be a bad move. We should revert this behavior.
Kyle, did you mean "the patches for bug 11512 have.." ?
(In reply to Blou from comment #3) > Kyle, did you mean "the patches for bug 11512 have.." ? Yes, yes I did ;)
Created attachment 82416 [details] [review] Bug 21495: Regression in hold override functionality The changes caused by the patches for bug 11512 have broken existing workflows for many libraries and are widely considered to be a bad move. We should revert this behavior.
Created attachment 84282 [details] [review] Bug 21495: Regression in hold override functionality The changes caused by the patches for bug 11512 have broken existing workflows for many libraries and are widely considered to be a bad move. We should revert this behavior. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <kuiper@roundrocktexas.gov>
Created attachment 84485 [details] [review] Bug 21495: Regression in hold override functionality The changes caused by the patches for bug 11512 have broken existing workflows for many libraries and are widely considered to be a bad move. We should revert this behavior. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <kuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
@RM: I understand the change done by this patch and why libraries are requesting the new behaviour to be reverted. My only doubt is what the upgrade path should be. Is anyone relying on the current behaviour 'features'?
I think advertising it in the release notes should be ok - the change was not that far ago. Maybe add some "text to go in the release notes". I'd like to have the revert followed-up by some patch that makes the changes keeping the additional use cases in mind. A hold that can't be trapped should not be placed with override as this is still confusing.
Awesome work all! Pushed to master for 19.05
(In reply to Katrin Fischer from comment #9) > I think advertising it in the release notes should be ok - the change was > not that far ago. Maybe add some "text to go in the release notes". > > I'd like to have the revert followed-up by some patch that makes the changes > keeping the additional use cases in mind. A hold that can't be trapped > should not be placed with override as this is still confusing. If we capture why the hold needs an override, we could tell the librarian if the force is likely to result in an unfillable hold or not.
Pushed to 18.11.x for 18.11.03
pushed to 18.05.x for 18.05.09
Depends on Bug 11512 not in 17.11.x