Description
Andrew Fuerste-Henry
2020-11-20 19:49:10 UTC
Confirmed this behavior in both master and 20.05. Holds from the OPAC can be placed correctly. Created attachment 113928 [details] [review] Bug 27071: Fix Hold pickup library when using hold groups Created attachment 113929 [details] [review] Bug 27071: Code simplification 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). Also, we should not see "pickupNotInHoldGroup" on the interface, the error code is not handled correctly to display a correct message to the user. (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? (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 :) 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 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. 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. Created attachment 116599 [details] [review] Bug 27071: Fix Hold pickup library when using hold groups Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 116600 [details] [review] Bug 27071: Code simplification Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> Created attachment 116603 [details] [review] Bug 27071: (follow-up) Provide a more meaningful error message Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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 Created attachment 117140 [details] [review] Bug 27071: Fix Hold pickup library when using hold groups Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 117141 [details] [review] Bug 27071: Code simplification Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> Created attachment 117144 [details] [review] Bug 27071: (follow-up) Provide a more meaningful error message Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> (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 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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 Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.04 Pushed to 20.05.x for 20.05.10 Missing dependencies for 19.11.x, not affected, no backport. |