From 1bf4f34049e99d1fc1efa0ae5c66a154fbfeab30 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Mon, 2 Mar 2015 11:17:11 -0500 Subject: [PATCH] FIX: topic and post counts are not updated when ownership of a post is changed --- app/controllers/topics_controller.rb | 25 +++------- app/services/post_owner_changer.rb | 23 +++++++++ lib/post_revisor.rb | 13 +++++ spec/controllers/topics_controller_spec.rb | 27 ++-------- spec/services/post_owner_changer_spec.rb | 58 ++++++++++++++++++++++ 5 files changed, 107 insertions(+), 39 deletions(-) create mode 100644 app/services/post_owner_changer.rb create mode 100644 spec/services/post_owner_changer_spec.rb diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 499fba7bb06..0f76977cc68 100644 --- a/app/controllers/topics_controller.rb +++ b/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 diff --git a/app/services/post_owner_changer.rb b/app/services/post_owner_changer.rb new file mode 100644 index 00000000000..bf1311d0f66 --- /dev/null +++ b/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 diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index c612e04535a..251922b361e 100644 --- a/lib/post_revisor.rb +++ b/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 diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 4432b25b493..490545cfe55 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/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 diff --git a/spec/services/post_owner_changer_spec.rb b/spec/services/post_owner_changer_spec.rb new file mode 100644 index 00000000000..699aa2e9fed --- /dev/null +++ b/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