This is a large and sparsely documented routine that could use some cleanup/explanation
Created attachment 159351 [details] [review] Bug 35432: Add comments and POD
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
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
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
Created attachment 159355 [details] [review] Bug 35432: Expand comments to explain the priority of filling requests
Created attachment 159381 [details] [review] Bug 35432: Add comments and POD Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
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>
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>
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>
Created attachment 159385 [details] [review] Bug 35432: Expand comments to explain the priority of filling requests Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Moving to enh. Correct me if I am mistaken.
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>
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>
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>
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>
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>
Code is considerably clearer, unit tests all continue to pass and qa script is happy. Passing QA
I wonder if we should not have tests for MapItemsToHoldRequests as well? But all holds related tests I could find still pass...
Pushed for 24.05! Well done everyone, thank you!
*** Bug 35805 has been marked as a duplicate of this bug. ***
Not backported to 23.11.x
Looks like backend modifications. Nothing to add/edit in the Koha manual. Please reopen with more details if that is not the case!