Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly
Summary: Complete-subfield searches TraceCompleteSubfields syspref not working correctly
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: 3.6
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Magnus Enger
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 7537
  Show dependency treegraph
 
Reported: 2011-10-26 15:02 UTC by Thatcher Rea
Modified: 2013-12-05 19:57 UTC (History)
5 users (show)

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


Attachments
Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly (9.27 KB, patch)
2012-03-05 13:44 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly (9.33 KB, patch)
2012-03-08 03:51 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly (12.10 KB, patch)
2012-03-09 12:43 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly (12.14 KB, patch)
2012-03-10 15:18 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly (12.20 KB, patch)
2012-03-10 15:21 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly (12.99 KB, patch)
2012-03-16 13:23 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly (13.12 KB, patch)
2012-03-16 13:36 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Thatcher Rea 2011-10-26 15:02:41 UTC
If TraceCompleteSubfields in 3.6 is set to force, clicking on the subject search links for a given bib generates a URL that looks similar to the following: 

opac-search.pl?q=(su,complete-subfield:{Cryptography})%20and%20(su,complete-subfield:{Juvenile%20literature.})

This will return no results when performing a subfield search for a given bib.

I set TraceCompleteSubfields to "Don't force" and was able to search subjects again.
Comment 1 Joy Nelson 2012-02-27 14:05:06 UTC
Note: 
This system preference seems to fail on specific subject headings.  It works on the 650 subject heading, but not the 651 subject heading.
Comment 2 Magnus Enger 2012-03-02 12:56:08 UTC
<magnuse> jcamins: got any hunches about bug 7092? like where to start digging? 
<jcamins> magnuse: yes. Try changing the {} quotes to "".
<jcamins> magnuse: alternatively, use complete-field instead of complete-subfield.
<magnuse> ok, that gives me somewhere to start - thank you very much! 
<magnuse> jcamins: chaning {} to "" seems to solve it! 
<jcamins> magnuse: yeah, Search.pm doesn't play well with ICU.
<magnuse> jcamins: so a patch to replace {} with "" in the XSLT would solve bug 7092? or is it more of a quick and dirty workaround? 
<jcamins> magnuse: alas, it will *cause* bug 7092 on the installations that are still working.
<magnuse> jcamins: ?? 
<magnuse> sounds like catch 22 or something, then...
<jcamins> magnuse: C4::Search can't handle regular quotes with ICU enabled.
<jcamins> magnuse: without ICU, C4::Search apparently can't handle {}
<magnuse> ah, so what should be used depends on whether icu is used or not? 
<jcamins> That's what I've concluded.
<magnuse> nice
<jcamins> And, of course, you can't tell whether ICU is enabled.
<magnuse> what to do then? add a syspref to choose {} or ""? 
<jcamins> magnuse: that's the best idea I can come up with.
<magnuse> hm, i might try and add the syspref, then
<magnuse> probably better to call it e.g. TraceCompleteSubfieldsQuoting than UseICU, right? cause it wouldn't actually influence the indexing...
<jcamins> Better TracingQuotes or something like that.
Comment 3 Magnus Enger 2012-03-05 10:30:26 UTC
Looks like UNIMARC does not implement TraceSubjectSubdivisions and TraceCompleteSubfields, so I'll just do MARC21. Please correct me if I'm missing something!
Comment 4 Magnus Enger 2012-03-05 13:44:46 UTC Comment hidden (obsolete)
Comment 5 Jared Camins-Esakov 2012-03-08 03:51:50 UTC Comment hidden (obsolete)
Comment 6 Magnus Enger 2012-03-09 12:43:37 UTC Comment hidden (obsolete)
Comment 7 Magnus Enger 2012-03-09 12:44:43 UTC
I forgot to implement TracingQuotes for the intranet. This new patch replaces the old one, and implements it for both the OPAC and the intranet.
Comment 8 Jared Camins-Esakov 2012-03-10 15:18:09 UTC Comment hidden (obsolete)
Comment 9 Jared Camins-Esakov 2012-03-10 15:21:13 UTC Comment hidden (obsolete)
Comment 10 Ian Walls 2012-03-14 15:17:05 UTC
Despite the length test plan, the change here is really pretty simple.  It just adds a new system preference on how to quote tracings, and then implements that wrapper in the MARC21 Details XSLT using variables.  Change to C4/XSLT.pm is just to make the module aware of the new preference.

Marking as Passed QA.  Still need followups for the other MARC flavours.
Comment 11 Paul Poulain 2012-03-16 11:51:16 UTC
QA comment : if i'm not mistaking, the syspref is dependant on ICU use : if you've ICU, you *must* set it to {}, if you haven't it *must* be set to ""

So I think this syspref should be called "ICU set yes/no", and it could be usefull for another use, and the TracingQuote is a wrong name
Comment 12 Magnus Enger 2012-03-16 13:23:10 UTC Comment hidden (obsolete)
Comment 13 Magnus Enger 2012-03-16 13:24:37 UTC
New patch attached, changing the name of the syspref from TracingQuotes to UseICU.
Comment 14 Jared Camins-Esakov 2012-03-16 13:36:02 UTC
Created attachment 8227 [details] [review]
Bug 7092 - Complete-subfield searches TraceCompleteSubfields syspref not working correctly

This patch adds a syspref UseICU for toggling between using "" and {} for
quoting in subject tracings. With normal Zebra indexing, "" must be used. With
ICU Zebra indexing, {} must be used.

This syspref interacts with TraceCompleteSubfields and TraceSubjectSubdivisions.

To test:

- marcflavour = MARC21
- make sure you have a record with a "complex subject", like
  "Internet -- Law and legislation"

Toggle UseICU, TraceCompleteSubfields, and TraceSubjectSubdivisions and check
that you get the expected results for the clickable links in the OPAC detail
view:

TraceCompleteSubfields   = Don't force
TraceSubjectSubdivisions = Don't include
UseICU                   = Not using
  opac-search.pl?q=su:"Internet"
UseICU                   = Using
  opac-search.pl?q=su:{Internet}

TraceCompleteSubfields   = Force
TraceSubjectSubdivisions = Don't include
UseICU                   = Not using
  opac-search.pl?q=su,complete-subfield:"Internet"
UseICU                   = Using
  opac-search.pl?q=su,complete-subfield:{Internet}

TraceCompleteSubfields   = Don't force
TraceSubjectSubdivisions = Include
UsingICU                 = Not using
  opac-search.pl?q=(su:"Internet") and (su:"Law and legislation.")
UsingICU                 = Using
  opac-search.pl?q=(su:{Internet}) and (su:{Law and legislation.})

TraceCompleteSubfields   = Force
TraceSubjectSubdivisions = Include
UseICU                   = Not using
  opac-search.pl?q=(su,complete-subfield:"Internet") and (su,complete-subfield:"Law and legislation.")
UseICU                   = Using
  opac-search.pl?q=(su,complete-subfield:{Internet}) and (su,complete-subfield:{Law and legislation.})

Check that clicking on the generated links gives the expected search results,
but with normal and ICU indexing. (I have actually not tested this with ICU, so
doing that before signing off is crucial.)

UPDATE 2012-03-16:

Changed the name of the syspref from TracingQuotes to UseICU after a suggestion
from Paul P., to make it more generally useful.

I have used "Using/Not using" instead of "Use/Don't use" in the description of
the syspref, to reflect the fact that this syspref will not change how Zebra
does its indexing - it's just a way of telling Koha how the indexing is
actually done.

I have also set UseICU = 1 in updatedatabase.pl, since that will preserve the
current behaviour of Koha, but UseICU = 0 in sysprefs.sql, since most new
installations will not have ICU activated.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 15 Jared Camins-Esakov 2012-03-16 13:41:57 UTC
I think this should probably go straight to Passed QA, as there has been no material change in the functionality code compared to the last iteration, which had passed QA. If anyone disagrees, you can change this back to "Signed off."
Comment 16 Paul Poulain 2012-03-21 10:22:03 UTC
(In reply to comment #12)
> I have also set UseICU = 1 in updatedatabase.pl, since that will preserve
> the current
> behaviour of Koha, but UseICU = 0 in sysprefs.sql, since most new
> installations will
> not have ICU activated.

mmm... If I understand correctly, it means that someone not using ICU don't has a problem before your patch, and will have after, because the default value is 1 ?
In this case, it's a bad idea : we should have libraries that have a working behaviour still working, and those with a buggy behaviour before the patch be warned that, if they change the new syspref, it will fix the behaviour

Please confirm i'm wrong or change the default value ;-)
Comment 17 Jared Camins-Esakov 2012-03-21 10:56:35 UTC
Paul,

(In reply to comment #16)
> (In reply to comment #12)
> > I have also set UseICU = 1 in updatedatabase.pl, since that will preserve
> > the current
> > behaviour of Koha, but UseICU = 0 in sysprefs.sql, since most new
> > installations will
> > not have ICU activated.
> 
> mmm... If I understand correctly, it means that someone not using ICU don't
> has a problem before your patch, and will have after, because the default
> value is 1 ?
> In this case, it's a bad idea : we should have libraries that have a working
> behaviour still working, and those with a buggy behaviour before the patch
> be warned that, if they change the new syspref, it will fix the behaviour
> 
> Please confirm i'm wrong or change the default value ;-)

Not exactly. All existing Koha installations have tracings working as if UseICU is set to 1. While this may be causing problems that would be solved by changing UseICU to 0, setting UseICU to 1 will not break any existing installations that are working, while setting UseICU to 0 might. So the problem you described with the update breaking working installations is exactly why updatedatabase needs to set UseICU to 1. ;)

Jared
Comment 18 Magnus Enger 2012-03-21 11:20:51 UTC
Thanks Jared, that was exactly what I was going to say. :-)
Comment 19 Paul Poulain 2012-03-21 13:56:42 UTC
thanks for the confirmation i was wrong ;-)

patch pushed (I haven't tested it, just checked that the updatedatabase is OK, te sysprefs.sql file can be loaded and the syspref can be set)
Comment 20 Jared Camins-Esakov 2012-05-09 16:59:19 UTC
This bug will be included in the Koha 3.6.5 release.