Description
Kyle M Hall (khall)
2021-06-03 10:44:42 UTC
Created attachment 121578 [details] [review] 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 Created attachment 121579 [details] [review] 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. Created attachment 121588 [details] [review] 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. Created attachment 122193 [details] [review] 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> Created attachment 122194 [details] [review] 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> Created attachment 122195 [details] [review] 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> FAIL C4/HoldsQueue.pm FAIL critic # Variables::ProhibitUnusedVariables: Got 1 violation(s). Created attachment 124842 [details] [review] Bug 28510: Remove unused variable Created attachment 125280 [details] [review] 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> Created attachment 125281 [details] [review] 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> Created attachment 125282 [details] [review] 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> Created attachment 125283 [details] [review] Bug 28510: Remove unused variable Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice work, QA scripts happy, cleaner code and tests still pass. Passing QA Pushed to master for 21.11, thanks to everybody involved! 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) 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. Pushed to 21.05.x for 21.05.05 Pushed to 20.11.x for 20.11.11 Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed. |