[Bug 12630] New: Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well!
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Bug ID: 12630 Summary: Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well! Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: olli-antti.kivilahti@jns.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com ------------------------- -- 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? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #1 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 29962 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29962&action=edit 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? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff --- Comment #2 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- _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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Is this the same as bug 10883? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #4 from Chris Cormack <chris@bigballofwax.co.nz> --- Comment on attachment 29962 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29962 Bug 12630 - Prioritizing "Hold starts on date" -holds causes all other holds to be prioritized as well! Review of attachment 29962: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=12630&attachment=29962) ----------------------------------------------------------------- ::: 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #5 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #6 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- *** Bug 10883 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |8918 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|8918 | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10883 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #7 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- This bug doesn't exactly depend on Bug 10883 but is linked to it by similarity. Maybe a joint patch? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Looks good, but the sub _ShiftPriorityByDateAndPriority should be renamed if you are removing the date part. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Also: FAIL C4/Reserves.pm OK pod FAIL forbidden patterns forbidden pattern: tab char (line 166) OK valid OK critic -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29962|0 |1 is obsolete| | --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 31140 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31140&action=edit [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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 31141 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31141&action=edit Bug 12630 [QA Followup] - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@biblibre.co | |m --- Comment #12 from Jonathan Druart <jonathan.druart@biblibre.com> --- Olli, could provide tests to highlight this change please? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #13 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Sorry I can't atm. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #15 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33238 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33238&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31141|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33239 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33239&action=edit Bug 12630 [QA Followup] - Rename _ShiftPriorityByDateAndPriority to _ShiftPriority Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #17 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33240 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33240&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #18 from Jonathan Druart <jonathan.druart@biblibre.com> --- The patch look good, I added 2 regression tests. Marked as Passed QA. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Assignee|koha-bugs@lists.koha-commun |olli-antti.kivilahti@jns.fi |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #31140|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion --- Comment #19 from Jonathan Druart <jonathan.druart@biblibre.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #20 from Kyle M Hall <kyle@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |13202 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |BLOCKED -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12630 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org