Bug 19580 - Elasticsearch: QueryAutoTruncate exclude period as splitting character in autotruncation
Summary: Elasticsearch: QueryAutoTruncate exclude period as splitting character in aut...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 19559
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-04 01:27 UTC by Nick Clemens
Modified: 2018-06-04 20:15 UTC (History)
5 users (show)

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


Attachments
Bug 19580: Unit tests (1.91 KB, patch)
2017-11-04 01:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19580: Exclude period as splitting character for autotrunction (1.30 KB, patch)
2017-11-04 01:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19580: Unit tests (1.97 KB, patch)
2017-11-10 21:06 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19580: Exclude period as splitting character for autotrunction (1.69 KB, patch)
2017-11-10 21:06 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19580: Unit tests (2.06 KB, patch)
2018-01-12 09:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19580: Exclude period as splitting character for autotrunction (1.78 KB, patch)
2018-01-12 09:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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