Bug 33493

Summary: Add a filter relationship for branchtransfers
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens (kidclamp) <nick>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, m.de.rooy, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02
Bug Depends on: 33167    
Bug Blocks: 33497    
Attachments: Bug 33493: Add a filter relationship for branchtransfers
Bug 33493: Add a filter relationship for branchtransfers
Bug 33493: (follow-up) Rename relationship and add test
Bug 33493: Add a filter relationship for branchtransfers
Bug 33493: (follow-up) Rename relationship and add test
Bug 33493: Add a filter relationship for branchtransfers
Bug 33493: (follow-up) Rename relationship and add test
Bug 33493: (QA follow-up) Only bless the first transfer

Description Nick Clemens (kidclamp) 2023-04-11 18:21:17 UTC
Moved from bug 33167

Prefetching is useful, but we don't want to fetch all the inactive
transfers for an item. This patch will a filtered relationship and
update get_transfer/s to use this
Comment 1 Nick Clemens (kidclamp) 2023-04-11 18:37:16 UTC
Created attachment 149464 [details] [review]
Bug 33493: Add a filter relationship for branchtransfers

Prefetching is useful, but we don't want to fetch all the inactive
transfers for an item. This patch adds a filtered relationship and
updates get_transfer(s) to use this

To test:
prove -v t/db_dependent/Koha/Item.t
Comment 2 David Nind 2023-04-11 19:09:30 UTC
Created attachment 149469 [details] [review]
Bug 33493: Add a filter relationship for branchtransfers

Prefetching is useful, but we don't want to fetch all the inactive
transfers for an item. This patch adds a filtered relationship and
updates get_transfer(s) to use this

To test:
prove -v t/db_dependent/Koha/Item.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Katrin Fischer 2023-04-11 21:29:21 UTC
This is temporarily BLOCKED by the dependent patch waiting for SO.
Comment 4 Marcel de Rooy 2023-05-12 08:55:28 UTC
Back to SO
Comment 5 Marcel de Rooy 2023-05-12 08:56:43 UTC
Sorry but again marking as enh
Comment 6 Marcel de Rooy 2023-05-19 09:45:12 UTC
__PACKAGE__->has_many(
  "branchtransfer",

This name is confusing. Please do something with current and in plural.
current_branchtransfers ? 
branchtransfers_filter_by_current ?

Please add a test.
Did you have a use case where get_transfer did not return the right transfer?
Comment 7 Nick Clemens (kidclamp) 2023-05-31 12:03:15 UTC
Created attachment 151865 [details] [review]
Bug 33493: (follow-up) Rename relationship and add test
Comment 8 Nick Clemens (kidclamp) 2023-05-31 12:05:17 UTC
(In reply to Marcel de Rooy from comment #6)
> __PACKAGE__->has_many(
>   "branchtransfer",
> 
> This name is confusing. Please do something with current and in plural.
> current_branchtransfers ? 
> branchtransfers_filter_by_current ?

Ok, current_branchtransfers

> 
> Please add a test.

Ok

> Did you have a use case where get_transfer did not return the right transfer?

The use case is not fixing an incorrect return, but allowing to prefetch/join easily ad only get relevant data. See bug 33497 for using this as a prefetch on details page.
Comment 9 Jonathan Druart 2023-06-01 07:47:56 UTC
filter_by_current is not longer used apparently, shouldn't we remove it then?
Comment 10 Jonathan Druart 2023-06-01 07:52:04 UTC
Note that this is going to be reverted by bug 33568 in its current state. There is a FIXME in Koha/REST/V1/Biblios.pm

Please help there ;)
Comment 11 Jonathan Druart 2023-06-01 07:53:05 UTC
And, don't forget to perltidy your new blocks of code!
Comment 12 Nick Clemens (kidclamp) 2023-06-01 12:26:09 UTC
(In reply to Jonathan Druart from comment #10)
> Note that this is going to be reverted by bug 33568 in its current state.
> There is a FIXME in Koha/REST/V1/Biblios.pm
> 
> Please help there ;)

33493 is in your commit history on the branch - you use a new function 'transfer' which simply calls 'get_transfer' so will use the new relationship there if you apply latest version

The 'FIXME' just suggests adding the prefetch - so just yes, do that using this relationship
Comment 13 Nick Clemens (kidclamp) 2023-06-01 12:27:37 UTC
Created attachment 151923 [details] [review]
Bug 33493: Add a filter relationship for branchtransfers

Prefetching is useful, but we don't want to fetch all the inactive
transfers for an item. This patch adds a filtered relationship and
updates get_transfer(s) to use this

To test:
prove -v t/db_dependent/Koha/Item.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 Nick Clemens (kidclamp) 2023-06-01 12:27:39 UTC
Created attachment 151924 [details] [review]
Bug 33493: (follow-up) Rename relationship and add test
Comment 15 Marcel de Rooy 2023-06-02 09:13:17 UTC
Created attachment 151976 [details] [review]
Bug 33493: Add a filter relationship for branchtransfers

Prefetching is useful, but we don't want to fetch all the inactive
transfers for an item. This patch adds a filtered relationship and
updates get_transfer(s) to use this

To test:
prove -v t/db_dependent/Koha/Item.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2023-06-02 09:13:19 UTC
Created attachment 151977 [details] [review]
Bug 33493: (follow-up) Rename relationship and add test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2023-06-02 09:13:21 UTC
Created attachment 151978 [details] [review]
Bug 33493: (QA follow-up) Only bless the first transfer

No need to do a walk through on all the results.
Correcting two test descriptions on the way.

Test plan:
Run t/db_dependent/Koha/Item.t
Run t/db_dependent/Koha/Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Tomás Cohen Arazi 2023-06-05 18:37:21 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 19 Tomás Cohen Arazi 2023-06-09 13:54:46 UTC
Enhancement won't be backported to stable release.
Comment 20 Martin Renvoize 2023-07-18 13:23:32 UTC
I decided to backport this one in the end as it's a requirement for bug 33497 which I felt was a worthwhile performance enhancement.

Pushed for release in 23.05.02