FIX: Topic user bookmarked column logic was not correct (#9563)

Make sure the topic_user.bookmarked column is set correctly when user bookmarks/unbookmarks any post in a topic. For example, you bookmarked a post in the topic that was not the OP, the bookmark icon in the topic list would not be shown. Same if deleting a bookmark for the last bookmarked post in a topic, the bookmark icon in the topic list would not be removed.

Previously this was only setting it to true if bookmarking the OP/topic, which was not correct -- we want to show the icon on the topic list if any post is bookmarked.
Also set to false if unbookmarking the last bookmarked post in the topic.
Also in this PR is a migration to correct any out of sync topic_user.bookmarked columns, based on the new logic.

db/migrate/20200428014005_correct_topic_user_bookmarked_boolean.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+class CorrectTopicUserBookmarkedBoolean < ActiveRecord::Migration[6.0]
+  def up
+    # if the relation exists then we set to bookmarked because
+    # at least 1 bookmark for the user + topic exists
+    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

lib/bookmark_manager.rb

@@ -27,10 +27,7 @@ class BookmarkManager
       return add_errors_from(bookmark)
     end
 
-    # bookmarking the topic-level mean
-    if post.is_first_post?
-      update_topic_user_bookmarked(topic: post.topic, bookmarked: true)
-    end
+    update_topic_user_bookmarked(topic: post.topic, bookmarked: true)
 
     BookmarkReminderNotificationHandler.cache_pending_at_desktop_reminder(@user)
     bookmark
@@ -45,7 +42,12 @@ class BookmarkManager
     bookmark.destroy
     clear_at_desktop_cache_if_required
 
-    { topic_bookmarked: Bookmark.exists?(topic_id: bookmark.topic_id, user: @user) }
+    bookmarks_remaining_in_topic = Bookmark.exists?(topic_id: bookmark.topic_id, user: @user)
+    if !bookmarks_remaining_in_topic
+      update_topic_user_bookmarked(topic: bookmark.topic, bookmarked: false)
+    end
+
+    { topic_bookmarked: bookmarks_remaining_in_topic }
   end
 
   def destroy_for_topic(topic)

spec/lib/bookmark_manager_spec.rb

@@ -21,6 +21,16 @@ RSpec.describe BookmarkManager do
       expect(bookmark.topic_id).to eq(post.topic_id)
     end
 
+    it "updates the topic user bookmarked column to true if any post is bookmarked" do
+      subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
+      tu = TopicUser.find_by(user: user)
+      expect(tu.bookmarked).to eq(true)
+      tu.update(bookmarked: false)
+      subject.create(post_id: Fabricate(:post, topic: post.topic).id)
+      tu.reload
+      expect(tu.bookmarked).to eq(true)
+    end
+
     context "when a reminder time + type is provided" do
       it "saves the values correctly" do
         subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
@@ -32,14 +42,6 @@ RSpec.describe BookmarkManager do
       end
     end
 
-    context "when bookmarking the topic level (post is OP)" do
-      it "updates the topic user bookmarked column to true" do
-        subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
-        tu = TopicUser.find_by(user: user)
-        expect(tu.bookmarked).to eq(true)
-      end
-    end
-
     context "when the reminder type is at_desktop" do
       let(:reminder_type) { 'at_desktop' }
       let(:reminder_at) { nil }
@@ -134,6 +136,15 @@ RSpec.describe BookmarkManager do
       expect(result[:topic_bookmarked]).to eq(true)
     end
 
+    context "if the bookmark is the last one bookmarked in the topic" do
+      it "marks the topic user bookmarked column as false" do
+        TopicUser.create(user: user, topic: bookmark.post.topic, bookmarked: true)
+        subject.destroy(bookmark.id)
+        tu = TopicUser.find_by(user: user)
+        expect(tu.bookmarked).to eq(false)
+      end
+    end
+
     context "if the bookmark is belonging to some other user" do
       let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) }
       it "raises an invalid access error" do