The method currently requires unblessed objects only to access one attribute on them... and it can certainly be moved to Koha::*
Created attachment 135248 [details] [review] Bug 30825: Add unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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 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.
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.
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>
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>
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>
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.
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.
(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.
(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.
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.
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).
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)
s/Reserves/Holds, obviously
(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
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>
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>
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>
What are your opinions on the discussion, Kyle?
Setting IN DISCUSSION to avoid confusion. We need to come up with a better design for this business stuffs.
(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.
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>
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>
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>
Created attachment 152103 [details] [review] Bug 30825: Move holds_control_library to Koha::Policy::Holds
I've rebased the patches and moved the method to a separate module, Koha::Policy::Holds
Created attachment 152104 [details] [review] Bug 30825: Move holds_control_library to Koha::Policy::Holds
I am reusing Koha::Policy in bug 33940.
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
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>
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>
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>
Created attachment 155823 [details] [review] Bug 30825: Move holds_control_library to Koha::Policy::Holds
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>
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>
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>
Created attachment 155827 [details] [review] Bug 30825: Move holds_control_library to Koha::Policy::Holds Signed-off-by: David Nind <david@davidnind.com>
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x