Skip to content
Permalink
Browse files

FIX: Existing post timings could prevent moving posts

Post timings are created by `topic_id` and `post_number` and it's possible that the destination topic already contains post timings for non-existent posts. For example, this can happen if the destination topic was previously split and Discourse recorded post timings for moved posts in the destination topic.

This commit ensures that all timings which reference non-existent posts are deleted from the destination topic before the posts are moved.
  • Loading branch information...
gschlager committed Oct 8, 2019
1 parent eae5765 commit bee000bcec250dc7778a922a4af577e119863f6f
Showing with 44 additions and 11 deletions.
  1. +15 −0 app/models/post_mover.rb
  2. +29 −11 spec/models/post_mover_spec.rb
@@ -64,6 +64,7 @@ def move_posts_to(topic)
moving_all_posts = (@original_topic.posts.pluck(:id).sort == @post_ids.sort)

create_temp_table
delete_invalid_post_timings
move_each_post
notify_users_that_posts_have_moved
update_statistics
@@ -290,6 +291,20 @@ def copy_first_post_timings
SQL
end

def delete_invalid_post_timings
DB.exec(<<~SQL, topid_id: destination_topic.id)
DELETE
FROM post_timings pt
WHERE pt.topic_id = :topid_id
AND NOT EXISTS(
SELECT 1
FROM posts p
WHERE p.topic_id = pt.topic_id
AND p.post_number = pt.post_number
)
SQL
end

def move_post_timings
DB.exec <<~SQL
UPDATE post_timings pt
@@ -554,20 +554,38 @@ def create_topic_user(user, opts = {})
expect(Notification.exists?(admin_notification.id)).to eq(true)
end

it "moves post timings" do
some_user = Fabricate(:user)
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)
context "post timings" do
fab!(:some_user) { Fabricate(:user) }

moved_to = topic.move_posts(user, [p1.id, p4.id], destination_topic_id: destination_topic.id)
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)

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])
moved_to = topic.move_posts(user, [p1.id, p4.id], destination_topic_id: destination_topic.id)

expect(PostTiming.where(topic_id: moved_to.id, user_id: some_user.id).pluck(:post_number, :msecs))
.to contain_exactly([2, 500], [3, 750])
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([2, 500], [3, 750])
end

it "moves timings when post timing exists in destination topic" do
PostTiming.create!(
topic_id: destination_topic.id,
user_id: some_user.id,
post_number: 2,
msecs: 800
)
create_post_timing(p1, some_user, 500)

moved_to = topic.move_posts(user, [p1.id], destination_topic_id: destination_topic.id)

expect(PostTiming.where(topic_id: moved_to.id, user_id: some_user.id).pluck(:post_number, :msecs))
.to contain_exactly([2, 500])
end
end

context "read state and other stats per user" do

2 comments on commit bee000b

@discoursebot

This comment has been minimized.

Copy link

replied Oct 8, 2019

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/can-not-move-any-reply-into-an-existing-topic/129763/7

@discoursebot

This comment has been minimized.

Copy link

replied Oct 8, 2019

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/moving-posts-returns-502-bad-gateway/75654/13

Please sign in to comment.
You can’t perform that action at this time.