[Bug 28510] New: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Bug ID: 28510 Summary: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com Right now we skip closed branch's items as we iterate over all items looking for ones to fill a hold. If HoldsQueueSkipClosed is enabled, no items held be a closed library can be targeted, so it would be more efficient if we never selected the items from those branches to begin with. This is how the holds queue works if we are not using the transport cost matrix, so we should make it work the same way if we *are* using the matrix. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 121578 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121578&action=edit Bug 28510: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled Right now we skip closed branch's items as we iterate over all items looking for ones to fill a hold. If HoldsQueueSkipClosed is enabled, no items held be a closed library can be targeted, so it would be more efficient if we never selected the items from those branches to begin with. This is how the holds queue works if we are not using the transport cost matrix, so we should make it work the same way if we *are* using the matrix. Test Plan: 1) Apply this patch 2) prove prove t/db_dependent/HoldsQueue.t 3) All tests should continue to pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 121579 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121579&action=edit Bug 28510: Remove marking of closed branches as 'disable_transfer' We no longer need to act as if closed branches were marked as disable_transfer. This allows us to clean up a nice bit of code. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 121588 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121588&action=edit Bug 28510: Remove unnecessary conditional It makes sense to use items by branch to get the list of branches, as it already tells use which branches have available items. We could use branches to pull from instead, but all we would accomplish is added extra ununsed loop iterations. We already know that any additional branches in the branches to pull loop have no items to fill holds. If they did, they would be in the items_by_branch hash. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121578|0 |1 is obsolete| | --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 122193 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122193&action=edit Bug 28510: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled Right now we skip closed branch's items as we iterate over all items looking for ones to fill a hold. If HoldsQueueSkipClosed is enabled, no items held be a closed library can be targeted, so it would be more efficient if we never selected the items from those branches to begin with. This is how the holds queue works if we are not using the transport cost matrix, so we should make it work the same way if we *are* using the matrix. Test Plan: 1) Apply this patch 2) prove prove t/db_dependent/HoldsQueue.t 3) All tests should continue to pass Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121579|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 122194 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122194&action=edit Bug 28510: Remove marking of closed branches as 'disable_transfer' We no longer need to act as if closed branches were marked as disable_transfer. This allows us to clean up a nice bit of code. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121588|0 |1 is obsolete| | --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 122195 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122195&action=edit Bug 28510: Remove unnecessary conditional It makes sense to use items by branch to get the list of branches, as it already tells use which branches have available items. We could use branches to pull from instead, but all we would accomplish is added extra ununsed loop iterations. We already know that any additional branches in the branches to pull loop have no items to fill holds. If they did, they would be in the items_by_branch hash. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Status|Signed Off |Failed QA --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- FAIL C4/HoldsQueue.pm FAIL critic # Variables::ProhibitUnusedVariables: Got 1 violation(s). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 124842 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124842&action=edit Bug 28510: Remove unused variable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122193|0 |1 is obsolete| | --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 125280 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125280&action=edit Bug 28510: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled Right now we skip closed branch's items as we iterate over all items looking for ones to fill a hold. If HoldsQueueSkipClosed is enabled, no items held be a closed library can be targeted, so it would be more efficient if we never selected the items from those branches to begin with. This is how the holds queue works if we are not using the transport cost matrix, so we should make it work the same way if we *are* using the matrix. Test Plan: 1) Apply this patch 2) prove prove t/db_dependent/HoldsQueue.t 3) All tests should continue to pass Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122194|0 |1 is obsolete| | --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 125281 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125281&action=edit Bug 28510: Remove marking of closed branches as 'disable_transfer' We no longer need to act as if closed branches were marked as disable_transfer. This allows us to clean up a nice bit of code. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122195|0 |1 is obsolete| | --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 125282 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125282&action=edit Bug 28510: Remove unnecessary conditional It makes sense to use items by branch to get the list of branches, as it already tells use which branches have available items. We could use branches to pull from instead, but all we would accomplish is added extra ununsed loop iterations. We already know that any additional branches in the branches to pull loop have no items to fill holds. If they did, they would be in the items_by_branch hash. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #124842|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 125283 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=125283&action=edit Bug 28510: Remove unused variable Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Signed Off |Passed QA --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work, QA scripts happy, cleaner code and tests still pass. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |21.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed CC| |jonathan.druart+koha@gmail. | |com --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Some tests are failing randomly, please fix ASAP. See https://jenkins.koha-community.org/job/Koha_Master_U20/191/consoleFull for instance (Koha_Master_D11_CPAN as well). # Failed test 'take from lowest cost branch (don't use cost matrix) holding branch' # at t/db_dependent/HoldsQueue.t line 1494. # got: 'Dgj7iwys5' # expected: 'qKpmffi' # Wrong pick-up/hold for first target (pick_branch, hold_branch, reserves, hold_fill_targets, tmp_holdsqueue) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Reverting "Bug 28510: Remove unnecessary conditional" fixes the problem. I am going to revert this patch as a first step, but feel free to submit a better fix if you understand what's going on. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|21.11.00 |21.11.00,21.05.05 released in| | Status|Pushed to master |Pushed to stable --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #125282|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Version(s)|21.11.00,21.05.05 |21.11.00,21.05.05,20.11.11 released in| | Status|Pushed to stable |Pushed to oldstable --- Comment #18 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 20.11.x for 20.11.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28510 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |RESOLVED CC| |victor@tuxayo.net Resolution|--- |FIXED --- Comment #19 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org