From 20c9a312c7d957ba9d02cd2b4cf6a87b74e54325 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Sun, 22 Feb 2015 20:47:18 +0100
Subject: [PATCH] FIX: clicks counter on attachments wasn't always working
---
.../javascripts/discourse/lib/click_track.js | 15 +++---
.../javascripts/discourse/views/post.js.es6 | 42 ++++++++-------
.../javascripts/discourse/views/topic.js.es6 | 2 +-
app/controllers/clicks_controller.rb | 6 ---
app/models/topic_link_click.rb | 52 ++++++++++++-------
lib/url_helper.rb | 2 +-
spec/controllers/clicks_controller_spec.rb | 26 +++-------
7 files changed, 70 insertions(+), 75 deletions(-)
diff --git a/app/assets/javascripts/discourse/lib/click_track.js b/app/assets/javascripts/discourse/lib/click_track.js
index f53d0e1ed..9672c860e 100644
--- a/app/assets/javascripts/discourse/lib/click_track.js
+++ b/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
- if ($link.closest('.badge-category').length === 0) {
- // nor in oneboxes (except when we force it)
- if (($link.closest(".onebox-result").length === 0 && $link.closest('.onebox-body').length === 0) || $link.hasClass("track-link")) {
- var html = $badge.html();
- if (/^\d+$/.test(html)) {
- $badge.html(parseInt(html, 10) + 1);
- }
- }
+ // don't update counts in category badge nor in oneboxes (except when we force it)
+ if ($link.hasClass("track-link") ||
+ $link.closest('.badge-category,.onebox-result,.onebox-body').length === 0) {
+ var html = $badge.html();
+ if (/^\d+$/.test(html)) { $badge.html(parseInt(html, 10) + 1); }
}
}
}
diff --git a/app/assets/javascripts/discourse/views/post.js.es6 b/app/assets/javascripts/discourse/views/post.js.es6
index 422771c06..42bf9dfc4 100644
--- a/app/assets/javascripts/discourse/views/post.js.es6
+++ b/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() {
- var self = this,
- link_counts = this.get('post.link_counts');
+ _showLinkCounts() {
+ const self = this,
+ 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);
- if ((!lc.internal || lc.url[0] === "/") && link.attr('href') === lc.url) {
- // 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"))) {
- link.append("<span class='badge badge-notification clicks' title='" +
- I18n.t("topic_map.clicks", {count: lc.clicks}) +
- "'>" + Discourse.Formatter.number(lc.clicks) + "</span>");
+ const $link = $(this),
+ href = $link.attr('href');
+
+ let valid = !lc.internal && href === lc.url;
+
+ // this might be an attachment
+ 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');
diff --git a/app/assets/javascripts/discourse/views/topic.js.es6 b/app/assets/javascripts/discourse/views/topic.js.es6
index 758a8a06e..1050dc5a3 100644
--- a/app/assets/javascripts/discourse/views/topic.js.es6
+++ b/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'),
diff --git a/app/controllers/clicks_controller.rb b/app/controllers/clicks_controller.rb
index 056a8e84d..8472c9e7f 100644
--- a/app/controllers/clicks_controller.rb
+++ b/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
diff --git a/app/models/topic_link_click.rb b/app/models/topic_link_click.rb
index e06ad2ce7..008263567 100644
--- a/app/models/topic_link_click.rb
+++ b/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
- if args[:url] =~ /^http/
- http_url = args[:url].sub(/^https/, 'http')
- https_url = args[:url].sub(/^http\:/, 'https:')
- link = TopicLink.select([:id, :user_id]).where('url = ? OR url = ?', http_url, https_url)
- else
- link = TopicLink.select([:id, :user_id]).where(url: args[:url])
+ helper = TopicLinkClickHelper.new
+ uri = URI.parse(url) rescue nil
+
+ urls = Set.new
+ urls << url
+ if url =~ /^http/
+ 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
- uri = URI.parse(args[:url])
- return args[:url] if uri.host == URI.parse(Discourse.base_url).host
- 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])
+ # 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: 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
diff --git a/lib/url_helper.rb b/lib/url_helper.rb
index 19d2fc23d..f34753100 100644
--- a/lib/url_helper.rb
+++ b/lib/url_helper.rb
@@ -18,7 +18,7 @@ module UrlHelper
end
def schemaless(url)
- url.gsub(/^https?:/, "")
+ url.sub(/^https?:/, "")
end
end
diff --git a/spec/controllers/clicks_controller_spec.rb b/spec/controllers/clicks_controller_spec.rb
index c86b1af45..6a1db2d83 100644
--- a/spec/controllers/clicks_controller_spec.rb
+++ b/spec/controllers/clicks_controller_spec.rb
@@ -18,9 +18,7 @@ describe ClicksController do
context 'correct params' do
let(:url) { "http://discourse.org" }
- before do
- request.stubs(:remote_ip).returns('192.168.0.1')
- end
+ before { request.stubs(:remote_ip).returns('192.168.0.1') }
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
- 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/')).returns(nil)
+ it "redirects" do
+ TopicLinkClick.expects(:create_from).with(has_entries('url' => 'http://discourse.org/?hello=123')).returns(url)
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
- 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
+ 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)
@@ -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)