On the item list of the bibliographic record page you actually can filter items by library, either using the dropdown list on top of the column or using the library_id (branchcode) in the global search. But you cannot search using the library's name in the global search.
Created attachment 172559 [details] [review] Bug 38130: Add the ability to filter item by library name On the item list of the bibliographic record page you actually can filter items by library, either using the dropdown list on top of the column or using the library_id (branchcode) in the global search. But you cannot search using the library's name in the global search. This patch add yet other embeds to allow this. I am not sure this is needed, we won't be able to do the same for item type and AV, so maybe it is not worth 2 additional JOINS. Maybe we should tell people you should use the column filters when a dropdown list is there, instead of: "it works, sometimes".
Created attachment 172574 [details] [review] Bug 38130: Add the ability to filter item by library name On the item list of the bibliographic record page you actually can filter items by library, either using the dropdown list on top of the column or using the library_id (branchcode) in the global search. But you cannot search using the library's name in the global search. This patch add yet other embeds to allow this. I am not sure this is needed, we won't be able to do the same for item type and AV, so maybe it is not worth 2 additional JOINS. Maybe we should tell people you should use the column filters when a dropdown list is there, instead of: "it works, sometimes". Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 172578 [details] [review] Bug 38130: Add the ability to filter item by library name On the item list of the bibliographic record page you actually can filter items by library, either using the dropdown list on top of the column or using the library_id (branchcode) in the global search. But you cannot search using the library's name in the global search. This patch add yet other embeds to allow this. I am not sure this is needed, we won't be able to do the same for item type and AV, so maybe it is not worth 2 additional JOINS. Maybe we should tell people you should use the column filters when a dropdown list is there, instead of: "it works, sometimes". Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Since Brendan signed off here after me I'll use my sign-off as QA. The script is happy, the change is trivial, it makes sense, fixes the problem. PQA
""" I am not sure this is needed, we won't be able to do the same for item type and AV, so maybe it is not worth 2 additional JOINS. Maybe we should tell people you should use the column filters when a dropdown list is there, instead of: "it works, sometimes". """ I would like us to discuss this part before we push it.
This is very similar to another bug I filed: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38333 In the end, I think it is much more important to be able to sort/filter on the name/description than it would be the code. That goes for branches, item types, and any AV.
> Maybe we should tell people you should use the column filters when a > dropdown list is there, instead of: "it works, sometimes". > """ Our libraries very much need to be able to sort by branch name, especially holding/home library. If we take Jonathan's patch and flip the order of the joins ( data: "holding_library.name:me.holding_library_id" ) it works for sorting and filtering. Prior to 24.05 one could not filter by branchcode, only name, so we won't be losing any previous functionality. We ultimately need to need to fix sorting/filtering by item type/collection as well. ( descriptions not codes ) but we are hearing from our libraries right now that home/holding library sorting/filtering is especially important.
(In reply to Lucas Gass (lukeg) from comment #7) > > Maybe we should tell people you should use the column filters when a > > dropdown list is there, instead of: "it works, sometimes". > > """ > > Our libraries very much need to be able to sort by branch name, especially > holding/home library. If we do it for the library names on this view, we need to do it for all the library columns (thinking about patrons right now but I am sure there are others). And to be consistent, for all the other coded values: "special AVs" (item types, cn source), regular AVs, patron category codes, etc. We will introduce additional JOINs and so maybe performance problems. This needs to be investigated before we take this route IMO.
Created attachment 174103 [details] [review] Bug 38130: Allow sorting and filtering on library id and name On the item list of the bib detail page and the patron search we want the library columns ('holding library', 'home library' and 'library') to be sorted by library names and to make them searchable on library id and name. Concerns about this approach (comment #8): """ If we do it for the library names on this view, we need to do it for all the library columns (thinking about patrons right now but I am sure there are others). And to be consistent, for all the other coded values: "special AVs" (item types, cn source), regular AVs, patron category codes, etc. We will introduce additional JOINs and so maybe performance problems. This needs to be investigated before we take this route IMO. """
So.. I advocate for some real benchmarks to actually prove the hypothesis here. In a related query though.. I'd be interested in investigating dbic's join vs prefetch pragma's... I think some of our performance challenges come from the fact we force prefetch on the API for all embeds. Whilst this makes sense from the perspective of having consistent data returned in the responses.. a simple join without the automatic collapse (grabbing all related fields instead of just the ones you are interested in) might be more performant for cases like this where we're not so much interested in all the fields as we are a select few. This is again conjecture though without a benchmark.
(In reply to Jonathan Druart from comment #9) > If we do it for the library names on this view, we need to do it for all the > library columns (thinking about patrons right now but I am sure there are > others). And to be consistent, for all the other coded values: "special AVs" > (item types, cn source), regular AVs, patron category codes, etc. Agreed. > We will introduce additional JOINs and so maybe performance problems. This > needs to be investigated before we take this route IMO. > """ I've tested using Nick's randitems: https://github.com/kidclamp/handy-koha-script/blob/main/randitems.pl Before patches: searching for "cpl" using the datatable search input - average of 0.27 secs for 300 items. - average of 0.37 secs for 3000 items. After patches: searching for "centerville" (or cpl) using the datatable search input - average of 0.3 secs for 300 items. - average of 0.45 secs for 3000 items. This exercise should also be done for patrons (when that time comes) but from my testing, the performance impact seems negligible on this table. For me, this is enough to not block this, the enhancement is clearly needed and makes sense. As a rule of thumb, anything that is displayed on the table should be retrievable by a keyword search.
Created attachment 174189 [details] [review] Bug 38130: Add the ability to filter item by library name On the item list of the bibliographic record page you actually can filter items by library, either using the dropdown list on top of the column or using the library_id (branchcode) in the global search. But you cannot search using the library's name in the global search. This patch add yet other embeds to allow this. I am not sure this is needed, we won't be able to do the same for item type and AV, so maybe it is not worth 2 additional JOINS. Maybe we should tell people you should use the column filters when a dropdown list is there, instead of: "it works, sometimes". Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 174190 [details] [review] Bug 38130: Allow sorting and filtering on library id and name On the item list of the bib detail page and the patron search we want the library columns ('holding library', 'home library' and 'library') to be sorted by library names and to make them searchable on library id and name. Concerns about this approach (comment #8): """ If we do it for the library names on this view, we need to do it for all the library columns (thinking about patrons right now but I am sure there are others). And to be consistent, for all the other coded values: "special AVs" (item types, cn source), regular AVs, patron category codes, etc. We will introduce additional JOINs and so maybe performance problems. This needs to be investigated before we take this route IMO. """ Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
I am seeing, for 1000 items: 700ms without this patch 950ms with this patch Also there is a bottleneck somewhere else, the UI is very slow to refresh (JS-side then). If we are adding 150ms per JOIN and are adding 3,4 per table, it's not a good...
(In reply to Jonathan Druart from comment #14) > Also there is a bottleneck somewhere else, the UI is very slow to refresh > (JS-side then). Found something, see bug 38391.
(In reply to Jonathan Druart from comment #14) > I am seeing, for 1000 items: > 700ms without this patch > 950ms with this patch Interesting, I wonder why our times differ so much, just tested again with patches still applied searching for 'centerville' 3000 items: 480ms, 434ms, 449ms, 455ms
(In reply to Jonathan Druart from comment #14) > > If we are adding 150ms per JOIN and are adding 3,4 per table, it's not a > good... I don't think we can affirm this just yet. Additional embeds need to be added to test this (e.g. status and/or collection)
(In reply to Pedro Amorim from comment #16) > (In reply to Jonathan Druart from comment #14) > > I am seeing, for 1000 items: > > 700ms without this patch > > 950ms with this patch > > Interesting, I wonder why our times differ so much, just tested again with > patches still applied searching for 'centerville' 3000 items: > 480ms, 434ms, 449ms, 455ms Do you want to swap our laptops? :D
To summary PM between Pedro and me. We should not use library's name to compare with and without the patch, we should compare with the same number of results returned (ofc!) Tested with the following script: use t::lib::TestBuilder; my $builder = t::lib::TestBuilder->new; my @branchcodes = Koha::Libraries->search->get_column('branchcode'); my $biblio = $builder->build_sample_biblio; for my $i ( 1..1000 ) { say $i; my $branchcode = @branchcodes[int(rand(scalar @branchcodes))]; $builder->build_sample_item({biblionumber => $biblio->biblionumber, library => $branchcode }); } say "biblionumber=".$biblio->biblionumber; Search "2024" in the global search to make sure 20 items will be displayed and the JOIN will happen. No need to use the column filters. I confirm the previous numbers ~700ms without vs ~900ms with those 2 patches.
(In reply to Pedro Amorim from comment #17) > (In reply to Jonathan Druart from comment #14) > > > > If we are adding 150ms per JOIN and are adding 3,4 per table, it's not a > > good... > > I don't think we can affirm this just yet. Additional embeds need to be > added to test this (e.g. status and/or collection) FYI status is happening on bug 37334, but is totally different.
I'm not sure about the state of the discussion here. Should this be pushed or are the benchmarks/performance tests a blocker?
(In reply to Katrin Fischer from comment #21) > I'm not sure about the state of the discussion here. > Should this be pushed or are the benchmarks/performance tests a blocker? Not blocker.
Pushed for 24.11! Well done everyone, thank you!
Backported to 24.05.x for upcoming 24.05.06