Bug 30825 - Get rid of GetReservesControlBranch
Summary: Get rid of GetReservesControlBranch
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 17728 33074
  Show dependency treegraph
 
Reported: 2022-05-20 14:33 UTC by Tomás Cohen Arazi
Modified: 2023-10-14 12:24 UTC (History)
6 users (show)

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


Attachments
Bug 30825: Add unit tests (1.58 KB, patch)
2022-05-20 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30825: Add Koha::Item->holds_control_library (1.84 KB, patch)
2022-05-20 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library (7.58 KB, patch)
2022-05-20 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30825: Add unit tests (1.64 KB, patch)
2022-05-23 14:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30825: Add Koha::Item->holds_control_library (1.90 KB, patch)
2022-05-23 14:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library (7.64 KB, patch)
2022-05-23 14:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30825: Pass by reference (6.97 KB, patch)
2022-05-23 16:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30825: Add unit tests (1.69 KB, patch)
2022-05-31 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30825: Add Koha::Item->holds_control_library (1.96 KB, patch)
2022-05-31 12:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library (7.70 KB, patch)
2022-05-31 12:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30825: Add unit tests (1.72 KB, patch)
2023-06-07 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Add Koha::Item->holds_control_library (1.96 KB, patch)
2023-06-07 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library (7.63 KB, patch)
2023-06-07 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Move holds_control_library to Koha::Policy::Holds (9.83 KB, patch)
2023-06-07 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Move holds_control_library to Koha::Policy::Holds (9.81 KB, patch)
2023-06-07 12:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Add unit tests (1.72 KB, patch)
2023-09-18 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Add Koha::Item->holds_control_library (1.96 KB, patch)
2023-09-18 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library (7.54 KB, patch)
2023-09-18 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Move holds_control_library to Koha::Policy::Holds (9.81 KB, patch)
2023-09-18 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30825: Add unit tests (1.77 KB, patch)
2023-09-18 14:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30825: Add Koha::Item->holds_control_library (2.00 KB, patch)
2023-09-18 14:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library (7.58 KB, patch)
2023-09-18 14:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30825: Move holds_control_library to Koha::Policy::Holds (9.84 KB, patch)
2023-09-18 14:29 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-05-20 14:33:34 UTC
The method currently requires unblessed objects only to access one attribute on them... and it can certainly be moved to Koha::*
Comment 1 Tomás Cohen Arazi 2022-05-20 14:56:47 UTC
Created attachment 135248 [details] [review]
Bug 30825: Add unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2022-05-20 14:56:52 UTC
Created attachment 135249 [details] [review]
Bug 30825: Add Koha::Item->holds_control_library

This simple method takes care of calculating the control branch for an
item and a patron, depending on a syspref.

It targets replacing C4::Reserves::GetReservesControlBranch

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2022-05-20 14:56:56 UTC
Created attachment 135250 [details] [review]
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library

This patch removes the GetReservesControlBranch method, and replaces its
uses with the newly introduced method.

To test:
1. Apply this patch
2. Verify that placing holds from the OPAC works
=> SUCCESS: Things work as expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Hold* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Martin Renvoize 2022-05-20 18:13:08 UTC
I've not looked in depth yet.. but I'm wondering if we aught to start adopting a pattern of generally accepting Object or Identifier for passed params. If one already has the object it's a no brainer to pass it through and save a lookup. In this case I could see the potential of having to fetch a patron to pass in then not actually need to use it, depending on the pref.. which is also wasteful?

Thoughts?
Comment 5 Tomás Cohen Arazi 2022-05-20 18:15:52 UTC
(In reply to Martin Renvoize from comment #4)
> I've not looked in depth yet.. but I'm wondering if we aught to start
> adopting a pattern of generally accepting Object or Identifier for passed
> params. If one already has the object it's a no brainer to pass it through
> and save a lookup. In this case I could see the potential of having to fetch
> a patron to pass in then not actually need to use it, depending on the
> pref.. which is also wasteful?
> 
> Thoughts?

In this particular case, what bothered me was that every place the method was called, the object was already there and it got unblessed just because (it was obviously a transition step when moving GetMember and friends to Koha::Patrons to avoid friction).

I'd say in this particular case either is fine. We always have the object in context anyway.
Comment 6 Martin Renvoize 2022-05-23 14:37:23 UTC
OK, this isn't a great example as we always have the patron to pass so on second thought it kinda makes sense as you've coded it.
Comment 7 Martin Renvoize 2022-05-23 14:40:13 UTC
Created attachment 135275 [details] [review]
Bug 30825: Add unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-05-23 14:40:17 UTC
Created attachment 135276 [details] [review]
Bug 30825: Add Koha::Item->holds_control_library

This simple method takes care of calculating the control branch for an
item and a patron, depending on a syspref.

It targets replacing C4::Reserves::GetReservesControlBranch

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-05-23 14:40:21 UTC
Created attachment 135277 [details] [review]
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library

This patch removes the GetReservesControlBranch method, and replaces its
uses with the newly introduced method.

To test:
1. Apply this patch
2. Verify that placing holds from the OPAC works
=> SUCCESS: Things work as expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Hold* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2022-05-23 15:10:11 UTC
On second look this is more involved to demonstrate than I thought...

We have:

$patron ||= Koha::Patron->find($patron_id);
$item->holds_control_library;

Which could plausibly be

$item->holds_control_library({ patron_id => $patron_id, patron =>$patron });

That would mean that if we didn't need the $patron object in the holds_control_library call there we could save ourselves a Koha::Patron->find call.

However, I then spotted that we do refer to $patron again lower down the containing method.. but again, inside a syspref controlled statement.. so we still might not need it.

That lead me to believe that a good structure would be to pass around references.. then upon the first time we actually need the patron object we do the fetch and populate the reference.. there onwards we have a populated ref we can refer to in other calls.
Comment 11 Martin Renvoize 2022-05-23 16:32:27 UTC
Created attachment 135281 [details] [review]
Bug 30825: Pass by reference

Not sure if this is nicer or not honestly. This changes the signature of
hols_control_library to accept a hashref of patron_id and patron object
reference. If the patron_object reference is found to be undefined and
our syspref requires it we populate the reference with a Koha::Patron
object as found from the patron_id.
Comment 12 Tomás Cohen Arazi 2022-05-24 17:28:15 UTC
(In reply to Martin Renvoize from comment #11)
> Created attachment 135281 [details] [review] [review]
> Bug 30825: Pass by reference
> 
> Not sure if this is nicer or not honestly. This changes the signature of
> hols_control_library to accept a hashref of patron_id and patron object
> reference. If the patron_object reference is found to be undefined and
> our syspref requires it we populate the reference with a Koha::Patron
> object as found from the patron_id.

I have the feeling this particular case would do better with just the patron_id. If we have it, good. If we have it via a Koha::Patron object, then $patron->id would be pretty clear to read.
Comment 13 Martin Renvoize 2022-05-25 09:29:52 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> (In reply to Martin Renvoize from comment #11)
> > Created attachment 135281 [details] [review] [review] [review]
> > Bug 30825: Pass by reference
> > 
> > Not sure if this is nicer or not honestly. This changes the signature of
> > hols_control_library to accept a hashref of patron_id and patron object
> > reference. If the patron_object reference is found to be undefined and
> > our syspref requires it we populate the reference with a Koha::Patron
> > object as found from the patron_id.
> 
> I have the feeling this particular case would do better with just the
> patron_id. If we have it, good. If we have it via a Koha::Patron object,
> then $patron->id would be pretty clear to read.

That goes the other way and introduces extra DB fetches....  I'd say we either go for passing Patron objects and ensure the caller always has that.. or we pass references so we can delay fetching.. I wouldn't go back and just pass ID's personally.
Comment 14 Martin Renvoize 2022-05-25 09:31:46 UTC
I worked on an alternative commit message for that last patch to clarify the intent as I think it got a little lost.

>This patch changes the signature of Koha::Item->holds_control_library to
>accept a hashref with patron_id and patron as keys.  The patron key
>should contain a reference to a variable that may or may not already
>contain a Koha::Patron object. In the case where a Koha::Patron object
>is required and the patron reference does not yet contain such an object
>the method will fetch the patron using the patron_id passed and populate
>the reference so that future calls can now use the populated object instead
>of having to fetch it themselves too.
Comment 15 Jonathan Druart 2022-05-31 08:36:27 UTC
IMO this does not belong to Koha::Item. We are making our Koha objects grow and it will be a nightmare soon.
This is not the responsibility of the item object, it must go on separate modules.

I don't have something clear to suggest right now. I have asked a couple of months ago to have a wider discussion about those naming/namespace/etc but it didn't move much (cannot remember the bug report however).
Comment 16 Jonathan Druart 2022-05-31 08:38:35 UTC
To clarify: we should have class methods, from classes that don't inherited from Koha::Object[s].

Something like
Koha::Policy::Reserves::holds_control_library($item, $patron)

(or that would take the ids if we don't want to fetch the objects prior to the call)
Comment 17 Jonathan Druart 2022-05-31 08:39:13 UTC
s/Reserves/Holds, obviously
Comment 18 Tomás Cohen Arazi 2022-05-31 11:33:35 UTC
(In reply to Jonathan Druart from comment #15)
> IMO this does not belong to Koha::Item. We are making our Koha objects grow
> and it will be a nightmare soon.
> This is not the responsibility of the item object, it must go on separate
> modules.

I agree. We talked about it with Martin as we felt it didn't belong here.

> I don't have something clear to suggest right now. I have asked a couple of
> months ago to have a wider discussion about those naming/namespace/etc but
> it didn't move much (cannot remember the bug report however).

It'd be interesting to find it.

I like what they are doing here about availability: https://github.com/NatLibFi/koha-plugin-rest-di/tree/main/Koha/Plugin/Fi/KohaSuomi/DI/Koha
Comment 19 Kyle M Hall 2022-05-31 12:12:52 UTC
Created attachment 135488 [details] [review]
Bug 30825: Add unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2022-05-31 12:13:03 UTC
Created attachment 135489 [details] [review]
Bug 30825: Add Koha::Item->holds_control_library

This simple method takes care of calculating the control branch for an
item and a patron, depending on a syspref.

It targets replacing C4::Reserves::GetReservesControlBranch

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2022-05-31 12:13:07 UTC
Created attachment 135490 [details] [review]
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library

This patch removes the GetReservesControlBranch method, and replaces its
uses with the newly introduced method.

To test:
1. Apply this patch
2. Verify that placing holds from the OPAC works
=> SUCCESS: Things work as expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Hold* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Jonathan Druart 2022-05-31 12:24:59 UTC
What are your opinions on the discussion, Kyle?
Comment 23 Tomás Cohen Arazi 2022-05-31 12:31:37 UTC
Setting IN DISCUSSION to avoid confusion. We need to come up with a better design for this business stuffs.
Comment 24 Kyle M Hall 2022-05-31 12:33:59 UTC
(In reply to Jonathan Druart from comment #16)
> To clarify: we should have class methods, from classes that don't inherited
> from Koha::Object[s].
> 
> Something like
> Koha::Policy::Reserves::holds_control_library($item, $patron)
> 
> (or that would take the ids if we don't want to fetch the objects prior to
> the call)

I think as a solution that would be sufficient but not necessary. If we always need to have both object to resolve the answer, it's just arbitrary to choose the item, the patron, or some third party as the "holder" of the method.
Comment 25 Jonathan Druart 2023-06-07 12:32:08 UTC
Created attachment 152100 [details] [review]
Bug 30825: Add unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Jonathan Druart 2023-06-07 12:32:12 UTC
Created attachment 152101 [details] [review]
Bug 30825: Add Koha::Item->holds_control_library

This simple method takes care of calculating the control branch for an
item and a patron, depending on a syspref.

It targets replacing C4::Reserves::GetReservesControlBranch

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Jonathan Druart 2023-06-07 12:32:15 UTC
Created attachment 152102 [details] [review]
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library

This patch removes the GetReservesControlBranch method, and replaces its
uses with the newly introduced method.

To test:
1. Apply this patch
2. Verify that placing holds from the OPAC works
=> SUCCESS: Things work as expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Hold* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Jonathan Druart 2023-06-07 12:32:18 UTC
Created attachment 152103 [details] [review]
Bug 30825: Move holds_control_library to Koha::Policy::Holds
Comment 29 Jonathan Druart 2023-06-07 12:32:36 UTC
I've rebased the patches and moved the method to a separate module, Koha::Policy::Holds
Comment 30 Jonathan Druart 2023-06-07 12:35:48 UTC
Created attachment 152104 [details] [review]
Bug 30825: Move holds_control_library to Koha::Policy::Holds
Comment 31 Jonathan Druart 2023-06-07 13:37:45 UTC
I am reusing Koha::Policy in bug 33940.
Comment 32 David Nind 2023-09-18 00:13:59 UTC
The patches no longer apply... 8-(

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 30825: Add unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Koha/Item.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Item.t
Applying: Bug 30825: Add Koha::Item->holds_control_library
Applying: Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	opac/opac-reserve.pl
M	t/db_dependent/Reserves.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Reserves.t
CONFLICT (content): Merge conflict in t/db_dependent/Reserves.t
Auto-merging opac/opac-reserve.pl
CONFLICT (content): Merge conflict in opac/opac-reserve.pl
Auto-merging C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library
Comment 33 Jonathan Druart 2023-09-18 14:18:26 UTC
Created attachment 155820 [details] [review]
Bug 30825: Add unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Jonathan Druart 2023-09-18 14:18:30 UTC
Created attachment 155821 [details] [review]
Bug 30825: Add Koha::Item->holds_control_library

This simple method takes care of calculating the control branch for an
item and a patron, depending on a syspref.

It targets replacing C4::Reserves::GetReservesControlBranch

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Jonathan Druart 2023-09-18 14:18:33 UTC
Created attachment 155822 [details] [review]
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library

This patch removes the GetReservesControlBranch method, and replaces its
uses with the newly introduced method.

To test:
1. Apply this patch
2. Verify that placing holds from the OPAC works
=> SUCCESS: Things work as expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Hold* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Jonathan Druart 2023-09-18 14:18:35 UTC
Created attachment 155823 [details] [review]
Bug 30825: Move holds_control_library to Koha::Policy::Holds
Comment 37 David Nind 2023-09-18 14:29:14 UTC
Created attachment 155824 [details] [review]
Bug 30825: Add unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 38 David Nind 2023-09-18 14:29:17 UTC
Created attachment 155825 [details] [review]
Bug 30825: Add Koha::Item->holds_control_library

This simple method takes care of calculating the control branch for an
item and a patron, depending on a syspref.

It targets replacing C4::Reserves::GetReservesControlBranch

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 39 David Nind 2023-09-18 14:29:19 UTC
Created attachment 155826 [details] [review]
Bug 30825: Remove GetReservesControlBranch in favour of Koha::Item->holds_control_library

This patch removes the GetReservesControlBranch method, and replaces its
uses with the newly introduced method.

To test:
1. Apply this patch
2. Verify that placing holds from the OPAC works
=> SUCCESS: Things work as expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Hold* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 40 David Nind 2023-09-18 14:29:22 UTC
Created attachment 155827 [details] [review]
Bug 30825: Move holds_control_library to Koha::Policy::Holds

Signed-off-by: David Nind <david@davidnind.com>
Comment 41 Tomás Cohen Arazi 2023-09-22 15:39:06 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 42 Fridolin Somers 2023-09-22 20:22:00 UTC
Enhancement not pushed to 23.05.x