Since valid boolean operators in the Elasticsearch mini language for Query String Queries needs to be upper case, there some occurrences of queries in Koha left that needs changing. I think I found most of them.
Created attachment 105411 [details] [review] Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch Uppercase occurances of all (hopefully) lowercase "and" used in ElasticSearch Query String Query contexts
Regarding how to test. I have tested the fix in bulkmarimport.pl but regarding the others I am not all that familiar with Koha from a user-perspective and it would take me too long to write an how to test for all of them. One thing that could go wrong is that operator precedence for AND/OR is not what you expect and parentheses needs to be used for queries mixing them both. I tried to review all replacements and i THINK that AND/OR is not mixed in those places so should be ok and no extra precedence tweaking needed. Also think this will fix more than it could potentially break, but help in writing an how to test would otherwise be appreciated if this feels to risky to merge without one.
The same issue has been address in bug 23322 och bug 24506, but some instances of where operator case need to be changed remains and is addressed in this patch.
Oh I've been searching hours on that bug. Need to be fixed.
Created attachment 130029 [details] [review] Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch Uppercase occurances of all (hopefully) lowercase "and" used in ElasticSearch Query String Query contexts Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
I found one more in C4::Search::_build_initial_query
This looks all good, but I think we might need to fix tests: t/db_dependent/Search.t .. # Failed test 'Built composed implicit-and CCL keyword query' # at t/db_dependent/Search.t line 423. # 'kw,wrdl=(rk=(salud)) AND kw,wrdl=(rk=(higiene)) ' # doesn't match '(?^u:kw\W.*salud\W.*and.*kw\W.*higiene)' # Looks like you failed 1 test of 90. t/db_dependent/Search.t .. 1/3 # Failed test 'MARC21 + DOM' # at t/db_dependent/Search.t line 906. t/db_dependent/Search.t .. 2/3 # Looks like you failed 1 test of 3. t/db_dependent/Search.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/3 subtests Test Summary Report ------------------- t/db_dependent/Search.t (Wstat: 256 Tests: 3 Failed: 1) Failed test: 1 Non-zero exit status: 1
Created attachment 130588 [details] [review] Bug 25616: Fix t/db_dependent/Search.t
Created attachment 130589 [details] [review] Bug 25616: (follow-up) Use uppercase in advanced search booleans
Seems logical to use uppercase in advanced search page OPAC and staff
Created attachment 130914 [details] [review] Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch Uppercase occurances of all (hopefully) lowercase "and" used in ElasticSearch Query String Query contexts Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 130915 [details] [review] Bug 25616: Fix t/db_dependent/Search.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 130916 [details] [review] Bug 25616: (follow-up) Use uppercase in advanced search booleans Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 131019 [details] [review] Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch Uppercase occurances of all (hopefully) lowercase "and" used in ElasticSearch Query String Query contexts Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131020 [details] [review] Bug 25616: Fix t/db_dependent/Search.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131021 [details] [review] Bug 25616: (follow-up) Use uppercase in advanced search booleans Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131022 [details] [review] Bug 25616: (QA follow-up) Catch a case in labels/label-item-search.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Good catch, no regressions found and QA scripts are happy. I found another obvious case, and there are lots more `' and '` occurrences in the codebase that a possible cases of this. I think we should push this one as is and handle follow-ups in more bugs.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Patches need rebased for 21.11.x if backport is needed.
Created attachment 131161 [details] [review] Bug 25616: [21.11.x] Uppercase hard coded lower case boolean operators for Elasticsearch Uppercase occurances of all (hopefully) lowercase "and" used in ElasticSearch Query String Query contexts Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131162 [details] [review] Bug 25616: [21.11.x] Fix t/db_dependent/Search.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131163 [details] [review] Bug 25616: [21.11.x] (follow-up) Use uppercase in advanced search booleans Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131164 [details] [review] Bug 25616: [21.11.x] (QA follow-up) Catch a case in labels/label-item-search.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Kyle M Hall from comment #20) > Patches need rebased for 21.11.x if backport is needed. Here we go. Conflict solved on Koha::Biblio::get_marc_authors still in C4::Biblio::GetMarcAuthors
Ah wait, t/Search.t/ is failling : # Failed test '$query built correctly (no operator)' # at t/Search.t line 90. # got: 'query AND parsed_operand' # expected: 'query and parsed_operand' # Failed test '$query_cgi built correctly (no operator)' # at t/Search.t line 92. # got: 'query_cgi&op=%20AND%20&idx=index&q=original_operand' # expected: 'query_cgi&op=%20and%20&idx=index&q=original_operand' # Failed test '$query_desc build correctly (no operator)' # at t/Search.t line 94. # got: 'query_desc AND index_plus original_operand' # expected: 'query_desc and index_plus original_operand' # Looks like you failed 3 tests of 20.
Created attachment 131292 [details] [review] Bug 25616: (follow-up) Fix t/Search.t
Created attachment 131293 [details] [review] Bug 25616: [21.11.x] (follow-up) Fix t/Search.t
Bug 25616: (follow-up) Fix t/Search.t pushed to master
Pushed to 21.11.x for 21.11.04
I'm getting these failures on the qa script when applying the 21.11 patches to 21.05: FAIL t/Search.t FAIL valid Test2::Hub::plan(Test2::Hub=HASH(0x55c009a7ad60), "SKIP") called Test2::Hub::process(Test2::Hub=HASH(0x55c009a7ad60), Test2::Event::Plan=HASH(0x55c00c82a630)) called Test2::Hub::send(Test2::Hub=HASH(0x55c009a7ad60), Test2::Event::Plan=HASH(0x55c00c82a630)) called Test2::API::Context::send_event(Test2::API::Context=HASH(0x55c00c831440), "Plan", "max", 0, "directive", "SKIP", "reason", "Skipping remaining tests since we don't have a schema") called Test2::API::Context::plan(Test2::API::Context=HASH(0x55c00c831440), 0, "SKIP", "Skipping remaining tests since we don't have a schema") called Test::Builder::skip_all(Test::Builder=HASH(0x55c0095c7af0), "Skipping remaining tests since we don't have a schema") called Test::DBIx::Class::_initialize_schema("Test::DBIx::Class", HASH(0x55c00a915a80)) called