diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 146617c360a..aa657a617d8 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -37,14 +37,12 @@ class PostMover Guardian.new(user).ensure_can_see! topic @destination_topic = topic - move_posts_to_destination_topic - destination_topic - end - - def move_posts_to_destination_topic move_each_post notify_users_that_posts_have_moved update_statistics + update_user_actions + + destination_topic end def move_each_post @@ -87,6 +85,10 @@ class PostMover original_topic.update_statistics end + def update_user_actions + UserAction.synchronize_target_topic_ids(posts.map(&:id)) + end + def notify_users_that_posts_have_moved enqueue_notification_job create_moderator_post_in_original_topic diff --git a/app/models/user_action.rb b/app/models/user_action.rb index 6f7f2536420..d23551fb1ea 100644 --- a/app/models/user_action.rb +++ b/app/models/user_action.rb @@ -187,6 +187,7 @@ ORDER BY p.created_at desc action.id, user_ids: [user_id], group_ids: group_ids ) + action rescue ActiveRecord::RecordNotUnique # can happen, don't care already logged @@ -205,6 +206,23 @@ ORDER BY p.created_at desc update_like_count(hash[:user_id], hash[:action_type], -1) end + def self.synchronize_target_topic_ids(post_ids = nil) + builder = SqlBuilder.new("UPDATE user_actions + SET target_topic_id = (select topic_id from posts where posts.id = target_post_id) + /*where*/") + + builder.where("target_topic_id <> (select topic_id from posts where posts.id = target_post_id)") + if post_ids + builder.where("target_post_id in (:post_ids)", post_ids: post_ids) + end + + builder.exec + end + + def self.ensure_consistency! + self.synchronize_target_topic_ids + end + protected def self.update_like_count(user_id, action_type, delta) diff --git a/lib/jobs/ensure_db_consistency.rb b/lib/jobs/ensure_db_consistency.rb index 9030889874c..e800b59c960 100644 --- a/lib/jobs/ensure_db_consistency.rb +++ b/lib/jobs/ensure_db_consistency.rb @@ -6,6 +6,7 @@ module Jobs UserVisit.ensure_consistency! Group.refresh_automatic_groups! Notification.ensure_consistency! + UserAction.ensure_consistency! end end end diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb new file mode 100644 index 00000000000..08deeb789ae --- /dev/null +++ b/spec/models/post_mover_spec.rb @@ -0,0 +1,178 @@ +require 'spec_helper' + +describe PostMover 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: another_user, raw: "Has a link to [evil trout](http://eviltrout.com) which is a cool site.")} + let!(:p3) { Fabricate(:post, topic: topic, user: user)} + let!(:p4) { Fabricate(:post, topic: topic, user: user)} + + before do + # add a like to a post, enable observers so we get user actions + ActiveRecord::Base.observers.enable :all + @like = PostAction.act(another_user, p4, PostActionType.types[:like]) + end + + context 'success' do + + it "enqueues a job to notify users" do + topic.stubs(:add_moderator_post) + Jobs.expects(:enqueue).with(:notify_moved_posts, post_ids: [p2.id, p4.id], moved_by_id: user.id) + topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name") + end + + it "adds a moderator post at the location of the first moved post" do + topic.expects(:add_moderator_post).with(user, instance_of(String), has_entries(post_number: 2)) + topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name") + end + + end + + context "errors" do + + it "raises an error when one of the posts doesn't exist" do + lambda { topic.move_posts(user, [1003], title: "new testing topic name") }.should raise_error(Discourse::InvalidParameters) + end + + it "raises an error and does not create a topic if no posts were moved" do + Topic.count.tap do |original_topic_count| + lambda { + topic.move_posts(user, [], title: "new testing topic name") + }.should raise_error(Discourse::InvalidParameters) + + expect(Topic.count).to eq original_topic_count + end + end + end + + context "successfully moved" do + before do + topic.expects(:add_moderator_post) + TopicUser.update_last_read(user, topic.id, p4.post_number, 0) + TopicLink.extract_from(p2) + end + + context "to a new topic" do + let!(:new_topic) { topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name") } + + it "works correctly" do + TopicUser.where(user_id: user.id, topic_id: topic.id).first.last_read_post_number.should == p3.post_number + + new_topic.should be_present + new_topic.featured_user1_id.should == another_user.id + new_topic.like_count.should == 1 + + new_topic.category.should == category + topic.featured_user1_id.should be_blank + new_topic.posts.should =~ [p2, p4] + + new_topic.reload + new_topic.posts_count.should == 2 + new_topic.highest_post_number.should == 2 + expect(new_topic.last_posted_at).to be_present + + p2.reload + p2.sort_order.should == 1 + p2.post_number.should == 1 + p2.topic_links.first.topic_id.should == new_topic.id + + p4.reload + p4.post_number.should == 2 + p4.sort_order.should == 2 + + topic.reload + topic.featured_user1_id.should be_blank + topic.like_count.should == 0 + topic.posts_count.should == 2 + topic.posts.should =~ [p1, p3] + topic.highest_post_number.should == p3.post_number + + # both the like and was_liked user actions should be correct + action = UserAction.where(user_id: another_user.id).first + action.target_topic_id.should == new_topic.id + end + end + + context "to an existing topic" do + + let!(:destination_topic) { Fabricate(:topic, user: user ) } + let!(:destination_op) { Fabricate(:post, topic: destination_topic, user: user) } + let!(:moved_to) { topic.move_posts(user, [p2.id, p4.id], destination_topic_id: destination_topic.id )} + + it "works correctly" do + moved_to.should == destination_topic + + # Check out new topic + moved_to.reload + moved_to.posts_count.should == 3 + moved_to.highest_post_number.should == 3 + moved_to.featured_user1_id.should == another_user.id + moved_to.like_count.should == 1 + moved_to.category.should be_blank + + # Posts should be re-ordered + p2.reload + p2.sort_order.should == 2 + p2.post_number.should == 2 + p2.topic_id.should == moved_to.id + + p4.reload + p4.post_number.should == 3 + p4.sort_order.should == 3 + p4.topic_id.should == moved_to.id + + # Check out the original topic + topic.reload + topic.posts_count.should == 2 + topic.highest_post_number.should == 3 + topic.featured_user1_id.should be_blank + topic.like_count.should == 0 + topic.posts_count.should == 2 + topic.posts.should =~ [p1, p3] + topic.highest_post_number.should == p3.post_number + + # Should update last reads + TopicUser.where(user_id: user.id, topic_id: topic.id).first.last_read_post_number.should == p3.post_number + end + end + + context "moving the first post" do + + let!(:new_topic) { topic.move_posts(user, [p1.id, p2.id], title: "new testing topic name") } + + it "copies the OP, doesn't delete it" do + new_topic.should be_present + new_topic.posts.first.raw.should == p1.raw + + new_topic.reload + new_topic.posts_count.should == 2 + new_topic.highest_post_number.should == 2 + + # First post didn't move + p1.reload + p1.sort_order.should == 1 + p1.post_number.should == 1 + p1.topic_id == topic.id + + # Second post is in a new topic + p2.reload + p2.post_number.should == 2 + p2.sort_order.should == 2 + p2.topic_id == new_topic.id + + topic.reload + topic.posts.should =~ [p1, p3, p4] + topic.highest_post_number.should == p4.post_number + end + + end + + + end + end +end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 9b13a30c3ad..bc287bed973 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -228,175 +228,6 @@ describe Topic do end - 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: another_user, raw: "Has a link to [evil trout](http://eviltrout.com) which is a cool site.")} - 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 - topic.stubs(:add_moderator_post) - Jobs.expects(:enqueue).with(:notify_moved_posts, post_ids: [p2.id, p4.id], moved_by_id: user.id) - topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name") - end - - it "adds a moderator post at the location of the first moved post" do - topic.expects(:add_moderator_post).with(user, instance_of(String), has_entries(post_number: 2)) - topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name") - end - - end - - context "errors" do - - it "raises an error when one of the posts doesn't exist" do - lambda { topic.move_posts(user, [1003], title: "new testing topic name") }.should raise_error(Discourse::InvalidParameters) - end - - it "raises an error and does not create a topic if no posts were moved" do - Topic.count.tap do |original_topic_count| - lambda { - topic.move_posts(user, [], title: "new testing topic name") - }.should raise_error(Discourse::InvalidParameters) - - expect(Topic.count).to eq original_topic_count - end - end - end - - context "successfully moved" do - before do - topic.expects(:add_moderator_post) - TopicUser.update_last_read(user, topic.id, p4.post_number, 0) - TopicLink.extract_from(p2) - end - - context "to a new topic" do - let!(:new_topic) { topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name") } - - it "works correctly" do - TopicUser.where(user_id: user.id, topic_id: topic.id).first.last_read_post_number.should == p3.post_number - - new_topic.should be_present - new_topic.featured_user1_id.should == another_user.id - new_topic.like_count.should == 1 - - new_topic.category.should == category - topic.featured_user1_id.should be_blank - new_topic.posts.should =~ [p2, p4] - - new_topic.reload - new_topic.posts_count.should == 2 - new_topic.highest_post_number.should == 2 - expect(new_topic.last_posted_at).to be_present - - p2.reload - p2.sort_order.should == 1 - p2.post_number.should == 1 - p2.topic_links.first.topic_id.should == new_topic.id - - p4.reload - p4.post_number.should == 2 - p4.sort_order.should == 2 - - topic.reload - topic.featured_user1_id.should be_blank - topic.like_count.should == 0 - topic.posts_count.should == 2 - topic.posts.should =~ [p1, p3] - topic.highest_post_number.should == p3.post_number - - end - end - - context "to an existing topic" do - - let!(:destination_topic) { Fabricate(:topic, user: user ) } - let!(:destination_op) { Fabricate(:post, topic: destination_topic, user: user) } - let!(:moved_to) { topic.move_posts(user, [p2.id, p4.id], destination_topic_id: destination_topic.id )} - - it "works correctly" do - moved_to.should == destination_topic - - # Check out new topic - moved_to.reload - moved_to.posts_count.should == 3 - moved_to.highest_post_number.should == 3 - moved_to.featured_user1_id.should == another_user.id - moved_to.like_count.should == 1 - moved_to.category.should be_blank - - # Posts should be re-ordered - p2.reload - p2.sort_order.should == 2 - p2.post_number.should == 2 - p2.topic_id.should == moved_to.id - - p4.reload - p4.post_number.should == 3 - p4.sort_order.should == 3 - p4.topic_id.should == moved_to.id - - # Check out the original topic - topic.reload - topic.posts_count.should == 2 - topic.highest_post_number.should == 3 - topic.featured_user1_id.should be_blank - topic.like_count.should == 0 - topic.posts_count.should == 2 - topic.posts.should =~ [p1, p3] - topic.highest_post_number.should == p3.post_number - - # Should update last reads - TopicUser.where(user_id: user.id, topic_id: topic.id).first.last_read_post_number.should == p3.post_number - end - end - - context "moving the first post" do - - let!(:new_topic) { topic.move_posts(user, [p1.id, p2.id], title: "new testing topic name") } - - it "copies the OP, doesn't delete it" do - new_topic.should be_present - new_topic.posts.first.raw.should == p1.raw - - new_topic.reload - new_topic.posts_count.should == 2 - new_topic.highest_post_number.should == 2 - - # First post didn't move - p1.reload - p1.sort_order.should == 1 - p1.post_number.should == 1 - p1.topic_id == topic.id - - # Second post is in a new topic - p2.reload - p2.post_number.should == 2 - p2.sort_order.should == 2 - p2.topic_id == new_topic.id - - topic.reload - topic.posts.should =~ [p1, p3, p4] - topic.highest_post_number.should == p4.post_number - end - - end - - - end - end context 'private message' do let(:coding_horror) { User.where(username: 'CodingHorror').first } diff --git a/spec/models/user_action_spec.rb b/spec/models/user_action_spec.rb index 2f5bdea4dba..9f0dfc13aeb 100644 --- a/spec/models/user_action_spec.rb +++ b/spec/models/user_action_spec.rb @@ -9,7 +9,6 @@ describe UserAction do it { should validate_presence_of :action_type } it { should validate_presence_of :user_id } - describe 'lists' do let(:public_post) { Fabricate(:post) } @@ -257,4 +256,26 @@ describe UserAction do end end + + describe 'ensure_consistency!' do + it 'correct target_topic_id' do + post = Fabricate(:post) + + action = UserAction.log_action!( + action_type: UserAction::NEW_PRIVATE_MESSAGE, + user_id: post.user.id, + acting_user_id: post.user.id, + target_topic_id: -1, + target_post_id: post.id, + ) + + action.reload + action.target_topic_id.should == -1 + + UserAction.ensure_consistency! + action.reload + action.target_topic_id.should == post.topic_id + + end + end end