Incorrect condition for generation of sort field mappings etc, will post patch shortly.
Created attachment 68889 [details] [review] Bug 19563: Generation of sort_fields uses incorrect condition Fix incorrect condition for if clause for generating <field>__sort mappings for Elasticsearch. Also remove redundant check for same condition when generating fixer rules. Test plan: 1. Inspect current mappings for example by viewing: http://<elasticsearch_host>:9200/koha_<koha_instance_name>_biblios/_mapping. 2. If using the default configuraion only "author" has a sort field (author__sort). 4. Appy patch. 5. Reindex using rebuild_elastic_search.pl. 6. All fields except those with sort sort set to "0" should now have sort fields, which in the default configuration is all but "author".
Created attachment 69191 [details] [review] Bug 19563: Unit tests To test: 1 - Apply first patch and this patch 2 - prove -v t/db_dependent/Koha_Elasticsearch.t 3 - New tests will fail 4 - Apply followup patch 5 - Tests shuld pass
Created attachment 69192 [details] [review] Bug 19563: Followup - Restore checking sort variable This code seems a bit odd, $self->sort_fields()->{$name} is checking if there is any mapping in ES for a field, so if one marc field in an index is marked as sortable this will exists. We need to check the individual marc field to see if it should be added to the sort index here. If you apply the first patch, reindex, and view a record in es: curl -XGET 'localhost:9200/koha_kohadev_biblios/data/19/?pretty' | grep -A 10 author__sort You will see that 245$c is included in the record without that additional check Apply this, reindex again, and that field should not be included
Hi David, I added tests and found a problem, I restored the check on the field and that seems to make things work as expected. -Nick
Created attachment 69212 [details] [review] Bug 19563: Generation of sort_fields uses incorrect condition Fix incorrect condition for if clause for generating <field>__sort mappings for Elasticsearch. Also remove redundant check for same condition when generating fixer rules. Test plan: 1. Inspect current mappings for example by viewing: http://<elasticsearch_host>:9200/koha_<koha_instance_name>_biblios/_mapping. 2. If using the default configuraion only "author" has a sort field (author__sort). 4. Appy patch. 5. Reindex using rebuild_elastic_search.pl. 6. All fields except those with sort sort set to "0" should now have sort fields, which in the default configuration is all but "author". Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Created attachment 69213 [details] [review] Bug 19563: Unit tests To test: 1 - Apply first patch and this patch 2 - prove -v t/db_dependent/Koha_Elasticsearch.t 3 - New tests will fail 4 - Apply followup patch 5 - Tests shuld pass Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Created attachment 69214 [details] [review] Bug 19563: Followup - Restore checking sort variable This code seems a bit odd, $self->sort_fields()->{$name} is checking if there is any mapping in ES for a field, so if one marc field in an index is marked as sortable this will exists. We need to check the individual marc field to see if it should be added to the sort index here. If you apply the first patch, reindex, and view a record in es: curl -XGET 'localhost:9200/koha_kohadev_biblios/data/19/?pretty' | grep -A 10 author__sort You will see that 245$c is included in the record without that additional check Apply this, reindex again, and that field should not be included Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Yes, my mistake! Did not realize that there can of course be multiple marc field mappings for one search field, and need to check each one.
Did I forget to set this as Signed-Off?
Created attachment 69640 [details] [review] Bug 19563: Generation of sort_fields uses incorrect condition Fix incorrect condition for if clause for generating <field>__sort mappings for Elasticsearch. Also remove redundant check for same condition when generating fixer rules. Test plan: 1. Inspect current mappings for example by viewing: http://<elasticsearch_host>:9200/koha_<koha_instance_name>_biblios/_mapping. 2. If using the default configuraion only "author" has a sort field (author__sort). 4. Appy patch. 5. Reindex using rebuild_elastic_search.pl. 6. All fields except those with sort sort set to "0" should now have sort fields, which in the default configuration is all but "author". Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 69641 [details] [review] Bug 19563: Unit tests To test: 1 - Apply first patch and this patch 2 - prove -v t/db_dependent/Koha_Elasticsearch.t 3 - New tests will fail 4 - Apply followup patch 5 - Tests shuld pass Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 69642 [details] [review] Bug 19563: Followup - Restore checking sort variable This code seems a bit odd, $self->sort_fields()->{$name} is checking if there is any mapping in ES for a field, so if one marc field in an index is marked as sortable this will exists. We need to check the individual marc field to see if it should be added to the sort index here. If you apply the first patch, reindex, and view a record in es: curl -XGET 'localhost:9200/koha_kohadev_biblios/data/19/?pretty' | grep -A 10 author__sort You will see that 245$c is included in the record without that additional check Apply this, reindex again, and that field should not be included Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all! Pushed to stable for 17.11.01