Bug 21887 - 856$u link problem in XSLT result lists and detail page
Summary: 856$u link problem in XSLT result lists and detail page
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Devinim
QA Contact: Chris Cormack
URL:
Keywords:
Depends on:
Blocks: 23423
  Show dependency treegraph
 
Reported: 2018-11-27 14:14 UTC by Devinim
Modified: 2020-11-30 21:45 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.01,18.11.07
Circulation function:


Attachments
Sample MARC record (1.35 KB, text/plain)
2018-11-27 14:14 UTC, Devinim
Details
Bug 21887: 856 link problem in XSLT result lists and detail page (13.72 KB, patch)
2018-11-27 14:17 UTC, Devinim
Details | Diff | Splinter Review
Bug 21887: 856 link problem in XSLT result lists and detail page (13.86 KB, patch)
2018-11-27 14:28 UTC, Devinim
Details | Diff | Splinter Review
Bug 21887: (For OPAC) 856 link problem in XSLT result lists and detail page (3.77 KB, patch)
2018-11-27 15:29 UTC, Devinim
Details | Diff | Splinter Review
Bug 21887: 856 link problem in XSLT result lists and detail page (13.91 KB, patch)
2019-05-23 15:56 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21887: (For OPAC) 856 link problem in XSLT result lists and detail page (3.81 KB, patch)
2019-05-23 15:56 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21887: 856 link problem in XSLT result lists and detail page (13.96 KB, patch)
2019-05-26 20:42 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 21887: (For OPAC) 856 link problem in XSLT result lists and detail page (3.86 KB, patch)
2019-05-26 20:43 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Devinim 2018-11-27 14:14:04 UTC
Created attachment 82674 [details]
Sample MARC record

İf you dont add http:// or ftp:// in front of link pages in 856$u field, intranet webpage search result and detail page yields to connect localhost.
Comment 1 Devinim 2018-11-27 14:17:02 UTC
Created attachment 82675 [details] [review]
Bug 21887: 856 link problem in XSLT result lists and detail page
Comment 2 Devinim 2018-11-27 14:28:26 UTC
Created attachment 82676 [details] [review]
Bug 21887: 856 link problem in XSLT result lists and detail page

Test plan;
1) Add web page link without http:// or ftp:// prefix in 856$u or use Sample Marc record
2) Search record from intranet web page and examine that link in online resources section shows localhost.
3) Apply this patch
4) Search similar record and test link is working correctly

Signed-off-by: Devinim <kohadevinim@devinim.com.tr>
Comment 3 Marcel de Rooy 2018-11-27 14:38:02 UTC
From LOC:
$u - Uniform Resource Identifier
Uniform Resource Identifier (URI), which provides standard syntax for locating an object using existing Internet protocols. Field 856 is structured to allow for the creation of a URL from the concatenation of other separate 856 subfields. Subfield $u may be used instead of those separate subfields or in addition to them.

I am not sure if we fully comply with that currently (probably not). This does not either. But still looks good to me..
Comment 4 Devinim 2018-11-27 15:29:09 UTC
Created attachment 82677 [details] [review]
Bug 21887: (For OPAC) 856 link problem in XSLT result lists and detail page

Test plan (for OPAC);
1) Add web page link without http:// or ftp:// prefix in 856$u or use Sample Marc record
2) Search record from OPAC web page and examine that link in online resources section shows localhost.
3) Apply this patch
4) Search similar record and test link is working correctly
Comment 5 David Cook 2018-11-28 05:08:25 UTC
Comment on attachment 82677 [details] [review]
Bug 21887: (For OPAC) 856 link problem in XSLT result lists and detail page

Review of attachment 82677 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl
@@ +1078,5 @@
>                        <xsl:attribute name="href">/cgi-bin/koha/tracklinks.pl?uri=<xsl:value-of select="str:encode-uri(marc:subfield[@code='u'], true())"/>&amp;biblionumber=<xsl:value-of select="$biblionumber"/></xsl:attribute>
>  				</xsl:when>
>  				<xsl:otherwise>
> +                    <xsl:attribute name="href">
> +                        <xsl:if test="not(contains(marc:subfield[@code='u'],'://'))">

I could imagine malformed (yet functional) URLs could cause some problems here. 

Maybe you'd like to try out http://exslt.org/regexp/index.html to be more precise?
Comment 6 David Cook 2018-11-28 05:09:50 UTC
I think you could use starts-with instead as well:

https://developer.mozilla.org/en-US/docs/Web/XPath/Functions/starts-with

Although I suppose leading whitespace could cause a problem
Comment 7 Devinim 2018-11-30 10:06:17 UTC
Hi David,
Can you share a sample for malformed but functional URL?
Thanks.
Comment 8 David Cook 2018-12-02 23:16:39 UTC
(In reply to Devinim from comment #7)
> Hi David,
> Can you share a sample for malformed but functional URL?
> Thanks.

Sure, I was thinking something like "koha-community.org?redirect=http://other-site.org"

That string would not be caught by this test:

<xsl:if test="not(contains(marc:subfield[@code='u'],'://'))">

Apologies for my wording of "malformed but functional URL". What I meant was a a URL missing a scheme but containing an unencoded URL in the query string. Technically, the value of redirect should be encoded, but browsers will typically work with it anyway. If I put "koha-community.org?redirect=http://other-site.org" into a Chrome browser, it'll work. But according to your use case, it will try to connect to localhost.
Comment 9 Devinim 2019-05-23 14:29:50 UTC
(In reply to David Cook from comment #6)
> I think you could use starts-with instead as well:
> 
> https://developer.mozilla.org/en-US/docs/Web/XPath/Functions/starts-with
> 
> Although I suppose leading whitespace could cause a problem

starts-with cannot work the version of xslt we use in koha, am I correct?
Comment 10 Liz Rea 2019-05-23 15:56:02 UTC
Created attachment 90041 [details] [review]
Bug 21887: 856 link problem in XSLT result lists and detail page

Signed-off-by: Nazlı Çetin <nazli@devinim.com.tr>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
I don't think we need to worry about David's edge case. This improves the behaviour significantly.
Comment 11 Liz Rea 2019-05-23 15:56:10 UTC
Created attachment 90042 [details] [review]
Bug 21887: (For OPAC) 856 link problem in XSLT result lists and detail page

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 12 Chris Cormack 2019-05-26 20:42:42 UTC
Created attachment 90097 [details] [review]
Bug 21887: 856 link problem in XSLT result lists and detail page

Signed-off-by: Nazlı Çetin <nazli@devinim.com.tr>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 13 Chris Cormack 2019-05-26 20:43:05 UTC
Created attachment 90098 [details] [review]
Bug 21887: (For OPAC) 856 link problem in XSLT result lists and detail page

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 14 David Cook 2019-05-27 00:06:22 UTC
(In reply to Devinim from comment #9)
> 
> starts-with cannot work the version of xslt we use in koha, am I correct?

starts-with should be part of XSLT 1.0, so it should be part of libxml used by Koha.
Comment 15 David Cook 2019-05-27 00:09:41 UTC
(In reply to Liz Rea from comment #10)
> I don't think we need to worry about David's edge case. This improves the
> behaviour significantly.

No argument here. In the case I mentioned, I prefer to update the URL data in the database anyway.
Comment 16 Martin Renvoize (ashimema) 2019-06-03 11:19:25 UTC
I'd love to see a selenium test to cover this to repent future regressions... and do we need to submit corresponding bugs for Unimarc?
Comment 17 Martin Renvoize (ashimema) 2019-06-04 09:20:04 UTC
Nice work!

Pushed to master for 19.11.00
Comment 18 Fridolin Somers 2019-06-11 13:36:53 UTC
Pushed to 19.05.x for 19.05.01
Comment 19 Lucas Gass (lukeg) 2019-06-12 22:14:59 UTC
backported to 18.11.x for 18.11.07
Comment 20 Katrin Fischer 2019-08-11 10:37:15 UTC
Unfortunately this causes a regression with any relative links added by the opload feature no longer working: bug 23423