Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FEATURE: support for chronologically merging posts into existing topic (#21374) 

When a user chooses to move a topic/message to an existing topic/message, they can now opt to merge the posts chronologically (using a checkbox in the UI).
This commit is contained in:
Renato Atilio 2023-05-25 15:38:34 -03:00 committed by GitHub
parent 6a65fa982d
commit c539f749f1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 1346 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
app/assets/javascripts/discourse/app/controllers/move-to-topic.js
View File

@ -22,6 +22,7 @@ export default Controller.extend(ModalFunctionality, {
newMessage: equal("selection", "new_message"),
existingMessage: equal("selection", "existing_message"),
participants: null,
chronologicalOrder: false,
init() {
this._super(...arguments);
@ -81,6 +82,8 @@ export default Controller.extend(ModalFunctionality, {
topicName: "",
tags: null,
participants: [],
selectedTopicId: null,
chronologicalOrder: false,
});
const isPrivateMessage = this.get("model.isPrivateMessage");
@ -125,7 +128,10 @@ export default Controller.extend(ModalFunctionality, {
let mergeOptions, moveOptions;
if (type === "existingTopic") {
mergeOptions = { destination_topic_id: this.selectedTopicId };
mergeOptions = {
destination_topic_id: this.selectedTopicId,
chronological_order: this.chronologicalOrder,
};
moveOptions = Object.assign(
{ post_ids: this.get("topicController.selectedPostIds") },
mergeOptions
@ -135,6 +141,7 @@ export default Controller.extend(ModalFunctionality, {
destination_topic_id: this.selectedTopicId,
participants: this.participants.join(","),
archetype: "private_message",
chronological_order: this.chronologicalOrder,
};
moveOptions = Object.assign(
{ post_ids: this.get("topicController.selectedPostIds") },

24
app/assets/javascripts/discourse/app/templates/modal/move-to-topic.hbs
View File

@ -68,6 +68,18 @@
@value={{this.participants}}
@onChange={{action (mut this.participants)}}
/>
{{#if this.selectedTopicId}}
<hr />
<label for="chronological-order" class="checkbox-label">
<Input
id="chronological-order"
@type="checkbox"
@checked={{this.chronologicalOrder}}
/>
{{i18n "topic.merge_topic.chronological_order"}}
</label>
{{/if}}
</form>
{{/if}}
@ -118,6 +130,18 @@
@currentTopicId={{this.model.id}}
@selectedTopicId={{this.selectedTopicId}}
/>
{{#if this.selectedTopicId}}
<hr />
<label for="chronological-order" class="checkbox-label">
<Input
id="chronological-order"
@type="checkbox"
@checked={{this.chronologicalOrder}}
/>
{{i18n "topic.merge_topic.chronological_order"}}
</label>
{{/if}}
</form>
{{/if}}

76
app/assets/javascripts/discourse/tests/acceptance/topic-move-posts-test.js
View File

@ -1,5 +1,9 @@
import { acceptance, query } from "discourse/tests/helpers/qunit-helpers";
import { click, visit } from "@ember/test-helpers";
import {
acceptance,
exists,
query,
} from "discourse/tests/helpers/qunit-helpers";
import { click, fillIn, visit } from "@ember/test-helpers";
import I18n from "I18n";
import { test } from "qunit";
@ -85,6 +89,40 @@ acceptance("Topic move posts", function (needs) {
);
});
test("moving posts to existing topic", async function (assert) {
await visit("/t/internationalization-localization");
await click(".toggle-admin-menu");
await click(".topic-admin-multi-select .btn");
await click("#post_1 .select-post");
await click(".selected-posts .move-to-topic");
assert.ok(
query(".choose-topic-modal .radios").innerHTML.includes(
I18n.t("topic.merge_topic.radio_label")
),
"it shows an option to move to an existing topic"
);
await click(".choose-topic-modal .radios #move-to-existing-topic");
await fillIn(".choose-topic-modal #choose-topic-title", "Topic");
assert.notOk(
exists(".choose-topic-modal .checkbox-label"),
"there is no chronological order checkbox when no topic is selected"
);
await click(".choose-topic-list .existing-topic:first-child input");
assert.ok(
query(".choose-topic-modal .checkbox-label").innerHTML.includes(
I18n.t("topic.merge_topic.chronological_order")
),
"it shows a checkbox to merge posts in chronological order"
);
});
test("moving posts from personal message", async function (assert) {
await visit("/t/pm-for-testing/12");
await click(".toggle-admin-menu");
@ -142,4 +180,38 @@ acceptance("Topic move posts", function (needs) {
"it opens move to modal"
);
});
test("moving posts from personal message to existing message", async function (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");
await click(".selected-posts .move-to-topic");
assert.ok(
query(".choose-topic-modal .radios").innerHTML.includes(
I18n.t("topic.move_to_existing_message.radio_label")
),
"it shows an option to move to an existing message"
);
await click(".choose-topic-modal .radios #move-to-existing-message");
await fillIn(".choose-topic-modal #choose-message-title", "Topic");
assert.notOk(
exists(".choose-topic-modal .checkbox-label"),
"there is no chronological order checkbox when no message is selected"
);
await click(".choose-topic-modal .existing-message:first-of-type input");
assert.ok(
query(".choose-topic-modal .checkbox-label").innerHTML.includes(
I18n.t("topic.merge_topic.chronological_order")
),
"it shows a checkbox to merge posts in chronological order"
);
});
});

8
app/assets/stylesheets/common/base/modal.scss
View File

@ -602,6 +602,14 @@
width: 100%;
}
}
#choosing-topic {
form {
hr {
margin-bottom: 0.5em;
}
}
}
}
.publish-page-modal {

2
app/assets/stylesheets/desktop/modal.scss
View File

@ -50,7 +50,7 @@
.choose-topic-modal {
.modal-body {
position: relative;
height: 350px;
min-height: 350px;
}
#choosing-topic {

4
app/controllers/topics_controller.rb
View File

@ -820,6 +820,7 @@ class TopicsController < ApplicationController
topic_id = params.require(:topic_id)
destination_topic_id = params.require(:destination_topic_id)
params.permit(:participants)
params.permit(:chronological_order)
params.permit(:archetype)
raise Discourse::InvalidAccess if params[:archetype] == "private_message" && !guardian.is_staff?
@ -832,6 +833,7 @@ class TopicsController < ApplicationController
args = {}
args[:destination_topic_id] = destination_topic_id.to_i
args[:chronological_order] = params[:chronological_order] == "true"
if params[:archetype].present?
args[:archetype] = params[:archetype]
@ -849,6 +851,7 @@ class TopicsController < ApplicationController
params.permit(:category_id)
params.permit(:tags)
params.permit(:participants)
params.permit(:chronological_order)
params.permit(:archetype)
raise Discourse::InvalidAccess if params[:archetype] == "private_message" && !guardian.is_staff?
@ -1273,6 +1276,7 @@ class TopicsController < ApplicationController
:destination_topic_id
].present?
args[:tags] = params[:tags] if params[:tags].present?
args[:chronological_order] = params[:chronological_order] == "true"
if params[:archetype].present?
args[:archetype] = params[:archetype]

172
app/models/post_mover.rb
View File

@ -14,8 +14,9 @@ class PostMover
@move_to_pm = move_to_pm
end
def to_topic(id, participants: nil)
def to_topic(id, participants: nil, chronological_order: false)
@move_type = PostMover.move_types[:existing_topic]
@chronological_order = chronological_order
topic = Topic.find_by_id(id)
if topic.archetype != @original_topic.archetype &&
@ -84,8 +85,13 @@ class PostMover
.count
moving_all_posts = original_topic_posts_count == posts.length
@first_post_number_moved =
posts.first.is_first_post? ? posts[1]&.post_number : posts.first.post_number
create_temp_table
move_each_post
handle_moved_references
create_moderator_post_in_original_topic
update_statistics
update_user_actions
@ -118,7 +124,30 @@ class PostMover
SQL
end
def handle_moved_references
move_incoming_emails
move_notifications
update_reply_counts
update_quotes
move_first_post_replies
delete_post_replies
copy_shifted_post_timings_to_temp
delete_invalid_post_timings
copy_shifted_post_timings_from_temp
move_post_timings
copy_first_post_timings
copy_topic_users
end
def move_each_post
if @chronological_order
move_each_post_chronological
else
move_each_post_sequential
end
end
def move_each_post_sequential
max_post_number = destination_topic.max_post_number + 1
@post_creator = nil
@ -133,7 +162,7 @@ class PostMover
end
posts.each do |post|
metadata = movement_metadata(post)
metadata = movement_metadata(post, new_post_number: @move_map[post.post_number])
new_post = post.is_first_post? ? create_first_post(post) : move(post)
store_movement(metadata, new_post)
@ -142,17 +171,72 @@ class PostMover
destination_topic.topic_allowed_users.create!(user_id: post.user_id)
end
end
end
move_incoming_emails
move_notifications
update_reply_counts
update_quotes
move_first_post_replies
delete_post_replies
delete_invalid_post_timings
copy_first_post_timings
move_post_timings
copy_topic_users
def move_each_post_chronological
destination_posts = destination_topic.ordered_posts.with_deleted
# drops posts from destination_topic until it finds one that was created after posts.first
min_created_at = posts.first.created_at
moved_posts = destination_posts.drop_while { |post| post.created_at <= min_created_at }
# if no post in destination_topic was created after posts.first it's equal to sequential
if moved_posts.empty?
initial_post_number = destination_topic.max_post_number + 1
else
initial_post_number = moved_posts.first.post_number
end
last_index = 0
posts.each do |post|
while last_index < moved_posts.length && moved_posts[last_index].created_at <= post.created_at
last_index += 1
end
moved_posts.insert(last_index, post)
end
@post_creator = nil
@move_map = {}
@shift_map = {}
@reply_count = {}
next_post_number = initial_post_number
moved_posts.each do |post|
if post.topic_id == destination_topic.id
# avoid shifting to a lower post number
next_post_number = post.post_number if post.post_number > next_post_number
@shift_map[post.post_number] = next_post_number
else
@move_map[post.post_number] = next_post_number
if post.reply_to_post_number.present?
@reply_count[post.reply_to_post_number] = (@reply_count[post.reply_to_post_number] || 0) +
1
end
end
next_post_number += 1
end
moved_posts.reverse_each do |post|
if post.topic_id == destination_topic.id
metadata = movement_metadata(post, new_post_number: @shift_map[post.post_number])
new_post = move_same_topic(post)
else
metadata = movement_metadata(post, new_post_number: @move_map[post.post_number])
new_post = post.is_first_post? ? create_first_post(post) : move(post)
if @move_to_pm && !destination_topic.topic_allowed_users.exists?(user_id: post.user_id)
destination_topic.topic_allowed_users.create!(user_id: post.user_id)
end
end
store_movement(metadata, new_post)
end
# change topic owner if there's a new first post
destination_topic.update_column(:user_id, posts.first.user_id) if initial_post_number == 1
end
def create_first_post(post)
@ -175,7 +259,15 @@ class PostMover
move_email_logs(post, new_post)
PostAction.copy(post, new_post)
new_post.update_column(:reply_count, @reply_count[1] || 0)
attrs_to_update = { reply_count: @reply_count[1] || 0 }
if new_post.post_number != @move_map[post.post_number]
attrs_to_update[:post_number] = @move_map[post.post_number]
attrs_to_update[:sort_order] = @move_map[post.post_number]
end
new_post.update_columns(attrs_to_update)
new_post.custom_fields = post.custom_fields
new_post.save_custom_fields
@ -189,8 +281,6 @@ class PostMover
end
def move(post)
@first_post_number_moved ||= post.post_number
update = {
reply_count: @reply_count[post.post_number] || 0,
post_number: @move_map[post.post_number],
@ -213,13 +303,30 @@ class PostMover
post
end
def movement_metadata(post)
def move_same_topic(post)
update = {
post_number: @shift_map[post.post_number],
sort_order: @shift_map[post.post_number],
baked_version: nil,
}
if @shift_map[post.reply_to_post_number]
update[:reply_to_post_number] = @shift_map[post.reply_to_post_number]
end
post.attributes = update
post.save(validate: false)
post
end
def movement_metadata(post, new_post_number: nil)
{
old_topic_id: post.topic_id,
old_post_id: post.id,
old_post_number: post.post_number,
new_topic_id: destination_topic.id,
new_post_number: @move_map[post.post_number],
new_post_number: new_post_number,
new_topic_title: destination_topic.title,
}
end
@ -240,6 +347,7 @@ class PostMover
post_id = mp.new_post_id
FROM moved_posts mp
WHERE ie.topic_id = mp.old_topic_id AND ie.post_id = mp.old_post_id
AND mp.old_topic_id <> mp.new_topic_id
SQL
end
@ -311,6 +419,31 @@ class PostMover
SQL
end
def copy_shifted_post_timings_to_temp
DB.exec("DROP TABLE IF EXISTS temp_post_timings") if Rails.env.test?
# copy post_timings for shifted posts to a temp table using the new_post_number
# they'll be copied back after delete_invalid_post_timings makes room for them
DB.exec <<~SQL
CREATE TEMPORARY TABLE temp_post_timings ON COMMIT DROP
AS (
SELECT pt.topic_id, mp.new_post_number as post_number, pt.user_id, pt.msecs
FROM post_timings pt
JOIN moved_posts mp
ON mp.old_topic_id = pt.topic_id
AND mp.old_post_number = pt.post_number
AND mp.old_topic_id = mp.new_topic_id
)
SQL
end
def copy_shifted_post_timings_from_temp
DB.exec <<~SQL
INSERT INTO post_timings (topic_id, user_id, post_number, msecs)
SELECT topic_id, user_id, post_number, msecs FROM temp_post_timings
SQL
end
def copy_first_post_timings
DB.exec <<~SQL
INSERT INTO post_timings (topic_id, user_id, post_number, msecs)
@ -342,6 +475,7 @@ class PostMover
WHERE pt.topic_id = mp.old_topic_id
AND pt.post_number = mp.old_post_number
AND mp.old_post_id = mp.new_post_id
AND mp.old_topic_id <> mp.new_topic_id
SQL
end
@ -371,12 +505,14 @@ class PostMover
FROM moved_posts lr
WHERE lr.old_topic_id = tu.topic_id
AND lr.old_post_number <= tu.last_read_post_number
AND lr.old_topic_id <> lr.new_topic_id
) AS last_read_post_number,
(
SELECT MAX(le.new_post_number)
FROM moved_posts le
WHERE le.old_topic_id = tu.topic_id
AND le.old_post_number <= tu.last_emailed_post_number
AND le.old_topic_id <> le.new_topic_id
) AS last_emailed_post_number,
GREATEST(tu.first_visited_at, t.created_at) AS first_visited_at,
GREATEST(tu.last_visited_at, t.created_at) AS last_visited_at,
@ -389,7 +525,7 @@ class PostMover
AND GREATEST(
tu.last_read_post_number,
tu.last_emailed_post_number
) >= (SELECT MIN(old_post_number) FROM moved_posts)
) >= (SELECT MIN(mp.old_post_number) FROM moved_posts mp WHERE mp.old_topic_id <> mp.new_topic_id)
ON CONFLICT (topic_id, user_id) DO UPDATE
SET posted = excluded.posted,
last_read_post_number = CASE

6
app/models/topic.rb
View File

@ -1236,7 +1236,11 @@ class Topic < ActiveRecord::Base
)
if opts[:destination_topic_id]
topic = post_mover.to_topic(opts[:destination_topic_id], participants: opts[:participants])
topic =
post_mover.to_topic(
opts[:destination_topic_id],
**opts.slice(:participants, :chronological_order),
)
DiscourseEvent.trigger(:topic_merged, post_mover.original_topic, post_mover.destination_topic)

1
config/locales/client.en.yml
View File

@ -3305,6 +3305,7 @@ en:
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 <b>%{count}</b> posts to."
chronological_order: "preserve chronological order after merging"
move_to_new_message:
title: "Move to New Message"

859
spec/models/post_mover_spec.rb
View File

@ -1286,6 +1286,769 @@ RSpec.describe PostMover do
end
end
context "when moving chronologically to an existing topic" do
fab!(:destination_topic) { Fabricate(:topic, user: user) }
fab!(:destination_op) do
Fabricate(
:post,
topic: destination_topic,
user: user,
created_at: p2.created_at + 30.minutes,
)
end
it "works correctly with post_number gap in destination" do
destination_p6 =
Fabricate(
:post,
topic: destination_topic,
user: another_user,
created_at: p3.created_at + 5.minutes,
post_number: 6,
reply_count: 1,
)
destination_p7 =
Fabricate(
:post,
topic: destination_topic,
user: another_user,
created_at: p3.created_at + 10.minutes,
reply_to_post_number: destination_p6.post_number,
)
destination_p6.replies << destination_p7
# after: p2(-2h) destination_op p3(-1h) destination_p6 destination_p7 p4(-45min)
topic.expects(:add_moderator_post).once
moved_to =
topic.move_posts(
user,
[p2.id, p3.id, p4.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to eq(destination_topic)
# Check out destination topic
moved_to.reload
expect(moved_to.posts_count).to eq(6)
expect(moved_to.highest_post_number).to eq(8)
expect(moved_to.user_id).to eq(p2.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_time(p4.created_at)
# Posts should be re-ordered
p2.reload
expect(p2.sort_order).to eq(1)
expect(p2.post_number).to eq(1)
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)
destination_op.reload
expect(destination_op.sort_order).to eq(2)
expect(destination_op.post_number).to eq(2)
expect(destination_op.reply_count).to eq(0)
expect(destination_op.reply_to_post_number).to eq(nil)
p3.reload
expect(p3.post_number).to eq(3)
expect(p3.sort_order).to eq(3)
expect(p3.topic_id).to eq(moved_to.id)
expect(p3.reply_count).to eq(0)
expect(p3.reply_to_post_number).to eq(nil)
destination_p6.reload
expect(destination_p6.post_number).to eq(6)
expect(destination_p6.sort_order).to eq(6)
expect(destination_p6.reply_count).to eq(1)
expect(destination_p6.reply_to_post_number).to eq(nil)
destination_p7.reload
expect(destination_p7.post_number).to eq(7)
expect(destination_p7.sort_order).to eq(7)
expect(destination_p7.reply_count).to eq(0)
expect(destination_p7.reply_to_post_number).to eq(6)
p4.reload
expect(p4.post_number).to eq(8)
expect(p4.sort_order).to eq(8)
expect(p4.topic_id).to eq(moved_to.id)
expect(p4.reply_count).to eq(0)
expect(p4.reply_to_post_number).to eq(1)
# Check out the original topic
topic.reload
expect(topic.posts_count).to eq(1)
expect(topic.featured_user1_id).to be_blank
expect(topic.like_count).to eq(0)
expect(topic.posts.by_post_number).to match_array([p1])
expect(topic.highest_post_number).to eq(p1.post_number)
# Should notify correctly
notification =
p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first
expect(notification.topic_id).to eq(destination_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(p1.post_number)
end
it "works correctly keeping replies in destination" do
destination_p2 =
Fabricate(
:post,
topic: destination_topic,
user: another_user,
created_at: p3.created_at - 10.minutes,
post_number: 2,
reply_count: 1,
)
destination_p3 =
Fabricate(
:post,
topic: destination_topic,
user: another_user,
created_at: p4.created_at + 5.minutes,
reply_to_post_number: destination_p2.post_number,
reply_count: 1,
)
destination_p4 =
Fabricate(
:post,
topic: destination_topic,
user: another_user,
created_at: p4.created_at + 10.minutes,
reply_to_post_number: destination_p3.post_number,
)
destination_p2.replies << destination_p3
destination_p3.replies << destination_p4
# after: destination_op destination_p2 p3(-1h) p4(-45min) destination_p3 destination_p4
topic.expects(:add_moderator_post).once
moved_to =
topic.move_posts(
user,
[p3.id, p4.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to eq(destination_topic)
# Check out destination topic
moved_to.reload
expect(moved_to.posts_count).to eq(6)
expect(moved_to.highest_post_number).to eq(6)
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)
# Posts should be re-ordered
destination_op.reload
expect(destination_op.sort_order).to eq(1)
expect(destination_op.post_number).to eq(1)
destination_p2.reload
expect(destination_p2.post_number).to eq(2)
expect(destination_p2.sort_order).to eq(2)
expect(destination_p2.reply_count).to eq(1)
expect(destination_p2.reply_to_post_number).to eq(nil)
p3.reload
expect(p3.post_number).to eq(3)
expect(p3.sort_order).to eq(3)
expect(p3.topic_id).to eq(moved_to.id)
expect(p3.reply_count).to eq(0)
expect(p3.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_count).to eq(0)
expect(p4.reply_to_post_number).to eq(nil)
destination_p3.reload
expect(destination_p3.post_number).to eq(5)
expect(destination_p3.sort_order).to eq(5)
expect(destination_p3.reply_count).to eq(1)
expect(destination_p3.reply_to_post_number).to eq(destination_p2.post_number)
destination_p4.reload
expect(destination_p4.post_number).to eq(6)
expect(destination_p4.sort_order).to eq(6)
expect(destination_p4.reply_count).to eq(0)
expect(destination_p4.reply_to_post_number).to eq(destination_p3.post_number)
# Check out the original topic
topic.reload
expect(topic.posts_count).to eq(2)
expect(topic.featured_user1_id).to eq(p2.user_id)
expect(topic.like_count).to eq(0)
expect(topic.posts.by_post_number).to match_array([p1, p2])
expect(topic.highest_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(p2.post_number)
end
it "works correctly when moving the first post" do
# forcing a different user_id than p1.user_id
destination_topic.update_column(:user_id, another_user.id)
destination_op.update_column(:user_id, another_user.id)
# after: p1(-3h) destination_op
topic.expects(:add_moderator_post).once
moved_to =
topic.move_posts(
user,
[p1.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to eq(destination_topic)
# Check out destination topic
moved_to.reload
expect(moved_to.posts_count).to eq(2)
expect(moved_to.highest_post_number).to eq(2)
expect(moved_to.user_id).to eq(p1.user_id)
expect(moved_to.like_count).to eq(0)
# 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(2)
# New first post
new_first = moved_to.posts.where(post_number: 1).first
expect(new_first.sort_order).to eq(1)
expect(new_first.reply_count).to eq(0)
expect(new_first.created_at).to eq_time(p1.created_at)
destination_op.reload
expect(destination_op.sort_order).to eq(2)
expect(destination_op.post_number).to eq(2)
# Check out the original topic
topic.reload
expect(topic.posts_count).to eq(4)
expect(topic.featured_user1_id).to eq(p2.user_id)
expect(topic.like_count).to eq(1)
expect(topic.posts.by_post_number).to match_array([p1, p2, p3, p4])
expect(topic.highest_post_number).to eq(p4.post_number)
end
it "correctly remaps quotes for shifted posts on destination topic" do
destination_p8 =
Fabricate(
:post,
topic: destination_topic,
user: another_user,
created_at: p6.created_at + 10.minutes,
)
raw = <<~RAW
[quote="dan, post:#{destination_p8.post_number}, topic:#{destination_p8.topic_id}, full:true"]
some quote from the other post
[/quote]
the quote above should be updated with new post number and topic id
RAW
p3.update!(raw: raw)
p3.rebake!
expect {
topic.move_posts(
user,
[p6.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
}.to change { p6.reload.topic_id }.and change {
destination_p8.reload.post_number
}.and change { p3.reload.raw }.and change { p3.baked_version }.to(nil)
expect(p3.raw).to include(
"post:#{destination_p8.post_number}, topic:#{destination_p8.topic_id}",
)
end
it "moving all posts will close the topic" do
topic.expects(:add_moderator_post).twice
posts_to_move = [p1.id, p2.id, p3.id, p4.id]
moved_to =
topic.move_posts(
user,
posts_to_move,
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to be_present
topic.reload
expect(topic).to be_closed
end
it "doesn't close the topic when not all posts were moved" do
topic.expects(:add_moderator_post).once
posts_to_move = [p2.id, p3.id]
moved_to =
topic.move_posts(
user,
posts_to_move,
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to be_present
topic.reload
expect(topic).to_not be_closed
end
it "doesn't close the topic when all posts except the first one were moved" do
topic.expects(:add_moderator_post).once
posts_to_move = [p2.id, p3.id, p4.id]
moved_to =
topic.move_posts(
user,
posts_to_move,
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to be_present
topic.reload
expect(topic).to_not be_closed
end
it "schedules topic deleting when all posts were moved" do
SiteSetting.delete_merged_stub_topics_after_days = 7
freeze_time
topic.expects(:add_moderator_post).twice
posts_to_move = [p1.id, p2.id, p3.id, p4.id]
moved_to =
topic.move_posts(
user,
posts_to_move,
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to be_present
timer = topic.topic_timers.find_by(status_type: TopicTimer.types[:delete])
expect(timer).to be_present
expect(timer.execute_at).to eq_time(7.days.from_now)
end
it "doesn't schedule topic deleting when not all posts were moved" do
SiteSetting.delete_merged_stub_topics_after_days = 7
topic.expects(:add_moderator_post).once
posts_to_move = [p1.id, p2.id, p3.id]
moved_to =
topic.move_posts(
user,
posts_to_move,
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to be_present
timer = topic.topic_timers.find_by(status_type: TopicTimer.types[:delete])
expect(timer).to be_nil
end
it "doesn't schedule topic deleting when all posts were moved if it's disabled in settings" do
SiteSetting.delete_merged_stub_topics_after_days = 0
topic.expects(:add_moderator_post).twice
posts_to_move = [p1.id, p2.id, p3.id, p4.id]
moved_to =
topic.move_posts(
user,
posts_to_move,
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(moved_to).to be_present
timer = topic.topic_timers.find_by(status_type: TopicTimer.types[:delete])
expect(timer).to be_nil
end
it "ignores moderator posts and closes the topic if all regular posts were moved" do
add_moderator_post_to topic, Post.types[:moderator_action]
add_moderator_post_to topic, Post.types[:small_action]
posts_to_move = [p1.id, p2.id, p3.id, p4.id]
topic.move_posts(
user,
posts_to_move,
destination_topic_id: destination_topic.id,
chronological_order: true,
)
topic.reload
expect(topic).to be_closed
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,
chronological_order: true,
)
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
it "updates existing notifications" do
n2 = Fabricate(:mentioned_notification, post: p2, user: another_user)
n4 = Fabricate(:mentioned_notification, post: p4, user: another_user)
dest_nop = Fabricate(:mentioned_notification, post: destination_op, user: another_user)
moved_to =
topic.move_posts(
user,
[p2.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
n2 = Notification.find(n2.id)
expect(n2.topic_id).to eq(moved_to.id)
expect(n2.post_number).to eq(1)
expect(n2.data_hash[:topic_title]).to eq(moved_to.title)
n4 = Notification.find(n4.id)
expect(n4.topic_id).to eq(topic.id)
expect(n4.post_number).to eq(4)
dest_nop = Notification.find(dest_nop.id)
expect(dest_nop.post_number).to eq(2)
end
it "deletes notifications for users not allowed to see the topic" do
another_admin = Fabricate(:admin)
staff_category = Fabricate(:private_category, group: Group[:staff])
user_notification = Fabricate(:mentioned_notification, post: p3, user: another_user)
admin_notification = Fabricate(:mentioned_notification, post: p3, user: another_admin)
destination_topic.update!(category_id: staff_category.id)
topic.move_posts(
user,
[p3.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(Notification.exists?(user_notification.id)).to eq(false)
expect(Notification.exists?(admin_notification.id)).to eq(true)
end
context "with post timings" do
fab!(:some_user) { Fabricate(:user) }
it "successfully moves timings" do
create_post_timing(p1, some_user, 500)
create_post_timing(p2, some_user, 1000)
create_post_timing(p3, some_user, 1500)
create_post_timing(p4, some_user, 750)
moved_to =
topic.move_posts(
user,
[p1.id, p4.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(
PostTiming.where(topic_id: topic.id, user_id: some_user.id).pluck(
:post_number,
:msecs,
),
).to contain_exactly([1, 500], [2, 1000], [3, 1500])
expect(
PostTiming.where(topic_id: moved_to.id, user_id: some_user.id).pluck(
:post_number,
:msecs,
),
).to contain_exactly([1, 500], [3, 750])
end
it "moves timings when post timing exists in destination topic" do
destination_p2 =
Fabricate(
:post,
topic: destination_topic,
user: another_user,
created_at: destination_op.created_at + 10.minutes,
post_number: 2,
)
destination_p3 =
Fabricate(
:post,
topic: destination_topic,
user: another_user,
created_at: destination_op.created_at + 15.minutes,
post_number: 3,
)
destination_topic.update!(highest_post_number: 3)
PostTiming.create!(
topic_id: destination_topic.id,
user_id: some_user.id,
post_number: 4,
msecs: 800,
)
create_post_timing(destination_op, some_user, 1500)
create_post_timing(destination_p2, some_user, 2000)
create_post_timing(destination_p3, some_user, 1250)
create_post_timing(p1, some_user, 500)
moved_to =
topic.move_posts(
user,
[p1.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(
PostTiming.where(topic_id: moved_to.id, user_id: some_user.id).pluck(
:post_number,
:msecs,
),
).to contain_exactly([1, 500], [2, 1500], [3, 2000], [4, 1250])
end
end
it "updates topic_user.liked values for both source and destination topics" do
expect(TopicUser.find_by(topic: topic, user: user).liked).to eq(false)
like =
Fabricate(
:post_action,
post: p3,
user: user,
post_action_type_id: PostActionType.types[:like],
)
expect(TopicUser.find_by(topic: topic, user: user).liked).to eq(true)
expect(TopicUser.find_by(topic: destination_topic, user: user)).to eq(nil)
topic.move_posts(
user,
[p3.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(TopicUser.find_by(topic: topic, user: user).liked).to eq(false)
expect(TopicUser.find_by(topic: destination_topic, user: user).liked).to eq(true)
end
context "with read state and other stats per user" do
def create_topic_user(user, topic, opts = {})
notification_level = opts.delete(:notification_level) || :regular
Fabricate(
:topic_user,
opts.merge(
notification_level: TopicUser.notification_levels[notification_level],
topic: topic,
user: user,
),
)
end
fab!(:user1) { Fabricate(:user) }
fab!(:user2) { Fabricate(:user) }
fab!(:user3) { Fabricate(:user) }
fab!(:admin1) { Fabricate(:admin) }
fab!(:admin2) { Fabricate(:admin) }
it "leaves post numbers unchanged when they were lower then the topic's highest post number" do
Fabricate(:post, topic: destination_topic)
Fabricate(:whisper, topic: destination_topic)
destination_topic.reload
expect(destination_topic.highest_post_number).to eq(2)
expect(destination_topic.highest_staff_post_number).to eq(3)
create_topic_user(user1, topic, last_read_post_number: 3, last_emailed_post_number: 3)
create_topic_user(
user1,
destination_topic,
last_read_post_number: 1,
last_emailed_post_number: 1,
)
create_topic_user(user2, topic, last_read_post_number: 3, last_emailed_post_number: 3)
create_topic_user(
user2,
destination_topic,
last_read_post_number: 2,
last_emailed_post_number: 2,
)
create_topic_user(
admin1,
topic,
last_read_post_number: 3,
last_emailed_post_number: 3,
)
create_topic_user(
admin1,
destination_topic,
last_read_post_number: 2,
last_emailed_post_number: 1,
)
create_topic_user(
admin2,
topic,
last_read_post_number: 3,
last_emailed_post_number: 3,
)
create_topic_user(
admin2,
destination_topic,
last_read_post_number: 3,
last_emailed_post_number: 3,
)
moved_to_topic =
topic.move_posts(
user,
[p1.id, p2.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(TopicUser.find_by(topic: moved_to_topic, user: user1)).to have_attributes(
last_read_post_number: 1,
last_emailed_post_number: 1,
)
expect(TopicUser.find_by(topic: moved_to_topic, user: user2)).to have_attributes(
last_read_post_number: 2,
last_emailed_post_number: 2,
)
expect(TopicUser.find_by(topic: moved_to_topic, user: admin1)).to have_attributes(
last_read_post_number: 2,
last_emailed_post_number: 1,
)
expect(TopicUser.find_by(topic: moved_to_topic, user: admin2)).to have_attributes(
last_read_post_number: 3,
last_emailed_post_number: 3,
)
end
it "correctly updates existing topic_user records" do
destination_topic.update!(created_at: 1.day.ago)
original_topic_user1 =
create_topic_user(
user1,
topic,
last_read_post_number: 5,
first_visited_at: 5.hours.ago,
last_visited_at: 30.minutes.ago,
notification_level: :tracking,
).reload
destination_topic_user1 =
create_topic_user(
user1,
destination_topic,
last_read_post_number: 5,
first_visited_at: 7.hours.ago,
last_visited_at: 2.hours.ago,
notification_level: :watching,
).reload
original_topic_user2 =
create_topic_user(
user2,
topic,
last_read_post_number: 5,
first_visited_at: 3.hours.ago,
last_visited_at: 1.hour.ago,
notification_level: :watching,
).reload
destination_topic_user2 =
create_topic_user(
user2,
destination_topic,
last_read_post_number: 5,
first_visited_at: 2.hours.ago,
last_visited_at: 1.hour.ago,
notification_level: :tracking,
).reload
new_topic =
topic.move_posts(
user,
[p1.id, p2.id],
destination_topic_id: destination_topic.id,
chronological_order: true,
)
expect(TopicUser.find_by(topic: new_topic, user: user)).to have_attributes(
notification_level: TopicUser.notification_levels[:tracking],
posted: true,
)
expect(TopicUser.find_by(topic: new_topic, user: user1)).to have_attributes(
first_visited_at: destination_topic_user1.first_visited_at,
last_visited_at: original_topic_user1.last_visited_at,
notification_level: destination_topic_user1.notification_level,
posted: false,
)
expect(TopicUser.find_by(topic: new_topic, user: user2)).to have_attributes(
first_visited_at: original_topic_user2.first_visited_at,
last_visited_at: destination_topic_user2.last_visited_at,
notification_level: destination_topic_user2.notification_level,
posted: false,
)
end
end
end
it "skips validations when moving posts" do
p1.update_attribute(:raw, "foo")
p2.update_attribute(:raw, "bar")
@ -1550,6 +2313,102 @@ RSpec.describe PostMover do
expect(post.raw).to eq(expected_text)
end
end
context "when moving chronologically 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",
chronological_order: true,
)
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",
chronological_order: true,
)
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
topic = Fabricate(:topic, created_at: 4.hours.ago)
expect {
personal_message.move_posts(
admin,
[p2.id, p5.id],
destination_topic_id: topic.id,
chronological_order: true,
)
}.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",
chronological_order: true,
)
expect(moved_to).to be_present
personal_message.reload
expect(personal_message.closed).to eq(true)
expect(moved_to.posts_count).to eq(6)
end
it "uses the correct small action post" do
moved_to =
personal_message.move_posts(
admin,
[p2.id],
destination_topic_id: another_personal_message.id,
archetype: "private_message",
chronological_order: true,
)
post = Post.find_by(topic_id: personal_message.id, post_type: Post.types[:whisper])
expected_text =
I18n.t(
"move_posts.existing_message_moderator_post",
count: 1,
topic_link: "[#{moved_to.title}](#{moved_to.relative_url})",
locale: :en,
)
expect(post.raw).to eq(expected_text)
end
end
end
context "with banner topic" do

208
spec/requests/topics_controller_spec.rb
View File

@ -358,6 +358,29 @@ RSpec.describe TopicsController do
end
end
describe "moving chronologically to an existing topic" do
before { sign_in(moderator) }
fab!(:p1) { Fabricate(:post, user: moderator, created_at: dest_topic.created_at - 1.hour) }
fab!(:topic) { p1.topic }
context "with success" do
it "returns success" do
post "/t/#{topic.id}/move-posts.json",
params: {
post_ids: [p1.id],
destination_topic_id: dest_topic.id,
chronological_order: "true",
}
expect(response.status).to eq(200)
result = response.parsed_body
expect(result["success"]).to eq(true)
expect(result["url"]).to be_present
end
end
end
describe "moving to an existing topic as a group moderator" do
fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) }
fab!(:topic) { Fabricate(:topic, category: category) }
@ -409,6 +432,40 @@ RSpec.describe TopicsController do
end
end
describe "moving chronologically to an existing topic as a group moderator" do
fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) }
fab!(:topic) { Fabricate(:topic, category: category) }
fab!(:p1) do
Fabricate(
:post,
user: group_user.user,
topic: topic,
created_at: dest_topic.created_at - 1.hour,
)
end
let!(:user) { group_user.user }
before do
sign_in(user)
SiteSetting.enable_category_group_moderation = true
end
it "moves the posts" do
post "/t/#{topic.id}/move-posts.json",
params: {
post_ids: [p1.id],
destination_topic_id: dest_topic.id,
chronological_order: "true",
}
expect(response.status).to eq(200)
result = response.parsed_body
expect(result["success"]).to eq(true)
expect(result["url"]).to be_present
end
end
describe "moving to a new message" do
fab!(:message) { pm }
fab!(:p1) { Fabricate(:post, user: user, post_number: 1, topic: message) }
@ -551,6 +608,48 @@ RSpec.describe TopicsController do
end
end
end
describe "moving chronologically to an existing message" do
before { sign_in(admin) }
fab!(:evil_trout) { Fabricate(:evil_trout) }
fab!(:message) { pm }
fab!(:dest_message) do
Fabricate(
:private_message_topic,
user: trust_level_4,
topic_allowed_users: [Fabricate.build(:topic_allowed_user, user: evil_trout)],
)
end
fab!(:p2) do
Fabricate(
:post,
user: evil_trout,
post_number: 2,
topic: message,
created_at: dest_message.created_at - 1.hour,
)
end
context "with success" do
it "returns success" do
post "/t/#{message.id}/move-posts.json",
params: {
post_ids: [p2.id],
destination_topic_id: dest_message.id,
archetype: "private_message",
chronological_order: "true",
}
expect(response.status).to eq(200)
result = response.parsed_body
expect(result["success"]).to eq(true)
expect(result["url"]).to be_present
end
end
end
end
describe "#merge_topic" do
@ -588,6 +687,27 @@ RSpec.describe TopicsController do
end
end
describe "merging chronologically into another topic" do
fab!(:p1) { Fabricate(:post, user: user, created_at: dest_topic.created_at - 1.hour) }
fab!(:topic) { p1.topic }
context "when moving all the posts to the destination topic" do
it "returns success" do
sign_in(moderator)
post "/t/#{topic.id}/merge-topic.json",
params: {
destination_topic_id: dest_topic.id,
chronological_order: "true",
}
expect(response.status).to eq(200)
result = response.parsed_body
expect(result["success"]).to eq(true)
expect(result["url"]).to be_present
end
end
end
describe "merging into another topic as a group moderator" do
fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) }
fab!(:topic) { Fabricate(:topic, category: category) }
@ -625,6 +745,47 @@ RSpec.describe TopicsController do
end
end
describe "merging chronologically into another topic as a group moderator" do
fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) }
fab!(:topic) { Fabricate(:topic, category: category) }
fab!(:p1) do
Fabricate(
:post,
user: post_author1,
post_number: 1,
topic: topic,
created_at: dest_topic.created_at - 1.hour,
)
end
fab!(:p2) do
Fabricate(
:post,
user: post_author2,
post_number: 2,
topic: topic,
created_at: dest_topic.created_at - 30.minutes,
)
end
before do
sign_in(group_user.user)
SiteSetting.enable_category_group_moderation = true
end
it "moves the posts" do
post "/t/#{topic.id}/merge-topic.json",
params: {
destination_topic_id: dest_topic.id,
chronological_order: "true",
}
expect(response.status).to eq(200)
result = response.parsed_body
expect(result["success"]).to eq(true)
expect(result["url"]).to be_present
end
end
describe "merging into another message" do
fab!(:message) { Fabricate(:private_message_topic, user: user) }
fab!(:p1) { Fabricate(:post, topic: message, user: trust_level_4) }
@ -672,6 +833,53 @@ RSpec.describe TopicsController do
end
end
end
describe "merging chronologically into another message" do
fab!(:message) { Fabricate(:private_message_topic, user: user) }
fab!(:dest_message) do
Fabricate(
:private_message_topic,
user: trust_level_4,
topic_allowed_users: [Fabricate.build(:topic_allowed_user, user: moderator)],
)
end
fab!(:p1) do
Fabricate(
:post,
topic: message,
user: trust_level_4,
created_at: dest_message.created_at - 1.hour,
)
end
fab!(:p2) do
Fabricate(
:post,
topic: message,
reply_to_post_number: p1.post_number,
user: user,
created_at: dest_message.created_at - 30.minutes,
)
end
context "when moving 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",
chronological_order: "true",
}
expect(response.status).to eq(200)
result = response.parsed_body
expect(result["success"]).to eq(true)
expect(result["url"]).to be_present
end
end
end
end
describe "#change_post_owners" do