[Bug 41959] New: Holds queue builder doesn't always check all holds when using transport cost matrix
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Bug ID: 41959 Summary: Holds queue builder doesn't always check all holds when using transport cost matrix Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, lisette@bywatersolutions.com, tomascohen@gmail.com The scenario presented itself under a specific circumstance Library A ordered a copy of a book and left in 'on order' status Library B purchased a copy and made it available Each library can only fill holds on their own items The transport cost matrix only allows items to transfer to their own branch The transfer limits only allows items to transfer to their own branch A patron of Library A places a title level hold, it can't fill because library A's item is unavailable (on order) A patron of Library B places a title level hold, it should be queued for library B's item In C4::HoldsQueue::MapItemsToHoldRequests we call _allocateWithTransportCostMatrix In _allocateWithTranportCostMatrix we have this code: if ( $num_tasks > $num_agents ) { @remaining = @requests[ $num_agents .. $num_tasks - 1 ]; @requests = @requests[ 0 .. $num_agents - 1 ]; $num_tasks = $num_agents; } task = requests and agents = items In our scenario - 2 tasks/requests and 1 item/agent The code above drops the second request, and sets tasks to 1 We then loop tasks and agents: for ( my $i = 0 ; $i < $num_agents ; $i++ ) { for ( my $j = $r ; $j < $num_tasks ; $j++ ) { But since we have dcereased things above, we only check the first task/request - the second hold, which should be filled, is never checked -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |35826 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35826 [Bug 35826] Optimize building of holds queue based on transport cost matrix -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Andreas Jonsson <andreas.jonsson@kreablo.se> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andreas.jonsson@kreablo.se --- Comment #1 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- This should not be an issue. Requests that could not be filled will be removed from the candidate set, and are replaced with new candidates from remaining requests before the allocation is repeated. if ( $retries-- > 0 && @unallocated && @remaining ) { # Remove the transport cost of unfilled holds and compact the matrix. # Also remove the hold request from the array. for ( my $i = 0 ; $i < $num_agents ; $i++ ) { my $u = 0; for ( my $j = 0 ; $j < $num_tasks ; $j++ ) { if ( $u < scalar(@unallocated) && $unallocated[$u] == $j ) { $u++; } elsif ( $u > 0 ) { $m[$i][ $j - $u ] = $m[$i][$j]; } } } for ( my $u = 0 ; $u < scalar(@unallocated) ; $u++ ) { splice @requests, $unallocated[$u], 1; } $num_tasks = scalar(@requests); $r = $num_tasks; } else { if ( $retries == 0 && @unallocated && @remaining ) { Koha::Logger->get->warn( "There are available items that have not been allocated and remaining holds, but we abort trying to fill these after $RETRIES retries." ); } last RETRY; } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #2 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 194560 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194560&action=edit Bug 41959: Unit tests This adds a test for the condition where there are 2 holds, 1 item, and the item cannot fulfill the first hold -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #3 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 194561 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194561&action=edit Bug 41959: Handle edge case With a single hold at least the RETRY loop was never activated after an initial pass with 1 hold We try the hold - it cannot be filled - is removed from the requests array The number of tasks is now 0, less than agents (1) and we don't enter remaining loop For this case we also need to always splice 1 from remaining, or it doesn't shrink -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #4 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- That is a great find! But the fix is not correct, I think. The mistake I made is to forget the edge case where all candidate holds under consideration fails to be allocated, leaving $num_tasks == 0. This will incorrectly abort the RETRY-loop when there are remaining candidate holds in the queue. The purpose of the condition at the start of the RETRY-loop is to abort with empty result if there is nothing more to consider, so the fix should be as follows: diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm index e0602e595eb..6f82ca46d91 100644 --- a/C4/HoldsQueue.pm +++ b/C4/HoldsQueue.pm @@ -479,7 +479,7 @@ sub _allocateWithTransportCostMatrix { RETRY: while (1) { - return [] if $num_agents == 0 || $num_tasks == 0; + return [] if $num_agents == 0 || $num_tasks + scalar(@remaining) == 0; if ( $num_tasks < $num_agents && @remaining ) { -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Andreas Jonsson <andreas.jonsson@kreablo.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194561|0 |1 is obsolete| | --- Comment #5 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- Created attachment 194907 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194907&action=edit Bug 41959: Handle edge case -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194560|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194907|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #6 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 195883 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195883&action=edit Bug 41959: Unit tests This adds a test for the condition where there are 2 holds, 1 item, and the item cannot fulfill the first hold Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #7 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 195884 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195884&action=edit Bug 41959: Handle edge case Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #8 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 195885 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195885&action=edit Bug 41959: (QA follow-up) Add missing POD Patch from commit e4e95d8 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #195883|0 |1 is obsolete| | Attachment #195884|0 |1 is obsolete| | Attachment #195885|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 195948 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195948&action=edit Bug 41959: Unit tests This adds a test for the condition where there are 2 holds, 1 item, and the item cannot fulfill the first hold Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 195949 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195949&action=edit Bug 41959: Handle edge case Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 195950 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195950&action=edit Bug 41959: (QA follow-up) Add missing POD Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Thanks for all the hard work! Pushed to main for the next 26.05.00 release as RM Assistant -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_25_11_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Jacob O'Mara <jacob.omara@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|26.05.00 |26.05.00,25.11.05 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #13 from Jacob O'Mara <jacob.omara@openfifth.co.uk> --- Thanks all, pushed to 25.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_25_05_candidate CC| |aleisha@catalyst.net.nz --- Comment #14 from Aleisha Amohia <aleisha@catalyst.net.nz> --- Can confirm this is also a bug in 25.05, please backport :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Status|Pushed to stable |Pushed to oldstable --- Comment #15 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Can I please get a test plan for this? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41959 --- Comment #16 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- (In reply to Wainui Witika-Park from comment #15)
Can I please get a test plan for this?
Hello, would I please be able to get a test plan for this so it can be backported to 25.05 before it goes out of support in one month. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org