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.
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!
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>
Processing additional checks * Author 'John Doe' seems invalid * Email's author 'you@example.com' seems invalid
Please add tests
Created attachment 156105 [details] [review] Bug 33794: Unit tests
Created attachment 156115 [details] [review] Bug 33794: Move relevent code to it's own subroutine
Created attachment 156117 [details] [review] Bug 33794: Move relevent code to it's own subroutine
Created attachment 156118 [details] [review] WIP Bug 33794: Unit tests
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.