[Bug 19563] New: Generation of sort_fields uses incorrect condition
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Bug ID: 19563 Summary: Generation of sort_fields uses incorrect condition Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Searching - Elasticsearch Assignee: koha-bugs@lists.koha-community.org Reporter: glasklas@gmail.com Incorrect condition for generation of sort field mappings etc, will post patch shortly. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 --- Comment #1 from David Gustafsson <glasklas@gmail.com> --- Created attachment 68889 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=68889&action=edit 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". -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |glasklas@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 69191 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69191&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 --- Comment #3 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 69192 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69192&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 David Bourgault <david.bourgault@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #68889|0 |1 is obsolete| | --- Comment #5 from David Bourgault <david.bourgault@inlibro.com> --- Created attachment 69212 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69212&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 David Bourgault <david.bourgault@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69191|0 |1 is obsolete| | --- Comment #6 from David Bourgault <david.bourgault@inlibro.com> --- Created attachment 69213 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69213&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 David Bourgault <david.bourgault@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69192|0 |1 is obsolete| | --- Comment #7 from David Bourgault <david.bourgault@inlibro.com> --- Created attachment 69214 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69214&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 --- Comment #8 from David Gustafsson <glasklas@gmail.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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 David Bourgault <david.bourgault@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david.bourgault@inlibro.com Status|Needs Signoff |Signed Off --- Comment #9 from David Bourgault <david.bourgault@inlibro.com> --- Did I forget to set this as Signed-Off? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69212|0 |1 is obsolete| | --- Comment #10 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 69640 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69640&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69213|0 |1 is obsolete| | --- Comment #11 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 69641 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69641&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69214|0 |1 is obsolete| | --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 69642 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69642&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 18.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to stable for 17.11.01 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org