From 2d686191b51f8fd0c05aae6d6965f757dec8cae3 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 29 Mar 2021 11:25:48 +1000 Subject: [PATCH] FIX: Bookmark topics were not being updated when the post moved (#12542) Because bookmarks have both topic and post ID, when the post was moved into another topic the bookmark was still attached to the post but did not show in the UI. This PR makes it so the all topic IDs for bookmarks attached to a post are updated when a post is moved. Also included is a migration to fix affected records (e.g. on Meta there are 20 affected records). See: https://meta.discourse.org/t/improved-bookmarks-with-reminders/144542/203 --- app/models/post_mover.rb | 5 ++++ ...3_fix_bookmarks_with_incorrect_topic_id.rb | 22 +++++++++++++++ spec/models/post_mover_spec.rb | 28 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 db/post_migrate/20210328233843_fix_bookmarks_with_incorrect_topic_id.rb diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 5ac0a416a5..6c494810dc 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -76,6 +76,7 @@ class PostMover update_user_actions update_last_post_stats update_upload_security_status + update_bookmarks if moving_all_posts @original_topic.update_status('closed', true, @user) @@ -491,6 +492,10 @@ class PostMover end end + def update_bookmarks + Bookmark.where(post_id: post_ids).update_all(topic_id: @destination_topic.id) + end + def watch_new_topic if @destination_topic.archetype == Archetype.private_message if @original_topic.archetype == Archetype.private_message diff --git a/db/post_migrate/20210328233843_fix_bookmarks_with_incorrect_topic_id.rb b/db/post_migrate/20210328233843_fix_bookmarks_with_incorrect_topic_id.rb new file mode 100644 index 0000000000..0c735dce62 --- /dev/null +++ b/db/post_migrate/20210328233843_fix_bookmarks_with_incorrect_topic_id.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class FixBookmarksWithIncorrectTopicId < ActiveRecord::Migration[6.0] + def up + result = DB.exec(<<~SQL) + UPDATE bookmarks bm + SET topic_id = subquery.correct_topic_id, updated_at = NOW() + FROM ( + SELECT bookmarks.id AS bookmark_id, bookmarks.post_id, bookmarks.topic_id, + posts.topic_id AS correct_topic_id + FROM bookmarks + INNER JOIN posts ON posts.id = bookmarks.post_id + WHERE posts.topic_id != bookmarks.topic_id + ) AS subquery + WHERE bm.id = subquery.bookmark_id; + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 018c850a3e..0cf705f7b2 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -385,6 +385,20 @@ describe PostMover do .to contain_exactly([1, 500], [2, 750]) end + it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do + some_user = Fabricate(:user) + new_post = Fabricate(:post, topic: p1.topic) + bookmark1 = Fabricate(:bookmark, topic: p1.topic, post: p1, user: some_user) + bookmark2 = Fabricate(:bookmark, topic: p4.topic, post: p4) + bookmark3 = Fabricate(:bookmark, topic: p4.topic, post: p4) + bookmark4 = Fabricate(:bookmark, topic: new_post.topic, post: new_post) + new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name") + expect(bookmark1.reload.topic_id).to eq(new_topic.id) + expect(bookmark2.reload.topic_id).to eq(new_topic.id) + expect(bookmark3.reload.topic_id).to eq(new_topic.id) + expect(bookmark4.reload.topic_id).to eq(new_post.topic_id) + end + context "read state and other stats per user" do def create_topic_user(user, opts = {}) notification_level = opts.delete(:notification_level) || :regular @@ -600,6 +614,20 @@ describe PostMover do expect(Notification.exists?(admin_notification.id)).to eq(true) end + it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do + some_user = Fabricate(:user) + new_post = Fabricate(:post, topic: p3.topic) + bookmark1 = Fabricate(:bookmark, topic: p3.topic, post: p3, user: some_user) + bookmark2 = Fabricate(:bookmark, topic: p3.topic, post: p3) + bookmark3 = Fabricate(:bookmark, topic: p3.topic, post: p3) + bookmark4 = Fabricate(:bookmark, topic: new_post.topic, post: new_post) + topic.move_posts(user, [p3.id], destination_topic_id: destination_topic.id) + expect(bookmark1.reload.topic_id).to eq(destination_topic.id) + expect(bookmark2.reload.topic_id).to eq(destination_topic.id) + expect(bookmark3.reload.topic_id).to eq(destination_topic.id) + expect(bookmark4.reload.topic_id).to eq(new_post.topic_id) + end + context "post timings" do fab!(:some_user) { Fabricate(:user) }