From 7f69362d9d59896974ff7cd3710374d78ec61745 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 20 Dec 2017 17:55:15 +0100 Subject: [PATCH] FIX: external links in whisper ended up in a white page FIX: clicking a link in a onebox wasn't properly extracting the post_id --- .../discourse/lib/click-track.js.es6 | 41 +++++++++---------- app/controllers/clicks_controller.rb | 11 +++-- app/models/topic_link.rb | 3 +- spec/controllers/clicks_controller_spec.rb | 12 +++++- 4 files changed, 40 insertions(+), 27 deletions(-) diff --git a/app/assets/javascripts/discourse/lib/click-track.js.es6 b/app/assets/javascripts/discourse/lib/click-track.js.es6 index e4932df07c5..648b49299bb 100644 --- a/app/assets/javascripts/discourse/lib/click-track.js.es6 +++ b/app/assets/javascripts/discourse/lib/click-track.js.es6 @@ -13,31 +13,35 @@ export default { // cancel click if triggered as part of selection. if (selectedText() !== "") { return false; } - var $link = $(e.currentTarget); + const $link = $(e.currentTarget); - // don't track lightboxes, group mentions or links with disabled tracking - if ($link.hasClass('lightbox') || $link.hasClass('mention-group') || - $link.hasClass('no-track-link') || $link.hasClass('hashtag')) { + // don't track + // - lightboxes + // - group mentions + // - links with disabled tracking + // - category links + // - quote back button + if ($link.is('.lightbox, .mention-group, .no-track-link, .hashtag, .back')) { return true; } // don't track links in quotes or in elided part if ($link.parents('aside.quote,.elided').length) { return true; } - var href = $link.attr('href') || $link.data('href'), - $article = $link.closest('article,.excerpt,#revisions'), - postId = $article.data('post-id'), - topicId = $('#topic').data('topic-id') || $article.data('topic-id'), - userId = $link.data('user-id'); + let href = $link.attr('href') || $link.data('href'); if (!href || href.trim().length === 0) { return false; } - if (href.indexOf("mailto:") === 0) { return true; } + if (href.indexOf('mailto:') === 0) { return true; } - if (!userId) userId = $article.data('user-id'); + const $article = $link.closest('article:not(.onebox-body), .excerpt, #revisions'); + const postId = $article.data('post-id'); + const topicId = $('#topic').data('topic-id') || $article.data('topic-id'); + const userId = $link.data('user-id') || $article.data('user-id'); + const ownLink = userId && (userId === Discourse.User.currentProp('id')); - var ownLink = userId && (userId === Discourse.User.currentProp('id')), - trackingUrl = Discourse.getURL("/clicks/track?url=" + encodeURIComponent(href)); - if (postId && (!$link.data('ignore-post-id'))) { + let trackingUrl = Discourse.getURL('/clicks/track?url=' + encodeURIComponent(href)); + + if (postId && !$link.data('ignore-post-id')) { trackingUrl += "&post_id=" + encodeURI(postId); } if (topicId) { @@ -62,8 +66,7 @@ export default { // If they right clicked, change the destination href if (e.which === 3) { - var destination = Discourse.SiteSettings.track_external_right_clicks ? trackingUrl : href; - $link.attr('href', destination); + $link.attr('href', Discourse.SiteSettings.track_external_right_clicks ? trackingUrl : href); return true; } @@ -83,9 +86,6 @@ export default { e.preventDefault(); - // We don't track clicks on quote back buttons - if ($link.hasClass('back')) { return true; } - // Remove the href, put it as a data attribute if (!$link.data('href')) { $link.addClass('no-href'); @@ -125,8 +125,7 @@ export default { // Otherwise, use a custom URL with a redirect if (Discourse.User.currentProp('external_links_in_new_tab')) { - var win = window.open(trackingUrl, '_blank'); - win.focus(); + window.open(trackingUrl, '_blank').focus(); } else { DiscourseURL.redirectTo(trackingUrl); } diff --git a/app/controllers/clicks_controller.rb b/app/controllers/clicks_controller.rb index 8678966ae57..443196fe840 100644 --- a/app/controllers/clicks_controller.rb +++ b/app/controllers/clicks_controller.rb @@ -12,9 +12,14 @@ class ClicksController < ApplicationController @redirect_url = TopicLinkClick.create_from(params) end - # Sometimes we want to record a link without a 302. Since XHR has to load the redirected - # URL we want it to not return a 302 in those cases. - if params[:redirect] == 'false' || @redirect_url.blank? + # links in whispers aren't extracted, just allow the redirection to staff + if @redirect_url.blank? && current_user&.staff? && params[:post_id].present? + @redirect_url = params[:url] if Post.exists?(id: params[:post_id], post_type: Post.types[:whisper]) + end + + # Sometimes we want to record a link without a 302. + # Since XHR has to load the redirected URL we want it to not return a 302 in those cases. + if params[:redirect] == "false" || @redirect_url.blank? render body: nil else redirect_to(@redirect_url) diff --git a/app/models/topic_link.rb b/app/models/topic_link.rb index 4c1b7fca889..a55cd5ca487 100644 --- a/app/models/topic_link.rb +++ b/app/models/topic_link.rb @@ -104,9 +104,8 @@ SQL end end - # Extract any urls in body def self.extract_from(post) - return unless post.present? && !post.whisper? + return if post.blank? || post.whisper? added_urls = [] TopicLink.transaction do diff --git a/spec/controllers/clicks_controller_spec.rb b/spec/controllers/clicks_controller_spec.rb index 0c342c851cb..238199db0b2 100644 --- a/spec/controllers/clicks_controller_spec.rb +++ b/spec/controllers/clicks_controller_spec.rb @@ -50,7 +50,18 @@ describe ClicksController do context 'with a post_id' do it 'redirects' do TopicLinkClick.expects(:create_from).with('url' => url, 'post_id' => '123', 'ip' => '192.168.0.1').returns(url) + get :track, params: { url: url, post_id: 123, format: :json } + + expect(response).to redirect_to(url) + end + + it "redirects links in whispers to staff members" do + log_in(:admin) + whisper = Fabricate(:post, post_type: Post.types[:whisper]) + + get :track, params: { url: url, post_id: whisper.id, format: :json } + expect(response).to redirect_to(url) end @@ -63,7 +74,6 @@ describe ClicksController do expect(response).not_to be_redirect end - end context 'with a topic_id' do