# External links in history don’t follow “Open all external links in a new tab” setting

**URL:** https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995
**Category:** Bug
**Created:** [2016 年 4 月 22 日午後 4:48 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995 "2016-04-22T16:48:15Z")
**Posts on this page:** 17
**Page:** 2

<div class="post-metadata">

### Author: ![fantasticfears](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/fantasticfears/32/119608_2.png) [@fantasticfears](https://meta.discourse.org/u/fantasticfears)
#### Post date: [2016 年 4 月 28 日午後 6:21 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/21 "2016-04-28T18:21:11Z")

</div>

> [@eviltrout](#):
>
> What exactly is the difference in the DOM that prevents it from working? I am curious about that. I definitely don’t like duplicating all the code as you’ve suspected 🙂

Simply because it starts listening when loaded a `TopicView`. (From there, `mouseup` is listened and passed to `ClickTrack`. Then `ClickTrack` follows event and try to find closest `article`.)

> [@eviltrout](#):
>
> I don’t see why not. Edit history is not very interesting but user profile clicks are definitely valid clicks.

The logic for `external_links_in_new_tab` settings happened with `ClickTrack` and ClicksController!

`ClicksController` explicit checks whether a `TopicLink` record is there. This eventually prevent redirection if `ClickTrack` send a url without post\_id/topic\_id.

> <https://github.com/discourse/discourse/blob/main/app/controllers/clicks_controller.rb#L12>

For this reason, @cpradio you will have to tweak this controller.

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 4 月 28 日午後 6:27 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/22 "2016-04-28T18:27:47Z")

</div>

> [@fantasticfears](#):
>
> For this reason, @cpradio you will have to tweak this controller.

Well I haven’t had to yet 😉 If we want the Profile Page to log clicks, then my goal would be to expose the topic link and post link to the DOM on the Profile Page so it can be absorbed by the ClickTrack logic too.

As of right now, I modified ClickTrack to ignore the Post and Topic id and not call out to the controller unless it is coming from a Topic Stream. This way, I don’t have to touch the controller. As the profile page won’t try and call the /click/ style URL at this point in time, it simply calls the HREF value and uses window.open or redirectTo or routeTo accordingly.

As my assumption for the Profile Page clicks would be that it log the click against the topic/post, but maybe that isn’t the assumption everyone else is making…

---

<div class="post-metadata">

### Author: ![eviltrout](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/eviltrout/32/5275_2.png) [@eviltrout](https://meta.discourse.org/u/eviltrout)
#### Post date: [2016 年 4 月 29 日午後 4:49 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/23 "2016-04-29T16:49:57Z")

</div>

Looking at the code though, it looks like it’s skipping the AJAX version unless `relation === TOPIC`? Is there a reason to do it that way rather than passing topicId and postId as null if they can’t be found?

Additionally, I suspect we can include those for the user stream as data attributes too, and create code that finds the proper parent if it exists.

Finally, as part of Ember 1.13 deprecation work I removed the `user-stream` view. (there’s a `user-stream` component that basically does the same thing)

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 4 月 29 日午後 4:56 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/24 "2016-04-29T16:56:08Z")

</div>

> [@eviltrout](#):
>
> Is there a reason to do it that way rather than passing topicId and postId as null if they can’t be found?

It doesn’t have to call a page to redirect to the url it is supposed to go to?

> [@eviltrout](#):
>
> Additionally, I suspect we can include those for the user stream as data attributes too, and create code that finds the proper parent if it exists.

Yes, I did say that was an option in my prior responses.

---

<div class="post-metadata">

### Author: ![eviltrout](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/eviltrout/32/5275_2.png) [@eviltrout](https://meta.discourse.org/u/eviltrout)
#### Post date: [2016 年 4 月 29 日午後 4:59 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/25 "2016-04-29T16:59:33Z")

</div>

> [@cpradio](#):
>
> It doesn’t have to call a page to redirect to the url it is supposed to go to?

I must be confused here, because middle clicking a link will in fact open a link in a new page, but it is not tracked unless the Ajax request is sent right? If viewing in the user stream wouldn’t that click not be tracked?

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 4 月 29 日午後 5:05 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/26 "2016-04-29T17:05:10Z")

</div>

> [@eviltrout](#):
>
> If viewing in the user stream wouldn’t that click not be tracked?

Today, on meta, nothing is tracked from the User Page. My PR didn’t alter that behavior, but I said it could, if we make the User Stream contain data so I could gather the TopicId and the PostId and pass it to ClickTrack.

So my PR simply permits the external new window setting to be honored on that User Stream (and nothing more at this point in time).

I’m only using ClickTrack to make use of its support for SHIFT, CTRL, META, middle-click, and the other countless scenarios it already understands and deals with.

---

<div class="post-metadata">

### Author: ![eviltrout](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/eviltrout/32/5275_2.png) [@eviltrout](https://meta.discourse.org/u/eviltrout)
#### Post date: [2016 年 4 月 29 日午後 5:08 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/27 "2016-04-29T17:08:25Z")

</div>

I understand that, I think I just have a problem using `ClickTrack` to not track links. I know it seems like a small thing, but decisions like that have a habit of sticking around in a code base for years and can really confuse future developers.

We should either:

a) extract that logic into a function that can be called in both places  
b) get click tracking working

I’d prefer b obviously 🙂

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 4 月 29 日午後 5:09 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/28 "2016-04-29T17:09:59Z")

</div>

> [@eviltrout](#):
>
> I’d prefer b obviously 🙂

I can work with that, I simply needed to know the direction Discourse wanted to go.

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 4 月 29 日午後 5:46 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/29 "2016-04-29T17:46:29Z")

</div>

Okay, I need to write some qunit tests since now click track can work with another set of HTML, but you can see the compare here.

[https://github.com/discourse/discourse/compare/master...cpradio:click-tracks-user-page?expand=1](https://github.com/discourse/discourse/compare/master...cpradio:click-tracks-user-page?expand=1)

@eviltrout, should I go the route I took previously and create a new click-track.profile-page-test.js.es6 file since I’ll have to setup a different DOM fixture? Or is there a better way to achieve this?

Prior Example:  
[https://github.com/cpradio/discourse/blob/open-profile-links-external/test/javascripts/lib/click-track-profile-page-test.js.es6](https://github.com/cpradio/discourse/blob/open-profile-links-external/test/javascripts/lib/click-track-profile-page-test.js.es6)

---

<div class="post-metadata">

### Author: ![eviltrout](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/eviltrout/32/5275_2.png) [@eviltrout](https://meta.discourse.org/u/eviltrout)
#### Post date: [2016 年 4 月 29 日午後 5:52 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/30 "2016-04-29T17:52:06Z")

</div>

> [@cpradio](#):
>
> should I go the route I took previously and create a new click-track.profile-page-test.js.es6 file since I’ll have to setup a different DOM fixture?

This is pretty hard to test elegantly, I think as long as your tests work I’ll accept them so use your judgment 🙂

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 4 月 29 日午後 6:54 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/31 "2016-04-29T18:54:30Z")

</div>

PR Submitted  
[https://github.com/discourse/discourse/pull/4203](https://github.com/discourse/discourse/pull/4203)

---

<div class="post-metadata">

### Author: ![eviltrout](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/eviltrout/32/5275_2.png) [@eviltrout](https://meta.discourse.org/u/eviltrout)
#### Post date: [2016 年 4 月 29 日午後 8:10 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/32 "2016-04-29T20:10:41Z")

</div>

Looks good! I’ll probably wait until Monday to accept it since it’s a bit risky to take in something like this right before the weekend. Thanks for the effort 🙂

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 4 月 29 日午後 8:57 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/33 "2016-04-29T20:57:13Z")

</div>

Not a problem. I’ll look at implementing the Edit History window in a similar fashion next week (maybe this weekend if I have time).

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 4 月 29 日午後 11:30 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/34 "2016-04-29T23:30:12Z")

</div>

@eviltrout, I added a couple more tests, since the User Page has multiple topics/posts, I wanted to make sure the data was sending the correct topic-id and post-id, so now my test fixture has two excerpts and tests to mimic clicks within each excerpt, that way if it ever breaks, it should get caught 🙂

Not that I was concerned my code was wrong, just felt bad not having a test that proved it in that scenario since it should be a common one.

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 5 月 2 日午後 3:27 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/35 "2016-05-02T15:27:23Z")

</div>

I’ve now got a rough version of this that works with the edit revision history too. I have a lot more testing to do before I submit a PR for that, plus I need to write up some new qunit tests around that logic as well.

Hopefully, I’ll have something close to complete by the end of this week.

---

<div class="post-metadata">

### Author: ![cpradio](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/cpradio/32/4970_2.png) [@cpradio](https://meta.discourse.org/u/cpradio)
#### Post date: [2016 年 5 月 2 日午後 6:09 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/36 "2016-05-02T18:09:00Z")

</div>

Well, my meetings this afternoon were cancelled, so I got to write the qunit tests I needed for the Revision History change 🙂

PR Sent  
[https://github.com/discourse/discourse/pull/4207](https://github.com/discourse/discourse/pull/4207)

This should close out this bug report.

---

<div class="post-metadata">

### Author: ![sam](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/sam/32/102149_2.png) [@sam](https://meta.discourse.org/u/sam)
#### Post date: [2016 年 5 月 3 日午前 7:10 UTC](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995/37 "2016-05-03T07:10:32Z")

</div>

change by @cpradio was merged, flag to reopen if still an issue.

[前のページ](https://meta.discourse.org/t/external-links-in-history-don-t-follow-open-all-external-links-in-a-new-tab-setting/42995.md?page=1)
