Bug 33074 - ReservesControlBranch not taken into account in opac-reserve.pl
Summary: ReservesControlBranch not taken into account in opac-reserve.pl
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 21263 30825
Blocks:
  Show dependency treegraph
 
Reported: 2023-02-27 09:04 UTC by Julian Maurice
Modified: 2023-10-17 17:31 UTC (History)
7 users (show)

See Also:
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


Attachments
Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl (2.74 KB, patch)
2023-02-27 09:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl (2.80 KB, patch)
2023-08-17 11:27 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 33074: (QA follow-up) tidy code (1.43 KB, patch)
2023-08-30 23:41 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33074: Use Koha::Policy::Holds->holds_control_library (1.22 KB, patch)
2023-09-22 13:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33074: Take ReservesControlBranch into account in opac-reserve.pl (3.12 KB, patch)
2023-09-29 08:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review

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