[Koha-bugs] [Bug 31106] Error searching for analytics in detail view

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31106

--- Comment #5 from Tomás Cohen Arazi <tomascohen at gmail.com> ---
(In reply to Martin Renvoize from comment #4)
> Comment on attachment 137185 [details] [review]
> Bug 31106: Fix links generated in XSLTs
> 
> Review of attachment 137185 [details] [review]:
> -----------------------------------------------------------------
> 
> Bit of a deeper review this time for master...
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slimUtils.xsl
> @@ +578,4 @@
> >          </xsl:if>
> >      </xsl:template>
> >  
> > +    <xsl:template name="fix_query_term">
> 
> Maybe we could call this 'quote_term' or something.. 'fix' feels a bit
> unspecific?

We are basically preparing a string so it can be used within bigger query
strings. I feel like 'prepare_query_term' is a better name. It is too
unespecific still...

> @@ +586,5 @@
> > +                    <xsl:value-of select="$term"/>
> > +                </xsl:with-param>
> > +            </xsl:call-template>
> > +        </xsl:variable>
> > +        <xsl:value-of select="str:encode-uri(translate($fixed_term, '()', ''), true())"/>
> 
> I'm a bit confused by the translate.. we're translating '()' here, but we
> were translating '/' in some places instead before this patch.. can you
> explain your choice?

That translate method is being used to remove (, ) and / alternatively in some
places. When you look at how Zebra searches work, you notice that those
characters are removed or translated into spaces for the query:

Z> f @attr 1=1033 "Uncond\"itional¿? ="
Sent searchRequest.
Received SearchResponse.
Search was a success.
Number of hits: 1, setno 1
SearchResult-1: term=Uncond cnt=1, term=itional cnt=1
records returned: 0
Elapsed: 0.011988
Z> f @attr 1=1033 "Uncond\"itional¿? )="
Sent searchRequest.
Received SearchResponse.
Search was a success.
Number of hits: 1, setno 2
SearchResult-1: term=Uncond cnt=1, term=itional cnt=1
records returned: 0
Elapsed: 0.002718
Z> f @attr 1=1033 "Uncond\"itional¿? ()="
Sent searchRequest.
Received SearchResponse.
Search was a success.
Number of hits: 1, setno 3
SearchResult-1: term=Uncond cnt=1, term=itional cnt=1
records returned: 0
Elapsed: 0.002441
Z> f @attr 1=1033 "Uncond\"itional¿?/ ()="
Sent searchRequest.
Received SearchResponse.
Search was a success.
Number of hits: 1, setno 4
SearchResult-1: term=Uncond cnt=1, term=itional cnt=1
records returned: 0
Elapsed: 0.003235

I think those translations were added because we just didn't quote the string
as this patch is doing. in fact, such 'fixes' should be engine-specific and
dealt with in the Koha::SearchEngine::{engine}::QueryBuilder module instead. I
would like to hear from David and Nick before moving on with just removing the
translates, but the feeling is... we were doing things wrong (not quoting) and
working around things...

> @@ +589,5 @@
> > +        </xsl:variable>
> > +        <xsl:value-of select="str:encode-uri(translate($fixed_term, '()', ''), true())"/>
> > +    </xsl:template>
> > +
> > +    <xsl:template name="escape_quotes">
> 
> Excellent, you renamed it here to 'escape_quotes' as opposed to
> 'remove_quotes' :)

Well, the 21.05 version of this patches actually remove the double quotes. I
might revisit that as well.

-- 
You are receiving this mail because:
You are watching all bug changes.