Bug 33200

Summary: IndependentBranchesTransfers does not prevent holds from creating transfers
Product: Koha Reporter: Caroline Cyr La Rose <caroline.cyr-la-rose>
Component: Hold requestsAssignee: Hammat wele <hammat.wele>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: caroline.cyr-la-rose, emily-rose.francoeur, gmcharlt, hammat.wele, kyle, lucas, m.de.rooy, philippe.blouin, sukhmandeep.benipal, william.lavoie
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 12599    
Attachments: Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm

Description Caroline Cyr La Rose 2023-03-10 19:35:37 UTC
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
Comment 1 Hammat wele 2023-03-15 14:20:15 UTC Comment hidden (obsolete)
Comment 2 Hammat wele 2023-03-21 16:55:58 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall (khall) 2023-03-27 14:33:51 UTC
* 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.
Comment 4 Caroline Cyr La Rose 2023-03-29 13:28:23 UTC
(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
Comment 5 Hammat wele 2023-03-29 13:44:03 UTC Comment hidden (obsolete)
Comment 6 Caroline Cyr La Rose 2023-03-29 13:47:47 UTC
(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
Comment 7 Caroline Cyr La Rose 2023-04-17 15:11:40 UTC
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.
Comment 8 Hammat wele 2023-05-12 11:58:16 UTC Comment hidden (obsolete)
Comment 9 Hammat wele 2023-05-12 11:58:19 UTC Comment hidden (obsolete)
Comment 10 Hammat wele 2023-05-17 16:04:20 UTC
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
Comment 11 Hammat wele 2023-05-17 16:04:23 UTC
Created attachment 151349 [details] [review]
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Comment 12 Marcel de Rooy 2023-07-21 07:20:59 UTC
+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.
Comment 13 Hammat wele 2023-08-17 12:15:30 UTC
Created attachment 154552 [details] [review]
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Comment 14 Hammat wele 2023-08-17 12:19:15 UTC
(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
Comment 15 Hammat wele 2023-09-11 12:22:24 UTC
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
Comment 16 Hammat wele 2023-09-11 12:22:27 UTC
Created attachment 155483 [details] [review]
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Comment 17 Hammat wele 2023-09-11 12:22:30 UTC
Created attachment 155484 [details] [review]
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Comment 18 Hammat wele 2023-09-11 12:23:04 UTC
Rebase
Comment 19 Emily-Rose Francoeur 2023-11-01 16:22:30 UTC
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
Comment 20 Emily-Rose Francoeur 2023-11-01 16:22:33 UTC
Created attachment 158189 [details] [review]
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve
Comment 21 Emily-Rose Francoeur 2023-11-01 16:22:37 UTC
Created attachment 158190 [details] [review]
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm
Comment 22 Emily-Rose Francoeur 2023-11-01 16:42:24 UTC
Rebased
Comment 23 Phil Ringnalda 2024-09-13 01:45:37 UTC
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>
Comment 24 Phil Ringnalda 2024-09-13 01:45:40 UTC
Created attachment 171435 [details] [review]
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 25 Phil Ringnalda 2024-09-13 01:45:43 UTC
Created attachment 171436 [details] [review]
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 26 Sukhmandeep 2024-10-11 20:36:56 UTC
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>
Comment 27 Sukhmandeep 2024-10-11 20:36:58 UTC
Created attachment 172708 [details] [review]
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 28 Sukhmandeep 2024-10-11 20:37:01 UTC
Created attachment 172709 [details] [review]
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 29 Sukhmandeep 2024-10-11 20:39:09 UTC
Rebased
Comment 30 Lucas Gass (lukeg) 2025-01-17 22:17:43 UTC
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)
Comment 31 William Lavoie 2025-02-11 16:08:45 UTC
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.