Bug 27071 - Hold pickup library match not enforced correctly on intranet when using hold groups
Summary: Hold pickup library match not enforced correctly on intranet when using hold ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 22284
Blocks: 28541 28118
  Show dependency tree / graph
 
Reported: 2020-11-20 19:49 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:
When using library groups, the rules for placing holds did not always work as expected. This fixes these rules so that when patrons are part of a library in a group, they can only place a hold for items held in that library group. It also improves the error messages. Example: - There are two library groups with distinct libraries in each (Group A and B). - Default rules for all libraries are: Hold Policy = "From local hold group" and Hold pickup library match to "Patron's hold group", AllowHoldPolicyOverride is Don't allow. - You can place a hold for a patron that belongs to one of the Group A libraries, for an item only held in a Group A library. - You can't place a hold for that item for a patron from a Group B library.
Version(s) released in:
21.05.00,20.11.04,20.05.10
Circulation function:

Attachments
Bug 27071: Fix Hold pickup library when using hold groups (912 bytes, patch)
2020-11-23 14:53 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 27071: Code simplification (2.35 KB, patch)
2020-11-23 14:53 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 27071: More tests for Koha::Library->validate_hold_sibling (4.50 KB, patch)
2020-11-24 17:33 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: Fix Hold pickup library when using hold groups (969 bytes, patch)
2021-02-09 15:36 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: Code simplification (2.41 KB, patch)
2021-02-09 15:36 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: More tests for Koha::Library->validate_hold_sibling (4.55 KB, patch)
2021-02-09 15:36 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: Use GetReservesControlBranch to pick the branch (3.07 KB, patch)
2021-02-09 15:36 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: (follow-up) Provide a more meaningful error message (2.21 KB, patch)
2021-02-09 15:36 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: Fix Hold pickup library when using hold groups (969 bytes, patch)
2021-02-22 12:33 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: Code simplification (2.37 KB, patch)
2021-02-22 12:34 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: More tests for Koha::Library->validate_hold_sibling (4.55 KB, patch)
2021-02-22 12:34 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: Use GetReservesControlBranch to pick the branch (3.07 KB, patch)
2021-02-22 12:34 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: (follow-up) Provide a more meaningful error message (2.21 KB, patch)
2021-02-22 12:35 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: (follow-up) Get rid of loop (1.28 KB, patch)
2021-02-22 12:35 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 27071: Fix Hold pickup library when using hold groups (1016 bytes, patch)
2021-02-23 10:32 UTC, David Nind 		Details | Diff | Splinter Review
Bug 27071: Code simplification (2.42 KB, patch)
2021-02-23 10:33 UTC, David Nind 		Details | Diff | Splinter Review
Bug 27071: More tests for Koha::Library->validate_hold_sibling (4.61 KB, patch)
2021-02-23 10:33 UTC, David Nind 		Details | Diff | Splinter Review
Bug 27071: Use GetReservesControlBranch to pick the branch (3.11 KB, patch)
2021-02-23 10:33 UTC, David Nind 		Details | Diff | Splinter Review
Bug 27071: (follow-up) Provide a more meaningful error message (2.25 KB, patch)
2021-02-23 10:33 UTC, David Nind 		Details | Diff | Splinter Review
Bug 27071: (follow-up) Get rid of loop (1.33 KB, patch)
2021-02-23 10:33 UTC, David Nind 		Details | Diff | Splinter Review
Bug 27071: Fix Hold pickup library when using hold groups (1.05 KB, patch)
2021-02-23 13:40 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27071: Code simplification (2.48 KB, patch)
2021-02-23 13:41 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27071: More tests for Koha::Library->validate_hold_sibling (4.66 KB, patch)
2021-02-23 13:41 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27071: Use GetReservesControlBranch to pick the branch (3.18 KB, patch)
2021-02-23 13:41 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27071: (follow-up) Provide a more meaningful error message (2.32 KB, patch)
2021-02-23 13:41 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 27071: (follow-up) Get rid of loop (1.39 KB, patch)
2021-02-23 13:41 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Show Obsolete (20) 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 19:49:10 UTC 
To recreate:
- Create two local hold groups containing distinct lists of libraries.
- In default rules for all libraries, set Hold Policy = "From local hold group" and Hold pickup library match to "Patron's hold group"
- Make sure AllowHoldPolicyOverride is set to Don't Allow

- Make sure you're logged in at a library in Group 1
- Find a bib with only 1 item from Group 1. Confirm you can place a hold on this title for a patron in Group 1 (correct), but not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), BUT you also cannot place a hold for a patron from Group 2 (incorrect) -- Koha gives the erroneous message "pickupNotInHoldGroup"

- Change your library to a something in Group 2
- Find a bib with only 1 item from Group 1. Confirm you cannot place a hold on this title for a patron in Group 1 (incorrect), and not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), but you can place a hold for a patron from Group 2 (correct)
Comment 1 Andrew Fuerste-Henry 2020-11-20 19:50:16 UTC 
Confirmed this behavior in both master and 20.05.
Holds from the OPAC can be placed correctly.
Comment 2 Jonathan Druart 2020-11-23 14:53:14 UTC 
Created attachment 113928 [details] [review]
Bug 27071: Fix Hold pickup library when using hold groups
Comment 3 Jonathan Druart 2020-11-23 14:53:18 UTC 
Created attachment 113929 [details] [review]
Bug 27071: Code simplification
Comment 4 Jonathan Druart 2020-11-23 14:55:11 UTC 
In my understanding Koha::Library->validate_hold_sibling is plain wrong. It only tests for the first from @hold_libraries is not the one we are looking for, then it's considered an invalid one.

However with this patch we have tests from t/db_dependent/Holds.t that are failing (maybe others as well).
Comment 5 Jonathan Druart 2020-11-23 14:55:46 UTC 
Also, we should not see "pickupNotInHoldGroup" on the interface, the error code is not handled correctly to display a correct message to the user.
Comment 6 Tomás Cohen Arazi (tcohen) 2020-11-24 12:22:05 UTC 
(In reply to Jonathan Druart from comment #3)
> Created attachment 113929 [details] [review] [review]
> Bug 27071: Code simplification

I've rather use List::MoreUtils::any, right?
Comment 7 Jonathan Druart 2020-11-24 12:45:24 UTC 
(In reply to Tomás Cohen Arazi from comment #6)
> (In reply to Jonathan Druart from comment #3)
> > Created attachment 113929 [details] [review] [review] [review]
> > Bug 27071: Code simplification
> 
> I've rather use List::MoreUtils::any, right?

Well, if you want. But we don't need a module for a grep :)
Comment 8 Tomás Cohen Arazi (tcohen) 2020-11-24 17:33:06 UTC 
Created attachment 113963 [details] [review]
Bug 27071: More tests for Koha::Library->validate_hold_sibling

This patch adds tests for more than one non-same sibling libraries in
the hold group. Tests pass which means it is working as expected.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Libraries.t
=> SUCCESS: Tests pass
Comment 9 Tomás Cohen Arazi (tcohen) 2020-11-24 17:37:08 UTC 
I wrote some tests trying to capture the failing situation. I didn't manage to make them fail, though.

The fact that this works correctly in OPAC, makes me think of controller issues. I will try following Andrew's test plan now.
Comment 10 Jonathan Druart 2020-11-25 14:12:51 UTC 
The problem is indeed in the controller:
reserve/request.pl
 554                 my $can_item_be_reserved = CanItemBeReserved( $patron->borrowernumber, $itemnumber, $pickup )->{status};

$pickup is coming from:
 70 my $pickup = $input->param('pickup') || C4::Context->userenv->{'branch'};

So we are testing if the hold can be picked up at the logged in library of the librarian.
Comment 11 Tomás Cohen Arazi (tcohen) 2021-02-09 15:36:00 UTC 
Created attachment 116599 [details] [review]
Bug 27071: Fix Hold pickup library when using hold groups

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi (tcohen) 2021-02-09 15:36:05 UTC 
Created attachment 116600 [details] [review]
Bug 27071: Code simplification

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi (tcohen) 2021-02-09 15:36:09 UTC 
Created attachment 116601 [details] [review]
Bug 27071: More tests for Koha::Library->validate_hold_sibling

This patch adds tests for more than one non-same sibling libraries in
the hold group. Tests pass which means it is working as expected.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Libraries.t
=> SUCCESS: Tests pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi (tcohen) 2021-02-09 15:36:14 UTC 
Created attachment 116602 [details] [review]
Bug 27071: Use GetReservesControlBranch to pick the branch

Pretty much like the opac-reserve.pl does, this patch makes the staff
request.pl script get the branch from the specialized routine from
C4::Reserves instead of falling back to userenv.

To test:
1. Follow the original test plan:
- Create two local hold groups containing distinct lists of libraries.
- In default rules for all libraries, set Hold Policy = "From local hold group" and Hold pickup library match to "Patron's hold group"
- Make sure AllowHoldPolicyOverride is set to Don't Allow

- Make sure you're logged in at a library in Group 1
- Find a bib with only 1 item from Group 1. Confirm you can place a hold on this title for a patron in Group 1 (correct), but not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), BUT you also cannot place a hold for a patron from Group 2 (incorrect) -- Koha gives the erroneous message "pickupNotInHoldGroup"

- Change your library to a something in Group 2
- Find a bib with only 1 item from Group 1. Confirm you cannot place a hold on this title for a patron in Group 1 (incorrect), and not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), but you can place a hold for a patron from Group 2 (correct)
=> FAIL: Things expected to fail on the plan, fail.
2. Apply this patches
3. Repeat 1
=> SUCCESS: It now works!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi (tcohen) 2021-02-09 15:36:21 UTC 
Created attachment 116603 [details] [review]
Bug 27071: (follow-up) Provide a more meaningful error message

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 David Nind 2021-02-21 10:25:13 UTC 
Patch no longer applies 8-(

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 27071: Fix Hold pickup library when using hold groups
Applying: Bug 27071: Code simplification
error: sha1 information is lacking or useless (Koha/Library.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 27071: Code simplification
Comment 17 Tomás Cohen Arazi (tcohen) 2021-02-22 12:33:56 UTC 
Created attachment 117140 [details] [review]
Bug 27071: Fix Hold pickup library when using hold groups

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi (tcohen) 2021-02-22 12:34:10 UTC 
Created attachment 117141 [details] [review]
Bug 27071: Code simplification

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi (tcohen) 2021-02-22 12:34:29 UTC 
Created attachment 117142 [details] [review]
Bug 27071: More tests for Koha::Library->validate_hold_sibling

This patch adds tests for more than one non-same sibling libraries in
the hold group. Tests pass which means it is working as expected.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Libraries.t
=> SUCCESS: Tests pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi (tcohen) 2021-02-22 12:34:51 UTC 
Created attachment 117143 [details] [review]
Bug 27071: Use GetReservesControlBranch to pick the branch

Pretty much like the opac-reserve.pl does, this patch makes the staff
request.pl script get the branch from the specialized routine from
C4::Reserves instead of falling back to userenv.

To test:
1. Follow the original test plan:
- Create two local hold groups containing distinct lists of libraries.
- In default rules for all libraries, set Hold Policy = "From local hold group" and Hold pickup library match to "Patron's hold group"
- Make sure AllowHoldPolicyOverride is set to Don't Allow

- Make sure you're logged in at a library in Group 1
- Find a bib with only 1 item from Group 1. Confirm you can place a hold on this title for a patron in Group 1 (correct), but not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), BUT you also cannot place a hold for a patron from Group 2 (incorrect) -- Koha gives the erroneous message "pickupNotInHoldGroup"

- Change your library to a something in Group 2
- Find a bib with only 1 item from Group 1. Confirm you cannot place a hold on this title for a patron in Group 1 (incorrect), and not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), but you can place a hold for a patron from Group 2 (correct)
=> FAIL: Things expected to fail on the plan, fail.
2. Apply this patches
3. Repeat 1
=> SUCCESS: It now works!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi (tcohen) 2021-02-22 12:35:08 UTC 
Created attachment 117144 [details] [review]
Bug 27071: (follow-up) Provide a more meaningful error message

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi (tcohen) 2021-02-22 12:35:31 UTC 
Created attachment 117145 [details] [review]
Bug 27071: (follow-up) Get rid of loop

This patch gets rid of a loop on the librarary's groups siblings, and
acknowledges ->get_hold_libraries returns a proper Koha::Libraries
iterator that can be chained with a new query on the passed branchcode.
This way we only fetch  a count from the DB and no loop needs to take
place on the code.

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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi (tcohen) 2021-02-22 12:40:23 UTC 
(In reply to Jonathan Druart from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > (In reply to Jonathan Druart from comment #3)
> > > Created attachment 113929 [details] [review] [review] [review] [review]
> > > Bug 27071: Code simplification
> > 
> > I've rather use List::MoreUtils::any, right?
> 
> Well, if you want. But we don't need a module for a grep :)

I wen't further and completely removed grep :-D
Comment 24 David Nind 2021-02-23 10:32:56 UTC 
Created attachment 117184 [details] [review]
Bug 27071: Fix Hold pickup library when using hold groups

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2021-02-23 10:33:02 UTC 
Created attachment 117185 [details] [review]
Bug 27071: Code simplification

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2021-02-23 10:33:35 UTC 
Created attachment 117186 [details] [review]
Bug 27071: More tests for Koha::Library->validate_hold_sibling

This patch adds tests for more than one non-same sibling libraries in
the hold group. Tests pass which means it is working as expected.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Libraries.t
=> SUCCESS: Tests pass

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2021-02-23 10:33:43 UTC 
Created attachment 117187 [details] [review]
Bug 27071: Use GetReservesControlBranch to pick the branch

Pretty much like the opac-reserve.pl does, this patch makes the staff
request.pl script get the branch from the specialized routine from
C4::Reserves instead of falling back to userenv.

To test:
1. Follow the original test plan:
- Create two local hold groups containing distinct lists of libraries.
- In default rules for all libraries, set Hold Policy = "From local hold group" and Hold pickup library match to "Patron's hold group"
- Make sure AllowHoldPolicyOverride is set to Don't Allow

- Make sure you're logged in at a library in Group 1
- Find a bib with only 1 item from Group 1. Confirm you can place a hold on this title for a patron in Group 1 (correct), but not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), BUT you also cannot place a hold for a patron from Group 2 (incorrect) -- Koha gives the erroneous message "pickupNotInHoldGroup"

- Change your library to a something in Group 2
- Find a bib with only 1 item from Group 1. Confirm you cannot place a hold on this title for a patron in Group 1 (incorrect), and not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), but you can place a hold for a patron from Group 2 (correct)
=> FAIL: Things expected to fail on the plan, fail.
2. Apply this patches
3. Repeat 1
=> SUCCESS: It now works!

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 David Nind 2021-02-23 10:33:48 UTC 
Created attachment 117188 [details] [review]
Bug 27071: (follow-up) Provide a more meaningful error message

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 29 David Nind 2021-02-23 10:33:52 UTC 
Created attachment 117189 [details] [review]
Bug 27071: (follow-up) Get rid of loop

This patch gets rid of a loop on the librarary's groups siblings, and
acknowledges ->get_hold_libraries returns a proper Koha::Libraries
iterator that can be chained with a new query on the passed branchcode.
This way we only fetch  a count from the DB and no loop needs to take
place on the code.

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

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 30 Martin Renvoize (ashimema) 2021-02-23 13:40:58 UTC 
Created attachment 117204 [details] [review]
Bug 27071: Fix Hold pickup library when using hold groups

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize (ashimema) 2021-02-23 13:41:01 UTC 
Created attachment 117205 [details] [review]
Bug 27071: Code simplification

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize (ashimema) 2021-02-23 13:41:05 UTC 
Created attachment 117206 [details] [review]
Bug 27071: More tests for Koha::Library->validate_hold_sibling

This patch adds tests for more than one non-same sibling libraries in
the hold group. Tests pass which means it is working as expected.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Libraries.t
=> SUCCESS: Tests pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize (ashimema) 2021-02-23 13:41:08 UTC 
Created attachment 117207 [details] [review]
Bug 27071: Use GetReservesControlBranch to pick the branch

Pretty much like the opac-reserve.pl does, this patch makes the staff
request.pl script get the branch from the specialized routine from
C4::Reserves instead of falling back to userenv.

To test:
1. Follow the original test plan:
- Create two local hold groups containing distinct lists of libraries.
- In default rules for all libraries, set Hold Policy = "From local hold group" and Hold pickup library match to "Patron's hold group"
- Make sure AllowHoldPolicyOverride is set to Don't Allow

- Make sure you're logged in at a library in Group 1
- Find a bib with only 1 item from Group 1. Confirm you can place a hold on this title for a patron in Group 1 (correct), but not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), BUT you also cannot place a hold for a patron from Group 2 (incorrect) -- Koha gives the erroneous message "pickupNotInHoldGroup"

- Change your library to a something in Group 2
- Find a bib with only 1 item from Group 1. Confirm you cannot place a hold on this title for a patron in Group 1 (incorrect), and not for a patron in Group 2 (correct).
- Find a bib with only 1 item from Group 2. Confirm you cannot place a hold for a patron from Group 1 (correct), but you can place a hold for a patron from Group 2 (correct)
=> FAIL: Things expected to fail on the plan, fail.
2. Apply this patches
3. Repeat 1
=> SUCCESS: It now works!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize (ashimema) 2021-02-23 13:41:12 UTC 
Created attachment 117208 [details] [review]
Bug 27071: (follow-up) Provide a more meaningful error message

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize (ashimema) 2021-02-23 13:41:15 UTC 
Created attachment 117209 [details] [review]
Bug 27071: (follow-up) Get rid of loop

This patch gets rid of a loop on the librarary's groups siblings, and
acknowledges ->get_hold_libraries returns a proper Koha::Libraries
iterator that can be chained with a new query on the passed branchcode.
This way we only fetch  a count from the DB and no loop needs to take
place on the code.

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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize (ashimema) 2021-02-23 13:42:05 UTC 
I like how this was broken down into nice manageable chunks :)

Works as expected, passes QA scripts, includes tests and passes all tests..


Passing QA
Comment 37 Jonathan Druart 2021-03-01 10:29:00 UTC 
Pushed to master for 21.05, thanks to everybody involved!
Comment 38 Fridolin Somers 2021-03-02 07:09:08 UTC 
Pushed to 20.11.x for 20.11.04
Comment 39 Andrew Fuerste-Henry 2021-03-05 18:09:15 UTC 
Pushed to 20.05.x for 20.05.10
Comment 40 Victor Grousset/tuxayo 2021-03-22 15:23:06 UTC 
Missing dependencies for 19.11.x, not affected, no backport.