From 37867af1bb6eefd9119c3899e9e45d5ca64c3772 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 24 Apr 2013 18:05:35 +1000 Subject: [PATCH] track incoming links, amend share link to include user fix pm styling --- .../javascripts/discourse/models/post.js | 4 + .../javascripts/discourse/models/topic.js | 29 +++--- .../discourse/templates/post.js.handlebars | 2 +- .../discourse/views/post_menu_view.js | 2 +- .../views/topic_footer_buttons_view.js | 2 +- .../stylesheets/application/compose.css.scss | 4 + app/controllers/application_controller.rb | 7 +- app/controllers/posts_controller.rb | 11 ++- app/controllers/topics_controller.rb | 9 +- app/models/incoming_link.rb | 14 +++ app/models/post.rb | 12 +-- app/models/site_setting.rb | 2 +- config/routes.rb | 3 + ...424055025_add_user_id_to_incoming_links.rb | 5 + spec/controllers/posts_controller_spec.rb | 9 ++ spec/models/incoming_link_spec.rb | 95 ++++++++++--------- 16 files changed, 136 insertions(+), 74 deletions(-) mode change 100644 => 100755 app/assets/javascripts/discourse/models/post.js mode change 100644 => 100755 app/assets/javascripts/discourse/models/topic.js mode change 100644 => 100755 app/assets/javascripts/discourse/templates/post.js.handlebars mode change 100644 => 100755 app/assets/javascripts/discourse/views/post_menu_view.js mode change 100644 => 100755 app/assets/javascripts/discourse/views/topic_footer_buttons_view.js create mode 100644 db/migrate/20130424055025_add_user_id_to_incoming_links.rb diff --git a/app/assets/javascripts/discourse/models/post.js b/app/assets/javascripts/discourse/models/post.js old mode 100644 new mode 100755 index f5ed905b0..258588a6c --- a/app/assets/javascripts/discourse/models/post.js +++ b/app/assets/javascripts/discourse/models/post.js @@ -8,6 +8,10 @@ **/ Discourse.Post = Discourse.Model.extend({ + shareUrl: function(){ + var user = Discourse.get('currentUser'); + return '/p/' + this.get('id') + (user ? '/' + user.get('id') : ''); + }.property('id'), new_user:(function(){ return this.get('trust_level') === 0; diff --git a/app/assets/javascripts/discourse/models/topic.js b/app/assets/javascripts/discourse/models/topic.js old mode 100644 new mode 100755 index 3060364dd..45009941a --- a/app/assets/javascripts/discourse/models/topic.js +++ b/app/assets/javascripts/discourse/models/topic.js @@ -36,13 +36,18 @@ Discourse.Topic = Discourse.Model.extend({ } }).property('categoryName', 'categories'), - url: (function() { + shareUrl: function(){ + var user = Discourse.get('currentUser'); + return '/st/' + this.get('id') + (user ? '/' + user.get('id') : ''); + }.property('id'), + + url: function() { var slug = this.get('slug'); if (slug.isBlank()) { slug = "topic"; } return Discourse.getURL("/t/") + slug + "/" + (this.get('id')); - }).property('id', 'slug'), + }.property('id', 'slug'), // Helper to build a Url with a post number urlForPostNumber: function(postNumber) { @@ -53,20 +58,20 @@ Discourse.Topic = Discourse.Model.extend({ return url; }, - lastReadUrl: (function() { + lastReadUrl: function() { return this.urlForPostNumber(this.get('last_read_post_number')); - }).property('url', 'last_read_post_number'), + }.property('url', 'last_read_post_number'), - lastPostUrl: (function() { + lastPostUrl: function() { return this.urlForPostNumber(this.get('highest_post_number')); - }).property('url', 'highest_post_number'), + }.property('url', 'highest_post_number'), // The last post in the topic lastPost: function() { return this.get('posts').last(); }, - postsChanged: (function() { + postsChanged: function() { var last, posts; posts = this.get('posts'); last = posts.last(); @@ -76,12 +81,12 @@ Discourse.Topic = Discourse.Model.extend({ }); last.set('lastPost', true); return true; - }).observes('posts.@each', 'posts'), + }.observes('posts.@each', 'posts'), // The amount of new posts to display. It might be different than what the server // tells us if we are still asynchronously flushing our "recently read" data. // So take what the browser has seen into consideration. - displayNewPosts: (function() { + displayNewPosts: function() { var delta, highestSeen, result; if (highestSeen = Discourse.get('highestSeenByTopic')[this.get('id')]) { delta = highestSeen - this.get('last_read_post_number'); @@ -94,10 +99,10 @@ Discourse.Topic = Discourse.Model.extend({ } } return this.get('new_posts'); - }).property('new_posts', 'id'), + }.property('new_posts', 'id'), // The coldmap class for the age of the topic - ageCold: (function() { + ageCold: function() { var createdAt, createdAtDays, daysSinceEpoch, lastPost, nowDays; if (!(lastPost = this.get('last_posted_at'))) return; if (!(createdAt = this.get('created_at'))) return; @@ -115,7 +120,7 @@ Discourse.Topic = Discourse.Model.extend({ if (createdAtDays < nowDays - 14) return 'coldmap-low'; } return null; - }).property('age', 'created_at'), + }.property('age', 'created_at'), viewsHeat: function() { var v = this.get('views'); diff --git a/app/assets/javascripts/discourse/templates/post.js.handlebars b/app/assets/javascripts/discourse/templates/post.js.handlebars old mode 100644 new mode 100755 index 9020603ad..533329ac2 --- a/app/assets/javascripts/discourse/templates/post.js.handlebars +++ b/app/assets/javascripts/discourse/templates/post.js.handlebars @@ -32,7 +32,7 @@

{{breakUp username}}

- {{date created_at}} + {{date created_at}}
{{#if hasHistory}}
diff --git a/app/assets/javascripts/discourse/views/post_menu_view.js b/app/assets/javascripts/discourse/views/post_menu_view.js old mode 100644 new mode 100755 index 1540788f4..08d7399a8 --- a/app/assets/javascripts/discourse/views/post_menu_view.js +++ b/app/assets/javascripts/discourse/views/post_menu_view.js @@ -132,7 +132,7 @@ Discourse.PostMenuView = Discourse.View.extend({ renderShare: function(post, buffer) { buffer.push(""); + "\" data-share-url=\"" + (post.get('shareUrl')) + "\">"); }, // Reply button diff --git a/app/assets/javascripts/discourse/views/topic_footer_buttons_view.js b/app/assets/javascripts/discourse/views/topic_footer_buttons_view.js old mode 100644 new mode 100755 index e0647ee1c..edeb5c39d --- a/app/assets/javascripts/discourse/views/topic_footer_buttons_view.js +++ b/app/assets/javascripts/discourse/views/topic_footer_buttons_view.js @@ -62,7 +62,7 @@ Discourse.TopicFooterButtonsView = Ember.ContainerView.extend({ this.addObject(Discourse.ButtonView.create({ textKey: 'topic.share.title', helpKey: 'topic.share.help', - 'data-share-url': topic.get('url'), + 'data-share-url': topic.get('shareUrl'), renderIcon: function(buffer) { buffer.push(""); diff --git a/app/assets/stylesheets/application/compose.css.scss b/app/assets/stylesheets/application/compose.css.scss index 1d6b8e948..72215d1db 100755 --- a/app/assets/stylesheets/application/compose.css.scss +++ b/app/assets/stylesheets/application/compose.css.scss @@ -3,6 +3,10 @@ @import "foundation/variables"; @import "foundation/mixins"; +// hack, this needs to be done cleaner +.private-message input.span8 { + width: 47%; +} .composer-popup { diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3684d2103..52f506f8f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -240,12 +240,7 @@ class ApplicationController < ActionController::Base alias :requires_parameter :requires_parameters def store_incoming_links - if request.referer.present? - parsed = URI.parse(request.referer) - if parsed.host != request.host - IncomingLink.create(url: request.url, referer: request.referer[0..999]) - end - end + IncomingLink.add(request) end def check_xhr diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index bdc23d53a..1fc6376eb 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -4,8 +4,17 @@ require_dependency 'post_destroyer' class PostsController < ApplicationController # Need to be logged in for all actions here - before_filter :ensure_logged_in, except: [:show, :replies, :by_number] + before_filter :ensure_logged_in, except: [:show, :replies, :by_number, :short_link] + skip_before_filter :store_incoming_links, only: [:short_link] + skip_before_filter :check_xhr, only: [:short_link] + + def short_link + post = Post.find(params[:post_id].to_i) + user = User.select(:id).where(id: params[:user_id].to_i).first + IncomingLink.add(request, user ? user.id : nil) + redirect_to post.url + end def create requires_parameter(:post) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 6fdb1bab2..88626c313 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -19,9 +19,16 @@ class TopicsController < ApplicationController before_filter :consider_user_for_promotion, only: :show - skip_before_filter :check_xhr, only: [:avatar, :show, :feed] + skip_before_filter :check_xhr, only: [:avatar, :show, :feed, :short_link] + skip_before_filter :store_incoming_links, only: [:short_link] caches_action :avatar, cache_path: Proc.new {|c| "#{c.params[:post_number]}-#{c.params[:topic_id]}" } + def short_link + topic = Topic.find(params[:topic_id].to_i) + user = User.select(:id).where(id: params[:user_id].to_i).first + IncomingLink.add(request, user ? user.id : nil) + redirect_to topic.relative_url + end def show create_topic_view diff --git a/app/models/incoming_link.rb b/app/models/incoming_link.rb index 8d7c9cbb3..f10f606a5 100644 --- a/app/models/incoming_link.rb +++ b/app/models/incoming_link.rb @@ -9,6 +9,20 @@ class IncomingLink < ActiveRecord::Base before_validation :extract_topic_and_post after_create :update_link_counts + def self.add(request, user_id = nil) + host, referer = nil + + if request.referer.present? + host = URI.parse(request.referer).host + referer = request.referer[0..999] + + if host != request.host + IncomingLink.create(url: request.url, referer: referer, user_id: user_id) + end + end + + end + # Internal: Extract the domain from link. def extract_domain if referer.present? diff --git a/app/models/post.rb b/app/models/post.rb index 464e5e6cb..91b2c70f2 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -270,11 +270,11 @@ class Post < ActiveRecord::Base end def url - Post.url(topic.title, topic.id, post_number) + Post.url(topic.slug, topic.id, post_number) end - def self.url(title, topic_id, post_number) - "/t/#{Slug.for(title)}/#{topic_id}/#{post_number}" + def self.url(slug, topic_id, post_number) + "/t/#{slug}/#{topic_id}/#{post_number}" end def self.urls(post_ids) @@ -282,12 +282,12 @@ class Post < ActiveRecord::Base if ids.length > 0 urls = {} Topic.joins(:posts).where('posts.id' => ids). - select(['posts.id as post_id','post_number', 'topics.title', 'topics.id']). + select(['posts.id as post_id','post_number', 'topics.slug', 'topics.title', 'topics.id']). each do |t| - urls[t.post_id.to_i] = url(t.title, t.id, t.post_number) + urls[t.post_id.to_i] = url(t.slug, t.id, t.post_number) end urls - else + else {} end end diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index cc0edd69c..831458250 100755 --- a/app/models/site_setting.rb +++ b/app/models/site_setting.rb @@ -17,7 +17,7 @@ class SiteSetting < ActiveRecord::Base setting(:company_domain, 'www.example.com') setting(:api_key, '') client_setting(:traditional_markdown_linebreaks, false) - client_setting(:top_menu, 'latest|hot|new|unread|favorited|categories') + client_setting(:top_menu, 'latest|new|unread|favorited|categories') client_setting(:post_menu, 'like|edit|flag|delete|share|bookmark|reply') client_setting(:share_links, 'twitter|facebook|google+|email') client_setting(:track_external_right_clicks, false) diff --git a/config/routes.rb b/config/routes.rb index 1312a0462..408b4bcd9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -129,6 +129,9 @@ Discourse::Application.routes.draw do end end + get 'p/:post_id/:user_id' => 'posts#short_link' + get 'st/:topic_id/:user_id' => 'topics#short_link' + resources :notifications resources :categories diff --git a/db/migrate/20130424055025_add_user_id_to_incoming_links.rb b/db/migrate/20130424055025_add_user_id_to_incoming_links.rb new file mode 100644 index 000000000..623e5af91 --- /dev/null +++ b/db/migrate/20130424055025_add_user_id_to_incoming_links.rb @@ -0,0 +1,5 @@ +class AddUserIdToIncomingLinks < ActiveRecord::Migration + def change + add_column :incoming_links, :user_id, :integer + end +end diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index c194dd4f2..9d067207f 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/spec/controllers/posts_controller_spec.rb @@ -3,6 +3,15 @@ require 'spec_helper' describe PostsController do + describe 'short_link' do + it 'logs the incoming link once' do + IncomingLink.expects(:add).once.returns(true) + p = Fabricate(:post) + get :short_link, post_id: p.id, user_id: 999 + response.should be_redirect + end + end + describe 'show' do let(:post) { Fabricate(:post, user: log_in) } diff --git a/spec/models/incoming_link_spec.rb b/spec/models/incoming_link_spec.rb index b5c7e10aa..ae6ea2202 100644 --- a/spec/models/incoming_link_spec.rb +++ b/spec/models/incoming_link_spec.rb @@ -8,68 +8,75 @@ describe IncomingLink do it { should ensure_length_of(:referer).is_at_least(3).is_at_most(1000) } it { should ensure_length_of(:domain).is_at_least(1).is_at_most(100) } + let :post do + Fabricate(:post) + end + + let :topic do + post.topic + end + + let :incoming_link do + IncomingLink.create(url: "/t/slug/#{topic.id}/#{post.post_number}", + referer: "http://twitter.com") + end + describe 'local topic link' do it 'should validate properly' do Fabricate.build(:incoming_link).should be_valid end + describe 'tracking link counts' do + it "increases the incoming link counts" do + incoming_link + lambda { post.reload }.should change(post, :incoming_link_count).by(1) + lambda { topic.reload }.should change(topic, :incoming_link_count).by(1) + end + end + describe 'saving local link' do - - before do - @post = Fabricate(:post) - @topic = @post.topic - @incoming_link = IncomingLink.create(url: "/t/slug/#{@topic.id}/#{@post.post_number}", - referer: "http://twitter.com") + it 'has correct info set' do + incoming_link.domain.should == "twitter.com" + incoming_link.topic_id.should == topic.id + incoming_link.post_number.should == post.post_number end - describe 'incoming link counts' do - it "increases the post's incoming link count" do - lambda { @incoming_link.save; @post.reload }.should change(@post, :incoming_link_count).by(1) - end + end + end - it "increases the topic's incoming link count" do - lambda { @incoming_link.save; @topic.reload }.should change(@topic, :incoming_link_count).by(1) - end + describe 'add' do + it "does nothing if referer is empty" do + env = Rack::MockRequest.env_for("http://somesite.com") + request = Rack::Request.new(env) + IncomingLink.expects(:create).never + IncomingLink.add(request) + end - end - - describe 'after save' do - before do - @incoming_link.save - end - - it 'has a domain' do - @incoming_link.domain.should == "twitter.com" - end - - it 'has the topic_id' do - @incoming_link.topic_id.should == @topic.id - end - - it 'has the post_number' do - @incoming_link.post_number.should == @post.post_number - end - end + it "does nothing if referer is same as host" do + env = Rack::MockRequest.env_for("http://somesite.com") + env['HTTP_REFERER'] = 'http://somesite.com' + request = Rack::Request.new(env) + IncomingLink.expects(:create).never + IncomingLink.add(request) + end + it "expects to be called with referer and user id" do + env = Rack::MockRequest.env_for("http://somesite.com") + env['HTTP_REFERER'] = 'http://some.other.site.com' + request = Rack::Request.new(env) + IncomingLink.expects(:create).once.returns(true) + IncomingLink.add(request, 100) end end describe 'non-topic url' do - - before do - @link = Fabricate(:incoming_link_not_topic) - end - - it 'has no topic_id' do - @link.topic_id.should be_blank - end - - it 'has no post_number' do - @link.topic_id.should be_blank + it 'has nothing set' do + link = Fabricate(:incoming_link_not_topic) + link.topic_id.should be_blank + link.user_id.should be_blank end end - end