Bug 28510 - Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled
Summary: Skip processing holds queue items from closed libraries when HoldsQueueSkipCl...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-03 10:44 UTC by Kyle M Hall
Modified: 2021-10-25 21:50 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.11


Attachments
Bug 28510: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled (2.06 KB, patch)
2021-06-03 10:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28510: Remove marking of closed branches as 'disable_transfer' (2.02 KB, patch)
2021-06-03 10:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28510: Remove unnecessary conditional (1.31 KB, patch)
2021-06-03 11:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28510: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled (2.09 KB, patch)
2021-06-20 19:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 28510: Remove marking of closed branches as 'disable_transfer' (2.05 KB, patch)
2021-06-20 19:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 28510: Remove unnecessary conditional (1.35 KB, patch)
2021-06-20 19:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 28510: Remove unused variable (735 bytes, patch)
2021-09-13 16:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28510: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled (2.16 KB, patch)
2021-09-24 15:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28510: Remove marking of closed branches as 'disable_transfer' (2.12 KB, patch)
2021-09-24 15:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28510: Remove unnecessary conditional (1.41 KB, patch)
2021-09-24 15:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28510: Remove unused variable (786 bytes, patch)
2021-09-24 15:59 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2021-06-03 10:44:42 UTC
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.
Comment 1 Kyle M Hall 2021-06-03 10:54:51 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
Comment 2 Kyle M Hall 2021-06-03 10:59:01 UTC
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.
Comment 3 Kyle M Hall 2021-06-03 11:34:06 UTC
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.
Comment 4 David Nind 2021-06-20 19:15:01 UTC
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>
Comment 5 David Nind 2021-06-20 19:15:04 UTC
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>
Comment 6 David Nind 2021-06-20 19:15:08 UTC
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>
Comment 7 Marcel de Rooy 2021-09-10 08:37:45 UTC
 FAIL   C4/HoldsQueue.pm
   FAIL   critic
                # Variables::ProhibitUnusedVariables: Got 1 violation(s).
Comment 8 Kyle M Hall 2021-09-13 16:00:08 UTC
Created attachment 124842 [details] [review]
Bug 28510: Remove unused variable
Comment 9 Martin Renvoize 2021-09-24 15:59:34 UTC
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>
Comment 10 Martin Renvoize 2021-09-24 15:59:38 UTC
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>
Comment 11 Martin Renvoize 2021-09-24 15:59:41 UTC
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>
Comment 12 Martin Renvoize 2021-09-24 15:59:45 UTC
Created attachment 125283 [details] [review]
Bug 28510: Remove unused variable

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-09-24 16:00:13 UTC
Nice work, QA scripts happy, cleaner code and tests still pass.

Passing QA
Comment 14 Jonathan Druart 2021-09-28 13:38:19 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 15 Jonathan Druart 2021-09-29 09:25:33 UTC
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)
Comment 16 Jonathan Druart 2021-10-01 14:25:51 UTC
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.
Comment 17 Kyle M Hall 2021-10-08 12:19:27 UTC
Pushed to 21.05.x for 21.05.05
Comment 18 Fridolin Somers 2021-10-21 03:10:17 UTC
Pushed to 20.11.x for 20.11.11
Comment 19 Victor Grousset/tuxayo 2021-10-25 21:50:30 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.