Because the field for language facets is part of a control field, it won't be missing from a record, but it may be blank. When indexing into ES this means we put the field in the document empty When aggregating, ES counts these correctly, but we cannot search for them The display of facets includes a link to the empty values, but it isn't visible
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