{{i18n 'loading'}}
+{{else}} + {{#if noResults}} +{{i18n 'choose_message.none_found'}}
+ {{else}} + {{#each messages as |m|}} + + {{/each}} + {{/if}} +{{/if}} diff --git a/app/assets/javascripts/discourse/templates/modal/merge-topic.hbs b/app/assets/javascripts/discourse/templates/modal/merge-topic.hbs deleted file mode 100644 index a94f8b40e41..00000000000 --- a/app/assets/javascripts/discourse/templates/modal/merge-topic.hbs +++ /dev/null @@ -1,13 +0,0 @@ -{{#d-modal-body id='move-selected'}} -{{{i18n 'topic.merge_topic.instructions' count=selectedPostsCount}}}
- - -{{/d-modal-body}} - - diff --git a/app/assets/javascripts/discourse/templates/modal/move-to-topic.hbs b/app/assets/javascripts/discourse/templates/modal/move-to-topic.hbs new file mode 100644 index 00000000000..dc880aea42b --- /dev/null +++ b/app/assets/javascripts/discourse/templates/modal/move-to-topic.hbs @@ -0,0 +1,112 @@ +{{#d-modal-body id='move-selected'}} + + {{#if model.isPrivateMessage}} +{{{i18n 'topic.move_to_new_message.instructions' count=selectedPostsCount}}}
+ + {{/if}} + {{/if}} + + {{#if existingMessage}} +{{{i18n 'topic.move_to_existing_message.instructions' count=selectedPostsCount}}}
+ + {{/if}} + + {{else}} + +{{{i18n 'topic.merge_topic.instructions' count=selectedPostsCount}}}
+ + {{/if}} + + {{#if canSplitTopic}} + {{#if newTopic}} +{{{i18n 'topic.split_topic.instructions' count=selectedPostsCount}}}
+ + {{/if}} + {{/if}} + + {{#if canSplitTopic}} + {{#if newMessage}} +{{{i18n 'topic.move_to_new_message.instructions' count=selectedPostsCount}}}
+ + {{/if}} + {{/if}} + {{/if}} + +{{/d-modal-body}} + + diff --git a/app/assets/javascripts/discourse/templates/modal/split-topic.hbs b/app/assets/javascripts/discourse/templates/modal/split-topic.hbs deleted file mode 100644 index 4022af325ad..00000000000 --- a/app/assets/javascripts/discourse/templates/modal/split-topic.hbs +++ /dev/null @@ -1,21 +0,0 @@ -{{#d-modal-body id="move-selected" title="topic.split_topic.title"}} - {{{i18n 'topic.split_topic.instructions' count=selectedPostsCount}}} - - -{{/d-modal-body}} - - diff --git a/app/assets/javascripts/discourse/templates/selected-posts.hbs b/app/assets/javascripts/discourse/templates/selected-posts.hbs index 781b2282769..452fc8b1878 100644 --- a/app/assets/javascripts/discourse/templates/selected-posts.hbs +++ b/app/assets/javascripts/discourse/templates/selected-posts.hbs @@ -12,12 +12,8 @@ {{d-button action="deleteSelected" icon="trash-o" label="topic.multi_select.delete" class="btn-danger"}} {{/if}} -{{#if canSplitTopic}} - {{d-button action="splitTopic" icon="sign-out" label="topic.split_topic.action"}} -{{/if}} - {{#if canMergeTopic}} - {{d-button action="mergeTopic" icon="sign-out" label="topic.merge_topic.action"}} + {{d-button action=(route-action "moveToTopic") icon="sign-out" label="topic.move_to.action" class="move-to-topic"}} {{/if}} {{#if canChangeOwner}} diff --git a/app/assets/stylesheets/desktop/modal.scss b/app/assets/stylesheets/desktop/modal.scss index 4fe10c2b897..895292e9437 100644 --- a/app/assets/stylesheets/desktop/modal.scss +++ b/app/assets/stylesheets/desktop/modal.scss @@ -113,19 +113,28 @@ } } -.split-modal { +.move-to-modal { .modal-body { position: relative; height: 350px; } #move-selected { + width: 475px; + p { margin-top: 0; } - input[type="radio"] { - margin-right: 10px; + .radios { + margin-bottom: 10px; + display: flex; + flex-direction: row; + + .radio-label { + display: inline-block; + padding-right: 15px; + } } button { @@ -142,9 +151,19 @@ width: 95%; margin-top: 20px; #split-topic-name, - #choose-topic-title { + #choose-topic-title, + #choose-message-title { width: 100%; } + + .participant-selector { + width: 100%; + } + + div.ac-wrap { + width: 100%; + margin-bottom: 9px; + } } } } diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index e022dd70eac..33b0ebecc06 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -607,13 +607,26 @@ class TopicsController < ApplicationController end def merge_topic - params.require(:destination_topic_id) + topic_id = params.require(:topic_id) + destination_topic_id = params.require(:destination_topic_id) + params.permit(:participants) + params.permit(:archetype) - topic = Topic.find_by(id: params[:topic_id]) + raise Discourse::InvalidAccess if params[:archetype] == "private_message" && !guardian.is_staff? + + topic = Topic.find_by(id: topic_id) guardian.ensure_can_move_posts!(topic) - dest_topic = topic.move_posts(current_user, topic.posts.pluck(:id), destination_topic_id: params[:destination_topic_id].to_i) - render_topic_changes(dest_topic) + args = {} + args[:destination_topic_id] = destination_topic_id.to_i + + if params[:archetype].present? + args[:archetype] = params[:archetype] + args[:participants] = params[:participants] if params[:participants].present? && params[:archetype] == "private_message" + end + + destination_topic = topic.move_posts(current_user, topic.posts.pluck(:id), args) + render_topic_changes(destination_topic) end def move_posts @@ -621,6 +634,10 @@ class TopicsController < ApplicationController topic_id = params.require(:topic_id) params.permit(:category_id) params.permit(:tags) + params.permit(:participants) + params.permit(:archetype) + + raise Discourse::InvalidAccess if params[:archetype] == "private_message" && !guardian.is_staff? topic = Topic.with_deleted.find_by(id: topic_id) guardian.ensure_can_move_posts!(topic) @@ -630,8 +647,8 @@ class TopicsController < ApplicationController return render_json_error("When moving posts to a new topic, the first post must be a regular post.") end - dest_topic = move_posts_to_destination(topic) - render_topic_changes(dest_topic) + destination_topic = move_posts_to_destination(topic) + render_topic_changes(destination_topic) rescue ActiveRecord::RecordInvalid => ex render_json_error(ex) end @@ -893,9 +910,15 @@ class TopicsController < ApplicationController args = {} args[:title] = params[:title] if params[:title].present? args[:destination_topic_id] = params[:destination_topic_id].to_i if params[:destination_topic_id].present? - args[:category_id] = params[:category_id].to_i if params[:category_id].present? args[:tags] = params[:tags] if params[:tags].present? + if params[:archetype].present? + args[:archetype] = params[:archetype] + args[:participants] = params[:participants] if params[:participants].present? && params[:archetype] == "private_message" + else + args[:category_id] = params[:category_id].to_i if params[:category_id].present? + end + topic.move_posts(current_user, post_ids_including_replies, args) end diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 359a08e6ed4..47b8f3c1334 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -5,18 +5,24 @@ class PostMover @move_types ||= Enum.new(:new_topic, :existing_topic) end - def initialize(original_topic, user, post_ids) + def initialize(original_topic, user, post_ids, move_to_pm: false) @original_topic = original_topic @user = user @post_ids = post_ids + @move_to_pm = move_to_pm end - def to_topic(id) + def to_topic(id, participants: nil) @move_type = PostMover.move_types[:existing_topic] + topic = Topic.find_by_id(id) + raise Discourse::InvalidParameters unless topic.archetype == @original_topic.archetype + Topic.transaction do - move_posts_to Topic.find_by_id(id) + move_posts_to topic end + add_allowed_users(participants) if participants.present? && @move_to_pm + topic end def to_new_topic(title, category_id = nil, tags = nil) @@ -24,13 +30,15 @@ class PostMover post = Post.find_by(id: post_ids.first) raise Discourse::InvalidParameters unless post + archetype = @move_to_pm ? Archetype.private_message : Archetype.default Topic.transaction do new_topic = Topic.create!( user: post.user, title: title, category_id: category_id, - created_at: post.created_at + created_at: post.created_at, + archetype: archetype ) DiscourseTagging.tag_topic_by_names(new_topic, Guardian.new(user), tags) move_posts_to new_topic @@ -79,7 +87,11 @@ class PostMover posts.each do |post| post.is_first_post? ? create_first_post(post) : move(post) + if @move_to_pm + destination_topic.topic_allowed_users.build(user_id: post.user_id) unless destination_topic.topic_allowed_users.where(user_id: post.user_id).exists? + end end + destination_topic.save! if @move_to_pm PostReply.where("reply_id IN (:post_ids) OR post_id IN (:post_ids)", post_ids: post_ids).each do |post_reply| if post_reply.post && post_reply.reply && post_reply.reply.topic_id != post_reply.post.topic_id @@ -189,6 +201,7 @@ class PostMover I18n.t( "move_posts.#{move_type_str}_moderator_post", count: posts.length, + entity: @move_to_pm ? "message" : "topic", topic_link: posts.first.is_first_post? ? "[#{destination_topic.title}](#{destination_topic.relative_url})" : "[#{destination_topic.title}](#{posts.first.url})" @@ -234,4 +247,14 @@ class PostMover notifications_reason_id: TopicUser.notification_reasons[:created_topic] ) end + + def add_allowed_users(usernames) + return unless usernames.present? + + names = usernames.split(',').flatten + User.where(username: names).find_each do |user| + destination_topic.topic_allowed_users.build(user_id: user.id) unless destination_topic.topic_allowed_users.where(user_id: user.id).exists? + end + destination_topic.save! + end end diff --git a/app/models/topic.rb b/app/models/topic.rb index 0afa508bf9d..b1baf0ecf0b 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -896,10 +896,10 @@ class Topic < ActiveRecord::Base end def move_posts(moved_by, post_ids, opts) - post_mover = PostMover.new(self, moved_by, post_ids) + post_mover = PostMover.new(self, moved_by, post_ids, move_to_pm: opts[:archetype].present? && opts[:archetype] == "private_message") if opts[:destination_topic_id] - topic = post_mover.to_topic(opts[:destination_topic_id]) + topic = post_mover.to_topic(opts[:destination_topic_id], participants: opts[:participants]) DiscourseEvent.trigger(:topic_merged, post_mover.original_topic, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d0b7c06d6ba..bb24a2ff5ae 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -347,9 +347,15 @@ en: choose_topic: none_found: "No topics found." title: - search: "Search for a Topic by name, url or id:" + search: "Search for a Topic by title, url or id:" placeholder: "type the topic title here" + choose_message: + none_found: "No messages found." + title: + search: "Search for a Message by title:" + placeholder: "type the message title here" + queue: topic: "Topic:" approve: 'Approve' @@ -2004,10 +2010,16 @@ en: other: "{{count}} posts" cancel: "Remove filter" + move_to: + title: "Move to" + action: "move to" + error: "There was an error moving posts." + split_topic: title: "Move to New Topic" action: "move to new topic" - topic_name: "New Topic Name" + topic_name: "New Topic Title" + radio_label: "New Topic" error: "There was an error moving posts to the new topic." instructions: one: "You are about to create a new topic and populate it with the post you've selected." @@ -2017,10 +2029,30 @@ en: title: "Move to Existing Topic" action: "move to existing topic" error: "There was an error moving posts into that topic." + radio_label: "Existing Topic" instructions: one: "Please choose the topic you'd like to move that post to." other: "Please choose the topic you'd like to move those {{count}} posts to." + move_to_new_message: + title: "Move to New Message" + action: "move to new message" + message_title: "New Message Title" + radio_label: "New Message" + participants: "Participants" + instructions: + one: "You are about to create a new message and populate it with the post you've selected." + other: "You are about to create a new message and populate it with the {{count}} posts you've selected." + + move_to_existing_message: + title: "Move to Existing Message" + action: "move to existing message" + radio_label: "Existing Message" + participants: "Participants" + instructions: + one: "Please choose the message you'd like to move that post to." + other: "Please choose the message you'd like to move those {{count}} posts to." + merge_posts: title: "Merge Selected Posts" action: "merge selected posts" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 4508de60d74..8be76b60db1 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1988,11 +1988,11 @@ en: move_posts: new_topic_moderator_post: - one: "A post was split to a new topic: %{topic_link}" - other: "%{count} posts were split to a new topic: %{topic_link}" + one: "A post was split to a new %{entity}: %{topic_link}" + other: "%{count} posts were split to a new %{entity}: %{topic_link}" existing_topic_moderator_post: - one: "A post was merged into an existing topic: %{topic_link}" - other: "%{count} posts were merged into an existing topic: %{topic_link}" + one: "A post was merged into an existing %{entity}: %{topic_link}" + other: "%{count} posts were merged into an existing %{entity}: %{topic_link}" change_owner: post_revision_text: "Ownership transferred" diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 667302353ba..482f75835c4 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -18,532 +18,647 @@ describe PostMover do end end - context 'move_posts' do - let(:user) { Fabricate(:user, admin: true) } - let(:another_user) { Fabricate(:evil_trout) } - let(:category) { Fabricate(:category, user: user) } - let!(:topic) { Fabricate(:topic, user: user) } - let!(:p1) { Fabricate(:post, topic: topic, user: user, created_at: 3.hours.ago) } + describe 'move_posts' do + context 'topics' do + let(:user) { Fabricate(:user, admin: true) } + let(:another_user) { Fabricate(:evil_trout) } + let(:category) { Fabricate(:category, user: user) } + let!(:topic) { Fabricate(:topic, user: user) } + let!(:p1) { Fabricate(:post, topic: topic, user: user, created_at: 3.hours.ago) } - let!(:p2) do - Fabricate(:post, - topic: topic, - user: another_user, - raw: "Has a link to [evil trout](http://eviltrout.com) which is a cool site.", - reply_to_post_number: p1.post_number) - end - - let!(:p3) { Fabricate(:post, topic: topic, reply_to_post_number: p1.post_number, user: user) } - let!(:p4) { Fabricate(:post, topic: topic, reply_to_post_number: p2.post_number, user: user) } - let!(:p5) { Fabricate(:post) } - let(:p6) { Fabricate(:post, topic: topic) } - - before do - SiteSetting.tagging_enabled = true - SiteSetting.queue_jobs = false - p1.replies << p3 - p2.replies << p4 - UserActionCreator.enable - @like = PostAction.act(another_user, p4, PostActionType.types[:like]) - end - - context 'success' do - - it "correctly handles notifications and bread crumbs" do - old_topic = p2.topic - - old_topic_id = p2.topic_id - - topic.move_posts(user, [p2.id, p4.id, p6.id], title: "new testing topic name") - - p2.reload - expect(p2.topic_id).not_to eq(old_topic_id) - expect(p2.reply_to_post_number).to eq(nil) - expect(p2.reply_to_user_id).to eq(nil) - - notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first - - expect(notification.topic_id).to eq(p2.topic_id) - expect(notification.post_number).to eq(1) - - # no message for person who made the move - expect(p4.user.notifications.where(notification_type: Notification.types[:moved_post]).length).to eq(0) - - # notify at the right spot in the stream - notification = p6.user.notifications.where(notification_type: Notification.types[:moved_post]).first - - expect(notification.topic_id).to eq(p2.topic_id) - - # this is the 3rd post we moved - expect(notification.post_number).to eq(3) - - old_topic.reload - move_message = old_topic.posts.find_by(post_number: 2) - expect(move_message.post_type).to eq(Post.types[:small_action]) - expect(move_message.raw).to include("3 posts were split") + let!(:p2) do + Fabricate(:post, + topic: topic, + user: another_user, + raw: "Has a link to [evil trout](http://eviltrout.com) which is a cool site.", + reply_to_post_number: p1.post_number) end - end + let!(:p3) { Fabricate(:post, topic: topic, reply_to_post_number: p1.post_number, user: user) } + let!(:p4) { Fabricate(:post, topic: topic, reply_to_post_number: p2.post_number, user: user) } + let!(:p5) { Fabricate(:post) } + let(:p6) { Fabricate(:post, topic: topic) } - context "errors" do - - it "raises an error when one of the posts doesn't exist" do - expect { topic.move_posts(user, [1003], title: "new testing topic name") }.to 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| - expect { - topic.move_posts(user, [], title: "new testing topic name") - }.to raise_error(Discourse::InvalidParameters) - - expect(Topic.count).to eq original_topic_count - end - end - end - - context "successfully moved" do before do - TopicUser.update_last_read(user, topic.id, p4.post_number, p4.post_number, 0) - TopicLink.extract_from(p2) + SiteSetting.tagging_enabled = true + SiteSetting.queue_jobs = false + p1.replies << p3 + p2.replies << p4 + UserActionCreator.enable + @like = PostAction.act(another_user, p4, PostActionType.types[:like]) end - context "post replies" do - describe "when a post with replies is moved" do - it "should update post replies correctly" do - topic.move_posts( - user, - [p2.id], - title: 'GOT is a very addictive show', category_id: category.id - ) + context 'success' do - expect(p2.reload.replies).to eq([]) - end + it "correctly handles notifications and bread crumbs" do + old_topic = p2.topic - it "doesn't raise errors with deleted replies" do - p4.trash! - topic.move_posts( - user, - [p2.id], - title: 'GOT is a very addictive show', category_id: category.id - ) + old_topic_id = p2.topic_id - expect(p2.reload.replies).to eq([]) - end - end - - describe "when replies of a post have been moved" do - it "should update post replies correctly" do - p5 = Fabricate( - :post, - topic: topic, - reply_to_post_number: p2.post_number, - user: another_user - ) - - p2.replies << p5 - - topic.move_posts( - user, - [p4.id], - title: 'GOT is a very addictive show', category_id: category.id - ) - - expect(p2.reload.replies).to eq([p5]) - end - end - - describe "when only one reply is left behind" do - it "should update post replies correctly" do - p5 = Fabricate( - :post, - topic: topic, - reply_to_post_number: p2.post_number, - user: another_user - ) - - p2.replies << p5 - - topic.move_posts( - user, - [p2.id, p4.id], - title: 'GOT is a very addictive show', category_id: category.id - ) - - expect(p2.reload.replies).to eq([p4]) - end - end - end - - context "to a new topic" do - - it "works correctly" do - topic.expects(:add_moderator_post).once - new_topic = topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name", category_id: category.id, tags: ["tag1", "tag2"]) - - expect(TopicUser.find_by(user_id: user.id, topic_id: topic.id).last_read_post_number).to eq(p3.post_number) - - expect(new_topic).to be_present - expect(new_topic.featured_user1_id).to eq(p4.user_id) - expect(new_topic.like_count).to eq(1) - - expect(new_topic.category).to eq(category) - expect(new_topic.tags.pluck(:name)).to contain_exactly("tag1", "tag2") - expect(topic.featured_user1_id).to be_blank - expect(new_topic.posts.by_post_number).to match_array([p2, p4]) - - new_topic.reload - expect(new_topic.posts_count).to eq(2) - expect(new_topic.highest_post_number).to eq(2) - - p4.reload - expect(new_topic.last_post_user_id).to eq(p4.user_id) - expect(new_topic.last_posted_at).to eq(p4.created_at) - expect(new_topic.bumped_at).to eq(p4.created_at) + topic.move_posts(user, [p2.id, p4.id, p6.id], title: "new testing topic name") p2.reload - expect(p2.sort_order).to eq(1) - expect(p2.post_number).to eq(1) - expect(p2.topic_links.first.topic_id).to eq(new_topic.id) - - expect(p4.post_number).to eq(2) - expect(p4.sort_order).to eq(2) - - topic.reload - expect(topic.featured_user1_id).to be_blank - expect(topic.like_count).to eq(0) - expect(topic.posts_count).to eq(2) - expect(topic.posts.by_post_number).to match_array([p1, p3]) - expect(topic.highest_post_number).to eq(p3.post_number) - - # both the like and was_liked user actions should be correct - action = UserAction.find_by(user_id: another_user.id) - expect(action.target_topic_id).to eq(new_topic.id) - end - - it "moving all posts will close the topic" do - topic.expects(:add_moderator_post).twice - new_topic = topic.move_posts(user, [p1.id, p2.id, p3.id, p4.id], title: "new testing topic name", category_id: category.id) - expect(new_topic).to be_present - - topic.reload - expect(topic.closed).to eq(true) - end - - it 'does not move posts that do not belong to the existing topic' do - new_topic = topic.move_posts( - user, [p2.id, p3.id, p5.id], title: 'Logan is a pretty good movie' - ) - - expect(new_topic.posts.pluck(:id).sort).to eq([p2.id, p3.id].sort) - end - - it "uses default locale for moderator post" do - I18n.locale = 'de' - - new_topic = topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name", category_id: category.id) - post = Post.find_by(topic_id: topic.id, post_type: Post.types[:small_action]) - - expected_text = I18n.with_locale(:en) do - I18n.t("move_posts.new_topic_moderator_post", - count: 2, - topic_link: "[#{new_topic.title}](#{new_topic.relative_url})") - end - - expect(post.raw).to eq(expected_text) - end - - it "does not try to move small action posts" do - small_action = Fabricate(:post, topic: topic, raw: "A small action", post_type: Post.types[:small_action]) - new_topic = topic.move_posts(user, [p2.id, p4.id, small_action.id], title: "new testing topic name", category_id: category.id) - - expect(new_topic.posts_count).to eq(2) - expect(small_action.topic_id).to eq(topic.id) - - 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 - let!(:destination_topic) { Fabricate(:topic, user: another_user) } - let!(:destination_op) { Fabricate(:post, topic: destination_topic, user: another_user) } - - it "works correctly" do - topic.expects(:add_moderator_post).once - moved_to = topic.move_posts(user, [p2.id, p4.id], destination_topic_id: destination_topic.id) - expect(moved_to).to eq(destination_topic) - - # Check out new topic - moved_to.reload - expect(moved_to.posts_count).to eq(3) - expect(moved_to.highest_post_number).to eq(3) - expect(moved_to.user_id).to eq(destination_op.user_id) - expect(moved_to.like_count).to eq(1) - expect(moved_to.category_id).to eq(SiteSetting.uncategorized_category_id) - p4.reload - expect(moved_to.last_post_user_id).to eq(p4.user_id) - expect(moved_to.last_posted_at).to eq(p4.created_at) - expect(moved_to.bumped_at).to eq(p4.created_at) - - # Posts should be re-ordered - p2.reload - expect(p2.sort_order).to eq(2) - expect(p2.post_number).to eq(2) - expect(p2.topic_id).to eq(moved_to.id) - expect(p2.reply_count).to eq(1) + expect(p2.topic_id).not_to eq(old_topic_id) expect(p2.reply_to_post_number).to eq(nil) + expect(p2.reply_to_user_id).to eq(nil) - expect(p4.post_number).to eq(3) - expect(p4.sort_order).to eq(3) - expect(p4.topic_id).to eq(moved_to.id) - expect(p4.reply_count).to eq(0) - expect(p4.reply_to_post_number).to eq(2) - - # Check out the original topic - topic.reload - expect(topic.posts_count).to eq(2) - expect(topic.highest_post_number).to eq(3) - expect(topic.featured_user1_id).to be_blank - expect(topic.like_count).to eq(0) - expect(topic.posts_count).to eq(2) - expect(topic.posts.by_post_number).to match_array([p1, p3]) - expect(topic.highest_post_number).to eq(p3.post_number) - - # Should notify correctly notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first expect(notification.topic_id).to eq(p2.topic_id) - expect(notification.post_number).to eq(p2.post_number) + expect(notification.post_number).to eq(1) - # Should update last reads - expect(TopicUser.find_by(user_id: user.id, topic_id: topic.id).last_read_post_number).to eq(p3.post_number) - end + # no message for person who made the move + expect(p4.user.notifications.where(notification_type: Notification.types[:moved_post]).length).to eq(0) - it "moving all posts will close the topic" do - topic.expects(:add_moderator_post).twice - moved_to = topic.move_posts(user, [p1.id, p2.id, p3.id, p4.id], destination_topic_id: destination_topic.id) - expect(moved_to).to be_present + # notify at the right spot in the stream + notification = p6.user.notifications.where(notification_type: Notification.types[:moved_post]).first - topic.reload - expect(topic.closed).to eq(true) - end + expect(notification.topic_id).to eq(p2.topic_id) - it "does not try to move small action posts" do - small_action = Fabricate(:post, topic: topic, raw: "A small action", post_type: Post.types[:small_action]) - moved_to = topic.move_posts(user, [p1.id, p2.id, p3.id, p4.id, small_action.id], destination_topic_id: destination_topic.id) + # this is the 3rd post we moved + expect(notification.post_number).to eq(3) - moved_to.reload - expect(moved_to.posts_count).to eq(5) - expect(small_action.topic_id).to eq(topic.id) - - moderator_post = topic.posts.find_by(post_number: 2) - expect(moderator_post.raw).to include("4 posts were merged") + old_topic.reload + move_message = old_topic.posts.find_by(post_number: 2) + expect(move_message.post_type).to eq(Post.types[:small_action]) + expect(move_message.raw).to include("3 posts were split") end end - shared_examples "moves email related stuff" do - it "moves incoming email" do - Fabricate(:incoming_email, user: old_post.user, topic: old_post.topic, post: old_post) + context "errors" do - new_topic = topic.move_posts(user, [old_post.id], title: "new testing topic name") - new_post = new_topic.first_post - email = new_post.incoming_email - - expect(email).to be_present - expect(email.topic_id).to eq(new_topic.id) - expect(email.post_id).to eq(new_post.id) - - expect(old_post.reload.incoming_email).to_not be_present unless old_post.id == new_post.id + it "raises an error when one of the posts doesn't exist" do + expect { topic.move_posts(user, [1003], title: "new testing topic name") }.to raise_error(Discourse::InvalidParameters) end - it "moves email log entries" do - old_topic = old_post.topic + it "raises an error and does not create a topic if no posts were moved" do + Topic.count.tap do |original_topic_count| + expect { + topic.move_posts(user, [], title: "new testing topic name") + }.to raise_error(Discourse::InvalidParameters) - 2.times do - Fabricate(:email_log, - user: old_post.user, - post: old_post, - email_type: :mailing_list - ) + expect(Topic.count).to eq original_topic_count + end + end + end + + context "successfully moved" do + before do + TopicUser.update_last_read(user, topic.id, p4.post_number, p4.post_number, 0) + TopicLink.extract_from(p2) + end + + context "post replies" do + describe "when a post with replies is moved" do + it "should update post replies correctly" do + topic.move_posts( + user, + [p2.id], + title: 'GOT is a very addictive show', category_id: category.id + ) + + expect(p2.reload.replies).to eq([]) + end + + it "doesn't raise errors with deleted replies" do + p4.trash! + topic.move_posts( + user, + [p2.id], + title: 'GOT is a very addictive show', category_id: category.id + ) + + expect(p2.reload.replies).to eq([]) + end end - some_post = Fabricate(:post) + describe "when replies of a post have been moved" do + it "should update post replies correctly" do + p5 = Fabricate( + :post, + topic: topic, + reply_to_post_number: p2.post_number, + user: another_user + ) - Fabricate(:email_log, - user: some_post.user, - post: some_post, - email_type: :mailing_list - ) + p2.replies << p5 - expect(EmailLog.where(post_id: old_post.id).count).to eq(2) + topic.move_posts( + user, + [p4.id], + title: 'GOT is a very addictive show', category_id: category.id + ) - new_topic = old_topic.move_posts( - user, - [old_post.id], - title: "new testing topic name" - ) + expect(p2.reload.replies).to eq([p5]) + end + end - new_post = new_topic.first_post + describe "when only one reply is left behind" do + it "should update post replies correctly" do + p5 = Fabricate( + :post, + topic: topic, + reply_to_post_number: p2.post_number, + user: another_user + ) - expect(EmailLog.where(post_id: new_post.id).count).to eq(2) + p2.replies << p5 + + topic.move_posts( + user, + [p2.id, p4.id], + title: 'GOT is a very addictive show', category_id: category.id + ) + + expect(p2.reload.replies).to eq([p4]) + end + end end - it "preserves post attributes" do - old_post.update_columns(cook_method: Post.cook_methods[:email], via_email: true, raw_email: "raw email content") + context "to a new topic" do - new_topic = old_post.topic.move_posts(user, [old_post.id], title: "new testing topic name") - new_post = new_topic.first_post + it "works correctly" do + topic.expects(:add_moderator_post).once + new_topic = topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name", category_id: category.id, tags: ["tag1", "tag2"]) - expect(new_post.cook_method).to eq(Post.cook_methods[:email]) - expect(new_post.via_email).to eq(true) - expect(new_post.raw_email).to eq("raw email content") + expect(TopicUser.find_by(user_id: user.id, topic_id: topic.id).last_read_post_number).to eq(p3.post_number) + + expect(new_topic).to be_present + expect(new_topic.featured_user1_id).to eq(p4.user_id) + expect(new_topic.like_count).to eq(1) + + expect(new_topic.category).to eq(category) + expect(new_topic.tags.pluck(:name)).to contain_exactly("tag1", "tag2") + expect(topic.featured_user1_id).to be_blank + expect(new_topic.posts.by_post_number).to match_array([p2, p4]) + + new_topic.reload + expect(new_topic.posts_count).to eq(2) + expect(new_topic.highest_post_number).to eq(2) + + p4.reload + expect(new_topic.last_post_user_id).to eq(p4.user_id) + expect(new_topic.last_posted_at).to eq(p4.created_at) + expect(new_topic.bumped_at).to eq(p4.created_at) + + p2.reload + expect(p2.sort_order).to eq(1) + expect(p2.post_number).to eq(1) + expect(p2.topic_links.first.topic_id).to eq(new_topic.id) + + expect(p4.post_number).to eq(2) + expect(p4.sort_order).to eq(2) + + topic.reload + expect(topic.featured_user1_id).to be_blank + expect(topic.like_count).to eq(0) + expect(topic.posts_count).to eq(2) + expect(topic.posts.by_post_number).to match_array([p1, p3]) + expect(topic.highest_post_number).to eq(p3.post_number) + + # both the like and was_liked user actions should be correct + action = UserAction.find_by(user_id: another_user.id) + expect(action.target_topic_id).to eq(new_topic.id) + end + + it "moving all posts will close the topic" do + topic.expects(:add_moderator_post).twice + new_topic = topic.move_posts(user, [p1.id, p2.id, p3.id, p4.id], title: "new testing topic name", category_id: category.id) + expect(new_topic).to be_present + + topic.reload + expect(topic.closed).to eq(true) + end + + it 'does not move posts that do not belong to the existing topic' do + new_topic = topic.move_posts( + user, [p2.id, p3.id, p5.id], title: 'Logan is a pretty good movie' + ) + + expect(new_topic.posts.pluck(:id).sort).to eq([p2.id, p3.id].sort) + end + + it "uses default locale for moderator post" do + I18n.locale = 'de' + + new_topic = topic.move_posts(user, [p2.id, p4.id], title: "new testing topic name", category_id: category.id) + post = Post.find_by(topic_id: topic.id, post_type: Post.types[:small_action]) + + expected_text = I18n.with_locale(:en) do + I18n.t("move_posts.new_topic_moderator_post", + count: 2, + entity: "topic", + topic_link: "[#{new_topic.title}](#{new_topic.relative_url})") + end + + expect(post.raw).to eq(expected_text) + end + + it "does not try to move small action posts" do + small_action = Fabricate(:post, topic: topic, raw: "A small action", post_type: Post.types[:small_action]) + new_topic = topic.move_posts(user, [p2.id, p4.id, small_action.id], title: "new testing topic name", category_id: category.id) + + expect(new_topic.posts_count).to eq(2) + expect(small_action.topic_id).to eq(topic.id) + + 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 - end - context "moving the first post" do + context "to an existing topic" do + let!(:destination_topic) { Fabricate(:topic, user: another_user) } + let!(:destination_op) { Fabricate(:post, topic: destination_topic, user: another_user) } + + it "works correctly" do + topic.expects(:add_moderator_post).once + moved_to = topic.move_posts(user, [p2.id, p4.id], destination_topic_id: destination_topic.id) + expect(moved_to).to eq(destination_topic) + + # Check out new topic + moved_to.reload + expect(moved_to.posts_count).to eq(3) + expect(moved_to.highest_post_number).to eq(3) + expect(moved_to.user_id).to eq(destination_op.user_id) + expect(moved_to.like_count).to eq(1) + expect(moved_to.category_id).to eq(SiteSetting.uncategorized_category_id) + p4.reload + expect(moved_to.last_post_user_id).to eq(p4.user_id) + expect(moved_to.last_posted_at).to eq(p4.created_at) + expect(moved_to.bumped_at).to eq(p4.created_at) + + # Posts should be re-ordered + p2.reload + expect(p2.sort_order).to eq(2) + expect(p2.post_number).to eq(2) + expect(p2.topic_id).to eq(moved_to.id) + expect(p2.reply_count).to eq(1) + expect(p2.reply_to_post_number).to eq(nil) + + expect(p4.post_number).to eq(3) + expect(p4.sort_order).to eq(3) + expect(p4.topic_id).to eq(moved_to.id) + expect(p4.reply_count).to eq(0) + expect(p4.reply_to_post_number).to eq(2) + + # Check out the original topic + topic.reload + expect(topic.posts_count).to eq(2) + expect(topic.highest_post_number).to eq(3) + expect(topic.featured_user1_id).to be_blank + expect(topic.like_count).to eq(0) + expect(topic.posts_count).to eq(2) + expect(topic.posts.by_post_number).to match_array([p1, p3]) + expect(topic.highest_post_number).to eq(p3.post_number) + + # Should notify correctly + notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first + + expect(notification.topic_id).to eq(p2.topic_id) + expect(notification.post_number).to eq(p2.post_number) + + # Should update last reads + expect(TopicUser.find_by(user_id: user.id, topic_id: topic.id).last_read_post_number).to eq(p3.post_number) + end + + it "moving all posts will close the topic" do + topic.expects(:add_moderator_post).twice + moved_to = topic.move_posts(user, [p1.id, p2.id, p3.id, p4.id], destination_topic_id: destination_topic.id) + expect(moved_to).to be_present + + topic.reload + expect(topic.closed).to eq(true) + end + + it "does not try to move small action posts" do + small_action = Fabricate(:post, topic: topic, raw: "A small action", post_type: Post.types[:small_action]) + moved_to = topic.move_posts(user, [p1.id, p2.id, p3.id, p4.id, small_action.id], destination_topic_id: destination_topic.id) + + moved_to.reload + expect(moved_to.posts_count).to eq(5) + expect(small_action.topic_id).to eq(topic.id) + + moderator_post = topic.posts.find_by(post_number: 2) + expect(moderator_post.raw).to include("4 posts were merged") + end + end + + shared_examples "moves email related stuff" do + it "moves incoming email" do + Fabricate(:incoming_email, user: old_post.user, topic: old_post.topic, post: old_post) + + new_topic = topic.move_posts(user, [old_post.id], title: "new testing topic name") + new_post = new_topic.first_post + email = new_post.incoming_email + + expect(email).to be_present + expect(email.topic_id).to eq(new_topic.id) + expect(email.post_id).to eq(new_post.id) + + expect(old_post.reload.incoming_email).to_not be_present unless old_post.id == new_post.id + end + + it "moves email log entries" do + old_topic = old_post.topic + + 2.times do + Fabricate(:email_log, + user: old_post.user, + post: old_post, + email_type: :mailing_list + ) + end + + some_post = Fabricate(:post) + + Fabricate(:email_log, + user: some_post.user, + post: some_post, + email_type: :mailing_list + ) + + expect(EmailLog.where(post_id: old_post.id).count).to eq(2) + + new_topic = old_topic.move_posts( + user, + [old_post.id], + title: "new testing topic name" + ) + + new_post = new_topic.first_post + + expect(EmailLog.where(post_id: new_post.id).count).to eq(2) + end + + it "preserves post attributes" do + old_post.update_columns(cook_method: Post.cook_methods[:email], via_email: true, raw_email: "raw email content") + + new_topic = old_post.topic.move_posts(user, [old_post.id], title: "new testing topic name") + new_post = new_topic.first_post + + expect(new_post.cook_method).to eq(Post.cook_methods[:email]) + expect(new_post.via_email).to eq(true) + expect(new_post.raw_email).to eq("raw email content") + end + end + + context "moving the first post" do + + it "copies the OP, doesn't delete it" do + topic.expects(:add_moderator_post).once + new_topic = topic.move_posts(user, [p1.id, p2.id], title: "new testing topic name") + + expect(new_topic).to be_present + expect(new_topic.posts.by_post_number.first.raw).to eq(p1.raw) + expect(new_topic.posts_count).to eq(2) + expect(new_topic.highest_post_number).to eq(2) + + # First post didn't move + p1.reload + expect(p1.sort_order).to eq(1) + expect(p1.post_number).to eq(1) + expect(p1.topic_id).to eq(topic.id) + expect(p1.reply_count).to eq(0) + + # New first post + new_first = new_topic.posts.where(post_number: 1).first + expect(new_first.reply_count).to eq(1) + expect(new_first.created_at).to be_within(1.second).of(p1.created_at) + + # Second post is in a new topic + p2.reload + expect(p2.post_number).to eq(2) + expect(p2.sort_order).to eq(2) + expect(p2.topic_id).to eq(new_topic.id) + expect(p2.reply_to_post_number).to eq(1) + expect(p2.reply_count).to eq(0) + + topic.reload + expect(topic.posts.by_post_number).to match_array([p1, p3, p4]) + expect(topic.highest_post_number).to eq(p4.post_number) + end + + it "preserves post actions in the new post" do + PostAction.act(another_user, p1, PostActionType.types[:like]) + + new_topic = topic.move_posts(user, [p1.id], title: "new testing topic name") + new_post = new_topic.posts.where(post_number: 1).first + + expect(new_topic.like_count).to eq(1) + expect(new_post.like_count).to eq(1) + expect(new_post.post_actions.size).to eq(1) + end + + it "preserves the custom_fields in the new post" do + custom_fields = { "some_field" => 'payload' } + p1.custom_fields = custom_fields + p1.save_custom_fields + + new_topic = topic.move_posts(user, [p1.id], title: "new testing topic name") + + expect(new_topic.first_post.custom_fields).to eq(custom_fields) + end + + include_examples "moves email related stuff" do + let!(:old_post) { p1 } + end + end + + context "moving replies" do + include_examples "moves email related stuff" do + let!(:old_post) { p3 } + end + end + + context "to an existing topic with a deleted post" do + + before do + topic.expects(:add_moderator_post) + end + + let!(:destination_topic) { Fabricate(:topic, user: user) } + let!(:destination_op) { Fabricate(:post, topic: destination_topic, user: user) } + let!(:destination_deleted_reply) { Fabricate(:post, topic: destination_topic, user: another_user) } + let(:moved_to) { topic.move_posts(user, [p2.id, p4.id], destination_topic_id: destination_topic.id) } + + it "works correctly" do + destination_deleted_reply.trash! + + expect(moved_to).to eq(destination_topic) + + # Check out new topic + moved_to.reload + expect(moved_to.posts_count).to eq(3) + expect(moved_to.highest_post_number).to eq(4) + + # Posts should be re-ordered + p2.reload + expect(p2.sort_order).to eq(3) + expect(p2.post_number).to eq(3) + expect(p2.topic_id).to eq(moved_to.id) + expect(p2.reply_count).to eq(1) + expect(p2.reply_to_post_number).to eq(nil) + + p4.reload + expect(p4.post_number).to eq(4) + expect(p4.sort_order).to eq(4) + expect(p4.topic_id).to eq(moved_to.id) + expect(p4.reply_to_post_number).to eq(p2.post_number) + end + end + + context "to an existing closed topic" do + let!(:destination_topic) { Fabricate(:topic, closed: true) } + + it "works correctly for admin" do + admin = Fabricate(:admin) + moved_to = topic.move_posts(admin, [p1.id, p2.id], destination_topic_id: destination_topic.id) + expect(moved_to).to be_present + + moved_to.reload + expect(moved_to.posts_count).to eq(2) + expect(moved_to.highest_post_number).to eq(2) + end + end + + it "skips validations when moving posts" do + p1.update_attribute(:raw, "foo") + p2.update_attribute(:raw, "bar") - it "copies the OP, doesn't delete it" do - topic.expects(:add_moderator_post).once new_topic = topic.move_posts(user, [p1.id, p2.id], title: "new testing topic name") expect(new_topic).to be_present expect(new_topic.posts.by_post_number.first.raw).to eq(p1.raw) + expect(new_topic.posts.by_post_number.last.raw).to eq(p2.raw) expect(new_topic.posts_count).to eq(2) - expect(new_topic.highest_post_number).to eq(2) - - # First post didn't move - p1.reload - expect(p1.sort_order).to eq(1) - expect(p1.post_number).to eq(1) - expect(p1.topic_id).to eq(topic.id) - expect(p1.reply_count).to eq(0) - - # New first post - new_first = new_topic.posts.where(post_number: 1).first - expect(new_first.reply_count).to eq(1) - expect(new_first.created_at).to be_within(1.second).of(p1.created_at) - - # Second post is in a new topic - p2.reload - expect(p2.post_number).to eq(2) - expect(p2.sort_order).to eq(2) - expect(p2.topic_id).to eq(new_topic.id) - expect(p2.reply_to_post_number).to eq(1) - expect(p2.reply_count).to eq(0) - - topic.reload - expect(topic.posts.by_post_number).to match_array([p1, p3, p4]) - expect(topic.highest_post_number).to eq(p4.post_number) - end - - it "preserves post actions in the new post" do - PostAction.act(another_user, p1, PostActionType.types[:like]) - - new_topic = topic.move_posts(user, [p1.id], title: "new testing topic name") - new_post = new_topic.posts.where(post_number: 1).first - - expect(new_topic.like_count).to eq(1) - expect(new_post.like_count).to eq(1) - expect(new_post.post_actions.size).to eq(1) - end - - it "preserves the custom_fields in the new post" do - custom_fields = { "some_field" => 'payload' } - p1.custom_fields = custom_fields - p1.save_custom_fields - - new_topic = topic.move_posts(user, [p1.id], title: "new testing topic name") - - expect(new_topic.first_post.custom_fields).to eq(custom_fields) - end - - include_examples "moves email related stuff" do - let!(:old_post) { p1 } end end + end - context "moving replies" do - include_examples "moves email related stuff" do - let!(:old_post) { p3 } - end + context 'messages' do + let(:user) { Fabricate(:user) } + let(:admin) { Fabricate(:admin) } + let(:evil_trout) { Fabricate(:evil_trout) } + let(:another_user) { Fabricate(:user) } + let(:regular_user) { Fabricate(:trust_level_4) } + let(:topic) { Fabricate(:topic) } + let(:personal_message) { Fabricate(:private_message_topic, user: evil_trout) } + let!(:p1) { Fabricate(:post, topic: personal_message, user: user) } + let!(:p2) { Fabricate(:post, topic: personal_message, reply_to_post_number: p1.post_number, user: another_user) } + let!(:p3) { Fabricate(:post, topic: personal_message, reply_to_post_number: p1.post_number, user: user) } + let!(:p4) { Fabricate(:post, topic: personal_message, reply_to_post_number: p2.post_number, user: user) } + let!(:p5) { Fabricate(:post, topic: personal_message, user: evil_trout) } + let(:another_personal_message) do + Fabricate(:private_message_topic, user: user, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: admin) + ]) + end + let!(:p6) { Fabricate(:post, topic: another_personal_message, user: evil_trout) } + + before do + SiteSetting.tagging_enabled = true + SiteSetting.queue_jobs = false + p1.replies << p3 + p2.replies << p4 + UserActionCreator.enable + @like = PostAction.act(another_user, p4, PostActionType.types[:like]) end - context "to an existing topic with a deleted post" do + context 'move to new message' do + it "adds post users as topic allowed users" do + personal_message.move_posts(admin, [p2.id, p5.id], title: "new testing message name", tags: ["tag1", "tag2"], archetype: "private_message") - before do - topic.expects(:add_moderator_post) + p2.reload + expect(p2.topic.archetype).to eq(Archetype.private_message) + expect(p2.topic.topic_allowed_users.where(user_id: another_user.id).count).to eq(1) + expect(p2.topic.topic_allowed_users.where(user_id: another_user.id).count).to eq(1) + expect(p2.topic.topic_allowed_users.where(user_id: evil_trout.id).count).to eq(1) + expect(p2.topic.tags.pluck(:name)).to eq([]) end - let!(:destination_topic) { Fabricate(:topic, user: user) } - let!(:destination_op) { Fabricate(:post, topic: destination_topic, user: user) } - let!(:destination_deleted_reply) { Fabricate(:post, topic: destination_topic, user: another_user) } - let(:moved_to) { topic.move_posts(user, [p2.id, p4.id], destination_topic_id: destination_topic.id) } + it "can add tags to new message when allow_staff_to_tag_pms is enabled" do + SiteSetting.allow_staff_to_tag_pms = true + personal_message.move_posts(admin, [p2.id, p5.id], title: "new testing message name", tags: ["tag1", "tag2"], archetype: "private_message") - it "works correctly" do - destination_deleted_reply.trash! - - expect(moved_to).to eq(destination_topic) - - # Check out new topic - moved_to.reload - expect(moved_to.posts_count).to eq(3) - expect(moved_to.highest_post_number).to eq(4) - - # Posts should be re-ordered p2.reload - expect(p2.sort_order).to eq(3) - expect(p2.post_number).to eq(3) - expect(p2.topic_id).to eq(moved_to.id) - expect(p2.reply_count).to eq(1) + expect(p2.topic.tags.pluck(:name)).to contain_exactly("tag1", "tag2") + end + + it "correctly handles notifications" do + old_message = p2.topic + old_message_id = p2.topic_id + + personal_message.move_posts(admin, [p2.id, p4.id], title: "new testing message name", archetype: "private_message") + + p2.reload + expect(p2.topic_id).not_to eq(old_message_id) expect(p2.reply_to_post_number).to eq(nil) + expect(p2.reply_to_user_id).to eq(nil) - p4.reload - expect(p4.post_number).to eq(4) - expect(p4.sort_order).to eq(4) - expect(p4.topic_id).to eq(moved_to.id) - expect(p4.reply_to_post_number).to eq(p2.post_number) + notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first + + expect(notification.topic_id).to eq(p2.topic_id) + expect(notification.post_number).to eq(1) + + # no message for person who made the move + expect(admin.notifications.where(notification_type: Notification.types[:moved_post]).length).to eq(0) + + old_message.reload + move_message = old_message.posts.find_by(post_number: 2) + expect(move_message.post_type).to eq(Post.types[:small_action]) + expect(move_message.raw).to include("2 posts were split") end end - context "to an existing closed topic" do - let!(:destination_topic) { Fabricate(:topic, closed: true) } + context 'move to existing message' do + it "adds post users as topic allowed users" do + personal_message.move_posts(admin, [p2.id, p5.id], destination_topic_id: another_personal_message.id, archetype: "private_message") - it "works correctly for admin" do - admin = Fabricate(:admin) - moved_to = topic.move_posts(admin, [p1.id, p2.id], destination_topic_id: destination_topic.id) + p2.reload + expect(p2.topic_id).to eq(another_personal_message.id) + + another_personal_message.reload + expect(another_personal_message.topic_allowed_users.where(user_id: another_user.id).count).to eq(1) + expect(another_personal_message.topic_allowed_users.where(user_id: evil_trout.id).count).to eq(1) + end + + it "can add additional participants" do + personal_message.move_posts(admin, [p2.id, p5.id], destination_topic_id: another_personal_message.id, participants: [regular_user.username], archetype: "private_message") + + another_personal_message.reload + expect(another_personal_message.topic_allowed_users.where(user_id: another_user.id).count).to eq(1) + expect(another_personal_message.topic_allowed_users.where(user_id: evil_trout.id).count).to eq(1) + expect(another_personal_message.topic_allowed_users.where(user_id: regular_user.id).count).to eq(1) + end + + it "does not allow moving regular topic posts in personal message" do + expect { + personal_message.move_posts(admin, [p2.id, p5.id], destination_topic_id: topic.id) + }.to raise_error(Discourse::InvalidParameters) + end + + it "moving all posts will close the message" do + moved_to = personal_message.move_posts(admin, [p1.id, p2.id, p3.id, p4.id, p5.id], destination_topic_id: another_personal_message.id, archetype: "private_message") expect(moved_to).to be_present - moved_to.reload - expect(moved_to.posts_count).to eq(2) - expect(moved_to.highest_post_number).to eq(2) + personal_message.reload + expect(personal_message.closed).to eq(true) + expect(moved_to.posts_count).to eq(6) end end - - it "skips validations when moving posts" do - p1.update_attribute(:raw, "foo") - p2.update_attribute(:raw, "bar") - - new_topic = topic.move_posts(user, [p1.id, p2.id], title: "new testing topic name") - - expect(new_topic).to be_present - expect(new_topic.posts.by_post_number.first.raw).to eq(p1.raw) - expect(new_topic.posts.by_post_number.last.raw).to eq(p2.raw) - expect(new_topic.posts_count).to eq(2) - end end end end diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index aa636c82d15..f225b3c5ece 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -241,6 +241,140 @@ RSpec.describe TopicsController do end end end + + describe 'moving to a new message' do + let(:user) { Fabricate(:user) } + let(:trust_level_4) { Fabricate(:trust_level_4) } + let(:moderator) { Fabricate(:moderator) } + let!(:message) { Fabricate(:private_message_topic) } + let!(:p1) { Fabricate(:post, user: user, post_number: 1, topic: message) } + let!(:p2) { Fabricate(:post, user: user, post_number: 2, topic: message) } + + it "raises an error without post_ids" do + sign_in(moderator) + post "/t/#{message.id}/move-posts.json", params: { title: 'blah', archetype: 'private_message' } + expect(response.status).to eq(400) + end + + it "raises an error when the user doesn't have permission to move the posts" do + sign_in(trust_level_4) + + post "/t/#{message.id}/move-posts.json", params: { + title: 'blah', post_ids: [p1.post_number, p2.post_number], archetype: 'private_message' + } + + expect(response.status).to eq(403) + result = ::JSON.parse(response.body) + expect(result['errors']).to be_present + end + + context 'success' do + before { sign_in(Fabricate(:admin)) } + + it "returns success" do + SiteSetting.allow_staff_to_tag_pms = true + + expect do + post "/t/#{message.id}/move-posts.json", params: { + title: 'Logan is a good movie', + post_ids: [p2.id], + archetype: 'private_message', + tags: ["tag1", "tag2"] + } + end.to change { Topic.count }.by(1) + + expect(response.status).to eq(200) + + result = ::JSON.parse(response.body) + + expect(result['success']).to eq(true) + expect(result['url']).to eq(Topic.last.relative_url) + expect(Tag.all.pluck(:name)).to contain_exactly("tag1", "tag2") + end + + describe 'when message has been deleted' do + it 'should still be able to move posts' do + PostDestroyer.new(Fabricate(:admin), message.first_post).destroy + + expect(message.reload.deleted_at).to_not be_nil + + expect do + post "/t/#{message.id}/move-posts.json", params: { + title: 'Logan is a good movie', + post_ids: [p2.id], + archetype: 'private_message' + } + end.to change { Topic.count }.by(1) + + expect(response.status).to eq(200) + + result = JSON.parse(response.body) + + expect(result['success']).to eq(true) + expect(result['url']).to eq(Topic.last.relative_url) + end + end + end + + context 'failure' do + it "returns JSON with a false success" do + sign_in(moderator) + post "/t/#{message.id}/move-posts.json", params: { + post_ids: [p2.id], + archetype: 'private_message' + } + expect(response.status).to eq(200) + result = ::JSON.parse(response.body) + expect(result['success']).to eq(false) + expect(result['url']).to be_blank + end + end + end + + describe 'moving to an existing message' do + let!(:user) { sign_in(Fabricate(:admin)) } + let(:trust_level_4) { Fabricate(:trust_level_4) } + let(:evil_trout) { Fabricate(:evil_trout) } + let(:message) { Fabricate(:private_message_topic) } + let(:p1) { Fabricate(:post, user: user, post_number: 1, topic: message) } + let(:p2) { Fabricate(:post, user: evil_trout, post_number: 2, topic: message) } + + let(:dest_message) do + Fabricate(:private_message_topic, user: trust_level_4, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: evil_trout) + ]) + end + + context 'success' do + it "returns success" do + user + post "/t/#{message.id}/move-posts.json", params: { + post_ids: [p2.id], + destination_topic_id: dest_message.id, + archetype: 'private_message' + } + + expect(response.status).to eq(200) + result = ::JSON.parse(response.body) + expect(result['success']).to eq(true) + expect(result['url']).to be_present + end + end + + context 'failure' do + it "returns JSON with a false success" do + post "/t/#{message.id}/move-posts.json", params: { + post_ids: [p2.id], + archetype: 'private_message' + } + + expect(response.status).to eq(200) + result = ::JSON.parse(response.body) + expect(result['success']).to eq(false) + expect(result['url']).to be_blank + end + end + end end describe '#merge_topic' do @@ -251,7 +385,7 @@ RSpec.describe TopicsController do expect(response.status).to eq(403) end - describe 'moving to a new topic' do + describe 'merging into another topic' do let(:moderator) { Fabricate(:moderator) } let(:user) { Fabricate(:user) } let(:p1) { Fabricate(:post, user: user) } @@ -285,6 +419,53 @@ RSpec.describe TopicsController do end end end + + describe 'merging into another message' do + let(:moderator) { Fabricate(:moderator) } + let(:user) { Fabricate(:user) } + let(:trust_level_4) { Fabricate(:trust_level_4) } + let(:message) { Fabricate(:private_message_topic, user: user) } + let!(:p1) { Fabricate(:post, topic: message, user: trust_level_4) } + let!(:p2) { Fabricate(:post, topic: message, reply_to_post_number: p1.post_number, user: user) } + + it "raises an error without destination_topic_id" do + sign_in(moderator) + post "/t/#{message.id}/merge-topic.json", params: { + archetype: 'private_message' + } + expect(response.status).to eq(400) + end + + it "raises an error when the user doesn't have permission to merge" do + sign_in(trust_level_4) + post "/t/#{message.id}/merge-topic.json", params: { + destination_topic_id: 345, + archetype: 'private_message' + } + expect(response).to be_forbidden + end + + let(:dest_message) do + Fabricate(:private_message_topic, user: trust_level_4, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: moderator) + ]) + end + + context 'moves all the posts to the destination message' do + it "returns success" do + sign_in(moderator) + post "/t/#{message.id}/merge-topic.json", params: { + destination_topic_id: dest_message.id, + archetype: 'private_message' + } + + expect(response.status).to eq(200) + result = ::JSON.parse(response.body) + expect(result['success']).to eq(true) + expect(result['url']).to be_present + end + end + end end describe '#change_post_owners' do diff --git a/test/javascripts/acceptance/topic-move-posts-test.js.es6 b/test/javascripts/acceptance/topic-move-posts-test.js.es6 new file mode 100644 index 00000000000..c86af80d314 --- /dev/null +++ b/test/javascripts/acceptance/topic-move-posts-test.js.es6 @@ -0,0 +1,121 @@ +import { acceptance } from "helpers/qunit-helpers"; +acceptance("Topic move posts", { loggedIn: true }); + +QUnit.test("default", async assert => { + await visit("/t/internationalization-localization"); + await click(".toggle-admin-menu"); + await click(".topic-admin-multi-select .btn"); + await click("#post_11 .select-below"); + + assert.equal( + find(".selected-posts .move-to-topic") + .text() + .trim(), + I18n.t("topic.move_to.action"), + "it should show the move to button" + ); + + await click(".selected-posts .move-to-topic"); + + assert.ok( + find(".move-to-modal .title") + .html() + .includes(I18n.t("topic.move_to.title")), + "it opens move to modal" + ); + + assert.ok( + find(".move-to-modal .radios") + .html() + .includes(I18n.t("topic.split_topic.radio_label")), + "it shows an option to move to new topic" + ); + + assert.ok( + find(".move-to-modal .radios") + .html() + .includes(I18n.t("topic.merge_topic.radio_label")), + "it shows an option to move to existing topic" + ); + + assert.ok( + find(".move-to-modal .radios") + .html() + .includes(I18n.t("topic.move_to_new_message.radio_label")), + "it shows an option to move to new message" + ); +}); + +QUnit.test("moving all posts", async assert => { + await visit("/t/internationalization-localization"); + await click(".toggle-admin-menu"); + await click(".topic-admin-multi-select .btn"); + await click(".select-all"); + await click(".selected-posts .move-to-topic"); + + assert.ok( + find(".move-to-modal .title") + .html() + .includes(I18n.t("topic.move_to.title")), + "it opens move to modal" + ); + + assert.not( + find(".move-to-modal .radios") + .html() + .includes(I18n.t("topic.split_topic.radio_label")), + "it does not show an option to move to new topic" + ); + + assert.ok( + find(".move-to-modal .radios") + .html() + .includes(I18n.t("topic.merge_topic.radio_label")), + "it shows an option to move to existing topic" + ); + + assert.not( + find(".move-to-modal .radios") + .html() + .includes(I18n.t("topic.move_to_new_message.radio_label")), + "it does not show an option to move to new message" + ); +}); + +QUnit.test("moving posts from personal message", async assert => { + await visit("/t/pm-for-testing/12"); + await click(".toggle-admin-menu"); + await click(".topic-admin-multi-select .btn"); + await click("#post_1 .select-post"); + + assert.equal( + find(".selected-posts .move-to-topic") + .text() + .trim(), + I18n.t("topic.move_to.action"), + "it should show the move to button" + ); + + await click(".selected-posts .move-to-topic"); + + assert.ok( + find(".move-to-modal .title") + .html() + .includes(I18n.t("topic.move_to.title")), + "it opens move to modal" + ); + + assert.ok( + find(".move-to-modal .radios") + .html() + .includes(I18n.t("topic.move_to_new_message.radio_label")), + "it shows an option to move to new message" + ); + + assert.ok( + find(".move-to-modal .radios") + .html() + .includes(I18n.t("topic.move_to_existing_message.radio_label")), + "it shows an option to move to existing message" + ); +}); diff --git a/test/javascripts/controllers/topic-test.js.es6 b/test/javascripts/controllers/topic-test.js.es6 index ac3c1329359..2ba53e12218 100644 --- a/test/javascripts/controllers/topic-test.js.es6 +++ b/test/javascripts/controllers/topic-test.js.es6 @@ -281,10 +281,6 @@ QUnit.test("Can split/merge topic", function(assert) { const controller = this.subject({ model }); const selectedPostIds = controller.get("selectedPostIds"); - assert.not( - controller.get("canSplitTopic"), - "can't split topic when no posts are selected" - ); assert.not( controller.get("canMergeTopic"), "can't merge topic when no posts are selected" @@ -292,10 +288,6 @@ QUnit.test("Can split/merge topic", function(assert) { selectedPostIds.pushObject(1); - assert.not( - controller.get("canSplitTopic"), - "can't split topic when can't move posts" - ); assert.not( controller.get("canMergeTopic"), "can't merge topic when can't move posts" @@ -303,16 +295,11 @@ QUnit.test("Can split/merge topic", function(assert) { model.set("details.can_move_posts", true); - assert.ok(controller.get("canSplitTopic"), "can split topic"); assert.ok(controller.get("canMergeTopic"), "can merge topic"); selectedPostIds.removeObject(1); selectedPostIds.pushObject(2); - assert.not( - controller.get("canSplitTopic"), - "can't split topic when 1st post is not a regular post" - ); assert.ok( controller.get("canMergeTopic"), "can merge topic when 1st post is not a regular post" @@ -320,10 +307,6 @@ QUnit.test("Can split/merge topic", function(assert) { selectedPostIds.pushObject(3); - assert.not( - controller.get("canSplitTopic"), - "can't split topic when all posts are selected" - ); assert.ok( controller.get("canMergeTopic"), "can merge topic when all posts are selected"