Description
Caroline Cyr La Rose
2023-03-10 19:35:37 UTC
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> Rebased 1. Unit tests now fail with this applied: t/db_dependent/Reserves.t 2. FAIL t/db_dependent/Reserves.t FAIL forbidden patterns forbidden pattern: tab char (line 148) forbidden pattern: tab char (line 153) I tried using a KTD, it seems like t/db_dependent/Reserves.t deletes information it shouldn't. Before running the tests, select count(*) from letter; gave me 107 whereas after the test it was only 2. Additionally on the first run with the patch applied, 73 tests were successful (why are there 81 tests planned? Unless I am mistaken the latest patch adds 4 to the pre-existing 69 tests), whereas on the second run 70/73 were successful, and I got the error message # Failed test 'patron notified when item set to waiting' # at t/db_dependent/Reserves.t line 452. # got: '0' # expected: '1' # Failed test 'patron not notified a second time (bug 11445)' # at t/db_dependent/Reserves.t line 457. # got: '0' # expected: '1' No circulation HOLD_SLIP letter transported by email at /kohadevbox/koha/C4/Letters.pm line 602. # Failed test 'can successfully generate hold slip (bug 10949)' # at t/db_dependent/Reserves.t line 469. No reserves HOLD letter transported by print at /kohadevbox/koha/C4/Letters.pm line 602. |