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

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.)

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/master/app/controllers/clicks_controller.rb#L12

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

Well I haven’t had to yet :wink: 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…

2 Likes

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)

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

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

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?

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.

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 :slight_smile:

1 Like

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

2 Likes

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

@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

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

4 Likes

PR Submitted
https://github.com/discourse/discourse/pull/4203

2 Likes

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 :slight_smile:

2 Likes

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).

@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 :slight_smile:

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.

3 Likes

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.

1 Like

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

PR Sent
https://github.com/discourse/discourse/pull/4207

This should close out this bug report.

7 Likes

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