Summary: | Remove blank facet for languages | ||
---|---|---|---|
Product: | Koha | Reporter: | Nick Clemens <nick> |
Component: | Searching - Elasticsearch | Assignee: | Nick Clemens <nick> |
Status: | RESOLVED FIXED | QA Contact: | Katrin Fischer <katrin.fischer> |
Severity: | normal | ||
Priority: | P5 - low | CC: | arthur.suzuki, david, fridolin.somers, jonathan.druart, lucas, martin.renvoize, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27136 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31668 |
||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This removes blank facets from search results when using Elasticsearch. Currently, this only seems to affect language fields, but could affect any facetable field that contains blank values.
|
Version(s) released in: |
22.11.00, 22.05.08, 21.11.15
|
Attachments: |
Bug 29561: Remove blank facets
Bug 29561: Remove blank facets Bug 29561: Remove blank facets Bug 29561: Unit test Bug 29561: Remove blank facets Bug 29561: (follow-up) Don't remove facet '0' Bug 29561: Unit test Bug 29561: Remove blank facets Bug 29561: (follow-up) Don't remove facet '0' |
Description
Nick Clemens
2021-11-23 14:44:27 UTC
Created attachment 127957 [details] [review] Bug 29561: Remove blank facets This patch strips the blank facets from the ES aggregations, this seems to only affect language fields currently, but could affect any facetable field that can contain blank values To test: 1 - Have Koha running Elasticsearch with standard test data, or add some records with blank values in the 008/35-37 2 - Search for 'a' 3 - Look at the language facets, they should appear normal 4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=&q=a&sort_by=relevance_dsc&count=20&limit=ln: 5 - Note there is now an 'x' under the Languages facet - to remove a limit you cannot see 6 - Apply patch 7 - Reload 8 - Note the x is not there 9 - Inspect HTML, confirm no blank link under language facets Created attachment 128100 [details] [review] Bug 29561: Remove blank facets This patch strips the blank facets from the ES aggregations, this seems to only affect language fields currently, but could affect any facetable field that can contain blank values To test: 1 - Have Koha running Elasticsearch with standard test data, or add some records with blank values in the 008/35-37 2 - Search for 'a' 3 - Look at the language facets, they should appear normal 4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=&q=a&sort_by=relevance_dsc&count=20&limit=ln: 5 - Note there is now an 'x' under the Languages facet - to remove a limit you cannot see 6 - Apply patch 7 - Reload 8 - Note the x is not there 9 - Inspect HTML, confirm no blank link under language facets Signed-off-by: David Nind <david@davidnind.com> Did you try to add a test for this change? Created attachment 129595 [details] [review] Bug 29561: Remove blank facets This patch strips the blank facets from the ES aggregations, this seems to only affect language fields currently, but could affect any facetable field that can contain blank values To test: 1 - Have Koha running Elasticsearch with standard test data, or add some records with blank values in the 008/35-37 2 - Search for 'a' 3 - Look at the language facets, they should appear normal 4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=&q=a&sort_by=relevance_dsc&count=20&limit=ln: 5 - Note there is now an 'x' under the Languages facet - to remove a limit you cannot see 6 - Apply patch 7 - Reload 8 - Note the x is not there 9 - Inspect HTML, confirm no blank link under language facets Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Clear code, reasonable fixme for future reference.. QA scripts happy. Passing QA (In reply to Jonathan Druart from comment #3) > Did you try to add a test for this change? Arf we have no unit tests on _convert_facets(). Since this change has a complex context, we should create on in order to avoid regression. + next unless $t Also this will remove facet with value zero. This may be odd but we should allow it no ? Created attachment 140862 [details] [review] Bug 29561: Unit test Created attachment 140863 [details] [review] Bug 29561: Remove blank facets This patch strips the blank facets from the ES aggregations, this seems to only affect language fields currently, but could affect any facetable field that can contain blank values To test: 1 - Have Koha running Elasticsearch with standard test data, or add some records with blank values in the 008/35-37 2 - Search for 'a' 3 - Look at the language facets, they should appear normal 4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=&q=a&sort_by=relevance_dsc&count=20&limit=ln: 5 - Note there is now an 'x' under the Languages facet - to remove a limit you cannot see 6 - Apply patch 7 - Reload 8 - Note the x is not there 9 - Inspect HTML, confirm no blank link under language facets Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 140864 [details] [review] Bug 29561: (follow-up) Don't remove facet '0' Stealing this QA from Martin ;) Turning on displayFacetCount helps to make this more visible. Created attachment 141543 [details] [review] Bug 29561: Unit test Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 141544 [details] [review] Bug 29561: Remove blank facets This patch strips the blank facets from the ES aggregations, this seems to only affect language fields currently, but could affect any facetable field that can contain blank values To test: 1 - Have Koha running Elasticsearch with standard test data, or add some records with blank values in the 008/35-37 2 - Search for 'a' 3 - Look at the language facets, they should appear normal 4 - Browse to: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?idx=&q=a&sort_by=relevance_dsc&count=20&limit=ln: 5 - Note there is now an 'x' under the Languages facet - to remove a limit you cannot see 6 - Apply patch 7 - Reload 8 - Note the x is not there 9 - Inspect HTML, confirm no blank link under language facets Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 141545 [details] [review] Bug 29561: (follow-up) Don't remove facet '0' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 22.11. Nice work everyone, thanks! *** Bug 31668 has been marked as a duplicate of this bug. *** Backported to 22.05.x for upcoming 22.05.08 applied to 21.11 for 21.11.15 Not backported to 21.05.x |