Bug 21018

Summary: OPAC Resource URL Broken if Tracklinks is enabled
Product: Koha Reporter: Mathieu Pelletier <mkpelletier>
Component: OPACAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: alexbuckley, fridolin.somers, jonathan.druart, katrin.fischer, m.de.rooy, martin.renvoize, mtj, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21088
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22703
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 21018: Fix TrackClicks for 952$u
Bug 21018: Fix TrackClicks for 952$u
Bug 21018: Allow uris defined in the item
Bug 21018: Fix TrackClicks for 952$u
Bug 21018: Allow uris defined in the item
Bug 21018: Fix TrackClicks for 952$u
Bug 21018: Allow uris defined in the item

Description Mathieu Pelletier 2018-06-29 11:06:51 UTC
With Tracklinks enabled in configuration (anonymous or otherwise), URI's in 952u return a koha 404 error.  However, if the link is also recorded in 856u, the 952u link works. As soon as you turn off tracklinks, 952u URI's function normally.

I tested the above only on https URI's. Perhaps this is related to #18714 earlier marked as invalid?
Comment 1 Jonathan Druart 2018-06-29 13:33:39 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2018-07-01 17:47:38 UTC
Created attachment 76601 [details] [review]
Bug 21018: Fix TrackClicks for 952$u

Caused by
  commit 51b6cf2aafbfb45b9fcb072403b986fe3c2add53
  Bug 19847: Track links within the records and 404 for others

We should also accept uri stored in items.uri (952$u)

Test plan:
- Turn on TrackClicks
- Defined uri for some items
- At the OPAC click on the link (from opac-detail.pl)
=> Without this patch you will get 404 (redirected from tracklinks.pl)
=> With this patch applied you will be redirected correctly and the
linktracker will be filled correctly

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 3 Jonathan Druart 2018-07-02 13:55:41 UTC
Important note: We certainly need to add an index on this column to boost this query.
Comment 4 Katrin Fischer 2018-07-04 20:26:30 UTC
(In reply to Jonathan Druart from comment #3)
> Important note: We certainly need to add an index on this column to boost
> this query.

Do you mean items.uri and deleteditems.uri?
Comment 5 Marcel de Rooy 2018-07-06 08:14:18 UTC
(In reply to Jonathan Druart from comment #3)
> Important note: We certainly need to add an index on this column to boost
> this query.

Koha::Items->search( { uri => $uri } )->count

You need to add biblionumber to limit the results. No need to scan the whole table.
Comment 6 Jonathan Druart 2018-07-11 14:30:34 UTC
Created attachment 76863 [details] [review]
Bug 21018: Allow uris defined in the item

We need to search for uri defined for the itemnumber we passed in
parameter.
Comment 7 Jonathan Druart 2018-07-11 14:32:39 UTC
(In reply to Katrin Fischer from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Important note: We certainly need to add an index on this column to boost
> > this query.
> 
> Do you mean items.uri and deleteditems.uri?

only items.uri, but with the last patch it's not needed.

(In reply to Marcel de Rooy from comment #5)
> (In reply to Jonathan Druart from comment #3)
> > Important note: We certainly need to add an index on this column to boost
> > this query.
> 
> Koha::Items->search( { uri => $uri } )->count
> 
> You need to add biblionumber to limit the results. No need to scan the whole
> table.

Oops, yes you are right. It's itemnumber actually.
Maybe we should add more tests here, items.uri check should only be if itemnumber is passed. But I would prefer to not complexity it now as it's improvement and not bug fixing.
Comment 8 Owen Leonard 2018-07-19 13:40:37 UTC
This isn't working for me. I'm still getting a 404 error after applying the patch.
Comment 9 Mathieu Pelletier 2018-07-19 14:18:11 UTC
This does work for me using the second patch.
Comment 10 Katrin Fischer 2018-08-01 22:07:17 UTC
*** Bug 21088 has been marked as a duplicate of this bug. ***
Comment 11 Katrin Fischer 2018-08-01 22:13:24 UTC
Created attachment 77413 [details] [review]
Bug 21018: Fix TrackClicks for 952$u

Caused by
  commit 51b6cf2aafbfb45b9fcb072403b986fe3c2add53
  Bug 19847: Track links within the records and 404 for others

We should also accept uri stored in items.uri (952$u)

Test plan:
- Turn on TrackClicks
- Defined uri for some items
- At the OPAC click on the link (from opac-detail.pl)
=> Without this patch you will get 404 (redirected from tracklinks.pl)
=> With this patch applied you will be redirected correctly and the
linktracker will be filled correctly

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2018-08-01 22:13:28 UTC
Created attachment 77414 [details] [review]
Bug 21018: Allow uris defined in the item

We need to search for uri defined for the itemnumber we passed in
parameter.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2018-08-01 22:14:21 UTC
With both patches it works for me as well. I have the 404 for before applying the patch and correct redirects after.
Comment 14 Martin Renvoize 2018-08-07 07:52:20 UTC
Created attachment 77519 [details] [review]
Bug 21018: Fix TrackClicks for 952$u

Caused by
  commit 51b6cf2aafbfb45b9fcb072403b986fe3c2add53
  Bug 19847: Track links within the records and 404 for others

We should also accept uri stored in items.uri (952$u)

Test plan:
- Turn on TrackClicks
- Defined uri for some items
- At the OPAC click on the link (from opac-detail.pl)
=> Without this patch you will get 404 (redirected from tracklinks.pl)
=> With this patch applied you will be redirected correctly and the
linktracker will be filled correctly

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2018-08-07 07:52:23 UTC
Created attachment 77520 [details] [review]
Bug 21018: Allow uris defined in the item

We need to search for uri defined for the itemnumber we passed in
parameter.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2018-08-07 07:52:53 UTC
Works well for me and I can't see any regressions.. passes the qa scripts too.

Passing QA
Comment 17 Nick Clemens 2018-08-08 21:32:57 UTC
Awesome work all!

Pushed to master for 18.11
Comment 18 Martin Renvoize 2018-08-09 10:08:53 UTC
Pushed to 18.05.x for 18.05.03
Comment 19 Fridolin Somers 2018-08-28 13:18:45 UTC
Pushed to 17.11.x for 17.11.09
Comment 20 Fridolin Somers 2018-09-24 13:19:04 UTC
Pushed to 17.05.x for 17.05.14