diff --git a/app/jobs/onceoff/clean_up_post_timings.rb b/app/jobs/onceoff/clean_up_post_timings.rb new file mode 100644 index 00000000000..43c0e5cabaa --- /dev/null +++ b/app/jobs/onceoff/clean_up_post_timings.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Jobs + class CleanUpPostTimings < Jobs::Onceoff + + # Remove post timings that are remnants of previous post moves + # or other shenanigans and don't reference a valid user or post anymore. + def execute_onceoff(args) + DB.exec <<~SQL + DELETE + FROM post_timings pt + WHERE NOT EXISTS( + SELECT 1 + FROM posts p + WHERE p.topic_id = pt.topic_id + AND p.post_number = pt.post_number + ) + SQL + + DB.exec <<~SQL + DELETE + FROM post_timings pt + WHERE NOT EXISTS( + SELECT 1 + FROM users u + WHERE pt.user_id = u.id + ) + SQL + end + end +end diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 3fcf4f3b53b..65cc60f6f9f 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -90,7 +90,10 @@ class PostMover new_topic_title VARCHAR, new_post_id INTEGER, new_post_number INTEGER - ) + ); + + CREATE INDEX moved_posts_old_post_number ON moved_posts(old_post_number); + CREATE INDEX moved_posts_old_post_id ON moved_posts(old_post_id); SQL end @@ -105,11 +108,8 @@ class PostMover @move_map = {} @reply_count = {} posts.each_with_index do |post, offset| - unless post.is_first_post? - @move_map[post.post_number] = offset + max_post_number - else - @move_map[post.post_number] = 1 - end + @move_map[post.post_number] = offset + max_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 @@ -131,6 +131,9 @@ class PostMover update_reply_counts move_first_post_replies delete_post_replies + copy_first_post_timings + move_post_timings + copy_topic_users end def create_first_post(post) @@ -260,7 +263,7 @@ class PostMover DB.exec <<~SQL UPDATE post_replies pr SET post_id = mp.new_post_id - FROM moved_posts mp, moved_posts mr + FROM moved_posts mp WHERE mp.old_post_id <> mp.new_post_id AND pr.post_id = mp.old_post_id AND EXISTS (SELECT 1 FROM moved_posts mr WHERE mr.new_post_id = pr.reply_id) SQL @@ -275,11 +278,124 @@ class PostMover SQL end + def copy_first_post_timings + DB.exec <<~SQL + INSERT INTO post_timings (topic_id, user_id, post_number, msecs) + SELECT mp.new_topic_id, pt.user_id, mp.new_post_number, pt.msecs + FROM post_timings pt + JOIN moved_posts mp ON (pt.topic_id = mp.old_topic_id AND pt.post_number = mp.old_post_number) + WHERE mp.old_post_id <> mp.new_post_id + ON CONFLICT (topic_id, post_number, user_id) DO UPDATE + SET msecs = GREATEST(post_timings.msecs, excluded.msecs) + SQL + end + + def move_post_timings + DB.exec <<~SQL + UPDATE post_timings pt + SET topic_id = mp.new_topic_id, + post_number = mp.new_post_number + FROM moved_posts mp + 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 + SQL + end + + def copy_topic_users + params = { + old_topic_id: original_topic.id, + new_topic_id: destination_topic.id, + old_highest_post_number: destination_topic.highest_post_number, + old_highest_staff_post_number: destination_topic.highest_staff_post_number + } + + DB.exec(<<~SQL, params) + INSERT INTO topic_users(user_id, topic_id, posted, last_read_post_number, highest_seen_post_number, + last_emailed_post_number, first_visited_at, last_visited_at, notification_level, + notifications_changed_at, notifications_reason_id) + SELECT tu.user_id, + :new_topic_id AS topic_id, + EXISTS( + SELECT 1 + FROM posts p + WHERE p.topic_id = tu.topic_id + AND p.user_id = tu.user_id + ) AS posted, + MAX(lr.new_post_number) AS last_read_post_number, + MAX(hs.new_post_number) AS highest_seen_post_number, + MAX(le.new_post_number) 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, + tu.notification_level, + tu.notifications_changed_at, + tu.notifications_reason_id + FROM topic_users tu + JOIN topics t + ON (t.id = :new_topic_id) + LEFT OUTER JOIN moved_posts lr + ON (lr.old_topic_id = tu.topic_id AND lr.old_post_number <= tu.last_read_post_number) + LEFT OUTER JOIN moved_posts hs + ON (hs.old_topic_id = tu.topic_id AND hs.old_post_number <= tu.highest_seen_post_number) + LEFT OUTER JOIN moved_posts le + ON (le.old_topic_id = tu.topic_id AND le.old_post_number <= tu.last_emailed_post_number) + WHERE tu.topic_id = :old_topic_id + AND GREATEST( + tu.last_read_post_number, + tu.highest_seen_post_number, + tu.last_emailed_post_number + ) >= (SELECT MIN(old_post_number) FROM moved_posts) + GROUP BY tu.topic_id, tu.user_id, tu.first_visited_at, tu.last_visited_at, t.created_at, tu.notification_level, + tu.notifications_changed_at, + tu.notifications_reason_id + ON CONFLICT (topic_id, user_id) DO UPDATE + SET posted = excluded.posted, + last_read_post_number = CASE + WHEN topic_users.last_read_post_number = :old_highest_staff_post_number OR ( + :old_highest_post_number < :old_highest_staff_post_number + AND topic_users.last_read_post_number = :old_highest_post_number + AND NOT EXISTS(SELECT 1 + FROM users u + WHERE u.id = topic_users.user_id + AND (admin OR moderator)) + ) THEN + GREATEST(topic_users.last_read_post_number, + excluded.last_read_post_number) + ELSE topic_users.last_read_post_number END, + highest_seen_post_number = CASE + WHEN topic_users.highest_seen_post_number = :old_highest_staff_post_number OR ( + :old_highest_post_number < :old_highest_staff_post_number + AND topic_users.highest_seen_post_number = :old_highest_post_number + AND NOT EXISTS(SELECT 1 + FROM users u + WHERE u.id = topic_users.user_id + AND (admin OR moderator)) + ) THEN + GREATEST(topic_users.highest_seen_post_number, + excluded.highest_seen_post_number) + ELSE topic_users.highest_seen_post_number END, + last_emailed_post_number = CASE + WHEN topic_users.last_emailed_post_number = :old_highest_staff_post_number OR ( + :old_highest_post_number < :old_highest_staff_post_number + AND topic_users.last_emailed_post_number = :old_highest_post_number + AND NOT EXISTS(SELECT 1 + FROM users u + WHERE u.id = topic_users.user_id + AND (admin OR moderator)) + ) THEN + GREATEST(topic_users.last_emailed_post_number, + excluded.last_emailed_post_number) + ELSE topic_users.last_emailed_post_number END, + first_visited_at = LEAST(topic_users.first_visited_at, excluded.first_visited_at), + last_visited_at = GREATEST(topic_users.last_visited_at, excluded.last_visited_at) + SQL + end + def update_statistics destination_topic.update_statistics original_topic.update_statistics - TopicUser.update_post_action_cache(topic_id: original_topic.id, post_action_type: :bookmark) - TopicUser.update_post_action_cache(topic_id: destination_topic.id, post_action_type: :bookmark) + TopicUser.update_post_action_cache(topic_id: original_topic.id) + TopicUser.update_post_action_cache(topic_id: destination_topic.id) end def update_user_actions @@ -340,7 +456,7 @@ class PostMover attrs = {} attrs[:last_posted_at] = post.created_at attrs[:last_post_user_id] = post.user_id - attrs[:bumped_at] = post.created_at unless post.no_bump + attrs[:bumped_at] = Time.now attrs[:updated_at] = Time.now destination_topic.update_columns(attrs) end diff --git a/spec/fabricators/post_fabricator.rb b/spec/fabricators/post_fabricator.rb index 8f0fef9259c..33c7598568f 100644 --- a/spec/fabricators/post_fabricator.rb +++ b/spec/fabricators/post_fabricator.rb @@ -162,3 +162,7 @@ Fabricator(:post_via_email, from: :post) do incoming_email.user = post.user end end + +Fabricator(:whisper, from: :post) do + post_type Post.types[:whisper] +end diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 80a5f97d6ba..77fdbfac951 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -114,6 +114,17 @@ describe PostMover do before do TopicUser.update_last_read(user, topic.id, p4.post_number, p4.post_number, 0) TopicLink.extract_from(p2) + + freeze_time Time.now + end + + def create_post_timing(post, user, msecs) + PostTiming.create!( + topic_id: post.topic_id, + user_id: user.id, + post_number: post.post_number, + msecs: msecs + ) end context "post replies" do @@ -207,7 +218,7 @@ describe PostMover do 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) + expect(new_topic.bumped_at).to be_within(1.second).of(Time.now) p2.reload expect(p2.sort_order).to eq(1) @@ -234,7 +245,7 @@ describe PostMover do notification_level: TopicUser.notification_levels[:watching], notifications_reason_id: TopicUser.notification_reasons[:created_topic] )).to eq(true) - expect(TopicUser.exists?(user_id: user, topic_id: new_topic.id)).to eq(false) + expect(TopicUser.exists?(user_id: user, topic_id: new_topic.id)).to eq(true) end it "moving all posts will close the topic" do @@ -334,11 +345,113 @@ describe PostMover do expect(Notification.exists?(user_notification.id)).to eq(false) 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) + + new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name") + + 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: new_topic.id, user_id: some_user.id).pluck(:post_number, :msecs)) + .to contain_exactly([1, 500], [2, 750]) + end + + context "read state and other stats per user" do + def create_topic_user(user, 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 "correctly moves topic_user records" do + create_topic_user( + user1, + last_read_post_number: 4, + highest_seen_post_number: 4, + last_emailed_post_number: 3, + notification_level: :tracking + ) + create_topic_user( + user3, + last_read_post_number: 1, + highest_seen_post_number: 2, + last_emailed_post_number: 4, + notification_level: :watching + ) + + new_topic = topic.move_posts(user, [p1.id, p2.id], title: "new testing topic name") + + expect(TopicUser.where(topic_id: topic.id).count).to eq(3) + expect(TopicUser.find_by(topic: topic, user: user)) + .to have_attributes( + last_read_post_number: 4, + highest_seen_post_number: 4, + last_emailed_post_number: nil, + notification_level: TopicUser.notification_levels[:tracking] + ) + expect(TopicUser.find_by(topic: topic, user: user1)) + .to have_attributes( + last_read_post_number: 4, + highest_seen_post_number: 4, + last_emailed_post_number: 3, + notification_level: TopicUser.notification_levels[:tracking] + ) + expect(TopicUser.find_by(topic: topic, user: user3)) + .to have_attributes( + last_read_post_number: 1, + highest_seen_post_number: 2, + last_emailed_post_number: 4, + notification_level: TopicUser.notification_levels[:watching] + ) + + expect(TopicUser.where(topic_id: new_topic.id).count).to eq(3) + expect(TopicUser.find_by(topic: new_topic, user: user)) + .to have_attributes( + last_read_post_number: 1, + highest_seen_post_number: 1, + last_emailed_post_number: nil, + notification_level: TopicUser.notification_levels[:watching], + posted: true + ) + expect(TopicUser.find_by(topic: new_topic, user: user1)) + .to have_attributes( + last_read_post_number: 2, + highest_seen_post_number: 2, + last_emailed_post_number: 2, + notification_level: TopicUser.notification_levels[:tracking], + posted: false + ) + expect(TopicUser.find_by(topic: new_topic, user: user3)) + .to have_attributes( + last_read_post_number: 1, + highest_seen_post_number: 2, + last_emailed_post_number: 2, + notification_level: TopicUser.notification_levels[:watching], + posted: false + ) + end + end end context "to an existing topic" do fab!(:destination_topic) { Fabricate(:topic, user: another_user) } - fab!(:destination_op) { Fabricate(:post, topic: destination_topic, user: another_user) } + fab!(:destination_op) { Fabricate(:post, topic: destination_topic, user: another_user, created_at: 1.day.ago) } it "works correctly" do topic.expects(:add_moderator_post).once @@ -355,7 +468,7 @@ describe PostMover do 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) + expect(moved_to.bumped_at).to be_within(1.second).of(Time.now) # Posts should be re-ordered p2.reload @@ -440,6 +553,189 @@ describe PostMover do expect(Notification.exists?(user_notification.id)).to eq(false) 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) + + moved_to = topic.move_posts(user, [p1.id, p4.id], destination_topic_id: destination_topic.id) + + 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 + + context "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, + highest_seen_post_number: 3, + last_emailed_post_number: 3 + ) + create_topic_user( + user1, destination_topic, + last_read_post_number: 1, + highest_seen_post_number: 2, + last_emailed_post_number: 1 + ) + + create_topic_user( + user2, topic, + last_read_post_number: 3, + highest_seen_post_number: 3, + last_emailed_post_number: 3 + ) + create_topic_user( + user2, destination_topic, + last_read_post_number: 2, + highest_seen_post_number: 1, + last_emailed_post_number: 2 + ) + + create_topic_user( + admin1, topic, + last_read_post_number: 3, + highest_seen_post_number: 3, + last_emailed_post_number: 3 + ) + create_topic_user( + admin1, destination_topic, + last_read_post_number: 2, + highest_seen_post_number: 3, + last_emailed_post_number: 1 + ) + + create_topic_user( + admin2, topic, + last_read_post_number: 3, + highest_seen_post_number: 3, + last_emailed_post_number: 3 + ) + create_topic_user( + admin2, destination_topic, + last_read_post_number: 3, + highest_seen_post_number: 2, + last_emailed_post_number: 3 + ) + + moved_to_topic = topic.move_posts(user, [p1.id, p2.id], destination_topic_id: destination_topic.id) + + expect(TopicUser.find_by(topic: moved_to_topic, user: user1)) + .to have_attributes( + last_read_post_number: 1, + highest_seen_post_number: 5, + last_emailed_post_number: 1 + ) + + expect(TopicUser.find_by(topic: moved_to_topic, user: user2)) + .to have_attributes( + last_read_post_number: 5, + highest_seen_post_number: 1, + last_emailed_post_number: 5 + ) + + expect(TopicUser.find_by(topic: moved_to_topic, user: admin1)) + .to have_attributes( + last_read_post_number: 2, + highest_seen_post_number: 5, + last_emailed_post_number: 1 + ) + + expect(TopicUser.find_by(topic: moved_to_topic, user: admin2)) + .to have_attributes( + last_read_post_number: 5, + highest_seen_post_number: 2, + last_emailed_post_number: 5 + ) + 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, + highest_seen_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, + highest_seen_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, + highest_seen_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, + highest_seen_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) + + 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 context "to a message" do @@ -465,7 +761,7 @@ describe PostMover do 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) + expect(new_topic.bumped_at).to be_within(1.second).of(Time.now) p2.reload expect(p2.sort_order).to eq(1)