From e18b93026ad4f04b4e8d81175b1c170f1147166a Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 4 Oct 2013 17:00:23 +1000 Subject: [PATCH] defer view creation on so updates are not performed when people navigate to topics --- app/controllers/topics_controller.rb | 16 ++++++++++++---- app/jobs/regular/view_tracker.rb | 17 +++++++++++++++++ app/models/topic_user.rb | 9 ++++++--- app/models/view.rb | 21 ++++++++++++--------- lib/guardian.rb | 2 +- spec/controllers/topics_controller_spec.rb | 4 +--- 6 files changed, 49 insertions(+), 20 deletions(-) create mode 100644 app/jobs/regular/view_tracker.rb diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 8f4a8996e..f56d1aa71 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -47,8 +47,12 @@ class TopicsController < ApplicationController # (see http://meta.discourse.org/t/noscript-tag-and-some-search-engines/8078) return render 'topics/plain', layout: false if (SiteSetting.enable_escaped_fragments && params.has_key?('_escaped_fragment_')) - View.create_for(@topic_view.topic, request.remote_ip, current_user) track_visit_to_topic + + if should_track_visit_to_topic? + @topic_view.draft = Draft.get(current_user, @topic_view.draft_key, @topic_view.draft_sequence) + end + perform_show_response end @@ -301,9 +305,13 @@ class TopicsController < ApplicationController end def track_visit_to_topic - return unless should_track_visit_to_topic? - TopicUser.track_visit! @topic_view.topic, current_user - @topic_view.draft = Draft.get(current_user, @topic_view.draft_key, @topic_view.draft_sequence) + Jobs.enqueue(:view_tracker, + topic_id: @topic_view.topic.id, + ip: request.remote_ip, + user_id: (current_user.id if current_user), + track_visit: should_track_visit_to_topic? + ) + end def should_track_visit_to_topic? diff --git a/app/jobs/regular/view_tracker.rb b/app/jobs/regular/view_tracker.rb new file mode 100644 index 000000000..d9ecb43ce --- /dev/null +++ b/app/jobs/regular/view_tracker.rb @@ -0,0 +1,17 @@ +module Jobs + + # Asynchronously send an email to a user + class ViewTracker < Jobs::Base + def execute(args) + topic_id = args[:topic_id] + user_id = args[:user_id] + ip = args[:ip] + track_visit = args[:track_visit] + + View.create_for_parent(Topic, topic_id, ip, user_id) + if track_visit + TopicUser.track_visit! topic_id, user_id + end + end + end +end diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index f5e2efda9..150dbf360 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -108,12 +108,15 @@ class TopicUser < ActiveRecord::Base end def track_visit!(topic,user) + topic_id = Topic === topic ? topic.id : topic + user_id = User === user ? user.id : topic + now = DateTime.now - rows = TopicUser.where({topic_id: topic.id, user_id: user.id}).update_all({last_visited_at: now}) + rows = TopicUser.where({topic_id: topic_id, user_id: user_id}).update_all({last_visited_at: now}) if rows == 0 - TopicUser.create(topic_id: topic.id, user_id: user.id, last_visited_at: now, first_visited_at: now) + TopicUser.create(topic_id: topic_id, user_id: user_id, last_visited_at: now, first_visited_at: now) else - observe_after_save_callbacks_for topic.id, user.id + observe_after_save_callbacks_for topic_id, user_id end end diff --git a/app/models/view.rb b/app/models/view.rb index 253c63a27..d0c641c7a 100644 --- a/app/models/view.rb +++ b/app/models/view.rb @@ -5,13 +5,11 @@ class View < ActiveRecord::Base belongs_to :user validates_presence_of :parent_type, :parent_id, :ip_address, :viewed_at - # TODO: This could happen asyncronously - def self.create_for(parent, ip, user=nil) - + def self.create_for_parent(parent_class, parent_id, ip, user_id) # Only store a view once per day per thing per user per ip - redis_key = "view:#{parent.class.name}:#{parent.id}:#{Date.today.to_s}" - if user.present? - redis_key << ":user-#{user.id}" + redis_key = "view:#{parent_class.name}:#{parent_id}:#{Date.today.to_s}" + if user_id + redis_key << ":user-#{user_id}" else redis_key << ":ip-#{ip}" end @@ -20,15 +18,20 @@ class View < ActiveRecord::Base $redis.expire(redis_key, 1.day.to_i) View.transaction do - View.create(parent: parent, ip_address: ip, viewed_at: Date.today, user: user) + View.create!(parent_id: parent_id, parent_type: parent_class.to_s, ip_address: ip, viewed_at: Date.today, user_id: user_id) # Update the views count in the parent, if it exists. - if parent.respond_to?(:views) - parent.class.where(id: parent.id).update_all 'views = views + 1' + if parent_class.columns_hash["views"] + parent_class.where(id: parent_id).update_all 'views = views + 1' end end end end + + def self.create_for(parent, ip, user=nil) + user_id = user.id if user + create_for_parent(parent.class, parent.id, ip, user_id) + end end # == Schema Information diff --git a/lib/guardian.rb b/lib/guardian.rb index c032ac3c4..060895c94 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -427,7 +427,7 @@ class Guardian def is_my_own?(obj) unless anonymous? - return obj.user_id == @user.id if obj.respond_to?(:user_id) + return obj.user_id == @user.id if obj.respond_to?(:user_id) && obj.user_id && @user.id return obj.user == @user if obj.respond_to?(:user) end diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index fb2c61ee3..7ab15c8f9 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -2,8 +2,6 @@ require 'spec_helper' describe TopicsController do - - context 'wordpress' do let!(:user) { log_in(:moderator) } let(:p1) { Fabricate(:post, user: user) } @@ -527,7 +525,7 @@ describe TopicsController do it 'tracks a visit for all html requests' do current_user = log_in(:coding_horror) - TopicUser.expects(:track_visit!).with(topic, current_user) + TopicUser.expects(:track_visit!).with(topic.id, current_user.id) get :show, topic_id: topic.id, slug: topic.slug end