Bug 8562 - RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty.
Summary: RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty.
Status: RESOLVED DUPLICATE of bug 12803
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-02 16:22 UTC by Kyle M Hall
Modified: 2019-05-08 17:51 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8562 - RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty. (1.20 KB, patch)
2012-08-02 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8562 - RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty. Force randomization instead. (1.40 KB, patch)
2012-08-02 16:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8562 - RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty. (1.69 KB, patch)
2013-03-19 08:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8562 - RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty. (1.80 KB, patch)
2013-03-19 08:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8562 - RandomizeHoldsQueueWeight ignored if StaticHoldsQueueWeight is empty. (1.86 KB, patch)
2013-03-22 14:27 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-08-02 16:22:08 UTC
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.
Comment 1 Kyle M Hall 2012-08-02 16:24:02 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-08-02 16:25:41 UTC
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.
Comment 3 Kyle M Hall 2012-08-02 16:29:45 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-08-02 16:33:55 UTC
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.
Comment 5 Jared Camins-Esakov 2012-11-07 02:57:32 UTC
Do these patches work together, or are they two options? If the latter, what is the difference?
Comment 6 Kyle M Hall 2012-11-07 12:14:54 UTC
(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().
Comment 7 Owen Leonard 2013-02-07 17:13:23 UTC
These patches have been in the queue for a long time. Perhaps good test plans for each would help?
Comment 8 Kyle M Hall 2013-03-19 08:54:29 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2013-03-19 08:56:44 UTC Comment hidden (obsolete)
Comment 10 Koha Team AMU 2013-03-22 14:25:54 UTC
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.
Comment 11 Paul Poulain 2013-03-22 14:27:05 UTC
Patch tested with a sandbox, by Leila and Frido <koha.aixmarseille@gmail.com>
Comment 12 Paul Poulain 2013-03-22 14:27:22 UTC
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 13 Jared Camins-Esakov 2013-04-07 16:30:51 UTC
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.
Comment 14 Kyle M Hall 2013-04-15 14:45:47 UTC
> 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?
Comment 15 Marcel de Rooy 2013-08-22 08:28:10 UTC
(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.
Comment 16 Marc Véron 2015-12-20 17:14:58 UTC
Still valid?
Comment 17 Phil Ringnalda 2019-05-03 21:05:04 UTC
Not still valid, he fixed it in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12803#c14 instead.
Comment 18 Phil Ringnalda 2019-05-08 17:51:26 UTC

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