Bug 27068 - HoldsQueue doesn't know how to use holds groups
Summary: HoldsQueue doesn't know how to use holds groups
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Agustín Moyano
QA Contact: Testopia
URL:
Keywords:
Depends on: 22284
Blocks: 28766 24359
  Show dependency tree / graph
 
Reported: 2020-11-20 17:32 UTC by Andrew Fuerste-Henry
Modified: 2022-06-06 20:25 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Koha 20.05 introduced local hold groups, but neglected to add support of them in the holds queue. Because of this, the holds queue will not show items the could have filled holds from other libraries in a hold group. This patch set adds support for hold groups to the holds queue builder thus improving Koha's ability to find items to fill hold requests.
Version(s) released in:
21.05.00,20.11.03,20.05.09
Circulation function:

Attachments
Bug 27068: Add hold group limits to Holds Queue builder (3.72 KB, patch)
2020-11-20 17:50 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Control hold group logic in HoldsQueue (8.75 KB, patch)
2020-12-02 23:37 UTC, Agustín Moyano 		Details | Diff | Splinter Review
Bug 27068: Control hold group logic in HoldsQueue (8.82 KB, patch)
2020-12-03 11:49 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Fix errors in _checkHoldPolicy (1.29 KB, patch)
2020-12-03 11:49 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Fetch libraries once for speed (1.66 KB, patch)
2020-12-03 11:49 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Fetch libraries once for speed (1.78 KB, patch)
2021-01-28 18:46 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Control hold group logic in HoldsQueue (8.85 KB, patch)
2021-01-29 12:49 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Fix errors in _checkHoldPolicy (2.02 KB, patch)
2021-01-29 12:49 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Fetch libraries once for speed (1.79 KB, patch)
2021-01-29 12:49 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Add unit tests (8.45 KB, patch)
2021-01-29 12:49 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Add unit tests (7.99 KB, patch)
2021-01-29 12:57 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Control hold group logic in HoldsQueue (8.90 KB, patch)
2021-02-09 17:03 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27068: Fix errors in _checkHoldPolicy (2.07 KB, patch)
2021-02-09 17:03 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27068: Fetch libraries once for speed (1.83 KB, patch)
2021-02-09 17:04 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27068: Add unit tests (8.04 KB, patch)
2021-02-09 17:04 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27068: Don't 'cache' Koha::Libraries (2.27 KB, patch)
2021-02-12 11:12 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 27068: Perltidy _checkHoldPolicy (2.58 KB, patch)
2021-02-12 11:12 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 27068: Don't 'cache' Koha::Libraries (2.34 KB, patch)
2021-02-12 11:25 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 27068: Perltidy _checkHoldPolicy (2.65 KB, patch)
2021-02-12 11:25 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
[20.05.x] Bug 27068: (follow-up) Remove local hold priority exclusion-related code for 20.05.x (2.39 KB, patch)
2021-02-23 18:31 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Show Obsolete (13) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-11-20 17:32:44 UTC 
Bug 22284 added local holds groups, but didn't teach the holds queue how to use them. It needs to do that.
Comment 1 Kyle M Hall (khall) 2020-11-20 17:50:22 UTC 
Created attachment 113900 [details] [review]
Bug 27068: Add hold group limits to Holds Queue builder
Comment 2 Kyle M Hall (khall) 2020-11-20 18:56:21 UTC 
My patch is incomplete. HoldsQueue needs to understand how to handle hold_fullfillment_policy for "holdgroup" and "patrongroup".
Comment 3 Tomás Cohen Arazi (tcohen) 2020-11-20 19:11:24 UTC 
It should be relying on $item->pickup_locations more heavily. Will take a look on monday with Agustin.
Comment 4 Agustín Moyano 2020-12-02 23:37:44 UTC 
Created attachment 114125 [details] [review]
Bug 27068: Control hold group logic in HoldsQueue
Comment 5 Agustín Moyano 2020-12-02 23:43:05 UTC 
Hello Kyle, could you please checkout this patch?

I've placed holds policy checking (including hold groups) inside a function called _checkHoldsPolicy, and call it whenever it's needed.
Comment 6 Kyle M Hall (khall) 2020-12-03 11:49:29 UTC 
Created attachment 114140 [details] [review]
Bug 27068: Control hold group logic in HoldsQueue

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall (khall) 2020-12-03 11:49:54 UTC 
Created attachment 114141 [details] [review]
Bug 27068: Fix errors in _checkHoldPolicy

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall (khall) 2020-12-03 11:49:57 UTC 
Created attachment 114142 [details] [review]
Bug 27068: Fetch libraries once for speed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Victor Grousset/tuxayo 2020-12-08 14:11:29 UTC 
What would be the test plan?

Here is a try:
1) In library groups add a root group and check it as hold group.
2) Add two libraries to the group
3) In circulation and fines rules, in 'Default checkout, hold and return policy', in Hold policy change the value to 'From local hold group'
4) Place a hold from a patron whose home library is from the group
5) Go to hold queue of the record
6) ???
Comment 10 Lisette Scheer 2021-01-05 18:28:19 UTC 
(In reply to Victor Grousset/tuxayo from comment #9)
> What would be the test plan?
> 
> Here is a try:
> 1) In library groups add a root group and check it as hold group.
> 2) Add two libraries to the group
> 3) In circulation and fines rules, in 'Default checkout, hold and return
> policy', in Hold policy change the value to 'From local hold group'
> 4) Place a hold from a patron whose home library is from the group

 5) Go to /cgi-bin/koha/circ/view_holdsqueue.pl
 6) Select the holding branch of the item with a hold
 7) observe no results
 8) Apply Patch
 9) Repeat 5-6
10) The item should come up on the holds queue results
11) Place a hold on an item where 1 record has 2 copies, 1 in the hold group, 1 not. 
12) Run the HoldQueue for the library not in the group and make sure the hold isn't showing. 
13) Turn on transportation cost matrix and set costs for the libraries within the group. 
14) Place a hold for a patron where multiple copies are on the bib.
15) Check both branch's hold queue for the item, it should only show on the lower cost branch's list if both copies are available. 

Agustin or Kyle could probably tell us if there are any steps missing in there. These are steps I wrote based on experience with the problem at hand and looking at the diff for the patches. 
Lisette
Comment 11 Victor Grousset/tuxayo 2021-01-07 22:51:22 UTC 
Thanks for completing the test plan :)

> 10) The item should come up on the holds queue results

Unfortunately it doesn't come up.

My patron's library is Centerville
The item's current and home library is Centerville
Centerville is in the group

This configuration should work right?
Comment 12 Katrin Fischer 2021-01-07 23:18:34 UTC 
(In reply to Victor Grousset/tuxayo from comment #11)
> Thanks for completing the test plan :)
> 
> > 10) The item should come up on the holds queue results
> 
> Unfortunately it doesn't come up.
> 
> My patron's library is Centerville
> The item's current and home library is Centerville
> Centerville is in the group
> 
> This configuration should work right?

Did you run the build_holds_queue script?
Comment 13 Nick Clemens (kidclamp) 2021-01-12 15:13:30 UTC 
Hi Augustin, this one needs unit tests for the changes
Comment 14 Kyle M Hall (khall) 2021-01-28 18:46:21 UTC 
Created attachment 115987 [details] [review]
Bug 27068: Fetch libraries once for speed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall (khall) 2021-01-29 12:49:19 UTC 
Created attachment 116022 [details] [review]
Bug 27068: Control hold group logic in HoldsQueue

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall (khall) 2021-01-29 12:49:31 UTC 
Created attachment 116023 [details] [review]
Bug 27068: Fix errors in _checkHoldPolicy

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall (khall) 2021-01-29 12:49:34 UTC 
Created attachment 116024 [details] [review]
Bug 27068: Fetch libraries once for speed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall (khall) 2021-01-29 12:49:38 UTC 
Created attachment 116025 [details] [review]
Bug 27068: Add unit tests
Comment 19 Kyle M Hall (khall) 2021-01-29 12:57:09 UTC 
Created attachment 116026 [details] [review]
Bug 27068: Add unit tests
Comment 20 Martin Renvoize (ashimema) 2021-02-09 17:03:55 UTC 
Created attachment 116607 [details] [review]
Bug 27068: Control hold group logic in HoldsQueue

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize (ashimema) 2021-02-09 17:03:59 UTC 
Created attachment 116608 [details] [review]
Bug 27068: Fix errors in _checkHoldPolicy

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize (ashimema) 2021-02-09 17:04:02 UTC 
Created attachment 116609 [details] [review]
Bug 27068: Fetch libraries once for speed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize (ashimema) 2021-02-09 17:04:06 UTC 
Created attachment 116610 [details] [review]
Bug 27068: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize (ashimema) 2021-02-09 17:04:59 UTC 
No regressions found, working as expected for me.

QA scripts passing and we've got unit tests and they pass.

Passing QA


Can we get some text for the release notes though pretty please ;)
Comment 25 Jonathan Druart 2021-02-12 11:12:48 UTC 
Created attachment 116814 [details] [review]
Bug 27068: Don't 'cache' Koha::Libraries
Comment 26 Jonathan Druart 2021-02-12 11:12:51 UTC 
Created attachment 116815 [details] [review]
Bug 27068: Perltidy _checkHoldPolicy
Comment 27 Jonathan Druart 2021-02-12 11:14:34 UTC 
The way we cache Koha::Libraries at package level is quite a poor design here.
I understand that we are doing it because _checkHoldPolicy is called in a loop, but we should maybe then redesign something else.

What do you think?
Comment 28 Kyle M Hall (khall) 2021-02-12 11:24:16 UTC 
(In reply to Jonathan Druart from comment #27)
> The way we cache Koha::Libraries at package level is quite a poor design
> here.
> I understand that we are doing it because _checkHoldPolicy is called in a
> loop, but we should maybe then redesign something else.
> 
> What do you think?

Undoing it is fine with me. This runs as a cronjob so speed isn't of the utmost concern. If we really wanted to, we could use Koha::Cache.
Comment 29 Kyle M Hall (khall) 2021-02-12 11:25:48 UTC 
Created attachment 116816 [details] [review]
Bug 27068: Don't 'cache' Koha::Libraries

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall (khall) 2021-02-12 11:25:57 UTC 
Created attachment 116817 [details] [review]
Bug 27068: Perltidy _checkHoldPolicy

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Jonathan Druart 2021-02-12 12:11:56 UTC 
Pushed to master for 21.05, thanks to everybody involved!
Comment 32 Fridolin Somers 2021-02-19 15:07:15 UTC 
Pushed to 20.11.x for 20.11.03
Comment 33 Andrew Fuerste-Henry 2021-02-23 12:35:31 UTC 
Pushed to 20.05.x for 20.05.09
Comment 34 Andrew Fuerste-Henry 2021-02-23 16:25:18 UTC 
Well drat. I've for test failures after backporting this to 20.05:
root@kohadevbox:koha(rmain2005)$ prove t/db_dependent/HoldsQueue.t
t/db_dependent/HoldsQueue.t .. 3/55 The method Koha::Item->exclude_from_local_holds_priority is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 780
Koha::Object::AUTOLOAD('Koha::Item=HASH(0x56534a141a70)') called at /kohadevbox/koha/C4/HoldsQueue.pm line 457
C4::HoldsQueue::MapItemsToHoldRequests('ARRAY(0x56534a155a30)', 'ARRAY(0x56534a11abd8)', 'ARRAY(0x56534a141008)', undef) called at /kohadevbox/koha/C4/HoldsQueue.pm line 212
C4::HoldsQueue::CreateQueue at t/db_dependent/HoldsQueue.t line 388
# Looks like your test exited with 11 just after 26.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 29/55 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 2816 Tests: 26 Failed: 0)
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 55 tests but ran 26.
Files=1, Tests=26,  3 wallclock secs ( 0.02 usr  0.01 sys +  1.76 cusr  0.36 csys =  2.15 CPU)
Result: FAIL
root@kohadevbox:koha(rmain2005)$
Comment 35 Victor Grousset/tuxayo 2021-02-23 17:16:58 UTC 
Missing dependencies for 19.11.x, it's not affected, no backport.
Comment 36 Tomás Cohen Arazi (tcohen) 2021-02-23 18:31:44 UTC 
Created attachment 117215 [details] [review]
[20.05.x] Bug 27068: (follow-up) Remove local hold priority exclusion-related code for 20.05.x

The local hold priority exclusions feature is only present in 20.11, so
the use case handling shouldn't be included in the backport. This code
removes the couple lines in which this is handled (trivial) and tested
(trivial, as it is disabled by default on the tests, we just remove the
places in which the original patch set the non-existent attributes).

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/HoldsQueue.t
=> FAIL: Tests fail!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>