Bug 35432

Summary: Clarify and simplify the workings of MapItemsToHoldRequests
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Hold requestsAssignee: Nick Clemens (kidclamp) <nick>
Status: Needs documenting --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: andreas.jonsson, fridolin.somers, gmcharlt, phil
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33794
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00
Circulation function:
Attachments: Bug 35432: Add comments and POD
Bug 35432: Add _can_item_fill_request subroutine
Bug 35432: Simplay HoldsQueuePrioritize branch check
Bug 35432: Remove PULL_BRANCHES and use ifs over unless
Bug 35432: Expand comments to explain the priority of filling requests
Bug 35432: Add comments and POD
Bug 35432: Add _can_item_fill_request subroutine
Bug 35432: Simplay HoldsQueuePrioritize branch check
Bug 35432: Remove PULL_BRANCHES and use ifs over unless
Bug 35432: Expand comments to explain the priority of filling requests
Bug 35432: Add comments and POD
Bug 35432: Add _can_item_fill_request subroutine
Bug 35432: Simplay HoldsQueuePrioritize branch check
Bug 35432: Remove PULL_BRANCHES and use ifs over unless
Bug 35432: Expand comments to explain the priority of filling requests

Description Nick Clemens (kidclamp) 2023-11-29 14:01:36 UTC
This is a large and sparsely documented routine that could use some cleanup/explanation
Comment 1 Nick Clemens (kidclamp) 2023-11-29 16:41:39 UTC
Created attachment 159351 [details] [review]
Bug 35432: Add comments and POD
Comment 2 Nick Clemens (kidclamp) 2023-11-29 16:41:41 UTC
Created attachment 159352 [details] [review]
Bug 35432: Add _can_item_fill_request subroutine

There are a series of repeated checks throughout MapItemsToHoldRequests.

This patch simply consolidates them into a single routine that can be called.

To test:
1 - prove t/db_dependent/HoldsQueue.t
2 - Confirm it passes before and after this patch is applied
Comment 3 Nick Clemens (kidclamp) 2023-11-29 16:41:43 UTC
Created attachment 159353 [details] [review]
Bug 35432: Simplay HoldsQueuePrioritize branch check

The code here is going to check items held the the pickup location for a request
or from the least cost branch matching the patron's home library against the HoldsQueuePrioritizeBranch
setting

The loop is the same in both case, so lets simplify this a bit to make the intent more clear

To test:
1 - prove -v t/db_dependent/HoldsQueue.t
2 - It should pass before and after this patch
Comment 4 Nick Clemens (kidclamp) 2023-11-29 16:41:45 UTC
Created attachment 159354 [details] [review]
Bug 35432: Remove PULL_BRANCHES and use ifs over unless

PULL_BRANCHES and PULL_BRANCHES2 felt a little arbitrary, moved
to using a last in the outer loop.

Swapped 'next unless' for an if to more directly show when we are acting
Comment 5 Nick Clemens (kidclamp) 2023-11-29 16:41:47 UTC
Created attachment 159355 [details] [review]
Bug 35432: Expand comments to explain the priority of filling requests
Comment 6 Phil Ringnalda 2023-11-29 23:25:11 UTC
Created attachment 159381 [details] [review]
Bug 35432: Add comments and POD

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 7 Phil Ringnalda 2023-11-29 23:25:14 UTC
Created attachment 159382 [details] [review]
Bug 35432: Add _can_item_fill_request subroutine

There are a series of repeated checks throughout MapItemsToHoldRequests.

This patch simply consolidates them into a single routine that can be called.

To test:
1 - prove t/db_dependent/HoldsQueue.t
2 - Confirm it passes before and after this patch is applied

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 8 Phil Ringnalda 2023-11-29 23:25:16 UTC
Created attachment 159383 [details] [review]
Bug 35432: Simplay HoldsQueuePrioritize branch check

The code here is going to check items held the the pickup location for a request
or from the least cost branch matching the patron's home library against the HoldsQueuePrioritizeBranch
setting

The loop is the same in both case, so lets simplify this a bit to make the intent more clear

To test:
1 - prove -v t/db_dependent/HoldsQueue.t
2 - It should pass before and after this patch

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 9 Phil Ringnalda 2023-11-29 23:25:18 UTC
Created attachment 159384 [details] [review]
Bug 35432: Remove PULL_BRANCHES and use ifs over unless

PULL_BRANCHES and PULL_BRANCHES2 felt a little arbitrary, moved
to using a last in the outer loop.

Swapped 'next unless' for an if to more directly show when we are acting

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 10 Phil Ringnalda 2023-11-29 23:25:21 UTC
Created attachment 159385 [details] [review]
Bug 35432: Expand comments to explain the priority of filling requests

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 11 Marcel de Rooy 2024-01-26 10:23:34 UTC
Moving to enh. Correct me if I am mistaken.
Comment 12 Martin Renvoize 2024-02-15 10:39:22 UTC
Created attachment 162194 [details] [review]
Bug 35432: Add comments and POD

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2024-02-15 10:39:25 UTC
Created attachment 162195 [details] [review]
Bug 35432: Add _can_item_fill_request subroutine

There are a series of repeated checks throughout MapItemsToHoldRequests.

This patch simply consolidates them into a single routine that can be called.

To test:
1 - prove t/db_dependent/HoldsQueue.t
2 - Confirm it passes before and after this patch is applied

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2024-02-15 10:39:28 UTC
Created attachment 162196 [details] [review]
Bug 35432: Simplay HoldsQueuePrioritize branch check

The code here is going to check items held the the pickup location for a request
or from the least cost branch matching the patron's home library against the HoldsQueuePrioritizeBranch
setting

The loop is the same in both case, so lets simplify this a bit to make the intent more clear

To test:
1 - prove -v t/db_dependent/HoldsQueue.t
2 - It should pass before and after this patch

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2024-02-15 10:39:30 UTC
Created attachment 162197 [details] [review]
Bug 35432: Remove PULL_BRANCHES and use ifs over unless

PULL_BRANCHES and PULL_BRANCHES2 felt a little arbitrary, moved
to using a last in the outer loop.

Swapped 'next unless' for an if to more directly show when we are acting

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2024-02-15 10:39:33 UTC
Created attachment 162198 [details] [review]
Bug 35432: Expand comments to explain the priority of filling requests

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2024-02-15 10:40:43 UTC
Code is considerably clearer, unit tests all continue to pass and qa script is happy.

Passing QA
Comment 18 Katrin Fischer 2024-04-16 15:48:00 UTC
I wonder if we should not have tests for MapItemsToHoldRequests as well? But all holds related tests I could find still pass...
Comment 19 Katrin Fischer 2024-04-16 16:01:52 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 20 Andreas Jonsson 2024-04-16 21:30:13 UTC
*** Bug 35805 has been marked as a duplicate of this bug. ***
Comment 21 Fridolin Somers 2024-05-23 13:07:03 UTC
Not backported to 23.11.x