Bug 33200 - IndependentBranchesTransfers does not prevent holds from creating transfers
Summary: IndependentBranchesTransfers does not prevent holds from creating transfers
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Hammat wele
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12599
  Show dependency treegraph
 
Reported: 2023-03-10 19:35 UTC by Caroline Cyr La Rose
Modified: 2023-11-01 16:42 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers (3.59 KB, patch)
2023-03-15 14:20 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers (3.63 KB, patch)
2023-03-21 16:55 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve (3.66 KB, patch)
2023-03-29 13:44 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers (3.63 KB, patch)
2023-05-12 11:58 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve (3.65 KB, patch)
2023-05-12 11:58 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers (3.66 KB, patch)
2023-05-17 16:04 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve (3.12 KB, patch)
2023-05-17 16:04 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm (1.10 KB, patch)
2023-08-17 12:15 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers (3.66 KB, patch)
2023-09-11 12:22 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve (3.12 KB, patch)
2023-09-11 12:22 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm (1.10 KB, patch)
2023-09-11 12:22 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 33200: IndependentBranchesTransfers does not prevent holds from creating transfers (3.66 KB, patch)
2023-11-01 16:22 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 33200: (follow-up) Adding Unit test, renaming _Removereserve (3.72 KB, patch)
2023-11-01 16:22 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 33200: (fix) remove SuperLibrarian check from Reserves.pm (1.11 KB, patch)
2023-11-01 16:22 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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