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

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
+    // don't track
-    if ($link.hasClass('lightbox') || $link.hasClass('mention-group') ||
+    //   - lightboxes
-        $link.hasClass('no-track-link') || $link.hasClass('hashtag')) {
+    //   - 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'),
+    let 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');
 
     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')),
+    let trackingUrl = Discourse.getURL('/clicks/track?url=' + encodeURIComponent(href));
-        trackingUrl = Discourse.getURL("/clicks/track?url=" + encodeURIComponent(href));
+
-    if (postId && (!$link.data('ignore-post-id'))) {
+    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', Discourse.SiteSettings.track_external_right_clicks ? trackingUrl : href);
-      $link.attr('href', destination);
       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');
+      window.open(trackingUrl, '_blank').focus();
-      win.focus();
     } else {
       DiscourseURL.redirectTo(trackingUrl);
     }

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
+    # links in whispers aren't extracted, just allow the redirection to staff
-    # URL we want it to not return a 302 in those cases.
+    if @redirect_url.blank? && current_user&.staff? && params[:post_id].present?
-    if params[:redirect] == 'false' || @redirect_url.blank?
+      @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)

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

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