Bug 23630 - Elasticsearch indexing is removing field 999
Summary: Elasticsearch indexing is removing field 999
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin Somers
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-17 12:25 UTC by Fridolin Somers
Modified: 2021-06-14 21:29 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
In Koha::SearchEngine::Elasticsearch::Indexer::update_index() first arg record ids is now mandatory
Version(s) released in:
19.11.00,19.05.05


Attachments
Bug 23630: Do not remove field 999 in Elasticsearch indexing (5.53 KB, patch)
2019-09-19 15:25 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 23630: Do not remove field 999 in Elasticsearch indexing (5.60 KB, patch)
2019-09-30 12:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23630: Do not remove field 999 in Elasticsearch indexing (5.65 KB, patch)
2019-10-29 11:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23630: (QA follow-up) Fix tests (15.21 KB, patch)
2019-10-31 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2019-09-17 12:25:51 UTC
Elasticsearch indexing uses 999$c to store record id by deleting the all field first !
So you can not store anything in field 999, even in UNIMARC and even in authorities records.

Looks like it is quick fix code added to start Elasticsearch use.
Comment 1 Fridolin Somers 2019-09-19 15:25:04 UTC
Created attachment 92971 [details] [review]
Bug 23630: Do not remove field 999 in Elasticsearch indexing

Elasticsearch indexing uses 999$c to store record id by deleting the all field first !
So you can not store anything in field 999, even in UNIMARC and even in authorities records.

Looks like it is quick fix code added to start Elasticsearch use.

This behavior is disturbing and very strange for UNIMARC flavour.

This patch corrects by defining record ids mandatory in Koha::SearchEngine::Elasticsearch::Indexer::update_index().
This ids array is actually always given (except in UT).
I think it is useless to allow adding a record without its id.

Test plan :
1) Use Elasticsearch as SearchEngine
2) Create a subfield 999$z in default framework
3) Create a record with default framework
4) Enter a random string (never used in catalog) like "tototata" in 999$z
5) In Search engine configuration, define search field "subject" for 999$z
6) Rebuild record : misc/search_tools/rebuild_elasticsearch.pl -b -bn <biblionumber> -v
7) Search for the random string => You get a result
8) Optionnaly look at records in ES : <es server>:9200/<es index name>/data/<biblionumber>
Comment 2 Nick Clemens 2019-09-30 12:50:38 UTC
Created attachment 93242 [details] [review]
Bug 23630: Do not remove field 999 in Elasticsearch indexing

Elasticsearch indexing uses 999$c to store record id by deleting the all field first !
So you can not store anything in field 999, even in UNIMARC and even in authorities records.

Looks like it is quick fix code added to start Elasticsearch use.

This behavior is disturbing and very strange for UNIMARC flavour.

This patch corrects by defining record ids mandatory in Koha::SearchEngine::Elasticsearch::Indexer::update_index().
This ids array is actually always given (except in UT).
I think it is useless to allow adding a record without its id.

Test plan :
1) Use Elasticsearch as SearchEngine
2) Create a subfield 999$z in default framework
3) Create a record with default framework
4) Enter a random string (never used in catalog) like "tototata" in 999$z
5) In Search engine configuration, define search field "subject" for 999$z
6) Rebuild record : misc/search_tools/rebuild_elasticsearch.pl -b -bn <biblionumber> -v
7) Search for the random string => You get a result
8) Optionnaly look at records in ES : <es server>:9200/<es index name>/data/<biblionumber>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 3 Nick Clemens 2019-09-30 12:53:07 UTC
Following the test plan worked, additionally I forced the record to have a 999c and 999d that did not match the biblionumber - the only issue I found was that the links were odd in the opac, however, they still worked,

If someone has this issue with their records, it is something they should fix, and we shouldn't clear the entire field for a rare occurrence.

Editing the record in Koha basic editor strips the 999c and 999d as it is.
Comment 4 Martin Renvoize 2019-10-29 11:09:59 UTC
Created attachment 94823 [details] [review]
Bug 23630: Do not remove field 999 in Elasticsearch indexing

Elasticsearch indexing uses 999$c to store record id by deleting the all field first !
So you can not store anything in field 999, even in UNIMARC and even in authorities records.

Looks like it is quick fix code added to start Elasticsearch use.

This behavior is disturbing and very strange for UNIMARC flavour.

This patch corrects by defining record ids mandatory in Koha::SearchEngine::Elasticsearch::Indexer::update_index().
This ids array is actually always given (except in UT).
I think it is useless to allow adding a record without its id.

Test plan :
1) Use Elasticsearch as SearchEngine
2) Create a subfield 999$z in default framework
3) Create a record with default framework
4) Enter a random string (never used in catalog) like "tototata" in 999$z
5) In Search engine configuration, define search field "subject" for 999$z
6) Rebuild record : misc/search_tools/rebuild_elasticsearch.pl -b -bn <biblionumber> -v
7) Search for the random string => You get a result
8) Optionnaly look at records in ES : <es server>:9200/<es index name>/data/<biblionumber>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2019-10-29 11:10:35 UTC
Seems like a sane enough change, works as expected and passes qa scripts.

Passing QA
Comment 6 Martin Renvoize 2019-10-29 12:17:00 UTC
Nice work!

Pushed to master for 19.11.00
Comment 7 Nick Clemens 2019-10-31 13:24:24 UTC
Created attachment 94912 [details] [review]
Bug 23630: (QA follow-up) Fix tests

marc_records_to_documents is now an arrayref of hashes, not an arrayref of arrays

_sanitise_records has been removed, we don't need those tests anymore
Comment 8 Martin Renvoize 2019-10-31 13:37:32 UTC
Nice work!

Pushed to master for 19.11.00
Comment 9 Fridolin Somers 2019-11-08 15:59:41 UTC
Pushed to 19.05.x for 19.05.05