Bug 29418 - Error searching for analytics in detail view
Summary: Error searching for analytics in detail view
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Zebra (show other bugs)
Version: 21.05
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
: 27575 30655 (view as bug list)
Depends on: 28316
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-04 18:00 UTC by Nick Clemens
Modified: 2022-07-13 23:16 UTC (History)
13 users (show)

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


Attachments
Bug 29418: Screen ending colons from search term (2.48 KB, patch)
2021-11-04 18:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29418: Screen ending colons from search term (717 bytes, patch)
2021-11-04 18:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29418: Unit test (1.29 KB, patch)
2022-05-11 11:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29418: Screen ending colons from search term (782 bytes, patch)
2022-05-11 11:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29418: Fix error searching for analytics in detail view (1.85 KB, patch)
2022-07-01 15:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29418: Fix error searching for analytics in detail view (1.91 KB, patch)
2022-07-03 16:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29418: Unit tests (1.35 KB, patch)
2022-07-04 20:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29418: Make clean_search_term escape double quotes (1.28 KB, patch)
2022-07-04 20:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29418: Make detail rendering use the cleaned-up title for searching (6.61 KB, patch)
2022-07-04 20:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29418: Unit tests (1.39 KB, patch)
2022-07-04 21:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 29418: Make clean_search_term escape double quotes (1.32 KB, patch)
2022-07-04 21:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 29418: Make detail rendering use the cleaned-up title for searching (6.69 KB, patch)
2022-07-04 21:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 29418: (follow-up) Fix links generated in XSLTs (9.02 KB, patch)
2022-07-05 00:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29418: Unit tests (1.46 KB, patch)
2022-07-05 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29418: Make clean_search_term escape double quotes (1.39 KB, patch)
2022-07-05 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29418: Make detail rendering use the cleaned-up title for searching (6.72 KB, patch)
2022-07-05 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29418: (follow-up) Fix links generated in XSLTs (9.08 KB, patch)
2022-07-05 13:51 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 Nick Clemens 2021-11-04 18:00:37 UTC
In fixing/moving analytics search code, zebra was set to simply pass the title through.

For any title with a subtitle, correct marc for subfield a ends with a semicolon.

This is treated as a field delimiter and the search fails

This is causing a spurious warning for many records under zebra:
There was an error searching for analytic records, please see the logs for details.
Comment 1 Nick Clemens 2021-11-04 18:01:52 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2021-11-04 18:08:58 UTC
Created attachment 127326 [details] [review]
Bug 29418: Screen ending colons from search term
Comment 3 Sheri Waite 2021-11-09 14:09:32 UTC
This error message also seems to be appearing with ! and () and maybe addtional punctuation in the 245 field.
Comment 4 Liz Rea 2021-11-09 15:47:54 UTC
And question marks (?)
Comment 5 Andrew Fuerste-Henry 2021-11-10 19:17:12 UTC
If you're not using analytics at all and want to just hide this error, you could put this in your intranetuserCSS:
/* Hide analytics error */
.analytics_error {
  	display: none;
}
Comment 6 Owen Leonard 2022-04-29 13:54:02 UTC
*** Bug 30655 has been marked as a duplicate of this bug. ***
Comment 7 Martin Renvoize 2022-05-11 11:01:05 UTC
Hmm, I started looking at adding unit tests for this and I see that there are a reasonable number for the same function in ElasticSearch.. wondering if we need to handle any other cases here that are already handled in the ES implementation..?
Comment 8 Martin Renvoize 2022-05-11 11:15:57 UTC
Created attachment 134865 [details] [review]
Bug 29418: Unit test
Comment 9 Martin Renvoize 2022-05-11 11:16:01 UTC
Created attachment 134866 [details] [review]
Bug 29418: Screen ending colons from search term

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2022-05-11 11:17:05 UTC
This improves things for me.. but I think it needs more eyes from someone more familiar with Zebra.

Signing off with the Unit test I added for now to get more eyes.
Comment 11 Martin Renvoize 2022-05-11 11:31:30 UTC
This goes some way towards resolving these errors:

[WARN] CCL parsing error (10014) Unknown qualifier ZOOM for query: Host-item=( Utensili = ) at /home/koha/kohaclone/C4/Search.pm line 242.
[2022/05/09 12:39:13] [WARN] Warning from simple_search_compat: 'CCL parsing error (10014) Unknown qualifier ZOOM' at /home/koha/kohaclone/Koha/Biblio.pm line 540.
Comment 12 Marcel de Rooy 2022-05-11 12:14:15 UTC
Looks good to me on itself.
Would be helpful to include some pointers to recreate these warnings. My configuration does not show them at all.

When should I expect a \: at the end of $a btw ?
Normally, I see only a :
Comment 13 Martin Renvoize 2022-05-11 14:15:14 UTC
(In reply to Marcel de Rooy from comment #12)
> Looks good to me on itself.
> Would be helpful to include some pointers to recreate these warnings. My
> configuration does not show them at all.
> 
> When should I expect a \: at the end of $a btw ?
> Normally, I see only a :

I think they only really appear if you are not using UseControlNumber... we tend to have it enabled so don't see it all that often.
Comment 14 Fridolin Somers 2022-05-11 21:11:24 UTC
*** Bug 27575 has been marked as a duplicate of this bug. ***
Comment 15 David Cook 2022-05-11 23:45:32 UTC
(In reply to Martin Renvoize from comment #11)
> [WARN] CCL parsing error (10014) Unknown qualifier ZOOM for query:
> Host-item=( Utensili = ) at /home/koha/kohaclone/C4/Search.pm line 242.
> [2022/05/09 12:39:13] [WARN] Warning from simple_search_compat: 'CCL parsing
> error (10014) Unknown qualifier ZOOM' at /home/koha/kohaclone/Koha/Biblio.pm
> line 540.

This warning actually reveals that we're going about this the wrong way.

Take this code:

$searchstr = "Host-item:($cleaned_title)";

We're just injecting an arbitrary string between those parentheses, and that's causing parsing errors. 

What we should be doing is this:

$searchstr = qq#Host-item:("$cleaned_title")#;

Observe the difference:

Z> find Host-item=( Utensili = )
CCL ERROR: Unknown qualifier
Z> find Host-item=(" Utensili = ")
Sent searchRequest.
Received SearchResponse.
Search was a success.
Number of hits: 0, setno 2
SearchResult-1: term=Utensili cnt=0
records returned: 0
Elapsed: 0.000349
Comment 16 David Cook 2022-05-11 23:49:47 UTC
In fact, an even better option would be this:

$searchstr = qq#(Host-item:"$cleaned_title")#;

This way, we're putting the key-value pair into its own little group. We can then add "and", "or", or whatever. (I really haven't gone down the rabbit hole of the purpose of $searchstr.
Comment 17 David Cook 2022-05-11 23:53:05 UTC
(In reply to Martin Renvoize from comment #7)
> Hmm, I started looking at adding unit tests for this and I see that there
> are a reasonable number for the same function in ElasticSearch.. wondering
> if we need to handle any other cases here that are already handled in the ES
> implementation..?

As Liz and Sheri have mentioned, there are other special characters in the CCL syntax. (See https://software.indexdata.com/yaz/doc/tools.html#ccl.syntax)

However, so long as those are within double quotes, they should be treated as search data and not reserved characters.

So I suppose clean_search_term() should really just escape double quotes.

--

Note: We're in a fortunate position here of building the search string ourselves rather than having to parse user input, so we have so much control and ability to do this the way that works for the search engine.
Comment 18 David Cook 2022-05-11 23:57:15 UTC
Ah, but as I look again at Koha/Biblio.pm, we have to factor in Elasticsearch. I don't know if Elasticsearch can handle double quotes around $cleaned_title.

I suppose clean_search_term() could escape double quotes within 245$a using a backslash[1], plus it could return the clean_search_term wrapped in double quotes, so that Zebra processes it correctly.

I'll leave that distinction up to someone more familiar with the Koha-ES integration...

[1]
Z> find (Any="shuffle "")
CCL ERROR: ')' expected

Z> find (Any="shuffle \"")
Sent searchRequest.
Received SearchResponse.
Search was a success.
Number of hits: 3, setno 28
SearchResult-1: term=shuffle cnt=3
records returned: 0
Elapsed: 0.000369
Comment 19 David Cook 2022-05-11 23:58:26 UTC
(In reply to Martin Renvoize from comment #10)
> This improves things for me.. but I think it needs more eyes from someone
> more familiar with Zebra.

Thanks for adding me. Hopefully my comments were a bit helpful...
Comment 20 David Cook 2022-05-30 04:09:26 UTC
Bumping into this again because of Bug 30865 but escaping a semicolon won't be enough.

You'd have to escape question marks as well, since they're a special case which is only handled by "Special attribute combos" (see https://software.indexdata.com/yaz/doc/tools.html#ccl.qualifiers)

There's % and ! as well as proximity operators. 

Also >=, <=, <>, >, and < as relational operators.

In the end I think it's just easier to compose queries more carefully, and go the route I'm going with bug 30865.
Comment 21 Tomás Cohen Arazi 2022-07-01 14:12:06 UTC
Please :-D
Comment 22 Tomás Cohen Arazi 2022-07-01 15:00:56 UTC
Created attachment 137023 [details] [review]
Bug 29418: Fix error searching for analytics in detail view

This patch fixes the problem of searching for analytics when the title
contains reserved characters like ?

To test:
1. Have a record with ? in the title
2. Open the detail view (staff)
=> FAIL: tells there's an error checking for analytics
3. Open the detail view (OPAC)
=> FAIL: tells there's an error checking for analytics
4. Apply this patch and restart all
5. Repeat 2 and 3
=> SUCCESS: Link shows correctly!
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Katrin Fischer 2022-07-02 21:22:25 UTC
I tried with Elasticsearch and Zebra on master, but the error was not shown, is this specific to 21.05?
Comment 24 Tomás Cohen Arazi 2022-07-02 22:18:25 UTC
(In reply to Katrin Fischer from comment #23)
> I tried with Elasticsearch and Zebra on master, but the error was not shown,
> is this specific to 21.05?

Yes. 21.11 got the Koha::Biblio->get_marc_components method with it's own bugfix.
Comment 25 Katrin Fischer 2022-07-03 16:31:41 UTC
Created attachment 137048 [details] [review]
Bug 29418: Fix error searching for analytics in detail view

This patch fixes the problem of searching for analytics when the title
contains reserved characters like ?

To test:
1. Have a record with ? in the title
2. Open the detail view (staff)
=> FAIL: tells there's an error checking for analytics
3. Open the detail view (OPAC)
=> FAIL: tells there's an error checking for analytics
4. Apply this patch and restart all
5. Repeat 2 and 3
=> SUCCESS: Link shows correctly!
6. Sign off :-D

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 David Nind 2022-07-03 23:55:56 UTC
This is now signed off by Katrin, but here are my testing notes for what they are worth!

I'm not very familiar with analytical records, and I wasn't sure whether the ? should be in the host record or the child/analytical record.

I tried combinations for the host record and the child/analytical record, and for both.

I haven't changed the bug status.

(Hopefully, I recorded my observations correctly!)

Testing notes using koha-testing-docker
=======================================

Starting 21.05.x instance:
a) For Koha repository, git checkout 21.05.x
b) Edit koha-testing-docker .env file and change IMAGE=master to IMAGE=21.05

Set up an analytical record:
a) Enable the EasyAnalyticalRecords system preference
b) For any record, select Edit > Link to host record - choose any other record (I tested with a record that had an item already attached to it)
c) For the record chosen as the host item, add a ? to the title

Used two records for testing:
- Host record: Programming Perl (biblionumber = 262)
- Child/analytical record: Perl best practices (biblionumber = 5)

Before the patch is applied
~~~~~~~~~~~~~~~~~~~~~~~~~~~

1. If only the host record has a ? in the title:
  - Viewing the host record in the staff interface - error displayed before the title: "There was an error searching for analytic records, please see the logs for details."
  - Viewing the host record in the OPAC - no visible error message displayed (like in the staff interface), but there is an error message in the /var/log/koha/kohadev/plack-opac-error.log log file:
    [2022/07/03 22:39:46] [WARN] CCL parsing error (10014) Embedded truncation not supported ZOOM for query: Host-item=(Programming ? Perl /) at /kohadevbox/koha/C4/Search.pm line 241.
    [2022/07/03 22:39:46] [WARN] Warning from simple_search_compat: CCL parsing error (10014) Embedded truncation not supported ZOOM at /kohadevbox/koha/opac/opac-detail.pl line 208.
 
2. If only the child/analytical record has a ? in the title:
   - Staff interface: 
     . Host record displays 'Analytics: Show analytics'
     . Clicking on 'Show analytics' link displays the child/analytical record
     . Error displayed before the title in the child/analytical record: "There was an error searching for analytic records, please see the logs for details."
   - Viewing the host record in the OPAC
     . no error displayed
     . displays 'Analytics: Show analytics'
     . clicking on show analytics: child/analytical record displayed with no error, but error in the logs as per 1
   - Viewing the child/analytical record in the OPAC:
     . no error displayed
     . but error in the logs as per 1
 
3. Both the host and child/analytical record have a ? in the title:
   . same as 1 for both the host and child/analytical record

After the patch is applied
~~~~~~~~~~~~~~~~~~~~~~~~~~

1. If only the host record has a ? in the title:
   - Staff interface:
     . Host record shows 'Analytics: Show analytics', no error messages
     . Clicking on the 'Show analytics' link: No results match your search for 'Host-item:(Programming ? Perl )'. 
     . Error in the /var/log/koha/kohadev/plack-intranet-error.log log file:
       [2022/07/03 23:40:01] [WARN] WARNING: query problem with Host-item=(Programming ? Perl ) ZOOM error 10014 "CCL parsing error" (addinfo: "Embedded truncation not supported") from diag-set 'ZOOM' at /kohadevbox/koha/C4/Search.pm line 349.
   - OPAC:
     . Host record shows 'Analytics: Show analytics'
     . Error message in the /var/log/koha/kohadev/plack-opac-error.log log file:
        [2022/07/03 23:41:25] [WARN] WARNING: query problem with Host-item=(Programming ? Perl ) ZOOM error 10014 "CCL parsing error" (addinfo: "Embedded truncation not supported") from diag-set 'ZOOM' at /kohadevbox/koha/C4/Search.pm line 349.
        [2022/07/03 23:41:25] [WARN] Use of uninitialized value $times in subtraction (-) at /kohadevbox/koha/C4/Search.pm line 1709.

2. If only the child/analytical record has a ? in the title: works as expected, no errors in the log

3. Both the host and child/analytical record has a ?:
   - Staff interface and OPAC:
     . Host record shows 'Analytics: Show analytics'
     . Clicking on the 'Show analytics' link: No results match your search for 'Host-item:(Programming ? Perl )'.
Comment 27 Katrin Fischer 2022-07-04 06:27:00 UTC
Hm, strange. The hos record having a ? case worked for me. Vut I had added the ? a the end of the string, maybe it could make a difference?
Comment 28 David Nind 2022-07-04 06:34:32 UTC
(In reply to Katrin Fischer from comment #27)
> Hm, strange. The hos record having a ? case worked for me. Vut I had added
> the ? a the end of the string, maybe it could make a difference?

I had added the ? in the middle of the title.
Comment 29 Tomás Cohen Arazi 2022-07-04 12:51:27 UTC
(In reply to David Nind from comment #28)
> (In reply to Katrin Fischer from comment #27)
> > Hm, strange. The hos record having a ? case worked for me. Vut I had added
> > the ? a the end of the string, maybe it could make a difference?
> 
> I had added the ? in the middle of the title.

Hi, the internal analytics search works correctly, but a follow-up is required for the XSLT to generate a good link. I overlooked it. Working on it now.
Comment 30 Tomás Cohen Arazi 2022-07-04 20:25:30 UTC
Created attachment 137135 [details] [review]
Bug 29418: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi 2022-07-04 20:25:35 UTC
Created attachment 137136 [details] [review]
Bug 29418: 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 32 Tomás Cohen Arazi 2022-07-04 20:25:40 UTC
Created attachment 137137 [details] [review]
Bug 29418: Make detail rendering use the cleaned-up title for searching

This patch makes both staff and OPAC detail pages generate a valid
search link so you can trace related records in the Show analytics
link.

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. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 David Nind 2022-07-04 21:53:54 UTC
Created attachment 137138 [details] [review]
Bug 29418: Unit tests

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 34 David Nind 2022-07-04 21:53:59 UTC
Created attachment 137139 [details] [review]
Bug 29418: 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 35 David Nind 2022-07-04 21:54:04 UTC
Created attachment 137140 [details] [review]
Bug 29418: Make detail rendering use the cleaned-up title for searching

This patch makes both staff and OPAC detail pages generate a valid
search link so you can trace related records in the Show analytics
link.

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. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 Tomás Cohen Arazi 2022-07-05 00:14:38 UTC
Created attachment 137142 [details] [review]
Bug 29418: (follow-up) Fix links generated in XSLTs

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

To test:
1. Repeat the test plan from the other patches
2. When sitting on the detail view of the host record, click on the link
   to open the child.
=> SUCCESS: The link works, because of the previous patches
3. Click on the 'In:' link so the search takes you to the host record
=> FAIL: The link leads no results
4. Apply this patch
5. Restart all
6. Reload the child record page and click on the link
=> SUCCESS: Voila! The host record is retrieved!
7. Bonus points: Retry the full test plan, but add a " or ? to the
   author in both host and child records
=> SUCCESS: Things don't work before this patch, and work afterwards.
8. Sign off :-D

Sponsored-by: Theke Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Martin Renvoize 2022-07-05 13:50:48 UTC
Created attachment 137171 [details] [review]
Bug 29418: 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 38 Martin Renvoize 2022-07-05 13:50:53 UTC
Created attachment 137172 [details] [review]
Bug 29418: 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 39 Martin Renvoize 2022-07-05 13:50:58 UTC
Created attachment 137173 [details] [review]
Bug 29418: Make detail rendering use the cleaned-up title for searching

This patch makes both staff and OPAC detail pages generate a valid
search link so you can trace related records in the Show analytics
link.

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. 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 40 Martin Renvoize 2022-07-05 13:51:03 UTC
Created attachment 137174 [details] [review]
Bug 29418: (follow-up) Fix links generated in XSLTs

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

To test:
1. Repeat the test plan from the other patches
2. When sitting on the detail view of the host record, click on the link
   to open the child.
=> SUCCESS: The link works, because of the previous patches
3. Click on the 'In:' link so the search takes you to the host record
=> FAIL: The link leads no results
4. Apply this patch
5. Restart all
6. Reload the child record page and click on the link
=> SUCCESS: Voila! The host record is retrieved!
7. Bonus points: Retry the full test plan, but add a " or ? to the
   author in both host and child records
=> SUCCESS: Things don't work before this patch, and work afterwards.
8. Sign off :-D

Sponsored-by: Theke Solutions
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize 2022-07-05 13:53:45 UTC
Nice work Tomas, this is a great solution for the 21.05 series.

I think we need the follow-up in 21.11+ too however.. and the approach of passing through the analytics_query as a variable (although actually all we pass here is the escaped title) make sense to me for a master additional too.. DRY our code out by passing the constructed and escaped analytics query from our biblio method rather than re-constructing a version in the xslts.

All in all.. PQA for this branch.
Comment 42 Tomás Cohen Arazi 2022-07-05 18:25:25 UTC
(In reply to Martin Renvoize from comment #41)
> Nice work Tomas, this is a great solution for the 21.05 series.
> 
> I think we need the follow-up in 21.11+ too however.. and the approach of
> passing through the analytics_query as a variable (although actually all we
> pass here is the escaped title) make sense to me for a master additional
> too.. DRY our code out by passing the constructed and escaped analytics
> query from our biblio method rather than re-constructing a version in the
> xslts.

This problem is present in (at least) master, and it doesn't need just the follow-up. I'll file a bug for master to avoid mixing things.
Comment 43 Victor Grousset/tuxayo 2022-07-13 23:02:37 UTC
Backported: Pushed to 21.05.x branch for 21.05.17
Comment 44 Tomás Cohen Arazi 2022-07-13 23:16:08 UTC
(In reply to Victor Grousset/tuxayo from comment #43)
> Backported: Pushed to 21.05.x branch for 21.05.17

Thanks!