|
Description
Nick Clemens (kidclamp)
2023-04-11 18:21:17 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 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> This is temporarily BLOCKED by the dependent patch waiting for SO. Back to SO Sorry but again marking as enh __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? Created attachment 151865 [details] [review] Bug 33493: (follow-up) Rename relationship and add test (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. filter_by_current is not longer used apparently, shouldn't we remove it then? 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 ;) And, don't forget to perltidy your new blocks of code! (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 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> Created attachment 151924 [details] [review] Bug 33493: (follow-up) Rename relationship and add test 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> 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> 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> Pushed to master for 23.11. Nice work everyone, thanks! Enhancement won't be backported to stable release. 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 |