Description
Nick Clemens (kidclamp)
2023-11-29 14:01:36 UTC
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 |