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.
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?
(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.
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
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>
Nice one Julian !
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>
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
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>
Not ready for QA, the two tests on sandboxes can't have ran the test and the test on comment 7 was a failure.
Hi everyone ! Any chance to see this patch updated soon ?
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>
Created attachment 126427 [details] [review] Bug 25030: (QA follow-up) Add POD
(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.
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.
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
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
Pushed to master for 21.11, thanks to everybody involved!
Works also for me, thanks all :)
Thank you guys for working on this one !! You're so awesome :D
Pushed to 21.05.x for 21.05.05
Pushed to 20.11.x for 20.11.11
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
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!