Bug 31106

Created attachment 137183 [details] [review]
Bug 31106: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 137184 [details] [review]
Bug 31106: Make clean_search_term escape double quotes

We noticed that several characters will break Zebra queries. So search
terms need to be quoted for things to work. In this context, double
quotes inside search terms are problematic because double quotes are
what we use for quoting strings.

This patch makes the clean_search_term method escape double quotes.

To test:
1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/Koha/SearchEngine/Zebra/QueryBuilder.t
=> FAIL: It doesn't work as it should!
3. Apply this patch
4. Repeat 2
=> SUCCESS: It does the job!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 137185 [details] [review]
Bug 31106: Fix links generated in XSLTs

This patch addresses the case of links that are generated on the XSLT
side, those linking to and from host records using 773$t and 773$a.

To test:
1. Pick a bibliographic record (I chose 'Unconditional' from the sample
   data
2. Add " and ? to the title statement. I changed it to
   'Uncond"itional?¿'
3. Add a child record to it
4. Open the detail page for the host record
=> FAIL: It doesn't show the 'Show analytics' link
5. Repeat 4 for the same record, in the OPAC
=> FAIL: It doesn't show the 'Show analytics' link
6. Apply this patch
7. Repeat 4 and 5.
=> SUCCESS: Links are shown!
8. Follow the links
=> SUCCESS: The links take you to the right resultset!
9. Go to the child record, and notice the link back to the parent works
   :-D
10. Sign off :-D

Sponsored-by: Theke Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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're doing two things here actually.. we're escaping quotes and URI escaping.. then we leave the overall quoting of the whole string to the caller..

Maybe 'escape_phrase' or 'escape_term' then?

@@ +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?

@@ +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' :)

(In reply to Martin Renvoize from comment #4)
> Comment on attachment 137185 [details] [review] [review]
> Bug 31106: Fix links generated in XSLTs
> 
> Review of attachment 137185 [details] [review] [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.

Everything works as per the test plan -  can sign off, or will do so if no additional changes area required.

For the test plan it stelfm, the 'Show Analytics' link is visible before the patch (steps 4 and 5) - for both manually added and using the EasyAnaltics EasyAnaltics system preference.

(In reply to David Nind from comment #6)
> Everything works as per the test plan -  can sign off, or will do so if no
> additional changes area required.
> 
> For the test plan it stelfm, the 'Show Analytics' link is visible before the
> patch (steps 4 and 5) - for both manually added and using the EasyAnaltics
> EasyAnaltics system preference.

I'm redoing it as per a conversation with Martin. Leaving as ASSIGNED until I resubmit. No behavior changes, just putting things in the right places to aid maintainability.

Created attachment 137237 [details] [review]
Bug 31106: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 137238 [details] [review]
Bug 31106: Make clean_search_term escape double quotes

We noticed that several characters will break Zebra queries. So search
terms need to be quoted for things to work. In this context, double
quotes inside search terms are problematic because double quotes are
what we use for quoting strings.

This patch makes the clean_search_term method escape double quotes.

To test:
1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/Koha/SearchEngine/Zebra/QueryBuilder.t
=> FAIL: It doesn't work as it should!
3. Apply this patch
4. Repeat 2
=> SUCCESS: It does the job!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 137239 [details] [review]
Bug 31106: Fix links generated in XSLTs

This patch addresses the case of links that are generated on the XSLT
side, those linking to and from host records using 773$t and 773$a.

To test:
1. Pick a bibliographic record (I chose 'Unconditional' from the sample
   data
2. Add " and ? to the title statement. I changed it to
   'Uncond"itional?¿'
3. Add a child record to it
4. Open the detail page for the host record
=> FAIL: It doesn't show the 'Show analytics' link
5. Repeat 4 for the same record, in the OPAC
=> FAIL: It doesn't show the 'Show analytics' link
6. Apply this patch
7. Repeat 4 and 5.
=> SUCCESS: Links are shown!
8. Follow the links
=> SUCCESS: The links take you to the right resultset!
9. Go to the child record, and notice the link back to the parent works
   :-D
10. Sign off :-D

Sponsored-by: Theke Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 137246 [details] [review]
Bug 31106: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 137247 [details] [review]
Bug 31106: Make clean_search_term escape double quotes

We noticed that several characters will break Zebra queries. So search
terms need to be quoted for things to work. In this context, double
quotes inside search terms are problematic because double quotes are
what we use for quoting strings.

This patch makes the clean_search_term method escape double quotes.

To test:
1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/Koha/SearchEngine/Zebra/QueryBuilder.t
=> FAIL: It doesn't work as it should!
3. Apply this patch
4. Repeat 2
=> SUCCESS: It does the job!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 137248 [details] [review]
Bug 31106: Fix links generated in XSLTs

This patch addresses the case of links that are generated on the XSLT
side, those linking to and from host records using 773$t and 773$a.

To test:
1. Pick a bibliographic record (I chose 'Unconditional' from the sample
   data
2. Add " and ? to the title statement. I changed it to
   'Uncond"itional?¿'
3. Add a child record to it
4. Open the detail page for the host record
=> FAIL: It doesn't show the 'Show analytics' link
5. Repeat 4 for the same record, in the OPAC
=> FAIL: It doesn't show the 'Show analytics' link
6. Apply this patch
7. Repeat 4 and 5.
=> SUCCESS: Links are shown!
8. Follow the links
=> SUCCESS: The links take you to the right resultset!
9. Go to the child record, and notice the link back to the parent works
   :-D
10. Sign off :-D

Sponsored-by: Theke Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Raised severity, because this is basically breaking analytics linking for many users.

QAing

Please provide feedback for this change:

-                        <xsl:attribute name="href">/cgi-bin/koha/catalogue/search.pl?q=Host-item:(<xsl:value-of select="str:encode-uri(translate(marc:datafield[@tag=245]/marc:subfield[@code='a'], '/', ''), true())"/>)</xsl:attribute>

+                        <xsl:attribute name="href">/cgi-bin/koha/catalogue/search.pl?q=<xsl:value-of select="str:encode-uri($title_query, true())"/></xsl:attribute>

You remove translate ?

Similar example:
-                                <xsl:attribute name="href">/cgi-bin/koha/catalogue/search.pl?q=ti,phr:<xsl:value-of select="str:encode-uri(translate(marc:subfield[@code='t'], '()', ''), true())"/><xsl:if test="marc:subfield[@code='a']">+AND+au:<xsl:value-of select="str:encode-uri(translate(marc:subfield[@code='a'], '()', ''), true())"/></xsl:if></xsl:attribute>

+                                    <xsl:with-param name="term"><xsl:value-of select="marc:subfield[@code='t']"/></xsl:with-param>

(In reply to Marcel de Rooy from comment #16)
> Please provide feedback for this change:
> 
> -                        <xsl:attribute
> name="href">/cgi-bin/koha/catalogue/search.pl?q=Host-item:(<xsl:value-of
> select="str:encode-uri(translate(marc:datafield[@tag=245]/marc:
> subfield[@code='a'], '/', ''), true())"/>)</xsl:attribute>
> 
> +                        <xsl:attribute
> name="href">/cgi-bin/koha/catalogue/search.pl?q=<xsl:value-of
> select="str:encode-uri($title_query, true())"/></xsl:attribute>
> 
> You remove translate ?

What I said:

> 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...

We can bring them back. But the fact that some places remove parenthesis, and others the slash smells... a lot. My testing showed that they are not taken into account in Zebra, and being quoted makes the query not break, which is why those got removed. See comment 5

(In reply to Marcel de Rooy from comment #16)
> Please provide feedback for this change:
> 
> -                        <xsl:attribute
> name="href">/cgi-bin/koha/catalogue/search.pl?q=Host-item:(<xsl:value-of
> select="str:encode-uri(translate(marc:datafield[@tag=245]/marc:
> subfield[@code='a'], '/', ''), true())"/>)</xsl:attribute>
> 
> +                        <xsl:attribute
> name="href">/cgi-bin/koha/catalogue/search.pl?q=<xsl:value-of
> select="str:encode-uri($title_query, true())"/></xsl:attribute>
> 
> You remove translate ?
> 
> Similar example:
> -                                <xsl:attribute
> name="href">/cgi-bin/koha/catalogue/search.pl?q=ti,phr:<xsl:value-of
> select="str:encode-uri(translate(marc:subfield[@code='t'], '()', ''),
> true())"/><xsl:if test="marc:subfield[@code='a']">+AND+au:<xsl:value-of
> select="str:encode-uri(translate(marc:subfield[@code='a'], '()', ''),
> true())"/></xsl:if></xsl:attribute>

I traced the introduction of this translate to Katrina, bug 5959 [1] and bug 4506 [2] repectively. I'm pretty sure what I did correct. If you look at the history of the MARC21slim2intranetDetail.xsl file, you will notice we slowly removed (, ) and / as things exploded in different scenarios and last thing we did was wrapping in parenthesis. Quoting the string and escaping double quotes in it is the sensible next step :-D

[1] c0ca9f3413d4c069565595919eced6ddf02e8f96
[2] d3605d9ae8d31953df1aed6c04631a578b690fcd

I'm in agreement with Tomas here.. took a while to work through how it all fits together but his history sluething helped a lot.. the tl;dr is "we've been doing some funky escaping for years when in reality we should have been quoting instead."

I've added some text for the release notes, but not sure if I've described this correctly. Corrections and updates to the release notes text welcome!

*** Bug 30900 has been marked as a duplicate of this bug. ***

Created attachment 137831 [details] [review]
Bug 31106: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 137832 [details] [review]
Bug 31106: Make clean_search_term escape double quotes

We noticed that several characters will break Zebra queries. So search
terms need to be quoted for things to work. In this context, double
quotes inside search terms are problematic because double quotes are
what we use for quoting strings.

This patch makes the clean_search_term method escape double quotes.

To test:
1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/Koha/SearchEngine/Zebra/QueryBuilder.t
=> FAIL: It doesn't work as it should!
3. Apply this patch
4. Repeat 2
=> SUCCESS: It does the job!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 137833 [details] [review]
Bug 31106: Fix links generated in XSLTs

This patch addresses the case of links that are generated on the XSLT
side, those linking to and from host records using 773$t and 773$a.

To test:
1. Pick a bibliographic record (I chose 'Unconditional' from the sample
   data
2. Add " and ? to the title statement. I changed it to
   'Uncond"itional?¿'
3. Add a child record to it
4. Open the detail page for the host record
=> FAIL: It doesn't show the 'Show analytics' link
5. Repeat 4 for the same record, in the OPAC
=> FAIL: It doesn't show the 'Show analytics' link
6. Apply this patch
7. Repeat 4 and 5.
=> SUCCESS: Links are shown!
8. Follow the links
=> SUCCESS: The links take you to the right resultset!
9. Go to the child record, and notice the link back to the parent works
   :-D
10. Sign off :-D

Sponsored-by: Theke Solutions
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Great to see the work followed through here.. Passing QA

Pushed to master for 22.11.

Nice work everyone, thanks!

Backported to 22.05.x for 22.05.04

pushed to 21.11.x for 21.11.11

Missing dependencies for 21.05.x, it shouldn't be affected, no backport.
Nothing to document, marking resolved.