FIX: clicks counter on attachments wasn't always working

app/assets/javascripts/discourse/lib/click_track.js

@@ -5,6 +5,7 @@
   @namespace Discourse
   @module Discourse
 **/
+
 Discourse.ClickTrack = {
 
   /**
@@ -43,15 +44,11 @@ Discourse.ClickTrack = {
     if (!ownLink) {
       var $badge = $('span.badge', $link);
       if ($badge.length === 1) {
-        // don't update counts in category badge
+        // don't update counts in category badge nor in oneboxes (except when we force it)
-        if ($link.closest('.badge-category').length === 0) {
+        if ($link.hasClass("track-link") ||
-          // nor in oneboxes (except when we force it)
+            $link.closest('.badge-category,.onebox-result,.onebox-body').length === 0) {
-          if (($link.closest(".onebox-result").length === 0 && $link.closest('.onebox-body').length === 0) || $link.hasClass("track-link")) {
+          var html = $badge.html();
-            var html = $badge.html();
+          if (/^\d+$/.test(html)) { $badge.html(parseInt(html, 10) + 1); }
-            if (/^\d+$/.test(html)) {
-              $badge.html(parseInt(html, 10) + 1);
-            }
-          }
         }
       }
     }

app/assets/javascripts/discourse/views/post.js.es6

@@ -48,12 +48,12 @@ var PostView = Discourse.GroupedView.extend(Ember.Evented, {
     Em.run.scheduleOnce('afterRender', this, '_cookedWasChanged');
   }.observes('post.cooked'),
 
-  _cookedWasChanged: function() {
+  _cookedWasChanged() {
     this.trigger('postViewUpdated', this.$());
     this._insertQuoteControls();
   },
 
-  mouseUp: function(e) {
+  mouseUp(e) {
     if (this.get('controller.multiSelect') && (e.metaKey || e.ctrlKey)) {
       this.get('controller').toggledSelectedPost(this.get('post'));
     }
@@ -74,7 +74,7 @@ var PostView = Discourse.GroupedView.extend(Ember.Evented, {
 
   repliesShown: Em.computed.gt('post.replies.length', 0),
 
-  _updateQuoteElements: function($aside, desc) {
+  _updateQuoteElements($aside, desc) {
     var navLink = "",
         quoteTitle = I18n.t("post.follow_quote"),
         postNumber = $aside.data('post');
@@ -108,7 +108,7 @@ var PostView = Discourse.GroupedView.extend(Ember.Evented, {
     $('.quote-controls', $aside).html(expandContract + navLink);
   },
 
-  _toggleQuote: function($aside) {
+  _toggleQuote($aside) {
     if (this.get('expanding')) { return; }
     this.set('expanding', true);
 
@@ -151,23 +151,29 @@ var PostView = Discourse.GroupedView.extend(Ember.Evented, {
   },
 
   // Show how many times links have been clicked on
-  _showLinkCounts: function() {
+  _showLinkCounts() {
-    var self = this,
+    const self = this,
-        link_counts = this.get('post.link_counts');
+          link_counts = this.get('post.link_counts');
 
-    if (!link_counts) return;
+    if (!link_counts) { return; }
 
     link_counts.forEach(function(lc) {
-      if (!lc.clicks || lc.clicks < 1) return;
+      if (!lc.clicks || lc.clicks < 1) { return; }
 
       self.$(".cooked a[href]").each(function() {
-        var link = $(this);
+        const $link = $(this),
-        if ((!lc.internal || lc.url[0] === "/") && link.attr('href') === lc.url) {
+              href = $link.attr('href');
-          // don't display badge counts on category badge
+
-          if (link.closest('.badge-category').length === 0 && ((link.closest(".onebox-result").length === 0 && link.closest('.onebox-body').length === 0) || link.hasClass("track-link"))) {
+        let valid = !lc.internal && href === lc.url;
-            link.append("<span class='badge badge-notification clicks' title='" +
+
-                        I18n.t("topic_map.clicks", {count: lc.clicks}) +
+        // this might be an attachment
-                        "'>" + Discourse.Formatter.number(lc.clicks) + "</span>");
+        if (lc.internal) { valid = href.indexOf(lc.url) >= 0; }
+
+        if (valid) {
+          // don't display badge counts on category badge & oneboxes (unless when explicitely stated)
+          if ($link.hasClass("track-link") ||
+              $link.closest('.badge-category,.onebox-result,.onebox-body').length === 0) {
+            $link.append("<span class='badge badge-notification clicks' title='" + I18n.t("topic_map.clicks", {count: lc.clicks}) + "'>" + Discourse.Formatter.number(lc.clicks) + "</span>");
           }
         }
       });
@@ -176,7 +182,7 @@ var PostView = Discourse.GroupedView.extend(Ember.Evented, {
 
   actions: {
     // Toggle the replies this post is a reply to
-    toggleReplyHistory: function(post) {
+    toggleReplyHistory(post) {
 
       var replyHistory = post.get('replyHistory'),
           topicController = this.get('controller'),
@@ -227,7 +233,7 @@ var PostView = Discourse.GroupedView.extend(Ember.Evented, {
   },
 
   // Add the quote controls to a post
-  _insertQuoteControls: function() {
+  _insertQuoteControls() {
     var self = this,
         $quotes = this.$('aside.quote');
 

app/assets/javascripts/discourse/views/topic.js.es6

@@ -51,8 +51,8 @@ var TopicView = Discourse.View.extend(AddCategoryClass, Discourse.Scrolling, {
 
       var $target = $(e.target);
       if ($target.hasClass('mention') || $target.parents('.expanded-embed').length) { return false; }
-      return Discourse.ClickTrack.trackClick(e);
 
+      return Discourse.ClickTrack.trackClick(e);
     });
 
   }.on('didInsertElement'),

app/controllers/clicks_controller.rb

@@ -8,12 +8,6 @@ class ClicksController < ApplicationController
     if params[:topic_id].present? || params[:post_id].present?
       params.merge!({ user_id: current_user.id }) if current_user.present?
       @redirect_url = TopicLinkClick.create_from(params)
-
-      if @redirect_url.blank? && params[:url].index('?')
-        # Check the url without query parameters
-        params[:url].sub!(/\?.*$/, '')
-        @redirect_url = TopicLinkClick.create_from(params)
-      end
     end
 
     # Sometimes we want to record a link without a 302. Since XHR has to load the redirected

app/models/topic_link_click.rb

@@ -1,5 +1,10 @@
 require_dependency 'discourse'
 require 'ipaddr'
+require 'url_helper'
+
+class TopicLinkClickHelper
+  include UrlHelper
+end
 
 class TopicLinkClick < ActiveRecord::Base
   belongs_to :topic_link, counter_cache: :clicks
@@ -10,15 +15,27 @@ class TopicLinkClick < ActiveRecord::Base
 
   # Create a click from a URL and post_id
   def self.create_from(args={})
+    url = args[:url]
+    return nil if url.blank?
 
-    # If the URL is absolute, allow HTTPS and HTTP versions of it
+    helper = TopicLinkClickHelper.new
-    if args[:url] =~ /^http/
+    uri = URI.parse(url) rescue nil
-      http_url = args[:url].sub(/^https/, 'http')
+
-      https_url = args[:url].sub(/^http\:/, 'https:')
+    urls = Set.new
-      link = TopicLink.select([:id, :user_id]).where('url = ? OR url = ?', http_url, https_url)
+    urls << url
-    else
+    if url =~ /^http/
-      link = TopicLink.select([:id, :user_id]).where(url: args[:url])
+      urls << url.sub(/^https/, 'http')
+      urls << url.sub(/^http:/, 'https:')
+      urls << helper.schemaless(url)
     end
+    urls << helper.absolute_without_cdn(url)
+    urls << uri.path if uri.try(:host) == Discourse.current_hostname
+    urls << url.sub(/\?.*$/, '') if url.include?('?')
+
+    link = TopicLink.select([:id, :user_id])
+
+    # test for all possible URLs
+    link = link.where(Array.new(urls.count, "url = ?").join(" OR "), *urls)
 
     # Find the forum topic link
     link = link.where(post_id: args[:post_id]) if args[:post_id].present?
@@ -27,23 +44,18 @@ class TopicLinkClick < ActiveRecord::Base
     link = link.where(topic_id: args[:topic_id]) if args[:topic_id].present?
     link = link.first
 
-    # If no link is found, return the url for relative links
+    # If no link is found...
     unless link.present?
-      return args[:url] if args[:url] =~ /^\//
+      # ... return the url for relative links or when using the same host
+      return url if url =~ /^\// || uri.try(:host) == Discourse.current_hostname
 
-      begin
+      # If we have it somewhere else on the site, just allow the redirect.
-        uri = URI.parse(args[:url])
+      # This is likely due to a onebox of another topic.
-        return args[:url] if uri.host == URI.parse(Discourse.base_url).host
+      link = TopicLink.find_by(url: url)
-      rescue
-      end
-
-      # If we have it somewhere else on the site, just allow the redirect. This is
-      # likely due to a onebox of another topic.
-      link = TopicLink.find_by(url: args[:url])
       return link.present? ? link.url : nil
     end
 
-    return args[:url] if (args[:user_id] && (link.user_id == args[:user_id]))
+    return url if args[:user_id] && link.user_id == args[:user_id]
 
     # Rate limit the click counts to once in 24 hours
     rate_key = "link-clicks:#{link.id}:#{args[:user_id] || args[:ip]}"
@@ -52,7 +64,7 @@ class TopicLinkClick < ActiveRecord::Base
       create!(topic_link_id: link.id, user_id: args[:user_id], ip_address: args[:ip])
     end
 
-    args[:url]
+    url
   end
 
 end

lib/url_helper.rb

@@ -18,7 +18,7 @@ module UrlHelper
   end
 
   def schemaless(url)
-    url.gsub(/^https?:/, "")
+    url.sub(/^https?:/, "")
   end
 
 end

spec/controllers/clicks_controller_spec.rb

@@ -18,9 +18,7 @@ describe ClicksController do
     context 'correct params' do
       let(:url) { "http://discourse.org" }
 
-      before do
+      before { request.stubs(:remote_ip).returns('192.168.0.1') }
-        request.stubs(:remote_ip).returns('192.168.0.1')
-      end
 
       context "with a made up url" do
         it "doesn't redirect" do
@@ -31,27 +29,15 @@ describe ClicksController do
       end
 
       context "with a query string" do
-        it "tries again without the query if it fails" do
+        it "redirects" do
-          TopicLinkClick.expects(:create_from).with(has_entries('url' => 'http://discourse.org/?hello=123')).returns(nil)
+          TopicLinkClick.expects(:create_from).with(has_entries('url' => 'http://discourse.org/?hello=123')).returns(url)
-          TopicLinkClick.expects(:create_from).with(has_entries('url' => 'http://discourse.org/')).returns(nil)
           xhr :get, :track, url: 'http://discourse.org/?hello=123', post_id: 123
+          expect(response).to redirect_to(url)
         end
       end
 
       context 'with a post_id' do
-        it 'calls create_from' do
+        it 'redirects' do
-          TopicLinkClick.expects(:create_from).with('url' => url, 'post_id' => '123', 'ip' => '192.168.0.1').returns(url)
-          xhr :get, :track, url: url, post_id: 123
-          expect(response).to redirect_to(url)
-        end
-
-        it 'redirects to the url' do
-          TopicLinkClick.stubs(:create_from).returns(url)
-          xhr :get, :track, url: url, post_id: 123
-          expect(response).to redirect_to(url)
-        end
-
-        it 'will pass the user_id to create_from' do
           TopicLinkClick.expects(:create_from).with('url' => url, 'post_id' => '123', 'ip' => '192.168.0.1').returns(url)
           xhr :get, :track, url: url, post_id: 123
           expect(response).to redirect_to(url)
@@ -66,7 +52,7 @@ describe ClicksController do
       end
 
       context 'with a topic_id' do
-        it 'calls create_from' do
+        it 'redirects' do
           TopicLinkClick.expects(:create_from).with('url' => url, 'topic_id' => '789', 'ip' => '192.168.0.1').returns(url)
           xhr :get, :track, url: url, topic_id: 789
           expect(response).to redirect_to(url)