Bug 21495 - Regression in hold override functionality
Summary: Regression in hold override functionality
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Tomás Cohen Arazi
URL:
Keywords: regression
Depends on: 11512
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-05 09:03 UTC by Stefan Berndtsson
Modified: 2020-01-06 20:15 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21495 - Regression in hold override functionality (2.04 KB, patch)
2018-11-15 13:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21495: Regression in hold override functionality (2.04 KB, patch)
2018-11-15 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21495: Regression in hold override functionality (2.04 KB, patch)
2018-11-16 17:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21495: Regression in hold override functionality (2.15 KB, patch)
2019-01-22 14:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21495: Regression in hold override functionality (2.19 KB, patch)
2019-01-28 14:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Berndtsson 2018-10-05 09:03:51 UTC
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.
Comment 1 Kyle M Hall 2018-11-15 13:39:32 UTC
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.
Comment 2 Kyle M Hall 2018-11-15 13:45:58 UTC
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.
Comment 3 Blou 2018-11-15 15:07:33 UTC
Kyle, did you mean "the patches for bug 11512 have.." ?
Comment 4 Kyle M Hall 2018-11-16 17:29:01 UTC
(In reply to Blou from comment #3)
> Kyle, did you mean "the patches for bug 11512 have.." ?

Yes, yes I did ;)
Comment 5 Kyle M Hall 2018-11-16 17:30:09 UTC
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.
Comment 6 Kyle M Hall 2019-01-22 14:09:56 UTC
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>
Comment 7 Tomás Cohen Arazi 2019-01-28 14:12:37 UTC
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>
Comment 8 Tomás Cohen Arazi 2019-01-28 14:14:25 UTC
@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'?
Comment 9 Katrin Fischer 2019-01-28 18:47:25 UTC
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.
Comment 10 Nick Clemens 2019-01-28 21:06:19 UTC
Awesome work all!

Pushed to master for 19.05
Comment 11 Kyle M Hall 2019-01-29 11:47:38 UTC
(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.
Comment 12 Martin Renvoize 2019-02-01 15:29:41 UTC
Pushed to 18.11.x for 18.11.03
Comment 13 Lucas Gass 2019-02-07 01:31:26 UTC
pushed to 18.05.x for 18.05.09
Comment 14 Fridolin Somers 2019-02-15 06:05:20 UTC
Depends on Bug 11512 not in 17.11.x