From 1103dde5cd033727f52ceb6c0b1826ab2db90f51 Mon Sep 17 00:00:00 2001
From: Robin Ward <robin.ward@gmail.com>
Date: Tue, 12 Mar 2013 12:33:42 -0400
Subject: [PATCH] Fix: When you split topics, featured users and like counts
 were incorrect.

---
 app/models/post_action.rb       |  2 +-
 app/models/topic.rb             | 59 ++++++++++++++++++++++++++++++---
 lib/jobs/feature_topic_users.rb | 23 +------------
 spec/models/topic_spec.rb       | 28 +++++++++++++++-
 4 files changed, 83 insertions(+), 29 deletions(-)

diff --git a/app/models/post_action.rb b/app/models/post_action.rb
index d8a6dc57009..b8a1746d29b 100644
--- a/app/models/post_action.rb
+++ b/app/models/post_action.rb
@@ -136,7 +136,7 @@ class PostAction < ActiveRecord::Base
 
     # Voting also changes the sort_order
     if post_action_type == :vote
-      Post.update_all ["vote_count = vote_count + :delta, sort_order = :max - (vote_count + :delta)", delta: delta, max: Topic::MAX_SORT_ORDER], id: post_id
+      Post.update_all ["vote_count = vote_count + :delta, sort_order = :max - (vote_count + :delta)", delta: delta, max: Topic.max_sort_order], id: post_id
     else
       Post.update_all ["#{column} = #{column} + ?", delta], id: post_id
     end
diff --git a/app/models/topic.rb b/app/models/topic.rb
index d7b012c0b6f..6bc15d7691d 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -8,8 +8,13 @@ class Topic < ActiveRecord::Base
   include ActionView::Helpers
   include RateLimiter::OnCreateRecord
 
-  MAX_SORT_ORDER = 2**31 - 1
-  FEATURED_USERS = 4
+  def self.max_sort_order
+    2**31 - 1
+  end
+
+  def self.featured_users_count
+    4
+  end
 
   versioned if: :new_version_required?
   acts_as_paranoid
@@ -123,7 +128,7 @@ class Topic < ActiveRecord::Base
     return title unless SiteSetting.title_fancy_entities?
 
     # We don't always have to require this, if fancy is disabled
-    # see: http://meta.discourse.org/t/pattern-for-defer-loading-gems-and-profiling-with-perftools-rb/4629 
+    # see: http://meta.discourse.org/t/pattern-for-defer-loading-gems-and-profiling-with-perftools-rb/4629
     require 'redcarpet' unless defined? Redcarpet
 
     Redcarpet::Render::SmartyPants.render(title)
@@ -416,8 +421,6 @@ class Topic < ActiveRecord::Base
       end
 
       # Update denormalized values since we've manually moved stuff
-      Topic.reset_highest(topic.id)
-      Topic.reset_highest(id)
     end
 
     # Add a moderator post explaining that the post was moved
@@ -427,15 +430,61 @@ class Topic < ActiveRecord::Base
 
       add_moderator_post(moved_by, I18n.t("move_posts.moderator_post", count: post_ids.size, topic_link: topic_link), post_number: first_post_number)
       Jobs.enqueue(:notify_moved_posts, post_ids: post_ids, moved_by_id: moved_by.id)
+
+      topic.update_statistics
+      update_statistics
     end
 
+
     topic
   end
 
+  # Updates the denormalized statistics of a topic including featured posters. They shouldn't
+  # go out of sync unless you do something drastic live move posts from one topic to another.
+  # this recalculates everything.
+  def update_statistics
+    feature_topic_users
+    update_action_counts
+    Topic.reset_highest(id)
+  end
+
   def update_flagged_posts_count
     PostAction.update_flagged_posts_count
   end
 
+  def update_action_counts
+    PostActionType.types.keys.each do |type|
+      count_field = "#{type}_count"
+      update_column(count_field, Post.where(topic_id: id).sum(count_field))
+    end
+  end
+
+  # Chooses which topic users to feature
+  def feature_topic_users(args={})
+    reload
+
+    to_feature = posts
+
+    # Don't include the OP or the last poster
+    to_feature = to_feature.where('user_id NOT IN (?, ?)', user_id, last_post_user_id)
+
+    # Exclude a given post if supplied (in the case of deletes)
+    to_feature = to_feature.where("id <> ?", args[:except_post_id]) if args[:except_post_id].present?
+
+    # Clear the featured users by default
+    Topic.featured_users_count.times do |i|
+      send("featured_user#{i+1}_id=", nil)
+    end
+
+    # Assign the featured_user{x} columns
+    to_feature = to_feature.group(:user_id).order('count_all desc').limit(Topic.featured_users_count)
+
+    to_feature.count.keys.each_with_index do |user_id, i|
+      send("featured_user#{i+1}_id=", user_id)
+    end
+
+    save
+  end
 
   # Create the summary of the interesting posters in a topic. Cheats to avoid
   # many queries.
diff --git a/lib/jobs/feature_topic_users.rb b/lib/jobs/feature_topic_users.rb
index aea4504343a..a13e5f79b32 100644
--- a/lib/jobs/feature_topic_users.rb
+++ b/lib/jobs/feature_topic_users.rb
@@ -6,28 +6,7 @@ module Jobs
       topic = Topic.where(id: args[:topic_id]).first
       raise Discourse::InvalidParameters.new(:topic_id) unless topic.present?
 
-      to_feature = topic.posts
-
-      # Don't include the OP or the last poster
-      to_feature = to_feature.where('user_id <> ?', topic.user_id)
-      to_feature = to_feature.where('user_id <> ?', topic.last_post_user_id)
-
-      # Exclude a given post if supplied (in the case of deletes)
-      to_feature = to_feature.where("id <> ?", args[:except_post_id]) if args[:except_post_id].present?
-
-
-      # Clear the featured users by default
-      Topic::FEATURED_USERS.times do |i|
-        topic.send("featured_user#{i+1}_id=", nil)
-      end
-
-      # Assign the featured_user{x} columns
-      to_feature = to_feature.group(:user_id).order('count_all desc').limit(Topic::FEATURED_USERS)
-      to_feature.count.keys.each_with_index do |user_id, i|
-        topic.send("featured_user#{i+1}_id=", user_id)
-      end
-
-      topic.save
+      topic.feature_topic_users(args)
     end
 
   end
diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb
index 32a8afdd718..d6a1397246a 100644
--- a/spec/models/topic_spec.rb
+++ b/spec/models/topic_spec.rb
@@ -181,13 +181,19 @@ describe Topic do
 
   context 'move_posts' do
     let(:user) { Fabricate(:user) }
+    let(:another_user) { Fabricate(:evil_trout) }
     let(:category) { Fabricate(:category, user: user) }
     let!(:topic) { Fabricate(:topic, user: user, category: category) }
     let!(:p1) { Fabricate(:post, topic: topic, user: user) }
-    let!(:p2) { Fabricate(:post, topic: topic, user: user)}
+    let!(:p2) { Fabricate(:post, topic: topic, user: another_user)}
     let!(:p3) { Fabricate(:post, topic: topic, user: user)}
     let!(:p4) { Fabricate(:post, topic: topic, user: user)}
 
+    before do
+      # add a like to a post
+      PostAction.act(another_user, p4, PostActionType.types[:like])
+    end
+
     context 'success' do
 
       it "enqueues a job to notify users" do
@@ -233,10 +239,22 @@ describe Topic do
           new_topic.should be_present
         end
 
+        it "has the featured user" do
+          new_topic.featured_user1_id.should == another_user.id
+        end
+
+        it "has the correct like_count" do
+          new_topic.like_count.should == 1
+        end
+
         it "has the correct category" do
           new_topic.category.should == category
         end
 
+        it "has removed the second poster from the featured users, since they moved" do
+          topic.featured_user1_id.should be_blank
+        end
+
         it "has two posts" do
           new_topic.reload
           new_topic.posts_count.should == 2
@@ -277,6 +295,14 @@ describe Topic do
           topic.reload
         end
 
+        it "has removed the second poster from the featured users, since they moved" do
+          topic.featured_user1_id.should be_blank
+        end
+
+        it "has the correct like_count" do
+          topic.like_count.should == 0
+        end
+
         it "has 2 posts now" do
           topic.posts_count.should == 2
         end