Bug 35431 - TransportCostMatrix should be checked after HoldsQueuePrioritizeBranch
Summary: TransportCostMatrix should be checked after HoldsQueuePrioritizeBranch
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-11-29 13:03 UTC by Nick Clemens (kidclamp)
Modified: 2024-07-04 20:37 UTC (History)
1 user (show)

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


Attachments
Bug 35431: Unit tests (3.74 KB, patch)
2023-11-29 13:37 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35431: Check TransportCostMatrix if hold not filled by HoldsQueuePrioritizeBranch (1.15 KB, patch)
2023-11-29 13:37 UTC, Nick Clemens (kidclamp)
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 13:03:24 UTC
Current behavior is:
LocalHoldsPriority is checked if enabled
Item level holds are filled if possible
Now we loop requests:
 - We check if there are items held at the pickup branch
   - We check if these items HoldsQueuePrioritizeBranch match the patron branch
 - Otherwise we check the transport cost matrix
 - continued checks

The correct behavior would be
Now we loop requests:
 - We check if there are items held at the pickup branch
   - We check if these items HoldsQueuePrioritizeBranch match the patron branch
 - We check the transport cost matrix
 - continued checks

The difference being that if we can't fill a hold by match HoldsQueuePrioritizeBranch we should next check the matrix, not skip it.

The matrix, if enabled, will select where the holding and pickup library match (as it is the least cost transfer to not transfer) and HoldsQueuePrioritize branch doesn't match, so we want this fallback
Comment 1 Nick Clemens (kidclamp) 2023-11-29 13:37:33 UTC
Created attachment 159335 [details] [review]
Bug 35431: Unit tests
Comment 2 Nick Clemens (kidclamp) 2023-11-29 13:37:35 UTC
Created attachment 159336 [details] [review]
Bug 35431: Check TransportCostMatrix if hold not filled by HoldsQueuePrioritizeBranch

This patch changes an elsif to an if to ensure the matrix is checked every time

To test:
1 - Apply unit test patch only
2 - prove -v t/db_dependent/HoldQueue.t
3 - It fails
4 - Apply this patch
5 - prove -v t/db_dependent/HoldQueue.t
6 - It passes
7 - All other tests continue to pass as well
Comment 3 Nick Clemens (kidclamp) 2023-11-29 13:44:30 UTC
This was a lesson in how the holds queue works, the TCM is skipped here in the case where there are items available/held at the pickup location of the hold, even when checked in this piece of the code, we are looking for HoldsQueuePrioritizeBranch matches

We set the 'holdingbranch' variable here to either the pickup library (if there are items held there) or to the lowest cost branch.

The code next checks if there are items where the items home branch and the holds pickup location match, and fills those - trying to get items back to where they are from

If there are none, then the holdingbranch variable is checked to see if items from that library can fill - this is the actual matrix checking

Filing a new bug for adding more comments to the routine here