Bug 33493 - Add a filter relationship for branchtransfers
Summary: Add a filter relationship for branchtransfers
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 33167
Blocks: 33497
  Show dependency treegraph
 
Reported: 2023-04-11 18:21 UTC by Nick Clemens
Modified: 2023-08-03 20:58 UTC (History)
5 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,23.05.02


Attachments
Bug 33493: Add a filter relationship for branchtransfers (2.19 KB, patch)
2023-04-11 18:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33493: Add a filter relationship for branchtransfers (2.24 KB, patch)
2023-04-11 19:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 33493: (follow-up) Rename relationship and add test (3.20 KB, patch)
2023-05-31 12:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33493: Add a filter relationship for branchtransfers (2.24 KB, patch)
2023-06-01 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33493: (follow-up) Rename relationship and add test (3.51 KB, patch)
2023-06-01 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33493: Add a filter relationship for branchtransfers (2.33 KB, patch)
2023-06-02 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33493: (follow-up) Rename relationship and add test (3.60 KB, patch)
2023-06-02 09:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33493: (QA follow-up) Only bless the first transfer (2.42 KB, patch)
2023-06-02 09:13 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 Nick Clemens 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 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 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 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 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 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 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