Bug 35432 - Clarify and simplify the workings of MapItemsToHoldRequests
Summary: Clarify and simplify the workings of MapItemsToHoldRequests
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize
URL:
Keywords:
: 35805 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-11-29 14:01 UTC by Nick Clemens (kidclamp)
Modified: 2024-07-25 11:14 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 35432: Add comments and POD (3.29 KB, patch)
2023-11-29 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35432: Add _can_item_fill_request subroutine (10.94 KB, patch)
2023-11-29 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35432: Simplay HoldsQueuePrioritize branch check (4.19 KB, patch)
2023-11-29 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35432: Remove PULL_BRANCHES and use ifs over unless (3.56 KB, patch)
2023-11-29 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35432: Expand comments to explain the priority of filling requests (3.99 KB, patch)
2023-11-29 16:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35432: Add comments and POD (3.34 KB, patch)
2023-11-29 23:25 UTC, Phil Ringnalda (back in August)
Details | Diff | Splinter Review
Bug 35432: Add _can_item_fill_request subroutine (10.99 KB, patch)
2023-11-29 23:25 UTC, Phil Ringnalda (back in August)
Details | Diff | Splinter Review
Bug 35432: Simplay HoldsQueuePrioritize branch check (4.24 KB, patch)
2023-11-29 23:25 UTC, Phil Ringnalda (back in August)
Details | Diff | Splinter Review
Bug 35432: Remove PULL_BRANCHES and use ifs over unless (3.62 KB, patch)
2023-11-29 23:25 UTC, Phil Ringnalda (back in August)
Details | Diff | Splinter Review
Bug 35432: Expand comments to explain the priority of filling requests (4.04 KB, patch)
2023-11-29 23:25 UTC, Phil Ringnalda (back in August)
Details | Diff | Splinter Review
Bug 35432: Add comments and POD (3.41 KB, patch)
2024-02-15 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35432: Add _can_item_fill_request subroutine (11.06 KB, patch)
2024-02-15 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35432: Simplay HoldsQueuePrioritize branch check (4.31 KB, patch)
2024-02-15 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35432: Remove PULL_BRANCHES and use ifs over unless (3.68 KB, patch)
2024-02-15 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35432: Expand comments to explain the priority of filling requests (4.10 KB, patch)
2024-02-15 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 (back in August) 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 (back in August) 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 (back in August) 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 (back in August) 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 (back in August) 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