|
Description
David Gustafsson
2020-05-28 12:28:40 UTC
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
|