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

Fix for issue #2 pushed.

https://bugs.launchpad.net/evergreen/+bug/1269865

Oops, wrong link for fix to #2:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/acq-avoid-dupe-li-asset-create

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.

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.

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.

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.

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.

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.

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)

The work on bug 1774277 fixes this bug.