Bug 33074

Summary: ReservesControlBranch not taken into account in opac-reserve.pl
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Hold requestsAssignee: Julian Maurice <julian.maurice>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, jzairo, m.de.rooy, matt.blenkinsop, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.05
Bug Depends on: 21263, 30825    
Bug Blocks:    
Attachments: Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl
Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl
Bug 33074: (QA follow-up) tidy code
Bug 33074: Use Koha::Policy::Holds->holds_control_library
Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl

Description Julian Maurice 2023-02-27 09:04:08 UTC
In opac-reserve.pl there is this line:

  my $rule = GetBranchItemRule( $patron->branchcode, $type );

$patron->branchcode should be replaced by the result of GetReservesControlBranch

Test plan:
1. Set system preferences:
   - Disable OPACAllowUserToChooseBranch
   - Set ReservesControlBranch to "item's home library"
   - Enable UseBranchTransferLimits
   - Set BranchTransferLimitsType to "item type"
   - Enable canreservefromotherbranches
   - Disable IndependantBranches
2. Create two libraries: ITEM_LIB and PATRON_LIB
3. Set circulation rules:
   - Remove all circulation rules (DELETE FROM circulation_rules)
   - Set a default rule that allow some holds and with "OPAC item level holds"
     = "force". Allow "on shelf" holds for easier testing.
   - In the rules for ITEM_LIB, under "Default checkout, hold and return
     policy",
     - set "Hold policy" to "From any library"
     - set "Hold pickup library match" to "item's home library"
   - In the rules for PATRON_LIB, under "Default checkout, hold and return policy",
     - set "Hold policy" to "From any library",
     - set "Hold pickup library match" to "any library"
4. In "Library transfer limits" disable all transfers from ITEM_LIB to
   PATRON_LIB for an item type (let's say "BOOK")
5. Create a biblio with an item:
   - item type must be the same as in previous step ("BOOK")
   - home branch and holding branch must be ITEM_LIB
6. Create a patron at PATRON_LIB, give it a password so it can log in.
7. Go to OPAC, and login with this patron
8. Try to place a hold for the new item. You should be allowed to place a hold,
   but when doing it, no holds will be created.
Comment 1 Julian Maurice 2023-02-27 09:08:19 UTC
Created attachment 147426 [details] [review]
Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl

Test plan:
1. Set system preferences:
   - Disable OPACAllowUserToChooseBranch
   - Set ReservesControlBranch to "item's home library"
   - Enable UseBranchTransferLimits
   - Set BranchTransferLimitsType to "item type"
   - Enable canreservefromotherbranches
   - Disable IndependantBranches
2. Create two libraries: ITEM_LIB and PATRON_LIB
3. Set circulation rules:
   - Remove all circulation rules (DELETE FROM circulation_rules)
   - Set a default rule that allow some holds and with "OPAC item level
     holds" = "force". Allow "on shelf" holds for easier testing.
   - In the rules for ITEM_LIB, under "Default checkout, hold and return
     policy",
     - set "Hold policy" to "From any library"
     - set "Hold pickup library match" to "item's home library"
   - In the rules for PATRON_LIB, under "Default checkout, hold and
     return policy",
     - set "Hold policy" to "From any library",
     - set "Hold pickup library match" to "any library"
4. In "Library transfer limits" disable all transfers from ITEM_LIB to
   PATRON_LIB for an item type (let's say "BOOK")
5. Create a biblio with an item:
   - item type must be the same as in previous step ("BOOK")
   - home branch and holding branch must be ITEM_LIB
6. Create a patron at PATRON_LIB, give it a password so it can log in.
7. Go to OPAC, and login with this patron
8. Try to place a hold for the new item. You should be allowed to place
   a hold, but when doing it, no holds will be created.
9. Apply patch, restart Koha
10. Try to place a hold for the new item. This time the hold should be
    created
Comment 2 ByWater Sandboxes 2023-08-17 11:27:43 UTC
Created attachment 154543 [details] [review]
Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl

Test plan:
1. Set system preferences:
   - Disable OPACAllowUserToChooseBranch
   - Set ReservesControlBranch to "item's home library"
   - Enable UseBranchTransferLimits
   - Set BranchTransferLimitsType to "item type"
   - Enable canreservefromotherbranches
   - Disable IndependantBranches
2. Create two libraries: ITEM_LIB and PATRON_LIB
3. Set circulation rules:
   - Remove all circulation rules (DELETE FROM circulation_rules)
   - Set a default rule that allow some holds and with "OPAC item level
     holds" = "force". Allow "on shelf" holds for easier testing.
   - In the rules for ITEM_LIB, under "Default checkout, hold and return
     policy",
     - set "Hold policy" to "From any library"
     - set "Hold pickup library match" to "item's home library"
   - In the rules for PATRON_LIB, under "Default checkout, hold and
     return policy",
     - set "Hold policy" to "From any library",
     - set "Hold pickup library match" to "any library"
4. In "Library transfer limits" disable all transfers from ITEM_LIB to
   PATRON_LIB for an item type (let's say "BOOK")
5. Create a biblio with an item:
   - item type must be the same as in previous step ("BOOK")
   - home branch and holding branch must be ITEM_LIB
6. Create a patron at PATRON_LIB, give it a password so it can log in.
7. Go to OPAC, and login with this patron
8. Try to place a hold for the new item. You should be allowed to place
   a hold, but when doing it, no holds will be created.
9. Apply patch, restart Koha
10. Try to place a hold for the new item. This time the hold should be
    created

Signed-off-by: Jessie Zairo <jzairo@bywatersolutions.com>
Comment 3 Jessie Zairo 2023-08-17 11:28:36 UTC
Very thorough testing steps.
Comment 4 Victor Grousset/tuxayo 2023-08-30 23:40:32 UTC
> You should be allowed to place a hold, but when doing it, no holds will be created.

I didn't get that, I got «One or more holds were not placed due to following errors: Cannot be transferred to pickup library»

I double checked having setup the right config and scenario.

@Julian: is that also an expected outcome?

@Jessie: what did you got when testing?

Other than that, the patch allows the hold to be placed so it seems to be working.
Comment 5 Victor Grousset/tuxayo 2023-08-30 23:41:06 UTC
Created attachment 154992 [details] [review]
Bug 33074: (QA follow-up) tidy code
Comment 6 Jonathan Druart 2023-09-22 13:47:41 UTC
Bug 30825 is PQA and it removes GetReservesControlBranch. We can still keep this patch for stable branches, but we need a new one built on top of it.
Comment 7 Jonathan Druart 2023-09-22 13:52:30 UTC
Created attachment 156094 [details] [review]
Bug 33074: Use Koha::Policy::Holds->holds_control_library
Comment 8 Jonathan Druart 2023-09-22 13:52:43 UTC
Sorry, it was unfair. I PQAed bug 30825 earlier today!
Comment 9 Marcel de Rooy 2023-09-29 08:22:50 UTC
Created attachment 156366 [details] [review]
Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl

Test plan:
1. Set system preferences:
   - Disable OPACAllowUserToChooseBranch
   - Set ReservesControlBranch to "item's home library"
   - Enable UseBranchTransferLimits
   - Set BranchTransferLimitsType to "item type"
   - Enable canreservefromotherbranches
   - Disable IndependantBranches
2. Create two libraries: ITEM_LIB and PATRON_LIB
3. Set circulation rules:
   - Remove all circulation rules (DELETE FROM circulation_rules)
   - Set a default rule that allow some holds and with "OPAC item level
     holds" = "force". Allow "on shelf" holds for easier testing.
   - In the rules for ITEM_LIB, under "Default checkout, hold and return
     policy",
     - set "Hold policy" to "From any library"
     - set "Hold pickup library match" to "item's home library"
   - In the rules for PATRON_LIB, under "Default checkout, hold and
     return policy",
     - set "Hold policy" to "From any library",
     - set "Hold pickup library match" to "any library"
4. In "Library transfer limits" disable all transfers from ITEM_LIB to
   PATRON_LIB for an item type (let's say "BOOK")
5. Create a biblio with an item:
   - item type must be the same as in previous step ("BOOK")
   - home branch and holding branch must be ITEM_LIB
6. Create a patron at PATRON_LIB, give it a password so it can log in.
7. Go to OPAC, and login with this patron
8. Try to place a hold for the new item. You should be allowed to place
   a hold, but when doing it, no holds will be created.
9. Apply patch, restart Koha
10. Try to place a hold for the new item. This time the hold should be
    created

Signed-off-by: Jessie Zairo <jzairo@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Merged tidy patch and switch to Koha::Policy::Holds here.
Comment 10 Tomás Cohen Arazi 2023-10-03 12:55:17 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 11 Fridolin Somers 2023-10-05 18:45:30 UTC
Bug 30825 is not in 23.05.x
I see old patches could be used, please attach a patch with [23.05.x] for stable backport
Comment 12 Julian Maurice 2023-10-06 06:59:14 UTC
(In reply to Fridolin Somers from comment #11)
> Bug 30825 is not in 23.05.x
> I see old patches could be used, please attach a patch with [23.05.x] for
> stable backport

You can use the original (obsoleted) patch. It does not depend on bug 30825
Comment 13 Fridolin Somers 2023-10-09 19:59:11 UTC
Pushed to 23.05.x for 23.05.05
Comment 14 Matt Blenkinsop 2023-10-17 17:31:19 UTC
Dependency missing for 22.11.x - not backporting