I noticed that when you have IndependentBranches (Yes), IndependentBranchesTransfers (Yes), and canreservefromotherbranches (Don't allow), holds can still trigger transfers. This is probably larger than transfers as holds should not be placed on items from other libraries with IndependentBranches, even if we choose "next available item". IndependentBranches/canreservefromotherbranches should imply "next available item (from this branch)". To reproduce, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click "Place hold" 3. Place a hold for a user, without changing anything (should be for "next available item") 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Optionally, you can log out and log back in as staff from library A 7. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there
Created attachment 148212 [details] [review] Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers This patch don't concider hold made from other branches when IndependentBranchesTransfers is set to Yes and the connected user is not a SuperLibrarian To test, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click 'Place hold' 3. Place a hold for a user, without changing anything (should be for 'next available item') 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Click on 'Confirm hold and transfer' 7. Optionally, you can log out and log back in as staff from library A 8. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there 9. Apply the patch 10. Create an other record with two items, one at each library, with barcodes, check out the one from library A 11. repeat step 1, 2, 3, 4, 5 --> The 'Hold found' popup is not shown, because there is no hold for this item in the user branch (library B) 12. repeat step 7,8 --> Item from other branch is not there
Created attachment 148484 [details] [review] Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers This patch don't concider hold made from other branches when IndependentBranchesTransfers is set to Yes and the connected user is not a SuperLibrarian To test, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click 'Place hold' 3. Place a hold for a user, without changing anything (should be for 'next available item') 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Click on 'Confirm hold and transfer' 7. Optionally, you can log out and log back in as staff from library A 8. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there 9. Apply the patch 10. Create an other record with two items, one at each library, with barcodes, check out the one from library A 11. repeat step 1, 2, 3, 4, 5 --> The 'Hold found' popup is not shown, because there is no hold for this item in the user branch (library B) 12. repeat step 7,8 --> Item from other branch is not there
* Needs unit tests * _Removereserve doesn't follow current naming conventions and is superfluous. Please use something like _FilterHoldsForIndependentBranches * Should this have a syspref? Not sure if this is a straight-forward bug, or if for some libraries this would be a "feature" and not a bug.
(In reply to Kyle M Hall from comment #3) > * Should this have a syspref? Not sure if this is a straight-forward bug, or > if for some libraries this would be a "feature" and not a bug. Hi Kyle, I'm not sure how this could be considered a feature when IndependentBranchesTransfers is set to "Yes" (prevent transfers) and canreservefromotherbranches is set to "Don't allow" (don't allow to place holds on items from other libraries). There are already two sysprefs to control this. Could you explain more about the behaviour you're thinking of? Thanks! Caroline
Created attachment 148918 [details] [review] Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
(In reply to Kyle M Hall from comment #3) > * Needs unit tests > * _Removereserve doesn't follow current naming conventions and is > superfluous. Please use something like _FilterHoldsForIndependentBranches > * Should this have a syspref? Not sure if this is a straight-forward bug, or > if for some libraries this would be a "feature" and not a bug. Hammat added a followup for the two first points (tests and renaming), but I told him to wait for an answer to my comment regarding the 3rd point. Thanks! Caroline
As I didn't receive an answer to my comment #4, I'm putting this back in Needs Signoff. Like I said, the two other points were addressed in the latest patches by Hammat.
Created attachment 151129 [details] [review] Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers This patch don't concider hold made from other branches when IndependentBranchesTransfers is set to Yes and the connected user is not a SuperLibrarian To test, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click 'Place hold' 3. Place a hold for a user, without changing anything (should be for 'next available item') 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Click on 'Confirm hold and transfer' 7. Optionally, you can log out and log back in as staff from library A 8. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there 9. Apply the patch 10. Create an other record with two items, one at each library, with barcodes, check out the one from library A 11. repeat step 1, 2, 3, 4, 5 --> The 'Hold found' popup is not shown, because there is no hold for this item in the user branch (library B) 12. repeat step 7,8 --> Item from other branch is not there
Created attachment 151130 [details] [review] Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Created attachment 151348 [details] [review] Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers This patch don't concider hold made from other branches when IndependentBranchesTransfers is set to Yes and the connected user is not a SuperLibrarian To test, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click 'Place hold' 3. Place a hold for a user, without changing anything (should be for 'next available item') 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Click on 'Confirm hold and transfer' 7. Optionally, you can log out and log back in as staff from library A 8. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there 9. Apply the patch 10. Create an other record with two items, one at each library, with barcodes, check out the one from library A 11. repeat step 1, 2, 3, 4, 5 --> The 'Hold found' popup is not shown, because there is no hold for this item in the user branch (library B) 12. repeat step 7,8 --> Item from other branch is not there
Created attachment 151349 [details] [review] Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
+sub _Removereserve { + my ( @reserves) = @_; + if ( C4::Context->preference("IndependentBranchesTransfers") && !C4::Context->IsSuperLibrarian() && scalar @reserves) { The test for superlibrarian is unusual. At first sight it should not be there. Please clarify.
Created attachment 154552 [details] [review] Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
(In reply to Marcel de Rooy from comment #12) > +sub _Removereserve { > + my ( @reserves) = @_; > + if ( C4::Context->preference("IndependentBranchesTransfers") && > !C4::Context->IsSuperLibrarian() && scalar @reserves) { > > The test for superlibrarian is unusual. At first sight it should not be > there. > Please clarify. The test for superlibrarian is made because the IndependentBranchesTransfers syspref is suppose to prevent staff from transfering but not superlibrarians
Created attachment 155482 [details] [review] Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers This patch don't concider hold made from other branches when IndependentBranchesTransfers is set to Yes and the connected user is not a SuperLibrarian To test, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click 'Place hold' 3. Place a hold for a user, without changing anything (should be for 'next available item') 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Click on 'Confirm hold and transfer' 7. Optionally, you can log out and log back in as staff from library A 8. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there 9. Apply the patch 10. Create an other record with two items, one at each library, with barcodes, check out the one from library A 11. repeat step 1, 2, 3, 4, 5 --> The 'Hold found' popup is not shown, because there is no hold for this item in the user branch (library B) 12. repeat step 7,8 --> Item from other branch is not there
Created attachment 155483 [details] [review] Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Created attachment 155484 [details] [review] Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Rebase
Created attachment 158188 [details] [review] Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers This patch don't concider hold made from other branches when IndependentBranchesTransfers is set to Yes and the connected user is not a SuperLibrarian To test, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click 'Place hold' 3. Place a hold for a user, without changing anything (should be for 'next available item') 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Click on 'Confirm hold and transfer' 7. Optionally, you can log out and log back in as staff from library A 8. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there 9. Apply the patch 10. Create an other record with two items, one at each library, with barcodes, check out the one from library A 11. repeat step 1, 2, 3, 4, 5 --> The 'Hold found' popup is not shown, because there is no hold for this item in the user branch (library B) 12. repeat step 7,8 --> Item from other branch is not there
Created attachment 158189 [details] [review] Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Created attachment 158190 [details] [review] Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Rebased
Created attachment 171434 [details] [review] Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers This patch don't concider hold made from other branches when IndependentBranchesTransfers is set to Yes and the connected user is not a SuperLibrarian To test, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click 'Place hold' 3. Place a hold for a user, without changing anything (should be for 'next available item') 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Click on 'Confirm hold and transfer' 7. Optionally, you can log out and log back in as staff from library A 8. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there 9. Apply the patch 10. Create an other record with two items, one at each library, with barcodes, check out the one from library A 11. repeat step 1, 2, 3, 4, 5 --> The 'Hold found' popup is not shown, because there is no hold for this item in the user branch (library B) 12. repeat step 7,8 --> Item from other branch is not there Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Created attachment 171435 [details] [review] Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Created attachment 171436 [details] [review] Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Created attachment 172707 [details] [review] Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers This patch don't concider hold made from other branches when IndependentBranchesTransfers is set to Yes and the connected user is not a SuperLibrarian To test, you must have - IndependentBranches, IndependentBranchesPatronModifications and IndependentBranchesTransfers preferences set to Yes - canreservefromotherbranches must be set to Don't allow - Circulation rules that allow holds and checkouts - Two libraries (I used Midway (A) and Centerville (B)) - A record with two items, one at each library, with barcodes, check out the one from library A - Two non-superlibrarian staff user, one in each library, with at least catalogue and holds permissions, this is what I gave them - circulate (all) - catalogue - delete_borrowers - edit_borrowers - reserveforothers (all) 1. Log in to the staff interface with the non-superlibrarian staff user from library A 2. Go to the record with the two items, click 'Place hold' 3. Place a hold for a user, without changing anything (should be for 'next available item') 4. Log out and log back in with the non-superlibrarian staff user from library B 5. Check in the item from library B, which should be available --> Hold is confirmed for patron from library A, transfer is created 6. Click on 'Confirm hold and transfer' 7. Optionally, you can log out and log back in as staff from library A 8. Go to Circulation > Transfers to receive (should this even be available with IndependentBranchesTransfers turned on?) --> Item from other branch is there 9. Apply the patch 10. Create an other record with two items, one at each library, with barcodes, check out the one from library A 11. repeat step 1, 2, 3, 4, 5 --> The 'Hold found' popup is not shown, because there is no hold for this item in the user branch (library B) 12. repeat step 7,8 --> Item from other branch is not there Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Created attachment 172708 [details] [review] Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Created attachment 172709 [details] [review] Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>