Bug 21018 - OPAC Resource URL Broken if Tracklinks is enabled
Summary: OPAC Resource URL Broken if Tracklinks is enabled
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: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 21088 (view as bug list)
Depends on: 19847
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-29 11:06 UTC by Mathieu Pelletier
Modified: 2018-09-10 03:41 UTC (History)
8 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 21018: Fix TrackClicks for 952$u (1.59 KB, patch)
2018-06-29 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21018: Fix TrackClicks for 952$u (1.65 KB, patch)
2018-07-01 17:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21018: Allow uris defined in the item (1.02 KB, patch)
2018-07-11 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21018: Fix TrackClicks for 952$u (1.70 KB, patch)
2018-08-01 22:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21018: Allow uris defined in the item (1.07 KB, patch)
2018-08-01 22:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21018: Fix TrackClicks for 952$u (1.77 KB, patch)
2018-08-07 07:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21018: Allow uris defined in the item (1.14 KB, patch)
2018-08-07 07:52 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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