Summary: | New hook when indexing with Elasticsearch | ||
---|---|---|---|
Product: | Koha | Reporter: | Stefan Berndtsson <stefan.berndtsson> |
Component: | Plugin architecture | Assignee: | Stefan Berndtsson <stefan.berndtsson> |
Status: | Failed QA --- | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | dcook, f.demians, fridolin.somers, m.de.rooy, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 31734 | ||
Attachments: |
Bug 31897: New before_index_action hook for Elasticsearch
Bug 31897: New before_index_action hook for Elasticsearch Bug 31897: New before_index_action hook for Elasticsearch Bug 31897: New before_index_action hook for Elasticsearch |
Description
Stefan Berndtsson
2022-10-20 16:45:18 UTC
Created attachment 142268 [details] [review] Bug 31897: New before_index_action hook for Elasticsearch Hooks added: before_index_action with new action update How to test: Run tests in t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t Sponsored by: Gothenburg University Library Created attachment 142280 [details] [review] Bug 31897: New before_index_action hook for Elasticsearch Hooks added: before_index_action with new action update How to test: Run tests in t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t Sponsored by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> FAIL t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t OK critic FAIL file permissions File must have the exec flag Sorry, but it should not take you too much time to get those four back in the queue ;) Created attachment 142308 [details] [review] Bug 31897: New before_index_action hook for Elasticsearch Hooks added: before_index_action with new action update How to test: Run tests in t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t Sponsored by: Gothenburg University Library Fixed file permissions and some trailing whitespace in this one as well. (In reply to Stefan Berndtsson from comment #5) > Fixed file permissions and some trailing whitespace in this one as well. If you only do cosmetic fixing, keep the previous signoff line and change status back to Signed off. No need to wait for another signoff imo. Created attachment 142441 [details] [review] Bug 31897: New before_index_action hook for Elasticsearch Hooks added: before_index_action with new action update How to test: Run tests in t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t Sponsored by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> $VAR1 = { 'cxn_pool' => 'Static', 'index_name' => 'koha_myclone', 'nodes' => [ 'localhost:9200' ] }; Result of Koha::SearchEngine::Elasticsearch->get_elasticsearch_params; You test that to determine if Elastic exists. Wrong test. Later you crash: Exception 'Koha::Exceptions::Elasticsearch::BadResponse' thrown 'Bad response received when submitting request to Elasticsearch' with details => No nodes are available: [http://localhost:9200], type => NoNodes $schema->storage->txn_rollback; Koha::Plugins::Methods->delete; Really, this is wrong. This still needs attention. (In reply to Marcel de Rooy from comment #8) > $VAR1 = { > 'cxn_pool' => 'Static', > 'index_name' => 'koha_myclone', > 'nodes' => [ > 'localhost:9200' > ] > }; > > Result of Koha::SearchEngine::Elasticsearch->get_elasticsearch_params; > You test that to determine if Elastic exists. > Wrong test. > > Later you crash: > Exception 'Koha::Exceptions::Elasticsearch::BadResponse' thrown 'Bad > response received when submitting request to Elasticsearch' with details => > No nodes are available: [http://localhost:9200], type => NoNodes I'm really not sure what I'm supposed to do here. This is based on what the test t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t does, with the Plugin parts added to it. That test does the same result-check of get_elasticsearch_params, and if there is no elasticsearch running (localhost:9200 or equivalent) it also fails with the NoNodes error. I can't see how I can come up with a solution to a test depending on a running Elasticsearch without having a running Elasticsearch. The current test does not like it when there is an Elastic running but Zebra is set as the engine, so it'll have to be tweaked (the existing Indexer.t works here). I did some more copy&paste from that one so I can make this test work as well regardless of that syspref, but not without actually having Elastic running. Suggestions are welcome. :) (In reply to Stefan Berndtsson from comment #11) > I'm really not sure what I'm supposed to do here. This is based on what the > test t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t does, with the > Plugin parts added to it. > > That test does the same result-check of get_elasticsearch_params, and if > there is no elasticsearch running (localhost:9200 or equivalent) it also > fails with the NoNodes error. > > I can't see how I can come up with a solution to a test depending on a > running Elasticsearch without having a running Elasticsearch. > > The current test does not like it when there is an Elastic running but Zebra > is set as the engine, so it'll have to be tweaked (the existing Indexer.t > works here). I did some more copy&paste from that one so I can make this > test work as well regardless of that syspref, but not without actually > having Elastic running. I would fail Indexer.t now too for the same reason indeed. But it is pushed already ;) I am not an Elastic expert. Please ask on IRC or dev list. (In reply to Marcel de Rooy from comment #12) > I would fail Indexer.t now too for the same reason indeed. But it is pushed > already ;) > I am not an Elastic expert. Please ask on IRC or dev list. I'll have to put this on hold for the time being. It's not within the scope of this patch, nor unfortunately within the scope of my available time and resources to deal with a solution to the generic Elasticsearch testing model. We're at the end of a release-phase now, and the actual code in this works for us. It'll have to be resumed at some later date. This looks great Ah we should give to the plugin the index name 'biblios' or 'authorities'.
Maybe with $self->index.
> a plugin can change the record so that it better suits the index
In this case we may better hook in Koha::SearchEngine::Elasticsearch::marc_records_to_documents().
There already is feature for ISBN forms and IncludeSeeFromInSearches.
See also bug 36433 |