Bug 27071

Summary: Hold pickup library match not enforced correctly on intranet when using hold groups
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: Hold requestsAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: critical    
Priority: P5 - low CC: david, fridolin.somers, gmcharlt, jonathan.druart, kyle, lisettepalouse+koha, martin.renvoize, nick, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27664
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
Bug Depends on: 22284    
Bug Blocks: 28541, 28118    
Attachments: Bug 27071: Fix Hold pickup library when using hold groups
Bug 27071: Code simplification
Bug 27071: More tests for Koha::Library->validate_hold_sibling
Bug 27071: Fix Hold pickup library when using hold groups
Bug 27071: Code simplification
Bug 27071: More tests for Koha::Library->validate_hold_sibling
Bug 27071: Use GetReservesControlBranch to pick the branch
Bug 27071: (follow-up) Provide a more meaningful error message
Bug 27071: Fix Hold pickup library when using hold groups
Bug 27071: Code simplification
Bug 27071: More tests for Koha::Library->validate_hold_sibling
Bug 27071: Use GetReservesControlBranch to pick the branch
Bug 27071: (follow-up) Provide a more meaningful error message
Bug 27071: (follow-up) Get rid of loop
Bug 27071: Fix Hold pickup library when using hold groups
Bug 27071: Code simplification
Bug 27071: More tests for Koha::Library->validate_hold_sibling
Bug 27071: Use GetReservesControlBranch to pick the branch
Bug 27071: (follow-up) Provide a more meaningful error message
Bug 27071: (follow-up) Get rid of loop
Bug 27071: Fix Hold pickup library when using hold groups
Bug 27071: Code simplification
Bug 27071: More tests for Koha::Library->validate_hold_sibling
Bug 27071: Use GetReservesControlBranch to pick the branch
Bug 27071: (follow-up) Provide a more meaningful error message
Bug 27071: (follow-up) Get rid of loop

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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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.