Bug 29561 - Remove blank facet for languages
Summary: Remove blank facet for languages
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Katrin Fischer
URL:
Keywords:
: 31668 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-11-23 14:44 UTC by Nick Clemens
Modified: 2023-12-28 20:44 UTC (History)
7 users (show)

See Also:
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 (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
Bug 29561: Unit test (2.22 KB, patch)
2022-10-08 11:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29561: Remove blank facets (1.89 KB, patch)
2022-10-08 11:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29561: (follow-up) Don't remove facet '0' (1.29 KB, patch)
2022-10-08 11:01 UTC, Katrin Fischer
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'
Comment 11 Katrin Fischer 2022-10-08 10:51:59 UTC
Stealing this QA from Martin ;)
Comment 12 Katrin Fischer 2022-10-08 11:01:09 UTC
Turning on displayFacetCount helps to make this more visible.
Comment 13 Katrin Fischer 2022-10-08 11:01:42 UTC
Created attachment 141543 [details] [review]
Bug 29561: Unit test

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2022-10-08 11:01:46 UTC
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>
Comment 15 Katrin Fischer 2022-10-08 11:01:51 UTC
Created attachment 141545 [details] [review]
Bug 29561: (follow-up) Don't remove facet '0'

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Tomás Cohen Arazi 2022-10-11 14:21:22 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 17 Nick Clemens 2022-10-12 13:13:25 UTC
*** Bug 31668 has been marked as a duplicate of this bug. ***
Comment 18 Lucas Gass 2022-12-05 18:20:09 UTC
Backported to 22.05.x for upcoming 22.05.08
Comment 19 Arthur Suzuki 2022-12-14 14:34:11 UTC
applied to 21.11 for 21.11.15
Comment 20 wainuiwitikapark 2023-03-15 00:52:33 UTC
Not backported to 21.05.x