There are some improvements we can make to this routine to improve the speed at which we return results
Created attachment 91913 [details] [review] Bug 23414: Use Koha Item objects in buildKohaItemsNamespace Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up To test I added 100 items to each of the first 20 records returned by a search for 'a' I saw a reduction from ~30 seconds to ~26 seconds This also makes the code a little cleaner and moves us toward the Koha namespace To test: 1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12) 2 - View the netwrok tab and see how long the pages take to load 3 - Apply the patches, restart all the things 4 - Repeat and note if the results are returned faster
Created attachment 91914 [details] [review] Bug 23414: Prefetch transfers, use only default framework, use elsif This further cleans up the code, before we tested each condition though we return only one status line. Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework Prefetching should offer a boost To test: Same as before - there is not as big a boost form this, but there shouldn't be a hit
Created attachment 91915 [details] [review] Bug 23414: Prefetch reserves and use them for determining waiting This dependss on bug 23413 Same tests as before, you shouldn't see a hit with this change
I like this enhancement. But would like to mention that it looks like there's an unnecessary processing of items from the MARC results, in the caller (searchResults) that is also making several DB calls. So it seems we need to make a decision, whether to use the data from Zebra, or use the one on the DB, and reuse the retrieved objects as much as we can (so maybe doing the search at a higher level, searchResults). Just a thought, and can be done later of course.
Created attachment 94537 [details] [review] Bug 23414: Use Koha::Holds->waiting
The if...if sequence vs elsif will introduce a behavior's change. For instance an item that is lost and damaged will now be marked as lost whereas it was damaged before this patch. Is it expected? If not, you should reverse the order.
Created attachment 94580 [details] [review] Bug 23414: Use Koha Item objects in buildKohaItemsNamespace Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up To test I added 100 items to each of the first 20 records returned by a search for 'a' I saw a reduction from ~30 seconds to ~26 seconds This also makes the code a little cleaner and moves us toward the Koha namespace To test: 1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12) 2 - View the netwrok tab and see how long the pages take to load 3 - Apply the patches, restart all the things 4 - Repeat and note if the results are returned faster
Created attachment 94581 [details] [review] Bug 23414: Prefetch transfers, use only default framework, use elsif This further cleans up the code, before we tested each condition though we return only one status line. Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework Prefetching should offer a boost To test: Same as before - there is not as big a boost form this, but there shouldn't be a hit
Nick, it was either you restored the elsif's, or you changed the order, not both :)
And so we need a test.
(In reply to Jonathan Druart from comment #9) > Nick, it was either you restored the elsif's, or you changed the order, not > both :) Forget that!
Created attachment 95010 [details] [review] Bug 23414: Unit tests To test: 1 - Apply this patch alone 2 - prove -v t/db_dependent/XSLT.t 3 - All pass 4 - Apply other patches 5 - prove -v t/db_dependent/XSLT.t 6 - All pass
Created attachment 95011 [details] [review] Bug 23414: Use Koha Item objects in buildKohaItemsNamespace Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up To test I added 100 items to each of the first 20 records returned by a search for 'a' I saw a reduction from ~30 seconds to ~26 seconds This also makes the code a little cleaner and moves us toward the Koha namespace To test: 1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12) 2 - View the netwrok tab and see how long the pages take to load 3 - Apply the patches, restart all the things 4 - Repeat and note if the results are returned faster
Created attachment 95012 [details] [review] Bug 23414: Prefetch transfers, use only default framework, use elsif This further cleans up the code, before we tested each condition though we return only one status line. Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework Prefetching should offer a boost To test: Same as before - there is not as big a boost form this, but there shouldn't be a hit
Not sure about $item->itype, it seems that it should be $item->effective_itemtype
Created attachment 95014 [details] [review] Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace This patch improves true/false logic and avoid autovivication. Also note that xml_escape already deals with empty string. An unecessary call to GetReserveStatus is removed.
Created attachment 95015 [details] [review] Bug 23414: Use effective_itemtype We should use effective_itemtype instead of itype to take the item-level_itypes pref into account. Note that we should not need to test for the existence of the key as we now assume that item types are correctly set to a valid item type.
Created attachment 95069 [details] [review] Bug 23414: Use Koha Item objects in buildKohaItemsNamespace Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up To test I added 100 items to each of the first 20 records returned by a search for 'a' I saw a reduction from ~30 seconds to ~26 seconds This also makes the code a little cleaner and moves us toward the Koha namespace To test: 1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12) 2 - View the netwrok tab and see how long the pages take to load 3 - Apply the patches, restart all the things 4 - Repeat and note if the results are returned faster Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 95070 [details] [review] Bug 23414: Prefetch transfers, use only default framework, use elsif This further cleans up the code, before we tested each condition though we return only one status line. Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework Prefetching should offer a boost To test: Same as before - there is not as big a boost form this, but there shouldn't be a hit Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 95071 [details] [review] Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace This patch improves true/false logic and avoid autovivication. Also note that xml_escape already deals with empty string. An unecessary call to GetReserveStatus is removed. Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 95072 [details] [review] Bug 23414: Use effective_itemtype We should use effective_itemtype instead of itype to take the item-level_itypes pref into account. Note that we should not need to test for the existence of the key as we now assume that item types are correctly set to a valid item type. Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 95073 [details] [review] Bug 23414: Unit tests To test: 1 - Apply this patch alone 2 - prove -v t/db_dependent/XSLT.t 3 - All pass 4 - Apply other patches 5 - prove -v t/db_dependent/XSLT.t 6 - All pass Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 95083 [details] [review] Bug 23414: Use Koha Item objects in buildKohaItemsNamespace Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up To test I added 100 items to each of the first 20 records returned by a search for 'a' I saw a reduction from ~30 seconds to ~26 seconds This also makes the code a little cleaner and moves us toward the Koha namespace To test: 1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12) 2 - View the netwrok tab and see how long the pages take to load 3 - Apply the patches, restart all the things 4 - Repeat and note if the results are returned faster Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 95084 [details] [review] Bug 23414: Prefetch transfers, use only default framework, use elsif This further cleans up the code, before we tested each condition though we return only one status line. Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework Prefetching should offer a boost To test: Same as before - there is not as big a boost form this, but there shouldn't be a hit Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 95085 [details] [review] Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace This patch improves true/false logic and avoid autovivication. Also note that xml_escape already deals with empty string. An unecessary call to GetReserveStatus is removed. Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 95086 [details] [review] Bug 23414: Use effective_itemtype We should use effective_itemtype instead of itype to take the item-level_itypes pref into account. Note that we should not need to test for the existence of the key as we now assume that item types are correctly set to a valid item type. Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 95087 [details] [review] Bug 23414: Unit tests To test: 1 - Apply this patch alone 2 - prove -v t/db_dependent/XSLT.t 3 - All pass 4 - Apply other patches 5 - prove -v t/db_dependent/XSLT.t 6 - All pass Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 95088 [details] [review] Bug 23414: Add tests for previous effective_itemtype change Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 95089 [details] [review] Bug 23414: Replace delimiter of regexs to ease readability Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Sorry, scope creep...
Well done getting this one over the finish line just in time for the freeze guys. I was about to update it to rel_20_05_target when I spotted the diligent final QAing.. Thanks! Looks good, clarifies things and improves performance, Pushing :)
Nice work! Pushed to master for 19.11.00
enhancement won't be backported to 19.05.x