FIX: topic and post counts are not updated when ownership of a post is changed

app/controllers/topics_controller.rb

@@ -332,24 +332,15 @@ class TopicsController < ApplicationController
 
     guardian.ensure_can_change_post_owner!
 
-    post_ids = params[:post_ids].to_a
-    topic = Topic.find_by(id: params[:topic_id].to_i)
-    new_user = User.find_by(username: params[:username])
-
-    return render json: failed_json, status: 422 unless post_ids && topic && new_user
-
-    ActiveRecord::Base.transaction do
-      post_ids.each do |post_id|
-        post = Post.find(post_id)
-        # update topic owner (first avatar)
-        topic.user = new_user if post.is_first_post?
-        post.set_owner(new_user, current_user)
-      end
+    begin
+      PostOwnerChanger.new( post_ids: params[:post_ids].to_a,
+                            topic_id: params[:topic_id].to_i,
+                            new_owner: User.find_by(username: params[:username]),
+                            acting_user: current_user ).change_owner!
+      render json: success_json
+    rescue ArgumentError
+      render json: failed_json, status: 422
     end
-
-    topic.update_statistics
-
-    render json: success_json
   end
 
   def clear_pin

app/services/post_owner_changer.rb

@@ -0,0 +1,23 @@
+class PostOwnerChanger
+
+  def initialize(params)
+    @post_ids = params[:post_ids]
+    @topic = Topic.find_by(id: params[:topic_id].to_i)
+    @new_owner = params[:new_owner]
+    @acting_user = params[:acting_user]
+
+    raise ArgumentError unless @post_ids && @topic && @new_owner && @acting_user
+  end
+
+  def change_owner!
+    ActiveRecord::Base.transaction do
+      @post_ids.each do |post_id|
+        post = Post.find(post_id)
+        @topic.user = @new_owner if post.is_first_post?
+        post.set_owner(@new_owner, @acting_user)
+      end
+    end
+
+    @topic.update_statistics
+  end
+end

lib/post_revisor.rb

@@ -200,6 +200,19 @@ class PostRevisor
   end
 
   def update_post
+    if @fields.has_key?("user_id") && @fields["user_id"] != @post.user_id
+      if prev_owner = User.find_by_id(@post.user_id)
+        prev_owner.user_stat.update_attributes({post_count: prev_owner.post_count - 1}.merge(
+          @post.is_first_post? ? {topic_count: prev_owner.topic_count - 1} : {}
+        ))
+      end
+      if new_owner = User.find_by_id(@fields["user_id"])
+        new_owner.user_stat.update_attributes({post_count: new_owner.post_count + 1}.merge(
+          @post.is_first_post? ? {topic_count: new_owner.topic_count + 1} : {}
+        ))
+      end
+    end
+
     POST_TRACKED_FIELDS.each do |field|
       @post.send("#{field}=", @fields[field]) if @fields.has_key?(field)
     end

spec/controllers/topics_controller_spec.rb

@@ -222,40 +222,23 @@ describe TopicsController do
       let(:topic) { Fabricate(:topic) }
       let(:user_a) { Fabricate(:user) }
       let(:p1) { Fabricate(:post, topic_id: topic.id) }
+      let(:p2) { Fabricate(:post, topic_id: topic.id) }
 
       it "raises an error with a parameter missing" do
         expect { xhr :post, :change_post_owners, topic_id: 111, post_ids: [1,2,3] }.to raise_error(ActionController::ParameterMissing)
         expect { xhr :post, :change_post_owners, topic_id: 111, username: 'user_a' }.to raise_error(ActionController::ParameterMissing)
       end
 
-      it "calls PostRevisor" do
-        PostRevisor.any_instance.expects(:revise!)
+      it "calls PostOwnerChanger" do
+        PostOwnerChanger.any_instance.expects(:change_owner!).returns(true)
         xhr :post, :change_post_owners, topic_id: topic.id, username: user_a.username_lower, post_ids: [p1.id]
         expect(response).to be_success
       end
 
-      it "changes the user" do
-        old_user = p1.user
-        xhr :post, :change_post_owners, topic_id: topic.id, username: user_a.username_lower, post_ids: [p1.id]
-        p1.reload
-        expect(old_user).not_to eq(p1.user)
-      end
-
-      # Make sure that p1.reload isn't changing the user for us
-      it "is not an artifact of the framework" do
-        old_user = p1.user
-        # xhr :post, :change_post_owners, topic_id: topic.id, username: user_a.username_lower, post_ids: [p1.id]
-        p1.reload
-        expect(p1.user).not_to eq(nil)
-        expect(old_user).to eq(p1.user)
-      end
-
-      let(:p2) { Fabricate(:post, topic_id: topic.id) }
-
       it "changes multiple posts" do
+        # an integration test
         xhr :post, :change_post_owners, topic_id: topic.id, username: user_a.username_lower, post_ids: [p1.id, p2.id]
-        p1.reload
-        p2.reload
+        p1.reload; p2.reload
         expect(p1.user).not_to eq(nil)
         expect(p1.user).to eq(p2.user)
       end

spec/services/post_owner_changer_spec.rb

@@ -0,0 +1,58 @@
+require "spec_helper"
+
+describe PostOwnerChanger do
+  describe "change_owner!" do
+    let!(:editor) { Fabricate(:admin) }
+    let(:topic) { Fabricate(:topic) }
+    let(:user_a) { Fabricate(:user) }
+    let(:p1) { Fabricate(:post, topic_id: topic.id) }
+    let(:p2) { Fabricate(:post, topic_id: topic.id) }
+
+    it "raises an error with a parameter missing" do
+      expect {
+        described_class.new(post_ids: [p1.id], topic_id: topic.id, new_owner: nil, acting_user: editor)
+      }.to raise_error(ArgumentError)
+    end
+
+    it "calls PostRevisor" do
+      PostRevisor.any_instance.expects(:revise!)
+      described_class.new(post_ids: [p1.id], topic_id: topic.id, new_owner: user_a, acting_user: editor).change_owner!
+    end
+
+    it "changes the user" do
+      old_user = p1.user
+      described_class.new(post_ids: [p1.id], topic_id: topic.id, new_owner: user_a, acting_user: editor).change_owner!
+      p1.reload
+      expect(old_user).not_to eq(p1.user)
+      expect(p1.user).to eq(user_a)
+    end
+
+    it "changes multiple posts" do
+      described_class.new(post_ids: [p1.id, p2.id], topic_id: topic.id, new_owner: user_a, acting_user: editor).change_owner!
+      p1.reload; p2.reload
+      expect(p1.user).not_to eq(nil)
+      expect(p1.user).to eq(user_a)
+      expect(p1.user).to eq(p2.user)
+    end
+
+    it "updates users' topic and post counts" do
+      p1user = p1.user
+      p2user = p2.user
+      topic.user_id = p1user.id
+      topic.save!
+
+      p1user.user_stat.update_attributes(topic_count: 1, post_count: 1)
+      p2user.user_stat.update_attributes(topic_count: 0, post_count: 1)
+
+      described_class.new(post_ids: [p1.id, p2.id], topic_id: topic.id, new_owner: user_a, acting_user: editor).change_owner!
+
+      p1user.reload; p2user.reload; user_a.reload
+      p1user.topic_count.should == 0
+      p1user.post_count.should == 0
+      p2user.topic_count.should == 0
+      p2user.post_count.should == 0
+      user_a.topic_count.should == 1
+      user_a.post_count.should == 2
+    end
+  end
+end