From 66d17fdd6b09d95ff5e3e5c06c7447688fa6b79e Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 14 Apr 2021 09:10:53 +1000 Subject: [PATCH] FIX: Topic user bookmarked column is out of sync after post moves (#12612) When posts are moved from one topic to another, the `topic_user.bookmarked` column for all users in the new and the old topic needs to be resynced, for example because a user bookmarks post 12 in topic 1, then it is moved to topic 2, the topic_user record for topic 1 should no longer be bookmarked. A background job has been added to sync the column for a specified topic, or for no topic at all, which does it for all topics like the migration. Also includes a migration that we have run in the past to fix bad data. ---- This has been addressed in other places in the past: https://github.com/discourse/discourse/pull/10211 https://github.com/discourse/discourse/pull/10188 --- .../regular/sync_topic_user_bookmarked.rb | 27 ++++++++++ app/models/post_mover.rb | 5 ++ ...topic_user_bookmarked_sync_issues_again.rb | 26 +++++++++ spec/jobs/sync_topic_user_bookmarked_spec.rb | 53 +++++++++++++++++++ spec/models/post_mover_spec.rb | 42 +++++++++++++++ 5 files changed, 153 insertions(+) create mode 100644 app/jobs/regular/sync_topic_user_bookmarked.rb create mode 100644 db/migrate/20210406060434_fix_topic_user_bookmarked_sync_issues_again.rb create mode 100644 spec/jobs/sync_topic_user_bookmarked_spec.rb diff --git a/app/jobs/regular/sync_topic_user_bookmarked.rb b/app/jobs/regular/sync_topic_user_bookmarked.rb new file mode 100644 index 00000000000..2e88f65e24d --- /dev/null +++ b/app/jobs/regular/sync_topic_user_bookmarked.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Jobs + class SyncTopicUserBookmarked < ::Jobs::Base + def execute(args = {}) + topic_id = args[:topic_id] + + DB.exec(<<~SQL, topic_id: topic_id) + UPDATE topic_users SET bookmarked = true + FROM bookmarks AS b + WHERE NOT topic_users.bookmarked 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 + + DB.exec(<<~SQL, topic_id: topic_id) + 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 + ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} + SQL + end + end +end diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 6c494810dcb..27a352738e6 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -494,6 +494,11 @@ class PostMover def update_bookmarks Bookmark.where(post_id: post_ids).update_all(topic_id: @destination_topic.id) + + DB.after_commit do + Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @original_topic.id) + Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @destination_topic.id) + end end def watch_new_topic diff --git a/db/migrate/20210406060434_fix_topic_user_bookmarked_sync_issues_again.rb b/db/migrate/20210406060434_fix_topic_user_bookmarked_sync_issues_again.rb new file mode 100644 index 00000000000..41aedc55544 --- /dev/null +++ b/db/migrate/20210406060434_fix_topic_user_bookmarked_sync_issues_again.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class FixTopicUserBookmarkedSyncIssuesAgain < ActiveRecord::Migration[6.0] + disable_ddl_transaction! + + def up + DB.exec( + <<~SQL + UPDATE topic_users SET bookmarked = true + FROM bookmarks AS b + WHERE NOT topic_users.bookmarked AND topic_users.topic_id = b.topic_id AND topic_users.user_id = b.user_id + SQL + ) + + DB.exec( + <<~SQL + 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) = 0 + SQL + ) + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/spec/jobs/sync_topic_user_bookmarked_spec.rb b/spec/jobs/sync_topic_user_bookmarked_spec.rb new file mode 100644 index 00000000000..25a0e14dbb2 --- /dev/null +++ b/spec/jobs/sync_topic_user_bookmarked_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Jobs::SyncTopicUserBookmarked do + it "corrects all topic_users.bookmarked records for the topic" do + topic = Fabricate(:topic) + Fabricate(:post, topic: topic) + Fabricate(:post, topic: topic) + Fabricate(:post, topic: topic) + + tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false) + tu2 = Fabricate(:topic_user, topic: topic, bookmarked: false) + tu3 = Fabricate(:topic_user, topic: topic, bookmarked: true) + tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true) + tu5 = Fabricate(:topic_user, bookmarked: false) + + Fabricate(:bookmark, user: tu1.user, topic: topic, post: topic.posts.sample) + Fabricate(:bookmark, user: tu4.user, topic: topic, post: topic.posts.sample) + + subject.execute(topic_id: topic.id) + + expect(tu1.reload.bookmarked).to eq(true) + expect(tu2.reload.bookmarked).to eq(false) + expect(tu3.reload.bookmarked).to eq(false) + expect(tu4.reload.bookmarked).to eq(true) + expect(tu5.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) + Fabricate(:post, topic: topic) + Fabricate(:post, topic: topic) + + tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false) + tu2 = Fabricate(:topic_user, topic: topic, bookmarked: false) + tu3 = Fabricate(:topic_user, topic: topic, bookmarked: true) + tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true) + tu5 = Fabricate(:topic_user, bookmarked: false) + + Fabricate(:bookmark, user: tu1.user, topic: topic, post: topic.posts.sample) + Fabricate(:bookmark, user: tu4.user, topic: topic, post: topic.posts.sample) + + subject.execute + + expect(tu1.reload.bookmarked).to eq(true) + expect(tu2.reload.bookmarked).to eq(false) + expect(tu3.reload.bookmarked).to eq(false) + expect(tu4.reload.bookmarked).to eq(true) + expect(tu5.reload.bookmarked).to eq(false) + end +end diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 0cf705f7b2b..2729981a399 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -399,6 +399,48 @@ describe PostMover do expect(bookmark4.reload.topic_id).to eq(new_post.topic_id) end + it "makes sure the topic_user.bookmarked value is reflected for users in the source and destination topic" do + Jobs.run_immediately! + user1 = Fabricate(:user) + user2 = Fabricate(:user) + + bookmark1 = Fabricate(:bookmark, topic: p1.topic, post: p1, user: user1) + bookmark2 = Fabricate(:bookmark, topic: p4.topic, post: p4, user: user1) + + bookmark3 = Fabricate(:bookmark, topic: p3.topic, post: p3, user: user2) + bookmark4 = Fabricate(:bookmark, topic: p4.topic, post: p4, user: user2) + + tu1 = Fabricate( + :topic_user, + user: user1, + topic: p1.topic, + bookmarked: true, + notification_level: TopicUser.notification_levels[:watching], + last_read_post_number: 4, + highest_seen_post_number: 4, + last_emailed_post_number: 3 + ) + tu2 = Fabricate( + :topic_user, + user: user2, + topic: p1.topic, + bookmarked: true, + notification_level: TopicUser.notification_levels[:watching], + last_read_post_number: 4, + highest_seen_post_number: 4, + last_emailed_post_number: 3 + ) + + new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name") + new_topic_user1 = TopicUser.find_by(topic: new_topic, user: user1) + new_topic_user2 = TopicUser.find_by(topic: new_topic, user: user2) + + expect(tu1.reload.bookmarked).to eq(false) + expect(tu2.reload.bookmarked).to eq(true) + expect(new_topic_user1.bookmarked).to eq(true) + expect(new_topic_user2.bookmarked).to eq(true) + end + context "read state and other stats per user" do def create_topic_user(user, opts = {}) notification_level = opts.delete(:notification_level) || :regular