Bug 12630 - Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well!
Summary: Prioritizing "Hold starts on date" -holds causes all other holds to be priori...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Keywords:
Depends on: 13202
Blocks: 10883
  Show dependency treegraph
 
Reported: 2014-07-23 05:35 UTC by Olli-Antti Kivilahti
Modified: 2023-06-08 22:26 UTC (History)
8 users (show)

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


Attachments
Bug 12630 - Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well! (2.08 KB, patch)
2014-07-23 05:45 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12630 - Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well! (2.15 KB, patch)
2014-08-25 14:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12630 [QA Followup] - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority (1.70 KB, patch)
2014-08-25 14:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12630 - Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well! (2.19 KB, patch)
2014-11-05 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12630 [QA Followup] - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority (1.75 KB, patch)
2014-11-05 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12630: Add regression tests (2.16 KB, patch)
2014-11-05 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12630: Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well! (2.39 KB, patch)
2022-06-10 15:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12630: (QA follow-up) - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority (1.76 KB, patch)
2022-06-10 15:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12630: Add regression tests (2.12 KB, patch)
2022-06-10 15:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 12630: Rebase tests and cover CheckReserves (4.82 KB, patch)
2022-06-10 15:02 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2014-07-23 05:35:13 UTC
-------------------------
-- REPLICATE LIKE THIS --
-------------------------

0. Enable AllowHoldDateInFuture-system preference!

1. Select a biblio with some holds.
2. Place a hold with the "Hold starts on date"-attribute set to future.
3. More the specific hold up on the priority queue.
4. Add another normal hold, observe how it is prioritized with the "Hold starts on date"-hold, leaving old holds to the prioritization queue tail.

Unfair eh?
Comment 1 Olli-Antti Kivilahti 2014-07-23 05:45:13 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2014-07-23 05:47:24 UTC
_ShiftPriorityByDateAndPriority() is not used from anywhere else than AddReserve().
This feture should have no need to even check for reservation date, because it shouldn't alter the priority.
Comment 3 Kyle M Hall 2014-07-28 19:43:49 UTC
Is this the same as bug 10883?
Comment 4 Chris Cormack 2014-07-29 06:27:53 UTC
Comment on attachment 29962 [details] [review]
Bug 12630 - Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well!

Review of attachment 29962 [details] [review]:
-----------------------------------------------------------------

::: C4/Reserves.pm
@@ +1982,3 @@
>  =cut
>  
>  sub _ShiftPriorityByDateAndPriority {

Doesn't make a lot of sense to leave this function name as it is, since it's not doing anything with the date any more.

Also the POD is now wrong.

This also means that a hold with a newer date, will not be placed before older date. Which is changing the functionality.
Comment 5 Olli-Antti Kivilahti 2014-07-29 08:20:34 UTC
(In reply to Chris Cormack from comment #4)
> This also means that a hold with a newer date, will not be placed before
> older date. Which is changing the functionality.

I understood that this feature is only to activate a hold after a certain period of time, not to make it automatically target an item (be top priority) when the date arrives.
Comment 6 Olli-Antti Kivilahti 2014-07-29 08:22:00 UTC
*** Bug 10883 has been marked as a duplicate of this bug. ***
Comment 7 Olli-Antti Kivilahti 2014-07-29 08:28:37 UTC
This bug doesn't exactly depend on Bug 10883 but is linked to it by similarity.

Maybe a joint patch?
Comment 8 Kyle M Hall 2014-07-29 14:08:31 UTC
Looks good, but the sub _ShiftPriorityByDateAndPriority should be renamed if you are removing the date part.
Comment 9 Kyle M Hall 2014-07-29 14:12:21 UTC
Also:

FAIL   C4/Reserves.pm
   OK     pod
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 166)
   OK     valid
   OK     critic
Comment 10 Kyle M Hall 2014-08-25 14:21:03 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2014-08-25 14:21:21 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-10-11 19:58:09 UTC
Olli, could provide tests to highlight this change please?
Comment 13 Olli-Antti Kivilahti 2014-10-13 08:58:26 UTC
Sorry I can't atm.
Comment 14 Kyle M Hall 2014-10-22 12:42:50 UTC
(In reply to Jonathan Druart from comment #12)
> Olli, could provide tests to highlight this change please?

This change is not unit testable, as the fix was to remove the code that would be tested! We could write a unit test to highlight the bug pre-patch, but that would be no more efficient than following the test plan to reproduce the issue.

Changing the status back to "Signed Off". If you disagree with me, or my logic is flawed, please update the status accordingly.
Comment 15 Jonathan Druart 2014-11-05 09:33:52 UTC
Created attachment 33238 [details] [review]
Bug 12630 - Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well!

-------------------------
-- REPLICATE LIKE THIS --
-------------------------

0. Enable AllowHoldDateInFuture-system preference!

1. Select a biblio with some holds.
2. Place a hold with the "Hold starts on date"-attribute set to future.
3. More the specific hold up on the priority queue.
4. Add another normal hold, observe how it is prioritized with the "Hold starts on date"-hold, leaving old holds to the prioritization queue tail.

Unfair eh?

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 16 Jonathan Druart 2014-11-05 09:33:55 UTC
Created attachment 33239 [details] [review]
Bug 12630 [QA Followup] - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 17 Jonathan Druart 2014-11-05 09:33:58 UTC
Created attachment 33240 [details] [review]
Bug 12630: Add regression tests

Verify that the 2 tests failed before applying this patch and return
green after.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 18 Jonathan Druart 2014-11-05 09:34:29 UTC
The patch look good, I added 2 regression tests.
Marked as Passed QA.
Comment 19 Jonathan Druart 2014-11-05 09:48:38 UTC
Actually I am not sure, the POD says
"This is most useful when the reservedate can be set by the user.  It allows the new reserve to be placed before other reserves that have a later reservedate".

So it is the expected behavior. We want to change it?
Comment 20 Kyle M Hall 2014-11-05 12:19:53 UTC
khall: hmm, that's an interesting point Joubu. imo this is at best an undocumented feature ( not documented in the manual afaik ) at best, but I and I'm sure many would consider it a bug In actuality
khall: If this patch doesn't get pushed, then we *must* at the very least syspref the behavior.
Joubu: my feeling is that the date should be take into account, otherwise the further holds could be blocked by the one "in the future"
khall: that is true. There was a bug where a patron could cheat his way to the front of the line by setting a starts on date way in the past, but I closed up that bug by disabling the calendar selector for past dates
khall: however, one could still cheat the system if they know what they are doing
khall: this bug closes that loophole permanently
khall: I suppose we could also have the script round up any hold starts on date in the past to the current date as well
Comment 21 Kyle M Hall 2014-11-05 12:29:33 UTC
khall: as far as I can tell C4::Reserves::CheckReserves does *not* take the reservedate into account. I think with a followup that does, this patch would be acceptable. Do you agree?
khall: That way one future hold doesn't hold back other reserves, and it still prevents cheating the system
Joubu: sounds good
khall: I'll file a new bug report and make it a blocker for this bug
Comment 22 Nick Clemens 2022-06-10 15:01:51 UTC
Created attachment 135939 [details] [review]
Bug 12630: Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well!

-------------------------
-- REPLICATE LIKE THIS --
-------------------------

0. Enable AllowHoldDateInFuture-system preference!

1. Select a biblio with some holds.
2. Place a hold with the "Hold starts on date"-attribute set to future.
3. More the specific hold up on the priority queue.
4. Add another normal hold, observe how it is prioritized with the "Hold starts on date"-hold, leaving old holds to the prioritization queue tail.

Unfair eh?

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 23 Nick Clemens 2022-06-10 15:01:55 UTC
Created attachment 135940 [details] [review]
Bug 12630: (QA follow-up) - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 24 Nick Clemens 2022-06-10 15:01:59 UTC
Created attachment 135941 [details] [review]
Bug 12630: Add regression tests

Verify that the 2 tests failed before applying this patch and return
green after.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 25 Nick Clemens 2022-06-10 15:02:03 UTC
Created attachment 135942 [details] [review]
Bug 12630: Rebase tests and cover CheckReserves

It turns out we do honor reservedate in CheckReserves, so a hold with a lower priority will
fill before a hold in the future. I add tests to cover this and fix the old tests to pass again
Comment 26 Tomás Cohen Arazi 2022-06-13 13:38:22 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 27 Lucas Gass 2022-07-12 19:43:04 UTC
Missing dependencies for 22.05.x, no backport
Comment 28 Lucas Gass 2022-07-15 17:40:11 UTC
Backing porting upon request to 22.05.x for 22.05.03
Comment 29 Arthur Suzuki 2022-07-21 14:18:45 UTC
backporting to 21.11.x for 21.11.11.
thx!
Comment 30 Victor Grousset/tuxayo 2022-07-23 20:09:20 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.