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: Signed Off
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: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-03 10:44 UTC by Kyle M Hall
Modified: 2021-09-13 16:00 UTC (History)
2 users (show)

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


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

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