------------------------- -- 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?
Created attachment 29962 [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?
_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.
Is this the same as bug 10883?
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.
(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.
*** Bug 10883 has been marked as a duplicate of this bug. ***
This bug doesn't exactly depend on Bug 10883 but is linked to it by similarity. Maybe a joint patch?
Looks good, but the sub _ShiftPriorityByDateAndPriority should be renamed if you are removing the date part.
Also: FAIL C4/Reserves.pm OK pod FAIL forbidden patterns forbidden pattern: tab char (line 166) OK valid OK critic
Created attachment 31140 [details] [review] [SIGNED-OFF] 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>
Created attachment 31141 [details] [review] Bug 12630 [QA Followup] - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority
Olli, could provide tests to highlight this change please?
Sorry I can't atm.
(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.
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>
Created attachment 33239 [details] [review] Bug 12630 [QA Followup] - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
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>
The patch look good, I added 2 regression tests. Marked as Passed QA.
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?
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
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
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>
Created attachment 135940 [details] [review] Bug 12630: (QA follow-up) - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
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>
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
Pushed to master for 22.11. Nice work everyone, thanks!
Missing dependencies for 22.05.x, no backport
Backing porting upon request to 22.05.x for 22.05.03
backporting to 21.11.x for 21.11.11. thx!
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. Nothing to document, marking resolved.