Bug 19580

Summary: Elasticsearch: QueryAutoTruncate exclude period as splitting character in autotruncation
Product: Koha Reporter: Nick Clemens <nick>
Component: Searching - ElasticsearchAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: david.bourgault, fridolin.somers, glasklas, m.de.rooy, severine.queune
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19559    
Bug Blocks:    
Attachments: Bug 19580: Unit tests
Bug 19580: Exclude period as splitting character for autotrunction
Bug 19580: Unit tests
Bug 19580: Exclude period as splitting character for autotrunction
Bug 19580: Unit tests
Bug 19580: Exclude period as splitting character for autotrunction

Description Nick Clemens 2017-11-04 01:27:22 UTC

    
Comment 1 Nick Clemens 2017-11-04 01:31:45 UTC
Created attachment 68942 [details] [review]
Bug 19580: Unit tests
Comment 2 Nick Clemens 2017-11-04 01:31:47 UTC
Created attachment 68943 [details] [review]
Bug 19580: Exclude period as splitting character for autotrunction

To test:
0 - Apply Unit test patch only
1 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
2 - Should fail
3 - Apply this patch
4 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
5 - should pass
6 - search for 'Local-number.raw:"4"' (or a vlid biblionumber)
7 - should get expected result
Comment 3 David Gustafsson 2017-11-04 21:51:23 UTC
I think you need to escape that hyphen unless its the last character within the character class brackets (like this: my @tokens = split /((?:[\w\-.]+:)?"[^"]+"|\s+)/, $query;)
Comment 4 David Gustafsson 2017-11-10 13:41:52 UTC
Tried making this change myself but got "'fatal: sha1 information is lacking or useless“ when apply a patch" when running git bz apply. Perhaps best to wait until https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19559 has been commited and then reroll this patch on master.
Comment 5 David Bourgault 2017-11-10 21:06:27 UTC
Created attachment 69094 [details] [review]
Bug 19580: Unit tests

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 6 David Bourgault 2017-11-10 21:06:29 UTC
Created attachment 69095 [details] [review]
Bug 19580: Exclude period as splitting character for autotrunction

To test:
0 - Apply Unit test patch only
1 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
2 - Should fail
3 - Apply this patch
4 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
5 - should pass
6 - search for 'Local-number.raw:"4"' (or a vlid biblionumber)
7 - should get expected result

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 7 David Bourgault 2017-11-10 21:07:48 UTC
Tests passed
Works as described

QA tools threw errors, as David Gustafsson said. I corrected and amend the patch accordingly.

QA tools passed green
Comment 8 David Gustafsson 2017-12-05 14:34:34 UTC
Looks good! Have also tested the patch and everything works as expected.
Comment 9 Marcel de Rooy 2018-01-12 09:39:55 UTC
Created attachment 70455 [details] [review]
Bug 19580: Unit tests

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2018-01-12 09:40:04 UTC
Created attachment 70456 [details] [review]
Bug 19580: Exclude period as splitting character for autotrunction

To test:
0 - Apply Unit test patch only
1 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
2 - Should fail
3 - Apply this patch
4 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
5 - should pass
6 - search for 'Local-number.raw:"4"' (or a vlid biblionumber)
7 - should get expected result

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2018-01-12 09:41:05 UTC
Looks good to me. Not tested with ES. No harm expected.
Comment 12 Jonathan Druart 2018-01-15 15:25:32 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 13 Nick Clemens 2018-01-23 04:06:59 UTC
Awesome work all!

Pushed to Stable for 17.11.02
Comment 14 Fridolin Somers 2018-01-30 07:58:57 UTC
Pushed to 17.05.x for v17.05.09