Bug 29561 - Remove blank facet for languages
Summary: Remove blank facet for languages
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-11-23 14:44 UTC by Nick Clemens
Modified: 2022-09-22 19:54 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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:


Attachments
Bug 29561: Remove blank facets (1.73 KB, patch)
2021-11-23 14:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29561: Remove blank facets (1.77 KB, patch)
2021-11-30 03:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 29561: Remove blank facets (1.84 KB, patch)
2022-01-19 10:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29561: Unit test (2.17 KB, patch)
2022-09-22 19:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29561: Remove blank facets (1.84 KB, patch)
2022-09-22 19:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29561: (follow-up) Don't remove facet '0' (1.23 KB, patch)
2022-09-22 19:54 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-11-23 14:44:27 UTC
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
Comment 1 Nick Clemens 2021-11-23 14:48:49 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
Comment 2 David Nind 2021-11-30 03:09:47 UTC
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>
Comment 3 Jonathan Druart 2021-12-01 13:11:11 UTC
Did you try to add a test for this change?
Comment 4 Martin Renvoize 2022-01-19 10:32:30 UTC
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>
Comment 5 Martin Renvoize 2022-01-19 10:33:00 UTC
Clear code, reasonable fixme for future reference.. QA scripts happy.

Passing QA
Comment 6 Fridolin Somers 2022-01-22 01:16:19 UTC
(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.
Comment 7 Fridolin Somers 2022-01-29 06:15:21 UTC
 + next unless $t
Also this will remove facet with value zero.
This may be odd but we should allow it no ?
Comment 8 Nick Clemens 2022-09-22 19:54:47 UTC
Created attachment 140862 [details] [review]
Bug 29561: Unit test
Comment 9 Nick Clemens 2022-09-22 19:54:51 UTC
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>
Comment 10 Nick Clemens 2022-09-22 19:54:55 UTC
Created attachment 140864 [details] [review]
Bug 29561: (follow-up) Don't remove facet '0'