[Koha-bugs] [Bug 35805] New: Two issues with HoldsQueue::MapItemsToHoldRequests

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35805

            Bug ID: 35805
           Summary: Two issues with HoldsQueue::MapItemsToHoldRequests
 Change sponsored?: ---
           Product: Koha
           Version: master
          Hardware: All
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5 - low
         Component: Hold requests
          Assignee: koha-bugs at lists.koha-community.org
          Reporter: andreas.jonsson at kreablo.se
        QA Contact: testopia at bugs.koha-community.org
                CC: gmcharlt at gmail.com

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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.