Bug 19563 - Generation of sort_fields uses incorrect condition
Summary: Generation of sort_fields uses incorrect condition
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: David Gustafsson
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-02 12:36 UTC by David Gustafsson
Modified: 2018-06-04 20:15 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19563: Generation of sort_fields uses incorrect condition (2.58 KB, patch)
2017-11-02 13:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19563: Unit tests (2.86 KB, patch)
2017-11-17 12:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19563: Followup - Restore checking sort variable (1.74 KB, patch)
2017-11-17 12:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19563: Generation of sort_fields uses incorrect condition (2.63 KB, patch)
2017-11-17 22:17 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19563: Unit tests (2.92 KB, patch)
2017-11-17 22:17 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19563: Followup - Restore checking sort variable (1.80 KB, patch)
2017-11-17 22:17 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19563: Generation of sort_fields uses incorrect condition (2.70 KB, patch)
2017-12-08 14:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19563: Unit tests (2.98 KB, patch)
2017-12-08 14:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19563: Followup - Restore checking sort variable (1.87 KB, patch)
2017-12-08 14:10 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2017-11-02 12:36:48 UTC
Incorrect condition for generation of sort field mappings etc, will post patch shortly.
Comment 1 David Gustafsson 2017-11-02 13:11:06 UTC
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".
Comment 2 Nick Clemens 2017-11-17 12:20:12 UTC
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
Comment 3 Nick Clemens 2017-11-17 12:20:15 UTC
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
Comment 4 Nick Clemens 2017-11-17 12:22:33 UTC
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
Comment 5 David Bourgault 2017-11-17 22:17:47 UTC
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>
Comment 6 David Bourgault 2017-11-17 22:17:50 UTC
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>
Comment 7 David Bourgault 2017-11-17 22:17:52 UTC
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>
Comment 8 David Gustafsson 2017-11-20 10:36:03 UTC
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.
Comment 9 David Bourgault 2017-11-24 19:24:10 UTC
Did I forget to set this as Signed-Off?
Comment 10 Julian Maurice 2017-12-08 14:10:35 UTC
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>
Comment 11 Julian Maurice 2017-12-08 14:10:39 UTC
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>
Comment 12 Julian Maurice 2017-12-08 14:10:43 UTC
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>
Comment 13 Jonathan Druart 2017-12-11 17:53:38 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 14 Nick Clemens 2017-12-20 11:34:12 UTC
Awesome work all! Pushed to stable for 17.11.01