Bug 20218

Summary: Tracklinks fails when URL has special characters
Product: Koha Reporter: Nick Clemens <nick>
Component: OPACAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: barton, fridolin.somers, maksim.sen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19847    
Bug Blocks:    
Attachments: Bug 20218: Fix matching of uri in tracklinks
Bug 20218: Fix matching of uri in tracklinks
Bug 20218: Fix matching of uri in tracklinks

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