From 547b571d8422acf7bba5bfa67cfd923fe1fead9b Mon Sep 17 00:00:00 2001 From: OsamaSayegh Date: Wed, 18 Jul 2018 15:23:32 +0300 Subject: [PATCH 1/2] FIX: topic owner should watch the new topic when moving posts to a new topic --- app/models/post_mover.rb | 11 ++++++++++ spec/requests/topics_controller_spec.rb | 27 +++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 1e18ea3d3e..ec4cb3f02f 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -34,6 +34,8 @@ class PostMover ) DiscourseTagging.tag_topic_by_names(new_topic, Guardian.new(user), tags) move_posts_to new_topic + force_user_to_watch_new_topic + new_topic end end @@ -223,4 +225,13 @@ class PostMover destination_topic.update_columns(attrs) end end + + def force_user_to_watch_new_topic + TopicUser.change( + destination_topic.user, + destination_topic.id, + notification_level: TopicUser.notification_levels[:watching], + notifications_reason_id: TopicUser.notification_reasons[:created_topic] + ) + end end diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index e98a2ab94a..b57c190756 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -58,9 +58,12 @@ RSpec.describe TopicsController do describe 'moving to a new topic' do let(:user) { Fabricate(:user) } + let(:user2) { Fabricate(:user) } let(:moderator) { Fabricate(:moderator) } let(:p1) { Fabricate(:post, user: user, post_number: 1) } let(:p2) { Fabricate(:post, user: user, post_number: 2, topic: p1.topic) } + let(:p3) { Fabricate(:post, user: user2, post_number: 3, topic: p1.topic) } + let(:p4) { Fabricate(:post, user: user2, post_number: 4, topic: p1.topic) } let!(:topic) { p1.topic } it "raises an error without post_ids" do @@ -116,6 +119,30 @@ RSpec.describe TopicsController do expect(Tag.all.pluck(:name)).to contain_exactly("tag1", "tag2") end + it "forces resulting topic owner to watch the new topic" do + expect do + post "/t/#{topic.id}/move-posts.json", params: { + title: 'Logan is a good movie', + post_ids: [p3.id, p4.id], + } + end.to change { Topic.count }.by(1) + + expect(response.status).to eq(200) + + result = ::JSON.parse(response.body) + + expect(result['success']).to eq(true) + new_topic = p3.reload.topic + expect(result['url']).to eq(new_topic.relative_url) + + expect(TopicUser.exists?( + user_id: user2.id, + topic_id: new_topic.id, + notification_level: TopicUser.notification_levels[:watching], + notifications_reason_id: TopicUser.notification_reasons[:created_topic] + )).to eq(true) + end + describe 'when topic has been deleted' do it 'should still be able to move posts' do PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy From 69450750d1e9398024a4bf022ca49051cf859677 Mon Sep 17 00:00:00 2001 From: OsamaSayegh Date: Fri, 20 Jul 2018 10:13:27 +0300 Subject: [PATCH 2/2] shorter method name and better specs --- app/models/post_mover.rb | 4 ++-- spec/models/post_mover_spec.rb | 13 ++++++++++++ spec/requests/topics_controller_spec.rb | 27 ------------------------- 3 files changed, 15 insertions(+), 29 deletions(-) diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index ec4cb3f02f..535ffe50d0 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -34,7 +34,7 @@ class PostMover ) DiscourseTagging.tag_topic_by_names(new_topic, Guardian.new(user), tags) move_posts_to new_topic - force_user_to_watch_new_topic + watch_new_topic new_topic end end @@ -226,7 +226,7 @@ class PostMover end end - def force_user_to_watch_new_topic + def watch_new_topic TopicUser.change( destination_topic.user, destination_topic.id, diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 9f8e49778e..83d6a68d51 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -264,6 +264,19 @@ describe PostMover do moderator_post = topic.posts.last expect(moderator_post.raw).to include("2 posts were split") end + + it "forces resulting topic owner to watch the new topic" do + new_topic = topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name", category_id: category.id) + + expect(new_topic.posts_count).to eq(2) + + expect(TopicUser.exists?( + user_id: another_user, + topic_id: new_topic.id, + notification_level: TopicUser.notification_levels[:watching], + notifications_reason_id: TopicUser.notification_reasons[:created_topic] + )).to eq(true) + end end context "to an existing topic" do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index b57c190756..e98a2ab94a 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -58,12 +58,9 @@ RSpec.describe TopicsController do describe 'moving to a new topic' do let(:user) { Fabricate(:user) } - let(:user2) { Fabricate(:user) } let(:moderator) { Fabricate(:moderator) } let(:p1) { Fabricate(:post, user: user, post_number: 1) } let(:p2) { Fabricate(:post, user: user, post_number: 2, topic: p1.topic) } - let(:p3) { Fabricate(:post, user: user2, post_number: 3, topic: p1.topic) } - let(:p4) { Fabricate(:post, user: user2, post_number: 4, topic: p1.topic) } let!(:topic) { p1.topic } it "raises an error without post_ids" do @@ -119,30 +116,6 @@ RSpec.describe TopicsController do expect(Tag.all.pluck(:name)).to contain_exactly("tag1", "tag2") end - it "forces resulting topic owner to watch the new topic" do - expect do - post "/t/#{topic.id}/move-posts.json", params: { - title: 'Logan is a good movie', - post_ids: [p3.id, p4.id], - } - end.to change { Topic.count }.by(1) - - expect(response.status).to eq(200) - - result = ::JSON.parse(response.body) - - expect(result['success']).to eq(true) - new_topic = p3.reload.topic - expect(result['url']).to eq(new_topic.relative_url) - - expect(TopicUser.exists?( - user_id: user2.id, - topic_id: new_topic.id, - notification_level: TopicUser.notification_levels[:watching], - notifications_reason_id: TopicUser.notification_reasons[:created_topic] - )).to eq(true) - end - describe 'when topic has been deleted' do it 'should still be able to move posts' do PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy