Bug 5369 - se queries with paranthesis fail
Summary: se queries with paranthesis fail
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Katrin Fischer
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-05 18:02 UTC by barrett
Modified: 2013-12-05 19:57 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 5369: se queries with paranthesis fail (9.11 KB, patch)
2011-11-28 20:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5369: se queries with paranthesis fail (9.21 KB, patch)
2011-12-02 00:28 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 5369: Make series tracings use phrase indexes (5.43 KB, patch)
2011-12-02 00:41 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5369: Make series tracings use phrase indexes (5.50 KB, patch)
2011-12-02 06:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5369: Follow up fixing quotes (3.59 KB, patch)
2012-01-05 07:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Signed follow up for quotes (3.77 KB, patch)
2012-01-05 08:38 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 barrett 2010-11-05 18:02:17 UTC
opac-search.pl?q=se:DHEW%20publication%20no.%20%28HSM%29%2073-1804
displays a screen error: 
Can't call method "sort" on an undefined value at /usr/share/koha/lib/C4/Search.pm line 394.

And logs:
[Fri Nov 05 13:47:34 2010] [error] [client 10.161.4.254] [Fri Nov  5 13:47:34 2010] opac-search.pl: WARNING: query problem with (se=DHEW publication no. (HSM) 73-1804) not Suppress=1 ZOOM error 10014 "CCL parsing error" (addinfo: "')' expected") from diag-set 'ZOOM' at /usr/share/koha/lib/C4/Search.pm line 346., referer: <...>/koha/opac-detail.pl?biblionumber=93114

Removing the html encoded parenthesis characters %28 and %29 from the query returned the expected results.  The query is a link generated by opac-detail.pl
Comment 1 Jared Camins-Esakov 2011-01-31 20:38:18 UTC
This seems to be the result of having the syspref QueryAutoTruncate set to "automatically." Disabling QueryAutoTruncate makes these searches work.
Comment 2 D Ruth Holloway 2011-03-29 13:50:58 UTC
Not for me it doesn't, Jared.  The site I am wrestling with is using XSLT.  If you edit the search string generated by the XSLT to exclude [();], then it works, with or without QueryAutoTruncate turned on, so that may be the best way to go, at least for XSLT-based sites.
Comment 3 Jared Camins-Esakov 2011-03-29 13:56:01 UTC
It's possible to escape parentheses. I'll get a patch for this written.
Comment 4 Marcel de Rooy 2011-08-18 13:13:58 UTC
Looking at this problem, I suspect that the problem is not in the parentheses but in the use of an index (like se) together with a number of search operands between spaces.
Search.pm interprets this as a ccl query. But ZOOM does not like this query since it expects operators between the operands. In other words, it does not like query [Field1=Value1 Value2]; it does like: [Field1=Value1 or Value2] or even better: [Field1=Value1 or Field2=Value2]. (Brackets for visibility only!) 

In case you would really want to do a phrase search, you need to tell Koha something like [se,phr: "Value1 Value2"]
Note that such a phrase search with parentheses could even work:
se,phr: "(Low countries)" worked for me;
se,phr: "Low (countries)" worked as well.
But I wonder if a regular user would ever come up with such searches..

This problem should be solved by refactoring Search.pm; especially routine buildQuery is here the problem.
The Advanced Search could offer some more assistance in making the difference between word and phrase search.
Comment 5 lculber 2011-08-18 13:51:21 UTC
In our catalog, the problem is in clicking on a link from /cgi-bin/koha/opac-detail.pl  such as Vicksburg (Miss.) -- Directories. which in the MARC record would be a 651 $aVicksburg (Miss)$vDirectories.  
A search for “kw,wrdl: vicksburg miss. directories"  does work  but the parenthesis when clicking on the link from the opac-detail doesn't.
Comment 6 lculber 2011-10-20 12:41:23 UTC
the fix of enclosing in quotes for bug 6553 may also work for this.  Please test.
Comment 7 Katrin Fischer 2011-11-28 20:00:21 UTC Comment hidden (obsolete)
Comment 8 Jared Camins-Esakov 2011-12-02 00:28:12 UTC
Created attachment 6499 [details] [review]
Bug 5369: se queries with paranthesis fail

This patch does several things to make the search work better:

1) Adding "" around search terms for queries on index se.
2) Make links for 8xx obey the UseControlNumber system preference
3) Fix the indexing for 8xx fields, as they were not included in the se index before

Note: Make sure you copy the new record.abs into your koha-dev directory and reindex
before testing.

To test:
1) Add records using the following fields:
440$a
490$a ind. 1 = empty or 0
490$a ind. 1 = 1
800$a, 810$a, 811$a or 830$a

Example value taken form bug description, should only work after applying the patch:
DHEW publication no. (HSM) 73-1804

Please also test with other examples.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 9 Jared Camins-Esakov 2011-12-02 00:32:07 UTC
Nice work, Katrin! A follow-up patch from me converting the series tracings to se,phr: is forthcoming.
Comment 10 Jared Camins-Esakov 2011-12-02 00:41:16 UTC Comment hidden (obsolete)
Comment 11 Jared Camins-Esakov 2011-12-02 00:42:02 UTC
The second patch still requires sign-off. The first patch has been signed off. Please note that the second patch depends on the first patch.
Comment 12 Katrin Fischer 2011-12-02 06:33:15 UTC
Created attachment 6507 [details] [review]
[SIGNED-OFF] Bug 5369: Make series tracings use phrase indexes

By definition, series tracings should search for other records in the same
series, i.e., those with the exact same series title. This patch forces the
tracings to use phrase indexes for series tracings.

Note that this patch DOES NOT respect TraceCompleteSubfields. While that may be
desirable, it is outside the scope of the current patch. An additional follow-up
patch could be written to add support for TraceCompleteSubfields, if community
concensus is that series tracings should obey TraceCompleteSubfields.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Thx for the follow up and sign off Jared :)
Comment 13 Marcel de Rooy 2012-01-04 13:09:23 UTC
Just a few questions on the first patch before looking further:

1) Should Title-series be only in 800$t, 810$t and 811$t? It is added to $a now too..
2) From LOC: 840 - SERIES ADDED ENTRY--TITLE (R) [OBSOLETE] Should we rather delete it?
Comment 14 Jared Camins-Esakov 2012-01-04 13:22:48 UTC
(In reply to comment #13)
> Just a few questions on the first patch before looking further:
> 
> 1) Should Title-series be only in 800$t, 810$t and 811$t? It is added to $a now
> too..

That's by design. Title-series is the index used for all series searching, so if someone is interested in a series that's stored in an 800, 810, or 811, they can't search for it using the series index without that change.

> 2) From LOC: 840 - SERIES ADDED ENTRY--TITLE (R) [OBSOLETE] Should we rather
> delete it?

No, libraries still use obsolete fields all the time. I actually was just looking at a library that uses 840 
for analytics that are part of a series.
Comment 15 Marcel de Rooy 2012-01-04 14:41:40 UTC
(In reply to comment #14)
> That's by design. Title-series is the index used for all series searching, so
> if someone is interested in a series that's stored in an 800, 810, or 811, they
> can't search for it using the series index without that change.
Design? Could you please comment on the distinction between $a and $t.
Comment 16 Jared Camins-Esakov 2012-01-04 15:21:30 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > That's by design. Title-series is the index used for all series searching, so
> > if someone is interested in a series that's stored in an 800, 810, or 811, they
> > can't search for it using the series index without that change.
> Design? Could you please comment on the distinction between $a and $t.

That was poorly worded, sorry. The *patch* does that by design, because Koha incorrectly handles series indexing. 8xx$a is not a "title," per se, but Koha only accepts "Title-series" when searching for series. This would be a prime example of why I recommend a strong aperitif before looking at indexing in Koha. I would argue that the results of the change are correct, since Koha misapprehends the semantics of these indexes anyway.
Comment 17 Marcel de Rooy 2012-01-04 15:49:49 UTC
(In reply to comment #16)
> That was poorly worded, sorry. The *patch* does that by design, because Koha
> incorrectly handles series indexing. 8xx$a is not a "title," per se, but Koha
> only accepts "Title-series" when searching for series. This would be a prime
> example of why I recommend a strong aperitif before looking at indexing in
> Koha. I would argue that the results of the change are correct, since Koha
> misapprehends the semantics of these indexes anyway.

You are right about that. Quality here is a relative thing ;)
Comment 18 Marcel de Rooy 2012-01-04 15:53:17 UTC
Another QA Comment however (sorry..)

I notice that in the xslt the resulting url contains only one double quote at the beginning (meaning the character ").
Example: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/catalogue/search.pl?q=se,phr:%22Rembrandt-reeks
The last %22 is what I mean. There should be a closing one too?

Could you resolve that?
Thanks.
Comment 19 Katrin Fischer 2012-01-05 07:39:13 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2012-01-05 08:38:57 UTC
Created attachment 7049 [details] [review]
Signed follow up for quotes
Comment 21 Marcel de Rooy 2012-01-05 08:40:38 UTC
QA Comment:
See also above. Code further looks good. Issue with quotes has been resolved. Marking all three patches as Passed QA.
Comment 22 Paul Poulain 2012-01-06 13:11:42 UTC
patch pushed, please test

QA comment: checked UNIMARC record.abs, the problem does not exist