Bug 35805 - Two issues with HoldsQueue::MapItemsToHoldRequests
Summary: Two issues with HoldsQueue::MapItemsToHoldRequests
Status: RESOLVED DUPLICATE of bug 35432
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-01-13 13:33 UTC by Andreas Jonsson
Modified: 2024-04-16 21:30 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:


Attachments
Bug 35805: Use transport cost matrix if local item allocation failed (899 bytes, patch)
2024-01-13 13:38 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 35805: Compare itemtype to the right item in MapItemsToHoldRequests (1.32 KB, patch)
2024-01-13 13:39 UTC, Andreas Jonsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 ***