Bug 28510

Summary: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, m.de.rooy, martin.renvoize, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
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
Bug 28510: Remove marking of closed branches as 'disable_transfer'
Bug 28510: Remove unnecessary conditional
Bug 28510: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled
Bug 28510: Remove marking of closed branches as 'disable_transfer'
Bug 28510: Remove unnecessary conditional
Bug 28510: Remove unused variable
Bug 28510: Skip processing holds queue items from closed libraries when HoldsQueueSkipClosed is enabled
Bug 28510: Remove marking of closed branches as 'disable_transfer'
Bug 28510: Remove unnecessary conditional
Bug 28510: Remove unused variable

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.