Bug 25030

Summary: IncludeSeeFromInSearches not honoured in Elasticsearch
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Searching - ElasticsearchAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: aholt, e.betemps, f.demians, fanny.mion-mouton, fridolin.somers, heather_hernandez, jonathan.druart, julian.maurice, kyle, lucy.vaux-harvey, marjorie.barry-vila, nick, nicolas.legrand, patrick.robitaille, severine.queune, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25028
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Feature enabled by system preference IncludeSeeFromInSearches was implemented in Zebra search engine but not in Elasticsearch. This feature allows in bibliographic searches to match also on authorities see from (non-preferred form) headings.
Version(s) released in:
21.11.00,21.05.05,20.11.11
Attachments: Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch
Bug 25030: (QA follow-up) Add POD
Bug 25030: (QA follow-up) Fix tests

Description Tomás Cohen Arazi 2020-03-31 17:42:35 UTC
In Elasticsearch there are some things lacking:

- Using the EmbedSeeFromHeadings RecordProcessor filter on indexing
- Probably add a record a normalizer (a RecordProcessor filter?) that strips the 100a that has ind1=z from the results when retrieving from ES. In Zebra we only do that for the facets, not sure what's the best approach to be honest.
Comment 1 Frédéric Demians 2020-04-28 08:11:41 UTC
This is an very annoying regression when using ES. You're right to point the
facet building process. I don't think filtering facets is a good option. It
isn't correct already with Zebra. With ES, since there is a separate field for
facets (fieldname__facet), this could be done differently.

For ES, we could disable see forms extraction from:
  Filter::MARC::EmbedSeeFromHeadings
and rather delegate populating ES fields with see forms to:
  Koha::SearchEngine::ElasticSearch::marc_records_to_documents

This way fieldname__facet fields could be populating only with heading,
skipping all see forms (put only in fieldname field).

Does it sound correct to you?
Comment 2 Tomás Cohen Arazi 2020-04-28 12:26:32 UTC
(In reply to Frédéric Demians from comment #1)
> This is an very annoying regression when using ES. You're right to point the
> facet building process. I don't think filtering facets is a good option. It
> isn't correct already with Zebra. With ES, since there is a separate field
> for
> facets (fieldname__facet), this could be done differently.
> 
> For ES, we could disable see forms extraction from:
>   Filter::MARC::EmbedSeeFromHeadings
> and rather delegate populating ES fields with see forms to:
>   Koha::SearchEngine::ElasticSearch::marc_records_to_documents
> 
> This way fieldname__facet fields could be populating only with heading,
> skipping all see forms (put only in fieldname field).
> 
> Does it sound correct to you?

I agree, Frederic. I was trying to understand how it got solved for ES and, to my dismay, I found it wasn't! And the same for availability filtering in my opinion.
Comment 3 Julian Maurice 2020-11-10 14:41:08 UTC
Created attachment 113415 [details] [review]
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch

Test plan:
1. Create an authority with at least a 1XX$a and a 4XX$a, for instance:
    100 $a Foo
    400 $a Bar
2. Create a biblio and add a link to this authority using the
   cataloguing plugin
3. Disable syspref IncludeSeeFromInSearches
4. Reindex the biblio. You should be able to find it when searching
   'Foo' but not when searching 'Bar'
5. Enable syspref IncludeSeeFromInSearches
6. Reindex the biblio. You should be able to find it when searching
   'Foo' and also when searching 'Bar'
7. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t
Comment 4 PTFS Europe Sandboxes 2021-03-11 14:52:13 UTC
Created attachment 118144 [details] [review]
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch

Test plan:
1. Create an authority with at least a 1XX$a and a 4XX$a, for instance:
    100 $a Foo
    400 $a Bar
2. Create a biblio and add a link to this authority using the
   cataloguing plugin
3. Disable syspref IncludeSeeFromInSearches
4. Reindex the biblio. You should be able to find it when searching
   'Foo' but not when searching 'Bar'
5. Enable syspref IncludeSeeFromInSearches
6. Reindex the biblio. You should be able to find it when searching
   'Foo' and also when searching 'Bar'
7. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 5 Séverine Queune 2021-03-11 14:53:04 UTC
Nice one Julian !
Comment 6 Victor Grousset/tuxayo 2021-05-15 03:12:39 UTC
Created attachment 121001 [details] [review]
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch

Test plan:
1. Create an authority with at least a 1XX$a and a 4XX$a, for instance:
    100 $a Foo
    400 $a Bar
2. Create a biblio and add a link to this authority using the
   cataloguing plugin
3. Disable syspref IncludeSeeFromInSearches
4. Reindex the biblio. You should be able to find it when searching
   'Foo' but not when searching 'Bar'
5. Enable syspref IncludeSeeFromInSearches
6. Reindex the biblio. You should be able to find it when searching
   'Foo' and also when searching 'Bar'
   without the patch, 'Bar' doesn't yeld results. With it, it does.
7. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 7 Victor Grousset/tuxayo 2021-05-15 03:25:11 UTC
Hi :)
Tried to double test on this, manual test plan went well, added a line on step 6 to make more clear what is expected.

But on the automated test I get:
#   Failed test 'No tests run for subtest "Koha::SearchEngine::Elasticsearch::marc_records_to_documents with IncludeSeeFromInSearches"'
#   at t/db_dependent/Koha/SearchEngine/Elasticsearch.t line 927.
Can't use string ("Can't locate Search/Elasticsearc"...) as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch/Indexer.pm line 135.

With ES 5 and ES 6, using koha-testing-docker.

The full message can be found by adding debug statements in
Koha/SearchEngine/Elasticsearch/Indexer.pm
line 127 at the begin of the catch.
(that was tedious I moved a `die;` statement everywhere, I'm sure there is a better way and I wish there was a way to get the full exception text)

'Can\'t locate Search/Elasticsearch/CxnPool/Fluid.pm in @INC (you may need to install the Search::Elasticsearch::CxnPool::Fluid module)

This Fluid module seems to not exists.

In
t/db_dependent/Koha/SearchEngine/Elasticsearch.t
line 110
if we change
cxn_pool => 'Fluid'
to
cxn_pool => 'Fluid_LOL_LOL_LOL'

we get
'Can\'t locate Search/Elasticsearch/CxnPool/Fluid_LOL_LOL_LOL.pm in @INC (you may need to install the Search::Elasticsearch::CxnPool::Fluid_LOL_LOL_LOL module)

No idea what this means o_o
Comment 8 PTFS Europe Sandboxes 2021-05-17 10:30:53 UTC
Created attachment 121036 [details] [review]
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch

Test plan:
1. Create an authority with at least a 1XX$a and a 4XX$a, for instance:
    100 $a Foo
    400 $a Bar
2. Create a biblio and add a link to this authority using the
   cataloguing plugin
3. Disable syspref IncludeSeeFromInSearches
4. Reindex the biblio. You should be able to find it when searching
   'Foo' but not when searching 'Bar'
5. Enable syspref IncludeSeeFromInSearches
6. Reindex the biblio. You should be able to find it when searching
   'Foo' and also when searching 'Bar'
   without the patch, 'Bar' doesn't yeld results. With it, it does.
7. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Lucy Vaux-Harvey <lucy.vaux-harvey@ptfs-europe.com>
Comment 9 Victor Grousset/tuxayo 2021-05-18 22:08:57 UTC
Not ready for QA, the two tests on sandboxes can't have ran the test and the 
test on comment 7 was a failure.
Comment 10 Séverine Queune 2021-09-30 14:23:40 UTC
Hi everyone !
Any chance to see this patch updated soon ?
Comment 11 Nick Clemens 2021-10-18 11:10:43 UTC
Created attachment 126426 [details] [review]
Bug 25030: Implement IncludeSeeFromInSearches with Elasticsearch

Test plan:
1. Create an authority with at least a 1XX$a and a 4XX$a, for instance:
    100 $a Foo
    400 $a Bar
2. Create a biblio and add a link to this authority using the
   cataloguing plugin
3. Disable syspref IncludeSeeFromInSearches
4. Reindex the biblio. You should be able to find it when searching
   'Foo' but not when searching 'Bar'
5. Enable syspref IncludeSeeFromInSearches
6. Reindex the biblio. You should be able to find it when searching
   'Foo' and also when searching 'Bar'
   without the patch, 'Bar' doesn't yeld results. With it, it does.
7. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Lucy Vaux-Harvey <lucy.vaux-harvey@ptfs-europe.com>
Comment 12 Nick Clemens 2021-10-18 11:10:47 UTC
Created attachment 126427 [details] [review]
Bug 25030: (QA follow-up) Add POD
Comment 13 Nick Clemens 2021-10-18 11:14:52 UTC
(In reply to Victor Grousset/tuxayo from comment #7)
> 'Can\'t locate Search/Elasticsearch/CxnPool/Fluid.pm in @INC (you may need
> to install the Search::Elasticsearch::CxnPool::Fluid module)
> 
> This Fluid module seems to not exists.
> 
> In
> t/db_dependent/Koha/SearchEngine/Elasticsearch.t
> line 110
> if we change
> cxn_pool => 'Fluid'
> to
> cxn_pool => 'Fluid_LOL_LOL_LOL'
> 
> we get
> 'Can\'t locate Search/Elasticsearch/CxnPool/Fluid_LOL_LOL_LOL.pm in @INC
> (you may need to install the
> Search::Elasticsearch::CxnPool::Fluid_LOL_LOL_LOL module)
> 
> No idea what this means o_o

Well, the second makes sense, you have made up a moduel that doesn't exist :-)

For the first, I suspect a configuration issue, I cannot recreate the failures

(In reply to Victor Grousset/tuxayo from comment #9)
> Not ready for QA, the two tests on sandboxes can't have ran the test and the 
> test on comment 7 was a failure.

Cannot recreate, passing QA

I will note, there is some duplicated code here, but I think fixing the issue is important, handling these fields separately makes sense. Could generate a subroutine with options in future, but I think this is straightforward and readable.
Comment 14 Victor Grousset/tuxayo 2021-10-18 20:44:13 UTC
This is weird, now I get this error:

root@kohadevbox:koha(testing-SO-QA)$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t
t/db_dependent/Koha/SearchEngine/Elasticsearch.t .. 4/7     # No tests run!

#   Failed test 'No tests run for subtest "Koha::SearchEngine::Elasticsearch::marc_records_to_documents with IncludeSeeFromInSearches"'
#   at t/db_dependent/Koha/SearchEngine/Elasticsearch.t line 927.
Undefined subroutine &main::AddAuthority called at t/db_dependent/Koha/SearchEngine/Elasticsearch.t line 874.
# Looks like your test exited with 11 just after 7.
Comment 15 Jonathan Druart 2021-10-19 07:37:18 UTC
Tests are not passing for me either.

Undefined subroutine &main::AddAuthority called at /kohadevbox/koha/t/db_dependent/Koha/SearchEngine/Elasticsearch.t line 874.

then, after the following change:
-use C4::AuthoritiesMarc;
+use C4::AuthoritiesMarc qw( AddAuthority );

I get:
#   Failed test 'No tests run for subtest "Koha::SearchEngine::Elasticsearch::marc_records_to_documents with IncludeSeeFromInSearches"'
#   at /kohadevbox/koha/t/db_dependent/Koha/SearchEngine/Elasticsearch.t line 927.
Can't use string ("Can't locate Search/Elasticsearc"...) as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch/Indexer.pm line 130.

After the removing the try in update_index:
Can't locate Search/Elasticsearch/CxnPool/Fluid.pm
Comment 16 Nick Clemens 2021-10-19 10:37:58 UTC
Created attachment 126479 [details] [review]
Bug 25030: (QA follow-up) Fix tests

Fluid is not a valid value for cxn_pool - valid values are:
Sniff,Static,NoPing

AddAuthority call calls index - the indexing call was trying to access
the ES server with mocked values and dying when they were invalid

This patch uses a correct value for cxn_pool and inserts directly into DB
to avoid indexing
Comment 17 Jonathan Druart 2021-10-21 08:32:45 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 18 Victor Grousset/tuxayo 2021-10-21 23:30:12 UTC
Works also for me, thanks all :)
Comment 19 Séverine Queune 2021-10-22 12:17:19 UTC
Thank you guys for working on this one !!
You're so awesome :D
Comment 20 Kyle M Hall 2021-10-22 15:26:16 UTC
Pushed to 21.05.x for 21.05.05
Comment 21 Fridolin Somers 2021-10-23 01:50:48 UTC
Pushed to 20.11.x for 20.11.11
Comment 22 Victor Grousset/tuxayo 2021-10-26 01:05:05 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
Comment 23 Nicolas Legrand 2021-11-29 14:27:26 UTC
A big thank you to everyone involved. We set this functionality yesterday and everyone is thrilled at the BULAC library. It was a most wanted feature here and now we can type 'Omar H̱ayyām,  Omar Khayyám,  'Umar-i-Khayyám, عمر الخيام أبو الفتح غياث الدين or ʿUmar al-H̱ayyām abū al-Fatḥ Ġayāṯ al-Dīn and find all books attached to authority Omar Khayyam.

I’ve just typed `apt upgrade` for our 20.11, set a syspref, reindex the biblios and now I’m a hero, thanks to all of you!

Thank you, thank you, thank you!