http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- Created attachment 28917 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=28917&action=edit Bug 12443 - Initial re-factoring of buildQuery -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #28917|0 |1 is obsolete| | --- Comment #2 from David Cook <dcook@prosentient.com.au> --- Created attachment 28918 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=28918&action=edit Bug 12443 - Initial re-factoring of buildQuery This patch reduces three repeated code fragments into a single internal subroutine, which is easier to read, has comments, and should make it easier to refactor more buildQuery code in the future. _TEST PLAN_ Before applying 1) Run a bunch of different searches in the staff client and OPAC in separate tabs 2) Apply the patch 3) Run the same searches again (maybe in yet more tabs) and notice that the results are exactly the same. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- You should probably turn off TryQueryParser when doing testing as well. While QueryParser sometimes falls back to non-QueryParser, it's just easier if you turn it off explicitly for testing purposes. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |bgkriegel@gmail.com -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #28927|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 29116 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29116&action=edit [PASSED QA] Bug 12443 - Initial re-factoring of buildQuery This patch reduces three repeated code fragments into a single internal subroutine, which is easier to read, has comments, and should make it easier to refactor more buildQuery code in the future. _TEST PLAN_ Before applying 1) Run a bunch of different searches in the staff client and OPAC in separate tabs 2) Apply the patch 3) Run the same searches again (maybe in yet more tabs) and notice that the results are exactly the same. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Same results, no errors. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi from comment #6)

I'd like to see some unit tests for such a sensitive core functionality. I know it is an almost trivial change, but we are introducing a new function to C4::Search which needs to be documented (ut-driven documentation).

It does, OTOH, look fine to me and would push it soon. If help is needed on writing those tests don't hesitate to write me.

I was thinking about unit tests when I wrote the patch, but I'm not really sure what sort of test would be appropriate for this type of function. I can write the test, but I don't know what to test, if that makes sense. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi from comment #8)

(In reply to David Cook from comment #7)

...

(In reply to Tomás Cohen Arazi from comment #6)

...

I'd like to see some unit tests for such a sensitive core functionality. I know it is an almost trivial change, but we are introducing a new function to C4::Search which needs to be documented (ut-driven documentation).

It does, OTOH, look fine to me and would push it soon. If help is needed on writing those tests don't hesitate to write me.

I was thinking about unit tests when I wrote the patch, but I'm not really sure what sort of test would be appropriate for this type of function.

I can write the test, but I don't know what to test, if that makes sense.

You could start by looking at t/db_dependent/Search.t for stuff like this:

<code> ( $error, $query, $simple_query, $query_cgi, $query_desc, $limit, $limit_cgi, $limit_desc, $stopwords_removed, $query_type ) = buildQuery([ 'and' ], [ 'salud', 'higiene' ], [], [], [], 0, 'en'); like($query, qr/kw\W.*salud\W.*and.*kw\W.*higiene/, "Built composed explicit-and CCL keyword query"); </code>

And build scenarios that cover all conditional paths.

Since I'm just changing the internals of buildQuery, shouldn't the existing plans cover that though? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 29300 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29300&action=edit Bug 12443: (QA followup) Unit tests for _build_initial_query This followup adds uint tests for the newly introduced function. It is really basic, but covers all scenarios for the new function, and will help us prevent regressions in future patches. Regards To+ Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=7518 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 --- Comment #13 from David Cook <dcook@prosentient.com.au> --- PSA on truncation with Zebra: It appears that the @truncation directive in ccl.properties doesn't work in versions before Yaz 4.2.21 (going back to an indeterminate time). "yaz-client -V" will tell you what version of Yaz you have. Debian Squeeze uses Yaz 4.0.11-1 Debian Wheezy uses Yaz 4.2.30 So...if you're on Wheezy, you're probably fine. If you're on Squeeze, your @truncation directive isn't working, which means the truncation character is the default "?" no matter what you put in ccl.properties. It does appear that you can provide multiple truncation characters based on the reference to "truncation_aliases" in the following code though: http://www.indexdata.com/yaz/dox/html/cclfind_8c_source.html --- 4.2.21 2011/11/23 Fix cql_sortby_to_sortkeys. Completely broken before. Make CCL's @truncation work again. Source: http://www.indexdata.com/yaz/doc/NEWS -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz --- Comment #15 from Robin Sheat <robin@catalyst.net.nz> --- The test case fails to build on Debian testing with: String found where operator expected at t/Search.t line 28, near "subtest "_build_initial_query tests"" (Do you need to predeclare subtest?) syntax error at t/Search.t line 28, near "subtest "_build_initial_query tests"" syntax error at t/Search.t line 158, near "}" Execution of t/Search.t aborted due to compilation errors. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to David Cook from comment #16)

(In reply to Robin Sheat from comment #15)

...

The test case fails to build on Debian testing with:

String found where operator expected at t/Search.t line 28, near "subtest "_build_initial_query tests"" (Do you need to predeclare subtest?) syntax error at t/Search.t line 28, near "subtest "_build_initial_query tests"" syntax error at t/Search.t line 158, near "}" Execution of t/Search.t aborted due to compilation errors.

Sounds like Debian testing uses an older version of Test::More than Tomas, me thinks.

Same issue below, I imagine:

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9142

I don't think it is the same problem, as we have subtests in: db_dependent/Koha.t db_dependent/Biblio.t db_dependent/Items.t db_dependent/Search.t Search.t and they are not failing in Debian 6, Debian 7, Ubuntu 12.04 and Ubuntu 14.04. Can anyone try those on Debian testing please? Also, we definitely would benefit of a Debian testing node. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 --- Comment #19 from Robin Sheat <robin@catalyst.net.nz> --- FYI, it fails in my squeeze chroot, but it passes in testing. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12443 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |12689 -- You are receiving this mail because: You are watching all bug changes.