From 70fdc1036598df09d236e013fea7fcf750c7363f Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Mon, 31 Dec 2018 17:17:22 +0530 Subject: [PATCH] FEATURE: move posts to new/existing PM (#6802) --- .../components/choose-message.js.es6 | 64 + .../discourse/controllers/merge-topic.js.es6 | 59 - .../controllers/move-to-topic.js.es6 | 155 +++ .../discourse/controllers/split-topic.js.es6 | 66 -- .../discourse/controllers/topic.js.es6 | 16 - .../javascripts/discourse/models/topic.js.es6 | 9 +- .../javascripts/discourse/routes/topic.js.es6 | 10 +- .../templates/components/choose-message.hbs | 22 + .../discourse/templates/modal/merge-topic.hbs | 13 - .../templates/modal/move-to-topic.hbs | 112 ++ .../discourse/templates/modal/split-topic.hbs | 21 - .../discourse/templates/selected-posts.hbs | 6 +- app/assets/stylesheets/desktop/modal.scss | 27 +- app/controllers/topics_controller.rb | 37 +- app/models/post_mover.rb | 31 +- app/models/topic.rb | 4 +- config/locales/client.en.yml | 36 +- config/locales/server.en.yml | 8 +- spec/models/post_mover_spec.rb | 1031 +++++++++-------- spec/requests/topics_controller_spec.rb | 183 ++- .../acceptance/topic-move-posts-test.js.es6 | 121 ++ .../javascripts/controllers/topic-test.js.es6 | 17 - 22 files changed, 1357 insertions(+), 691 deletions(-) create mode 100644 app/assets/javascripts/discourse/components/choose-message.js.es6 delete mode 100644 app/assets/javascripts/discourse/controllers/merge-topic.js.es6 create mode 100644 app/assets/javascripts/discourse/controllers/move-to-topic.js.es6 delete mode 100644 app/assets/javascripts/discourse/controllers/split-topic.js.es6 create mode 100644 app/assets/javascripts/discourse/templates/components/choose-message.hbs delete mode 100644 app/assets/javascripts/discourse/templates/modal/merge-topic.hbs create mode 100644 app/assets/javascripts/discourse/templates/modal/move-to-topic.hbs delete mode 100644 app/assets/javascripts/discourse/templates/modal/split-topic.hbs create mode 100644 test/javascripts/acceptance/topic-move-posts-test.js.es6 diff --git a/app/assets/javascripts/discourse/components/choose-message.js.es6 b/app/assets/javascripts/discourse/components/choose-message.js.es6 new file mode 100644 index 00000000000..b05dd54629e --- /dev/null +++ b/app/assets/javascripts/discourse/components/choose-message.js.es6 @@ -0,0 +1,64 @@ +import debounce from "discourse/lib/debounce"; +import { searchForTerm } from "discourse/lib/search"; +import { observes } from "ember-addons/ember-computed-decorators"; + +export default Ember.Component.extend({ + loading: null, + noResults: null, + messages: null, + + @observes("messageTitle") + messageTitleChanged() { + this.setProperties({ + loading: true, + noResults: true, + selectedTopicId: null + }); + this.search(this.get("messageTitle")); + }, + + @observes("messages") + messagesChanged() { + const messages = this.get("messages"); + if (messages) { + this.set("noResults", messages.length === 0); + } + this.set("loading", false); + }, + + search: debounce(function(title) { + const currentTopicId = this.get("currentTopicId"); + + if (Em.isEmpty(title)) { + this.setProperties({ messages: null, loading: false }); + return; + } + + searchForTerm(title, { + typeFilter: "private_messages", + searchForId: true + }).then(results => { + if (results && results.posts && results.posts.length > 0) { + this.set( + "messages", + results.posts + .mapBy("topic") + .filter(t => t.get("id") !== currentTopicId) + ); + } else { + this.setProperties({ messages: null, loading: false }); + } + }); + }, 300), + + actions: { + chooseMessage(message) { + const messageId = Em.get(message, "id"); + this.set("selectedTopicId", messageId); + Ember.run.next(() => + $(`#choose-message-${messageId}`).prop("checked", "true") + ); + return false; + } + } +}); diff --git a/app/assets/javascripts/discourse/controllers/merge-topic.js.es6 b/app/assets/javascripts/discourse/controllers/merge-topic.js.es6 deleted file mode 100644 index b0d488ef64b..00000000000 --- a/app/assets/javascripts/discourse/controllers/merge-topic.js.es6 +++ /dev/null @@ -1,59 +0,0 @@ -import ModalFunctionality from "discourse/mixins/modal-functionality"; -import { movePosts, mergeTopic } from "discourse/models/topic"; -import DiscourseURL from "discourse/lib/url"; -import computed from "ember-addons/ember-computed-decorators"; - -export default Ember.Controller.extend(ModalFunctionality, { - topicController: Ember.inject.controller("topic"), - - saving: false, - selectedTopicId: null, - - selectedPostsCount: Ember.computed.alias( - "topicController.selectedPostsCount" - ), - - @computed("saving", "selectedTopicId") - buttonDisabled(saving, selectedTopicId) { - return saving || Ember.isEmpty(selectedTopicId); - }, - - @computed("saving") - buttonTitle(saving) { - return saving ? I18n.t("saving") : I18n.t("topic.merge_topic.title"); - }, - - onShow() { - this.set("modal.modalClass", "split-modal"); - }, - - actions: { - movePostsToExistingTopic() { - const topicId = this.get("model.id"); - - this.set("saving", true); - - let promise = this.get("topicController.selectedAllPosts") - ? mergeTopic(topicId, this.get("selectedTopicId")) - : movePosts(topicId, { - destination_topic_id: this.get("selectedTopicId"), - post_ids: this.get("topicController.selectedPostIds") - }); - - promise - .then(result => { - this.send("closeModal"); - this.get("topicController").send("toggleMultiSelect"); - Ember.run.next(() => DiscourseURL.routeTo(result.url)); - }) - .catch(() => { - this.flash(I18n.t("topic.merge_topic.error")); - }) - .finally(() => { - this.set("saving", false); - }); - - return false; - } - } -}); diff --git a/app/assets/javascripts/discourse/controllers/move-to-topic.js.es6 b/app/assets/javascripts/discourse/controllers/move-to-topic.js.es6 new file mode 100644 index 00000000000..6df047caa5d --- /dev/null +++ b/app/assets/javascripts/discourse/controllers/move-to-topic.js.es6 @@ -0,0 +1,155 @@ +import ModalFunctionality from "discourse/mixins/modal-functionality"; +import { movePosts, mergeTopic } from "discourse/models/topic"; +import DiscourseURL from "discourse/lib/url"; +import { default as computed } from "ember-addons/ember-computed-decorators"; +import { extractError } from "discourse/lib/ajax-error"; + +export default Ember.Controller.extend(ModalFunctionality, { + topicName: null, + saving: false, + categoryId: null, + tags: null, + canAddTags: Ember.computed.alias("site.can_create_tag"), + canTagMessages: Ember.computed.alias("site.can_tag_pms"), + selectedTopicId: null, + newTopic: Ember.computed.equal("selection", "new_topic"), + existingTopic: Ember.computed.equal("selection", "existing_topic"), + newMessage: Ember.computed.equal("selection", "new_message"), + existingMessage: Ember.computed.equal("selection", "existing_message"), + moveTypes: ["newTopic", "existingTopic", "newMessage", "existingMessage"], + participants: null, + + topicController: Ember.inject.controller("topic"), + selectedPostsCount: Ember.computed.alias( + "topicController.selectedPostsCount" + ), + selectedAllPosts: Ember.computed.alias("topicController.selectedAllPosts"), + selectedPosts: Ember.computed.alias("topicController.selectedPosts"), + + @computed("saving", "selectedTopicId", "topicName") + buttonDisabled(saving, selectedTopicId, topicName) { + return ( + saving || (Ember.isEmpty(selectedTopicId) && Ember.isEmpty(topicName)) + ); + }, + + @computed( + "saving", + "newTopic", + "existingTopic", + "newMessage", + "existingMessage" + ) + buttonTitle(saving, newTopic, existingTopic, newMessage, existingMessage) { + if (newTopic) { + return I18n.t("topic.split_topic.title"); + } else if (existingTopic) { + return I18n.t("topic.merge_topic.title"); + } else if (newMessage) { + return I18n.t("topic.move_to_new_message.title"); + } else if (existingMessage) { + return I18n.t("topic.move_to_existing_message.title"); + } else { + return I18n.t("saving"); + } + }, + + onShow() { + this.setProperties({ + "modal.modalClass": "move-to-modal", + saving: false, + selection: "new_topic", + categoryId: null, + topicName: "", + tags: null, + participants: null + }); + + const isPrivateMessage = this.get("model.isPrivateMessage"); + const canSplitTopic = this.get("canSplitTopic"); + if (isPrivateMessage) { + this.set("selection", canSplitTopic ? "new_message" : "existing_message"); + } else if (!canSplitTopic) { + this.set("selection", "existing_topic"); + } + }, + + @computed("selectedAllPosts", "selectedPosts", "selectedPosts.[]") + canSplitTopic(selectedAllPosts, selectedPosts) { + return ( + !selectedAllPosts && + selectedPosts.length > 0 && + selectedPosts.sort((a, b) => a.post_number - b.post_number)[0] + .post_type === this.site.get("post_types.regular") + ); + }, + + actions: { + performMove() { + this.get("moveTypes").forEach(type => { + if (this.get(type)) { + this.send("movePostsTo", type); + } + }); + }, + + movePostsTo(type) { + this.set("saving", true); + const topicId = this.get("model.id"); + let mergeOptions, moveOptions; + + if (type === "existingTopic") { + mergeOptions = { destination_topic_id: this.get("selectedTopicId") }; + moveOptions = Object.assign( + { post_ids: this.get("topicController.selectedPostIds") }, + mergeOptions + ); + } else if (type === "existingMessage") { + mergeOptions = { + destination_topic_id: this.get("selectedTopicId"), + participants: this.get("participants"), + archetype: "private_message" + }; + moveOptions = Object.assign( + { post_ids: this.get("topicController.selectedPostIds") }, + mergeOptions + ); + } else if (type === "newTopic") { + mergeOptions = {}; + moveOptions = { + title: this.get("topicName"), + post_ids: this.get("topicController.selectedPostIds"), + category_id: this.get("categoryId"), + tags: this.get("tags") + }; + } else { + mergeOptions = {}; + moveOptions = { + title: this.get("topicName"), + post_ids: this.get("topicController.selectedPostIds"), + tags: this.get("tags"), + archetype: "private_message" + }; + } + + const promise = this.get("topicController.selectedAllPosts") + ? mergeTopic(topicId, mergeOptions) + : movePosts(topicId, moveOptions); + + promise + .then(result => { + this.send("closeModal"); + this.get("topicController").send("toggleMultiSelect"); + DiscourseURL.routeTo(result.url); + }) + .catch(xhr => { + this.flash(extractError(xhr, I18n.t("topic.move_to.error"))); + }) + .finally(() => { + this.set("saving", false); + }); + + return false; + } + } +}); diff --git a/app/assets/javascripts/discourse/controllers/split-topic.js.es6 b/app/assets/javascripts/discourse/controllers/split-topic.js.es6 deleted file mode 100644 index e7e3dc8230a..00000000000 --- a/app/assets/javascripts/discourse/controllers/split-topic.js.es6 +++ /dev/null @@ -1,66 +0,0 @@ -import ModalFunctionality from "discourse/mixins/modal-functionality"; -import { extractError } from "discourse/lib/ajax-error"; -import { movePosts } from "discourse/models/topic"; -import DiscourseURL from "discourse/lib/url"; -import { default as computed } from "ember-addons/ember-computed-decorators"; - -export default Ember.Controller.extend(ModalFunctionality, { - topicName: null, - saving: false, - categoryId: null, - tags: null, - canAddTags: Ember.computed.alias("site.can_create_tag"), - - topicController: Ember.inject.controller("topic"), - selectedPostsCount: Ember.computed.alias( - "topicController.selectedPostsCount" - ), - - @computed("saving", "topicName") - buttonDisabled(saving, topicName) { - return saving || Ember.isEmpty(topicName); - }, - - @computed("saving") - buttonTitle(saving) { - return saving ? I18n.t("saving") : I18n.t("topic.split_topic.action"); - }, - - onShow() { - this.setProperties({ - "modal.modalClass": "split-modal", - saving: false, - categoryId: null, - topicName: "", - tags: null - }); - }, - - actions: { - movePostsToNewTopic() { - this.set("saving", true); - - const options = { - title: this.get("topicName"), - post_ids: this.get("topicController.selectedPostIds"), - category_id: this.get("categoryId"), - tags: this.get("tags") - }; - - movePosts(this.get("model.id"), options) - .then(result => { - this.send("closeModal"); - this.get("topicController").send("toggleMultiSelect"); - Ember.run.next(() => DiscourseURL.routeTo(result.url)); - }) - .catch(xhr => { - this.flash(extractError(xhr, I18n.t("topic.split_topic.error"))); - }) - .finally(() => { - this.set("saving", false); - }); - - return false; - } - } -}); diff --git a/app/assets/javascripts/discourse/controllers/topic.js.es6 b/app/assets/javascripts/discourse/controllers/topic.js.es6 index f7863e14249..71e4e47a053 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js.es6 +++ b/app/assets/javascripts/discourse/controllers/topic.js.es6 @@ -1113,22 +1113,6 @@ export default Ember.Controller.extend(BufferedContent, { ); }, - @computed( - "canMergeTopic", - "selectedAllPosts", - "selectedPosts", - "selectedPosts.[]" - ) - canSplitTopic(canMergeTopic, selectedAllPosts, selectedPosts) { - return ( - canMergeTopic && - !selectedAllPosts && - selectedPosts.length > 0 && - selectedPosts.sort((a, b) => a.post_number - b.post_number)[0] - .post_type === 1 - ); - }, - @computed("model.details.can_move_posts", "selectedPostsCount") canMergeTopic(canMovePosts, selectedPostsCount) { return canMovePosts && selectedPostsCount > 0; diff --git a/app/assets/javascripts/discourse/models/topic.js.es6 b/app/assets/javascripts/discourse/models/topic.js.es6 index 66dc5d888b0..a02e9c7a7ad 100644 --- a/app/assets/javascripts/discourse/models/topic.js.es6 +++ b/app/assets/javascripts/discourse/models/topic.js.es6 @@ -752,11 +752,10 @@ export function movePosts(topicId, data) { ); } -export function mergeTopic(topicId, destinationTopicId) { - return ajax("/t/" + topicId + "/merge-topic", { - type: "POST", - data: { destination_topic_id: destinationTopicId } - }).then(moveResult); +export function mergeTopic(topicId, data) { + return ajax("/t/" + topicId + "/merge-topic", { type: "POST", data }).then( + moveResult + ); } export default Topic; diff --git a/app/assets/javascripts/discourse/routes/topic.js.es6 b/app/assets/javascripts/discourse/routes/topic.js.es6 index 9b52a255883..eef20e0d444 100644 --- a/app/assets/javascripts/discourse/routes/topic.js.es6 +++ b/app/assets/javascripts/discourse/routes/topic.js.es6 @@ -114,17 +114,13 @@ const TopicRoute = Discourse.Route.extend({ this.controllerFor("raw_email").loadRawEmail(model.get("id")); }, - mergeTopic() { - showModal("merge-topic", { + moveToTopic() { + showModal("move-to-topic", { model: this.modelFor("topic"), - title: "topic.merge_topic.title" + title: "topic.move_to.title" }); }, - splitTopic() { - showModal("split-topic", { model: this.modelFor("topic") }); - }, - changeOwner() { showModal("change-owner", { model: this.modelFor("topic"), diff --git a/app/assets/javascripts/discourse/templates/components/choose-message.hbs b/app/assets/javascripts/discourse/templates/components/choose-message.hbs new file mode 100644 index 00000000000..fa4f10d30e4 --- /dev/null +++ b/app/assets/javascripts/discourse/templates/components/choose-message.hbs @@ -0,0 +1,22 @@ + + +{{text-field value=messageTitle placeholderKey="choose_message.title.placeholder" id="choose-message-title"}} + +{{#if loading}} +

{{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}}}

- -
- {{choose-topic currentTopicId=model.id selectedTopicId=selectedTopicId}} -
-{{/d-modal-body}} - -
- {{#d-button class="btn-primary" disabled=buttonDisabled action="movePostsToExistingTopic"}} - {{d-icon 'sign-out'}} {{buttonTitle}} - {{/d-button}} -
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}} +
+ {{#if canSplitTopic}} + + {{/if}} + + +
+ + {{#if canSplitTopic}} + {{#if newMessage}} +

{{{i18n 'topic.move_to_new_message.instructions' count=selectedPostsCount}}}

+
+ + {{text-field value=topicName placeholderKey="composer.title_placeholder" elementId='split-topic-name'}} + + {{#if canTagMessages}} + + {{tag-chooser tags=tags filterable=true}} + {{/if}} +
+ {{/if}} + {{/if}} + + {{#if existingMessage}} +

{{{i18n 'topic.move_to_existing_message.instructions' count=selectedPostsCount}}}

+
+ {{choose-message currentTopicId=model.id selectedTopicId=selectedTopicId}} + + + {{user-selector usernames=participants class="participant-selector"}} +
+ {{/if}} + + {{else}} + +
+ {{#if canSplitTopic}} + + {{/if}} + + + + {{#if canSplitTopic}} + + {{/if}} +
+ + {{#if existingTopic}} +

{{{i18n 'topic.merge_topic.instructions' count=selectedPostsCount}}}

+
+ {{choose-topic currentTopicId=model.id selectedTopicId=selectedTopicId}} +
+ {{/if}} + + {{#if canSplitTopic}} + {{#if newTopic}} +

{{{i18n 'topic.split_topic.instructions' count=selectedPostsCount}}}

+
+ + {{text-field value=topicName placeholderKey="composer.title_placeholder" elementId='split-topic-name'}} + + + {{category-chooser value=categoryId class="small"}} + {{#if canAddTags}} + + {{tag-chooser tags=tags filterable=true categoryId=categoryId}} + {{/if}} +
+ {{/if}} + {{/if}} + + {{#if canSplitTopic}} + {{#if newMessage}} +

{{{i18n 'topic.move_to_new_message.instructions' count=selectedPostsCount}}}

+
+ + {{text-field value=topicName placeholderKey="composer.title_placeholder" elementId='split-topic-name'}} + + {{#if canTagMessages}} + + {{tag-chooser tags=tags filterable=true}} + {{/if}} +
+ {{/if}} + {{/if}} + {{/if}} + +{{/d-modal-body}} + +
+ {{#d-button class="btn-primary" disabled=buttonDisabled action=(action "performMove")}} + {{d-icon 'sign-out'}} {{buttonTitle}} + {{/d-button}} +
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}}} - -
- - {{text-field value=topicName placeholderKey="composer.title_placeholder" elementId='split-topic-name'}} - - - {{category-chooser value=categoryId class="small"}} - {{#if canAddTags}} - - {{tag-chooser tags=tags filterable=true categoryId=categoryId}} - {{/if}} -
-{{/d-modal-body}} - -
- {{#d-button class="btn-primary" disabled=buttonDisabled action="movePostsToNewTopic"}} - {{d-icon 'sign-out'}} {{buttonTitle}} - {{/d-button}} -
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"