Bug 33794 - Holds Queue builder skips least cost branch unless target item's homebranch is patron's homebranch
Summary: Holds Queue builder skips least cost branch unless target item's homebranch i...
Status: RESOLVED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Laura Escamilla
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 33795
  Show dependency treegraph
 
Reported: 2023-05-22 15:16 UTC by Kyle M Hall
Modified: 2024-02-14 14:06 UTC (History)
8 users (show)

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


Attachments
Bug 33794: Removing line that causes TCM to pick the most expensive item if it means that it's going back to it's home branch. (1.01 KB, patch)
2023-05-25 19:38 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 33794: Removing line that causes TCM to pick the most expensive item if it means that it's going back to it's home branch. (1.05 KB, patch)
2023-06-02 15:39 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 33794: Unit tests (2.72 KB, patch)
2023-09-22 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33794: Move relevent code to it's own subroutine (3.35 KB, patch)
2023-09-22 16:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33794: Move relevent code to it's own subroutine (3.35 KB, patch)
2023-09-22 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
WIP Bug 33794: Unit tests (3.89 KB, patch)
2023-09-22 16:41 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 2023-05-22 15:16:19 UTC
The line
next if $request->{borrowerbranch} ne $item->{homebranch};
has been in the holds queue builder since the very beginning, with no explanation.

My justification for this line is that we would prefer to send an item to a patron whose home library is the same is the item's home library, assuming that patron will return the item at that library, thus reducing transportation costs.

I will not argue here if this is a good feature or not. Bug 31557 makes in configurable, but perhaps it should have an 'off' feature as well. 

This feature does not make sense when using the transport cost matrix. We get the lowest cost branch, and then if the branches don't match we just move on the the "fall back to basics code". This line should be removed from the code section dealing with the transport cost matrix.
Comment 1 Laura Escamilla 2023-05-25 19:38:07 UTC
Created attachment 151712 [details] [review]
Bug 33794: Removing line that causes TCM to pick the most expensive item if it means that it's going back to it's home branch.

Test Plan:
1. Enable the UseTransportCostMatrix system preference 
2. Ensure that there are at least three test libraries set up (Library A, Library B, Library C)
3. Set up the TCM so that it is cheaper to transport an item from Library A to Library C rather than from Library B to Library C. 
4. Choose or create a biblio that has three items. 
    1. Item 1: 
        1. Set home library to Library C and the holding library to Library B — cost is more expensive
    2. Item 2:
        1. Set home library to Library A and the holding library is Library A - cost is least expensive (The TCM should be choosing this item)
    3. Item 3:
        1. Set home library to Library C and holding library to Library C, but check this item out to another patron so that it is unavailable. - Just a control variable.
5. Choose or create a patron that has their holds pickup library set to Library C. Place a hold on the biblio above for them. 
6. Build the holds queue
7. The holds queue shows that Item 1 (most expensive) is being chosen over Item 2 (least expensive)
8. Delete the hold and apply the patch
9. Restart the services
10. Repeat step 5
11. Run the holds queue — it is now showing that Item 2 (least expensive) is being chosen over Item 1 (most expensive)
12. Sign off!
Comment 2 Sam Lau 2023-06-02 15:39:54 UTC
Created attachment 151987 [details] [review]
Bug 33794: Removing line that causes TCM to pick the most expensive item if it means that it's going back to it's home branch.

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 3 Marcel de Rooy 2023-07-07 07:49:33 UTC
Processing additional checks

        * Author 'John Doe' seems invalid
        * Email's author 'you@example.com' seems invalid
Comment 4 Marcel de Rooy 2023-07-07 07:50:05 UTC
Please add tests
Comment 5 Kyle M Hall 2023-09-22 15:09:07 UTC
Created attachment 156105 [details] [review]
Bug 33794: Unit tests
Comment 6 Kyle M Hall 2023-09-22 16:37:18 UTC
Created attachment 156115 [details] [review]
Bug 33794: Move relevent code to it's own subroutine
Comment 7 Kyle M Hall 2023-09-22 16:41:36 UTC
Created attachment 156117 [details] [review]
Bug 33794: Move relevent code to it's own subroutine
Comment 8 Kyle M Hall 2023-09-22 16:41:43 UTC
Created attachment 156118 [details] [review]
WIP Bug 33794: Unit tests
Comment 9 Nick Clemens (kidclamp) 2023-11-29 16:46:09 UTC
See bug 35431 and bug 35432 - this seems to be the same misunderstanding I encountered.

The initial check of the TCM is actually part of HoldsQueuePrioritize branch.

Later in the code we use the holdingbranch variable that was set here to try and fill holds with the least cost.