Bug 32794 - mailto links in 856 can be incorrectly formed by XSLT
Summary: mailto links in 856 can be incorrectly formed by XSLT
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-02-02 13:29 UTC by Nick Clemens (kidclamp)
Modified: 2023-12-28 20:47 UTC (History)
2 users (show)

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


Attachments
Bug 32794: Expand tests for access method in XLST files (8.62 KB, patch)
2023-02-02 13:33 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 32794: Expand tests for access method in XLST files (8.67 KB, patch)
2023-03-04 04:06 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 32794: Expand tests for access method in XLST files (8.77 KB, patch)
2023-03-22 16:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32794: (follow-up) Consolidate shared code into one function (:template) (4.28 KB, patch)
2023-03-22 16:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32794: (follow-up) Replace code by new function (template) (10.55 KB, patch)
2023-03-22 16:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-02-02 13:29:00 UTC
The current code checks for '://' in the 856, and adds http:// or ftp://, depending on first indicator, if it isn't present.

The marc standard, however, lists more access methods that are not tested for or covered. mailto: won't meet the test for '://' and should be handled
Comment 1 Nick Clemens (kidclamp) 2023-02-02 13:33:56 UTC
Created attachment 145999 [details] [review]
Bug 32794: Expand tests for access method in XLST files

This patch expands the checks for 856u to check for mailto, and adds a few conditiona
checks of the first indicator to cover more methods

Reference: https://www.oclc.org/bibformats/en/8xx/856.html

To test:
1 - Edit a record and add a few 856 fields:
    856 0 0 $umailto:test@example.org
    856 0 0 $utest@example.org
    856 2 0 $utelnet://pucc.princeton.edu
    856 2 0 $upucc.princeton.edu
    856 3 0 $u8885555555
2 - View staff details and results and opac details and results
3 - Note all links have been prepended with http://
4 - Apply patch
5 - Refresh all views, ensure links are correctly formed
Comment 2 Phil Ringnalda 2023-03-04 04:06:35 UTC
Created attachment 147745 [details] [review]
Bug 32794: Expand tests for access method in XLST files

This patch expands the checks for 856u to check for mailto, and adds a few conditiona
checks of the first indicator to cover more methods

Reference: https://www.oclc.org/bibformats/en/8xx/856.html

To test:
1 - Edit a record and add a few 856 fields:
    856 0 0 $umailto:test@example.org
    856 0 0 $utest@example.org
    856 2 0 $utelnet://pucc.princeton.edu
    856 2 0 $upucc.princeton.edu
    856 3 0 $u8885555555
2 - View staff details and results and opac details and results
3 - Note all links have been prepended with http://
4 - Apply patch
5 - Refresh all views, ensure links are correctly formed

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 3 Phil Ringnalda 2023-03-04 04:08:38 UTC
The "dial-up" one was particularly horrifying, since once I clicked through all the browser and OS warnings about "Dude, are you really clicking on a tel: link?" the only available handler app was Facetime. Thanks for choosing a number that wouldn't have gone through :)
Comment 4 Marcel de Rooy 2023-03-22 14:33:26 UTC
This is a bit of teasing:

<xsl:when test="@ind1=7">
                                <xsl:value-of select="marc:subfield[@code='2']"/><xsl:text>://</xsl:text>

But when you would put mailto inside $2, this goes wrong again ;)
Not really a blocker, just noting a fictive improvement.
Comment 5 Marcel de Rooy 2023-03-22 14:50:15 UTC
From LOC
7 - Method specified in subfield $2
856	7#$3b&w film copy neg.$ddag$f3d01926$2file

Too bad that this example does not contain the full prefix in $2. Would have been easy to just remove the suffix :// from our xslt.
Comment 6 Marcel de Rooy 2023-03-22 14:50:58 UTC
Looking here / might add follow-up
Comment 7 Marcel de Rooy 2023-03-22 16:00:48 UTC
Created attachment 148565 [details] [review]
Bug 32794: Expand tests for access method in XLST files

This patch expands the checks for 856u to check for mailto, and adds a few conditiona
checks of the first indicator to cover more methods

Reference: https://www.oclc.org/bibformats/en/8xx/856.html

To test:
1 - Edit a record and add a few 856 fields:
    856 0 0 $umailto:test@example.org
    856 0 0 $utest@example.org
    856 2 0 $utelnet://pucc.princeton.edu
    856 2 0 $upucc.princeton.edu
    856 3 0 $u8885555555
2 - View staff details and results and opac details and results
3 - Note all links have been prepended with http://
4 - Apply patch
5 - Refresh all views, ensure links are correctly formed

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2023-03-22 16:00:51 UTC
Created attachment 148566 [details] [review]
Bug 32794: (follow-up) Consolidate shared code into one function (:template)

Few extra lines for the $2 exception as noted on Bugzilla.

Note: We should probably check why the utils xslt on opac and intranet
have so much differences currently.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2023-03-22 16:00:53 UTC
Created attachment 148567 [details] [review]
Bug 32794: (follow-up) Replace code by new function (template)

Test plan:
Same as first patch.
Bonus:
Remove protocol from $u. Set first ind1 to 7.
Put mailto in $2. Check link.
Change $2 to tel. Check link.
Change $2 to https. Check link.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2023-03-22 16:01:40 UTC
(In reply to Marcel de Rooy from comment #4)
> This is a bit of teasing:
> 
> <xsl:when test="@ind1=7">
>                                 <xsl:value-of
> select="marc:subfield[@code='2']"/><xsl:text>://</xsl:text>
> 
> But when you would put mailto inside $2, this goes wrong again ;)
> Not really a blocker, just noting a fictive improvement.

Ended up fixing it with three more lines.
Comment 11 Tomás Cohen Arazi 2023-03-31 10:17:44 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Matt Blenkinsop 2023-04-11 11:25:04 UTC
Nice work everyone!

Pushed to stable for 22.11.x