From c659e3e95bbf9ae4fe7ee580b4afda0f6bde2bb7 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 16 Jun 2021 08:30:40 +1000 Subject: [PATCH] FIX: Make sure topic_user.bookmarked is synced in more places (#13383) When we call Bookmark.cleanup! we want to make sure that topic_user.bookmarked is updated for topics linked to the bookmarks that were deleted. Also when PostDestroyer calls destroy and recover. We have a job for this already -- SyncTopicUserBookmarked -- so we just utilize that. --- .../regular/sync_topic_user_bookmarked.rb | 11 +++++++--- app/models/bookmark.rb | 8 +++++++- lib/post_destroyer.rb | 4 ++++ spec/components/post_destroyer_spec.rb | 13 ++++++++++++ spec/jobs/sync_topic_user_bookmarked_spec.rb | 18 +++++++++++++++++ spec/models/bookmark_spec.rb | 20 +++++++++++++++++++ 6 files changed, 70 insertions(+), 4 deletions(-) diff --git a/app/jobs/regular/sync_topic_user_bookmarked.rb b/app/jobs/regular/sync_topic_user_bookmarked.rb index 2e88f65e24d..cf8db86c582 100644 --- a/app/jobs/regular/sync_topic_user_bookmarked.rb +++ b/app/jobs/regular/sync_topic_user_bookmarked.rb @@ -8,7 +8,9 @@ module Jobs DB.exec(<<~SQL, topic_id: topic_id) UPDATE topic_users SET bookmarked = true FROM bookmarks AS b + INNER JOIN posts ON posts.id = b.post_id WHERE NOT topic_users.bookmarked AND + posts.deleted_at IS NULL AND topic_users.topic_id = b.topic_id AND topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} SQL @@ -17,9 +19,12 @@ module Jobs UPDATE topic_users SET bookmarked = false WHERE topic_users.bookmarked AND ( - SELECT COUNT(*) FROM bookmarks - WHERE topic_id = topic_users.topic_id - AND user_id = topic_users.user_id + SELECT COUNT(*) + FROM bookmarks + INNER JOIN posts ON posts.id = bookmarks.post_id + WHERE bookmarks.topic_id = topic_users.topic_id + AND bookmarks.user_id = topic_users.user_id + AND posts.deleted_at IS NULL ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} SQL end diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index c4ccd2f24fc..9137cf25ec4 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -125,12 +125,18 @@ class Bookmark < ActiveRecord::Base # is deleted so that there is a grace period to un-delete. def self.cleanup! grace_time = 3.days.ago - DB.exec(<<~SQL, grace_time: grace_time) + topics_deleted = DB.query(<<~SQL, grace_time: grace_time) DELETE FROM bookmarks b USING topics t, posts p WHERE (b.topic_id = t.id AND b.post_id = p.id) AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time) + RETURNING b.topic_id SQL + + topics_deleted_ids = topics_deleted.map(&:topic_id).uniq + topics_deleted_ids.each do |topic_id| + Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic_id) + end end end diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 60662b85cc1..04d8c6d7b5d 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -76,6 +76,7 @@ class PostDestroyer DiscourseEvent.trigger(:post_destroyed, @post, @opts, @user) WebHook.enqueue_post_hooks(:post_destroyed, @post, payload) + Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic.id) if topic if is_first_post UserProfile.remove_featured_topic_from_all_profiles(@topic) @@ -95,8 +96,11 @@ class PostDestroyer topic.update_column(:user_id, Discourse::SYSTEM_USER_ID) if !topic.user_id topic.recover!(@user) if @post.is_first_post? topic.update_statistics + UserActionManager.post_created(@post) DiscourseEvent.trigger(:post_recovered, @post, @opts, @user) + Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic.id) if topic + if @post.is_first_post? UserActionManager.topic_created(topic) DiscourseEvent.trigger(:topic_recovered, topic, @user) diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb index a5ae57d487a..66ae44a83fb 100644 --- a/spec/components/post_destroyer_spec.rb +++ b/spec/components/post_destroyer_spec.rb @@ -241,6 +241,13 @@ describe PostDestroyer do expect(UserAction.where(target_topic_id: post.topic_id, action_type: UserAction::NEW_TOPIC).count).to eq(1) expect(UserAction.where(target_topic_id: post.topic_id, action_type: UserAction::REPLY).count).to eq(1) end + + it "runs the SyncTopicUserBookmarked for the topic that the post is in so topic_users.bookmarked is correct" do + PostDestroyer.new(@user, @reply).destroy + expect_enqueued_with(job: :sync_topic_user_bookmarked, args: { topic_id: @reply.topic_id }) do + PostDestroyer.new(@user, @reply.reload).recover + end + end end context "recovered by admin" do @@ -465,6 +472,12 @@ describe PostDestroyer do expect(post.raw).to eq(I18n.t('js.post.deleted_by_author_simple')) end + it "runs the SyncTopicUserBookmarked for the topic that the post is in so topic_users.bookmarked is correct" do + post2 = create_post + PostDestroyer.new(post2.user, post2).destroy + expect_job_enqueued(job: :sync_topic_user_bookmarked, args: { topic_id: post2.topic_id }) + end + context "as a moderator" do it "deletes the post" do author = post.user diff --git a/spec/jobs/sync_topic_user_bookmarked_spec.rb b/spec/jobs/sync_topic_user_bookmarked_spec.rb index 25a0e14dbb2..4d4cd107251 100644 --- a/spec/jobs/sync_topic_user_bookmarked_spec.rb +++ b/spec/jobs/sync_topic_user_bookmarked_spec.rb @@ -27,6 +27,24 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do expect(tu5.reload.bookmarked).to eq(false) end + it "does not consider topic as bookmarked if the bookmarked post is deleted" do + topic = Fabricate(:topic) + post1 = Fabricate(:post, topic: topic) + + tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false) + tu2 = Fabricate(:topic_user, topic: topic, bookmarked: true) + + Fabricate(:bookmark, user: tu1.user, topic: topic, post: post1) + Fabricate(:bookmark, user: tu2.user, topic: topic, post: post1) + + post1.trash! + + subject.execute(topic_id: topic.id) + + expect(tu1.reload.bookmarked).to eq(false) + expect(tu2.reload.bookmarked).to eq(false) + end + it "works when no topic id is provided (runs for all topics)" do topic = Fabricate(:topic) Fabricate(:post, topic: topic) diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index 177c0037325..500f4bf475d 100644 --- a/spec/models/bookmark_spec.rb +++ b/spec/models/bookmark_spec.rb @@ -15,6 +15,26 @@ describe Bookmark do expect(Bookmark.find_by(id: bookmark2.id)).to eq(bookmark2) end + it "runs a SyncTopicUserBookmarked job for all deleted bookmark unique topics to make sure topic_user.bookmarked is in sync" do + post = Fabricate(:post) + post2 = Fabricate(:post) + bookmark = Fabricate(:bookmark, post: post, topic: post.topic) + bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) + bookmark3 = Fabricate(:bookmark, post: post2, topic: post2.topic) + bookmark4 = Fabricate(:bookmark, post: post2, topic: post2.topic) + post.trash! + post.update(deleted_at: 4.days.ago) + post2.trash! + post2.update(deleted_at: 4.days.ago) + Bookmark.cleanup! + expect(Bookmark.find_by(id: bookmark.id)).to eq(nil) + expect(Bookmark.find_by(id: bookmark2.id)).to eq(bookmark2) + expect(Bookmark.find_by(id: bookmark3.id)).to eq(nil) + expect(Bookmark.find_by(id: bookmark4.id)).to eq(nil) + expect_job_enqueued(job: :sync_topic_user_bookmarked, args: { topic_id: post.topic_id }) + expect_job_enqueued(job: :sync_topic_user_bookmarked, args: { topic_id: post2.topic_id }) + end + it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do post = Fabricate(:post) bookmark = Fabricate(:bookmark, post: post, topic: post.topic)