Bug 35805

Summary: Two issues with HoldsQueue::MapItemsToHoldRequests
Product: Koha Reporter: Andreas Jonsson <andreas.jonsson>
Component: Hold requestsAssignee: Bugs List <koha-bugs>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 35805: Use transport cost matrix if local item allocation failed
Bug 35805: Compare itemtype to the right item in MapItemsToHoldRequests

Description Andreas Jonsson 2024-01-13 13:33:03 UTC
1. This clause from the below if statement must be incorrect:

         ( !$request->{itemtype} # If hold itemtype is set, item's itemtype must match
                        || ( $request->{itemnumber} && ( $items_by_itemnumber{ $request->{itemnumber} }->{itype} eq $request->{itemtype} ) ) )

Because $request->{itemnumber} is guaranteed to be undefined due to this clause:

        next if defined($request->{itemnumber}); # already handled these

The clause was probably ment to be this:

         ( !$request->{itemtype} || ( $items_by_itemnumber{ $item->{itemnumber} }->{itype} eq $request->{itemtype} ) )

    foreach my $request (@$hold_requests) {
        last if $num_items_remaining == 0;
        next if $request->{allocated};
        next if defined($request->{itemnumber}); # already handled these

        # look for local match first
        my $pickup_branch = $request->{branchcode} || $request->{borrowerbranch};
        my ($itemnumber, $holdingbranch);

        my $holding_branch_items = $items_by_branch{$pickup_branch};
        my $priority_branch = C4::Context->preference('HoldsQueuePrioritizeBranch') // 'homebranch';
        if ( $holding_branch_items ) {
            foreach my $item (@$holding_branch_items) {
                next unless $items_by_itemnumber{ $item->{itemnumber} }->{_object}->can_be_transferred( { to => $libraries->{ $request->{branchcode} } } );

                if (
                    $request->{borrowerbranch} eq $item->{$priority_branch}
                    && _checkHoldPolicy($item, $request) # Don't fill item level holds that contravene the hold pickup policy at this time
                    && ( !$request->{itemtype} # If hold itemtype is set, item's itemtype must match
                        || ( $request->{itemnumber} && ( $items_by_itemnumber{ $request->{itemnumber} }->{itype} eq $request->{itemtype} ) ) )
                    && ( !$request->{item_group_id} # If hold item_group is set, item's item_group must match
                        || ( $item->{_object}->item_group && $item->{_object}->item_group->id eq $request->{item_group_id} ) )
                  )
                {
                    $itemnumber = $item->{itemnumber};
                    last;
                }
            }
            $holdingbranch = $pickup_branch;


2. Then the code continues:

        }
        elsif ($transport_cost_matrix) {

But surely this should be:

        }
        if (!defined $itemnumber && defined $transport_cost_matrix) {

Because the important condition here is that we failed to allocate from the local holding branch.  Surely the transport_cost_matrix should still be used just even if we tried allocating locally first, but failed?
Comment 1 Andreas Jonsson 2024-01-13 13:38:46 UTC
Created attachment 161007 [details] [review]
Bug 35805: Use transport cost matrix if local item allocation failed
Comment 2 Andreas Jonsson 2024-01-13 13:39:26 UTC
Created attachment 161008 [details] [review]
Bug 35805: Compare itemtype to the right item in MapItemsToHoldRequests
Comment 3 Andreas Jonsson 2024-04-16 21:30:13 UTC
This is actually resolved by bug 35432 and bug 35826.

*** This bug has been marked as a duplicate of bug 35432 ***