CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead.
Created attachment 18189 [details] [review] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead. Test Plan: 1) Create an item at Library A, place holds for it for patrons at Library B, Library C, and Library A in that order, for pickup at the patrons home library. 2) Make sure the holds policy for Library A is set to Hold Policy = "From home library" and Return Policy = "Item returns home". Make sure the holds policies for the other libraries are set to Hold Policy = "From any library". 3) Check the item in at Library C, the hold for the patron at Library B should pop up, even though it's in violation of the circulation rules. Don't click the confirm button! 4) Apply this patch, and reload the page, now the hold listed should be for the last hold, the hold for the patron at Library A, which is correct. This patch adds the subroutine C4::Reserves::GetReservesControlBranch as an equivilent to C4::Circulation::_GetCircControlBranch.
Created attachment 18190 [details] [review] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch Perltidy'ed the new sub.
Created attachment 18208 [details] [review] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead. Test Plan: 1) Create an item at Library A, place holds for it for patrons at Library B, Library C, and Library A in that order, for pickup at the patrons home library. 2) Make sure the holds policy for Library A is set to Hold Policy = "From home library" and Return Policy = "Item returns home". Make sure the holds policies for the other libraries are set to Hold Policy = "From any library". 3) Check the item in at Library C, the hold for the patron at Library B should pop up, even though it's in violation of the circulation rules. Don't click the confirm button! 4) Apply this patch, and reload the page, now the hold listed should be for the last hold, the hold for the patron at Library A, which is correct. This patch adds the subroutine C4::Reserves::GetReservesControlBranch as an equivilent to C4::Circulation::_GetCircControlBranch.
Kyle, The new routine GetReservesControlBranch needs a unit test. Marked as Failed QA.
Created attachment 18907 [details] [review] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead. Test Plan: 1) Create an item at Library A, place holds for it for patrons at Library B, Library C, and Library A in that order, for pickup at the patrons home library. 2) Make sure the holds policy for Library A is set to Hold Policy = "From home library" and Return Policy = "Item returns home". Make sure the holds policies for the other libraries are set to Hold Policy = "From any library". 3) Check the item in at Library C, the hold for the patron at Library B should pop up, even though it's in violation of the circulation rules. Don't click the confirm button! 4) Apply this patch, and reload the page, now the hold listed should be for the last hold, the hold for the patron at Library A, which is correct. This patch adds the subroutine C4::Reserves::GetReservesControlBranch as an equivilent to C4::Circulation::_GetCircControlBranch.
Good catch! Unit test added! (In reply to comment #4) > Kyle, > > The new routine GetReservesControlBranch needs a unit test. > > Marked as Failed QA.
Created attachment 18922 [details] [review] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead. Test Plan: 1) Create an item at Library A, place holds for it for patrons at Library B, Library C, and Library A in that order, for pickup at the patrons home library. 2) Make sure the holds policy for Library A is set to Hold Policy = "From home library" and Return Policy = "Item returns home". Make sure the holds policies for the other libraries are set to Hold Policy = "From any library". 3) Check the item in at Library C, the hold for the patron at Library B should pop up, even though it's in violation of the circulation rules. Don't click the confirm button! 4) Apply this patch, and reload the page, now the hold listed should be for the last hold, the hold for the patron at Library A, which is correct. This patch adds the subroutine C4::Reserves::GetReservesControlBranch as an equivilent to C4::Circulation::_GetCircControlBranch.
Last patch changes the expected number of tests (4 to 6).
Jonathan, did you mean to sign-off on this patch, or just revise the number of tests? Either way, thanks for fixing that! Kyle (In reply to comment #8) > Last patch changes the expected number of tests (4 to 6).
(In reply to comment #9) > Jonathan, did you mean to sign-off on this patch, or just revise the number > of tests? Either way, thanks for fixing that! No I didn't test the patch, just fix the unit tests.
Created attachment 19012 [details] [review] [SIGNED-OFF] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch
Thanks for the test plan, Kyle :) Tested OK. Unit tests pass. Signed off.
In fact it seems that it don't work. I retested it with Jonathan and at step 4, the hold returned is still the same (the one for patron at library B) But why should the hold for library A's patron be returned whereas it has the lowest priority ?
Created attachment 19081 [details] [review] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead. Test Plan: 1) Set ReservesControlBranch to "item's home library". 2) Create an item at Library A, place holds for it for patrons at Library B, Library C, and Library A in that order, for pickup at the patrons home library. 3) Make sure the holds policy for Library A is set to Hold Policy = "From home library" and Return Policy = "Item returns home". Make sure the holds policies for the other libraries are set to Hold Policy = "From any library". 4) Check the item in at Library C, the hold for the patron at Library B should pop up, even though it's in violation of the circulation rules. Don't click the confirm button! 5) Apply this patch, and reload the page, now the hold listed should be for the last hold, the hold for the patron at Library A, which is correct. This patch adds the subroutine C4::Reserves::GetReservesControlBranch as an equivilent to C4::Circulation::_GetCircControlBranch.
I forgot a step in the test plan! ReservesControlBranch needs to be set to "item's home library". (In reply to comment #13) > In fact it seems that it don't work. > I retested it with Jonathan and at step 4, the hold returned is still the > same (the one for patron at library B) > But why should the hold for library A's patron be returned whereas it has > the lowest priority ?
Created attachment 19435 [details] [review] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead. Test Plan: 1) Set ReservesControlBranch to "item's home library". 2) Create an item at Library A, place holds for it for patrons at Library B, Library C, and Library A in that order, for pickup at the patrons home library. 3) Make sure the holds policy for Library A is set to Hold Policy = "From home library" and Return Policy = "Item returns home". Make sure the holds policies for the other libraries are set to Hold Policy = "From any library". 4) Check the item in at Library C, the hold for the patron at Library B should pop up, even though it's in violation of the circulation rules. Don't click the confirm button! 5) Apply this patch, and reload the page, now the hold listed should be for the last hold, the hold for the patron at Library A, which is correct. This patch adds the subroutine C4::Reserves::GetReservesControlBranch as an equivilent to C4::Circulation::_GetCircControlBranch. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Kyle, can you please rebase and switch back to signed off?
Created attachment 20778 [details] [review] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead. Test Plan: 1) Set ReservesControlBranch to "item's home library". 2) Create an item at Library A, place holds for it for patrons at Library B, Library C, and Library A in that order, for pickup at the patrons home library. 3) Make sure the holds policy for Library A is set to Hold Policy = "From home library" and Return Policy = "Item returns home". Make sure the holds policies for the other libraries are set to Hold Policy = "From any library". 4) Check the item in at Library C, the hold for the patron at Library B should pop up, even though it's in violation of the circulation rules. Don't click the confirm button! 5) Apply this patch, and reload the page, now the hold listed should be for the last hold, the hold for the patron at Library A, which is correct. This patch adds the subroutine C4::Reserves::GetReservesControlBranch as an equivilent to C4::Circulation::_GetCircControlBranch. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 20864 [details] [review] [PASSED QA] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead. Test Plan: 1) Set ReservesControlBranch to "item's home library". 2) Create an item at Library A, place holds for it for patrons at Library B, Library C, and Library A in that order, for pickup at the patrons home library. 3) Make sure the holds policy for Library A is set to Hold Policy = "From home library" and Return Policy = "Item returns home". Make sure the holds policies for the other libraries are set to Hold Policy = "From any library". 4) Check the item in at Library C, the hold for the patron at Library B should pop up, even though it's in violation of the circulation rules. Don't click the confirm button! 5) Apply this patch, and reload the page, now the hold listed should be for the last hold, the hold for the patron at Library A, which is correct. This patch adds the subroutine C4::Reserves::GetReservesControlBranch as an equivilent to C4::Circulation::_GetCircControlBranch. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed POD so that arguments and explanation match (C<$item>). Also tested opac-reserves.pl for regressions. Passes all tests, QA script, and Reserves.t.
Pushed to master, along with a follow-up that adds a regression test. Thanks, Kyle! As a side-note, I care a /lot/ about there being regression tests, especially for changes that touch circulation and/or hold requests. While the unit test for the new routine was necessary, it wasn't sufficient.
This patch has been pushed to 3.12.x, will be in 3.12.6. Thanks Kyle!
Pushed to 3.10.x, will be in 3.10.12