Bug 20218 - Tracklinks fails when URL has special characters
Summary: Tracklinks fails when URL has special characters
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 19847
Blocks:
  Show dependency treegraph
 
Reported: 2018-02-15 18:52 UTC by Nick Clemens
Modified: 2018-03-27 11:33 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20218: Fix matching of uri in tracklinks (1.31 KB, patch)
2018-02-16 12:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20218: Fix matching of uri in tracklinks (1.36 KB, patch)
2018-02-26 15:15 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 20218: Fix matching of uri in tracklinks (1.42 KB, patch)
2018-02-26 23:16 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-02-15 18:52:54 UTC
To recreate: 
1 - Copy the url of a biblio in the opac
2 - Paste this into the 856$u ofd another record
3 - Enable anonymous link tracking
4 - View the second record in the opac
5 - Click the link
6 - Get a 404 error

It seems on this line:
if ( grep { /^$uri$/ } map { $_->{MARCURL} } @$marc_urls ) {

The regex is reading the '?' in the uri as a special character and so the uri is not matched
Comment 1 Nick Clemens 2018-02-16 12:49:28 UTC
Created attachment 71745 [details] [review]
Bug 20218: Fix matching of uri in tracklinks

The current code matches using a regex, this breaks when the url
contains special characters. We swtich it to equality check

To test:
1 - Enable TrackClicks (either track or anonymous)
2 - Find the URL of a biblio in the OPAC
3 - Paste this into the 856$u of another record
    (or use any url containing a '?' or other characters)
4 - View the record in the opac
5 - Click the URL
6 - 404 Error!
7 - Apply patch
8 - Try again
9 - Success!
Comment 2 Maksim Sen 2018-02-16 21:19:31 UTC
Hey Nick,

I tried applying the patch (I'm currently on the master branch), but I got this error,

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 20218: Fix matching of uri in tracklinks
Using index info to reconstruct a base tree...
M	opac/tracklinks.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/tracklinks.pl
CONFLICT (content): Merge conflict in opac/tracklinks.pl
error: Failed to merge in the changes.
Patch failed at 0001 Bug 20218: Fix matching of uri in tracklinks
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-20218-Fix-matching-of-uri-in-tracklinks-KoSJEI.patch

Is there an extra step needed to apply this patch?
Comment 3 Nick Clemens 2018-02-17 16:00:04 UTC
Applies cleanly for me on master, can you make sure you are up to date and try again?
Comment 4 Nick Clemens 2018-02-22 11:57:14 UTC
Bumping severity - this breaks functionality for anyone using trackclicks and affects users directly
Comment 5 Claire Gravely 2018-02-26 15:15:04 UTC
Created attachment 72215 [details] [review]
Bug 20218: Fix matching of uri in tracklinks

The current code matches using a regex, this breaks when the url
contains special characters. We swtich it to equality check

To test:
1 - Enable TrackClicks (either track or anonymous)
2 - Find the URL of a biblio in the OPAC
3 - Paste this into the 856$u of another record
    (or use any url containing a '?' or other characters)
4 - View the record in the opac
5 - Click the URL
6 - 404 Error!
7 - Apply patch
8 - Try again
9 - Success!

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 6 Katrin Fischer 2018-02-26 23:16:16 UTC
Created attachment 72246 [details] [review]
Bug 20218: Fix matching of uri in tracklinks

The current code matches using a regex, this breaks when the url
contains special characters. We swtich it to equality check

To test:
1 - Enable TrackClicks (either track or anonymous)
2 - Find the URL of a biblio in the OPAC
3 - Paste this into the 856$u of another record
    (or use any url containing a '?' or other characters)
4 - View the record in the opac
5 - Click the URL
6 - 404 Error!
7 - Apply patch
8 - Try again
9 - Success!

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Jonathan Druart 2018-02-27 18:59:37 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 8 Nick Clemens 2018-03-23 12:50:17 UTC
Awesome work all! Pushed to stable for 17.11.04
Comment 9 Fridolin SOMERS 2018-03-27 11:33:33 UTC
Pushed to 17.05.x for v17.05.10