One thought, I wonder if we should just always randomize them in the case that StaticHoldsQueueWeight is empty. I cannot imagine an instance where a library system would want holds to be filled in alphabetical order.

This second variation forces the shuffling in the case that StaticHoldsQueueWeight is empty. The larger benefit is the reduction of calls to C4::Context->preference from @bibs_with_pending_requests + 1 back down to 1.

Do these patches work together, or are they two options? If the latter, what is the difference?

(In reply to comment #5)
> Do these patches work together, or are they two options? If the latter, what
> is the difference?

The are mutually exclusive. Either one will fix the bug.

The first patch shuffles the branches to pull if RandomizeHoldsQueueWeight is enabled, even when StaticHoldsQueueWeight is empty.

The second patch forces randomization in the event that StaticHoldsQueueWeight is empty, under the assumption that no library would wish to have libraries fulfill hold requests in alphabetical order. It also is slightly more efficient in that it does not make a call to C4::Context->preference().

These patches have been in the queue for a long time. Perhaps good test plans for each would help?

Tested on a sandbox. The hold is chosen at random.

Test plan needed to be specified.
An hold (3 items, one in each library, and the pickup library is a 4th one because the pickup library for this test must be a library without the item).
Don't forget the syspref AllowOnShelfHolds.

Look at the holds queue report from the circulation home page.
If you choose all libraries : no barcode specified for the hold.
If you specify one library, the first library you choose is the one that must send the book.
Run build_holds_queue.pl
Change the library of the holds queue report.

So : ok.

Patch tested with a sandbox, by Leila and Frido <koha.aixmarseille@gmail.com>

Created attachment 16794 [details] [review]
Bug 8562 - RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty.

If StaticHoldsQueueWeight is empty, but RandomizeHoldsQueueWeight is enabled,
the script build_holds_queue.pl should select branches for fulfillment requests
at random. Instead, it will select them in alphabetical order.

Test Plan:
1) Apply patch
2) Set the system preference StaticHoldsQueueWeight to empty
3) Create a hold on an record with items at multiple libraries
4) Run build_holds_queue.pl
5) Look at the holds queue report from the circulation home page
6) Run build_holds_queue.pl again
7) Look at the holds queue report again, note the library chosen
   for fulfillment of the hold has likely changed.
8) Repeat steps 6-7 until you are satisfied the library to fulfill
   the hold is chosen at random.

Signed-off-by: Leila and Frido <koha.aixmarseille@gmail.com>

Comment on attachment 16794 [details] [review]
Bug 8562 - RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty.

Review of attachment 16794 [details] [review]:
-----------------------------------------------------------------

::: C4/HoldsQueue.pm
@@ +575,5 @@
>      my @branches_to_use = map _trim($_), split /,/, $static_branch_list;
>  
>      @branches_to_use = shuffle(@branches_to_use) if  C4::Context->preference("RandomizeHoldsQueueWeight");
>  
> +    @branches_to_use = shuffle keys %{GetBranches()} unless ( @branches_to_use );

I do not think that this is correct. The branches are shuffled regardless of the value of RandomizeHoldsQueueWeight. Perhaps better would be:
@branches_to_use ||= keys %{GetBranches()}; # (or the correct version of that statement, since I didn't test my suggestion)
right after the definition of @branches_to_use?

Also, I think there is a problem here with the default value of StaticHoldsQueueWeight, which is '0'(?). This could be addressed on a different bug.

> I do not think that this is correct. The branches are shuffled regardless of
> the value of RandomizeHoldsQueueWeight. Perhaps better would be:
> @branches_to_use ||= keys %{GetBranches()}; # (or the correct version of
> that statement, since I didn't test my suggestion)
> right after the definition of @branches_to_use?

The justification for this is that it is extremely unlikely that a library would want to fill holds from libraries in alphabetical order in the case that the library has not populated StaticHoldsQueueWeight. Do you believe this is an unreasonable assumption?

(In reply to Kyle M Hall from comment #14)
> > I do not think that this is correct. The branches are shuffled regardless of
> > the value of RandomizeHoldsQueueWeight. Perhaps better would be:
> > @branches_to_use ||= keys %{GetBranches()}; # (or the correct version of
> > that statement, since I didn't test my suggestion)
> > right after the definition of @branches_to_use?
> 
> The justification for this is that it is extremely unlikely that a library
> would want to fill holds from libraries in alphabetical order in the case
> that the library has not populated StaticHoldsQueueWeight. Do you believe
> this is an unreasonable assumption?

I tend to agree with Jared here. If the Randomize pref is not set, you should not shuffle. Otherwise this pref is really useless. 
You have a point however. I think that what you say, more pertains to the default values set for these prefs in Koha. Randomize should be 1 by default.
The description on the pref form could be improved/extended.

I also think you should take into account the '0' default value. Just interpret it as all branches here (empty string as advertised). If we definitely want to get rid of this zero, it could be removed completely on a new report.

Still valid?

Not still valid, he fixed it in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12803#c14 instead.

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