This patch adds a new hook "before_index_action" with the action "update". This is run on the array reference to the biblio records that are about to be updated in the Elasticsearch index. With this, a plugin can change the record so that it better suits the index without having to change the actual record in the database (something that might be prohibited or discouraged due to centralised cataloging).
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