ACQ user request can result in double (or quadruple) holds placement

Bug #1269865 reported by Bill Erickson on 2014-01-16
26
This bug affects 5 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Jason Etheridge

Bug Description

Evergreen 2.4+

1. Acquisitions user requests can be used to generate hold requests. Hold generation occurs when asset (bibs, volumes, copies) creation for a lineitem occurs. However, asset creation for lineitems can happen multiple times, since we always ensure assets are generated during activation, even if they were generated earlier in the process. In these cases, the assets themselves are not duplicated, since we check each copy to verify it needs importing. We are not, however, doing the same thing for hold requests generated from user requests. We need a way to indicate that a user request has already produced a hold. Or, alternatively, skip hold placement when a matching hold for the user already exists.

2. A secondary thinko bug further complicates this problem. In lineitem list asset creation, in some circumstances, we process each lineitem twice. I have a patch for this and will post it shortly.

So, when both issues occur, we can get up to 4 hold requests per user request.

Bill Erickson (berick) wrote :

I'm thinking for issue #1, we should add a column to acq.user_request to indicate when a request has been honored. (honor_date or some such). Simply preventing duplicates may be too strict. Consider an item the library previously owned and the user has placed a hold on before, but the user wants to place another hold later after the library buys more copies.

Steven Chan (schan2) wrote :

For the fix to #2,

+ for my $id (@$li_ids) {
+ push(@{$res->{li_ids}}, $id)
+ unless grep { $_ == $id } @$needs_importing;
+ }

I think it will be more efficient if the reference to the result array is cached before entering the for loop:

      my $x = $res->{li_ids};
+ for my $id (@$li_ids) {
+ push(@{$x}, $id)
+ unless grep { $_ == $id } @$needs_importing;
+ }

Also, the for loop is a generic array operation and so could be packaged as a subroutine:

# Returns the array @$x - @$y
sub array_minus {
 my ($x, $y) = @_;
 my $z = [];
 for my $n (@$x) {
  push @$z, $n unless grep { $_ == $n } @$y;
 }
 return $@z;
}

To use the sub in the particular context:

- push(@{$res->{li_ids}}, grep { $_ != @$needs_importing } @$li_ids)
+ push @{$res->{li_ids}}, @{array_minus($li_ids, $needs_importing)};

which makes it a bit more meaningful.

Ben Shum (bshum) wrote :

Marking status as confirmed since this bug seems to be an issue. There's some feedback review on the partial fix for part of the described issue, but the rest still needs work.

Changed in evergreen:
importance: Undecided → Medium
status: New → Confirmed
no longer affects: evergreen/2.4
no longer affects: evergreen/2.5
no longer affects: evergreen/2.7
no longer affects: evergreen/2.6

Was the duplicate holds from the Patron Request screens ever resolved? I'm still seeing duplicate holds being created from Patron Request in 2.11+ so I'm not clear as to the status of issue #1 reported on this bug.

It looks like there is just 1 entry in acq.user_request but I've seen 2 and 3 holds being created spun off of a single user_request entry basically one after the other.

Jason Etheridge (phasefx) wrote :

Would one possible improvement be the creation of a link from the user_request to the specific hold request created, and only create a hold if such a column is null and the original hold desired boolean is true? I think it would be useful to follow such a link to show the Hold Status, but it could also be used to prevent the creation of duplicate holds without needing to consider holds requested outside of the user request / acq process.

Jason Etheridge (phasefx) wrote :

Well, that last sentence is a misunderstanding on my part. Not sure about fulfilling multiple holds on the same user request when new copies are acquired. I lean toward that being new functionality that should be better specced out.

Jason Etheridge (phasefx) wrote :

re: hold linking, Mike suggested going at it in the other direction, linking from the hold to the user request (or lineitem or what not)

tags: added: patronrequests
Changed in evergreen:
assignee: nobody → Jason Etheridge (phasefx)
Andrea Neiman (aneiman) wrote :

The work on bug 1774277 fixes this bug.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers