Bug 31106 - Error searching for analytics in detail view
Summary: Error searching for analytics in detail view
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Zebra (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
: 30900 (view as bug list)
Depends on: 30865
Blocks:
  Show dependency treegraph
 
Reported: 2022-07-05 18:28 UTC by Tomás Cohen Arazi
Modified: 2023-06-08 22:26 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes two issues that affect searching and links for analytics on the detail view pages for records in the staff interface and OPAC: 1. Several characters will break Zebra search engine queries, so search terms need to be quoted by the query builder for things to work. Double quotes in titles and used in search terms were not escaped, cuasing issues with results. 2. This caused links to and from host records using 773$t and 773$a to fail (not find or display the expected results). Example: Before this was fixed, for a host record with the title 'Uncond"itional?¿' and child records linked using 773$t and 773$a: - the 'Show analytics' link was not displayed in the staff interface and OPAC for the host record - the link from the child record back to the host record ('In' Title of host record (linked)) didn't work.
Version(s) released in:
22.11.00, 22.05.04, 21.11.11


Attachments
Bug 31106: Unit tests (1.35 KB, patch)
2022-07-05 19:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31106: Make clean_search_term escape double quotes (1.28 KB, patch)
2022-07-05 19:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31106: Fix links generated in XSLTs (11.43 KB, patch)
2022-07-05 19:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31106: Unit tests (1.35 KB, patch)
2022-07-06 18:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31106: Make clean_search_term escape double quotes (1.28 KB, patch)
2022-07-06 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31106: Fix links generated in XSLTs (11.59 KB, patch)
2022-07-06 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31106: Unit tests (1.39 KB, patch)
2022-07-06 20:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 31106: Make clean_search_term escape double quotes (1.32 KB, patch)
2022-07-06 20:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 31106: Fix links generated in XSLTs (11.64 KB, patch)
2022-07-06 20:27 UTC, David Nind
Details | Diff | Splinter Review
Bug 31106: Unit tests (1.46 KB, patch)
2022-07-18 16:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31106: Make clean_search_term escape double quotes (1.39 KB, patch)
2022-07-18 16:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31106: Fix links generated in XSLTs (11.70 KB, patch)
2022-07-18 16:55 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-07-05 18:28:21 UTC
Bug 30865 fixed part of the problem. We need to implement the fix in bug 29418 for master and 21.11+.
Comment 1 Tomás Cohen Arazi 2022-07-05 19:02:14 UTC
Created attachment 137183 [details] [review]
Bug 31106: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2022-07-05 19:02:18 UTC
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>
Comment 3 Tomás Cohen Arazi 2022-07-05 19:02:22 UTC
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 4 Martin Renvoize 2022-07-06 09:58:38 UTC
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' :)
Comment 5 Tomás Cohen Arazi 2022-07-06 12:52:56 UTC
(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.
Comment 6 David Nind 2022-07-06 14:04:55 UTC
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.
Comment 7 Tomás Cohen Arazi 2022-07-06 14:18:04 UTC
(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.
Comment 8 Tomás Cohen Arazi 2022-07-06 18:51:41 UTC
Created attachment 137237 [details] [review]
Bug 31106: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-07-06 18:52:02 UTC
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>
Comment 10 Tomás Cohen Arazi 2022-07-06 18:52:28 UTC
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>
Comment 11 David Nind 2022-07-06 20:26:52 UTC
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>
Comment 12 David Nind 2022-07-06 20:26:57 UTC
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>
Comment 13 David Nind 2022-07-06 20:27:02 UTC
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>
Comment 14 Tomás Cohen Arazi 2022-07-07 14:11:10 UTC
Raised severity, because this is basically breaking analytics linking for many users.
Comment 15 Marcel de Rooy 2022-07-08 06:25:46 UTC
QAing
Comment 16 Marcel de Rooy 2022-07-08 06:35:09 UTC
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>
Comment 17 Tomás Cohen Arazi 2022-07-08 13:23:31 UTC
(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
Comment 18 Tomás Cohen Arazi 2022-07-08 18:04:33 UTC
(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
Comment 19 Martin Renvoize 2022-07-11 14:37:40 UTC
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."
Comment 20 David Nind 2022-07-12 00:03:35 UTC
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!
Comment 21 Katrin Fischer 2022-07-16 19:46:03 UTC
*** Bug 30900 has been marked as a duplicate of this bug. ***
Comment 22 Martin Renvoize 2022-07-18 16:55:44 UTC
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>
Comment 23 Martin Renvoize 2022-07-18 16:55:48 UTC
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>
Comment 24 Martin Renvoize 2022-07-18 16:55:53 UTC
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>
Comment 25 Martin Renvoize 2022-07-18 16:56:40 UTC
Great to see the work followed through here.. Passing QA
Comment 26 Tomás Cohen Arazi 2022-07-18 17:42:42 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 27 Lucas Gass 2022-08-12 14:41:57 UTC
Backported to 22.05.x for 22.05.04
Comment 28 Arthur Suzuki 2022-08-16 08:46:41 UTC
pushed to 21.11.x for 21.11.11
Comment 29 Victor Grousset/tuxayo 2022-08-19 20:26:14 UTC
Missing dependencies for 21.05.x, it shouldn't be affected, no backport.
Nothing to document, marking resolved.