FIX: Minor bookmark with reminder issue cleanup (#9436)

* Count user summary bookmarks from new Bookmark table if bookmarks with reminders enabled
* Update topic user bookmarked column when new topic bookmark changed
* Make in:bookmarks search work with new bookmarks
* Fix batch inserts for bookmark rake task (and thus migration). We were only inserting one bookmark at a time, completely defeating the purpose of batching!

app/models/user_summary.rb

@@ -117,10 +117,14 @@ class UserSummary
   end
 
   def bookmark_count
-    UserAction
-      .where(user: @user)
-      .where(action_type: UserAction::BOOKMARK)
-      .count
+    if SiteSetting.enable_bookmarks_with_reminders
+      Bookmark.where(user: @user).count
+    else
+      UserAction
+        .where(user: @user)
+        .where(action_type: UserAction::BOOKMARK)
+        .count
+    end
   end
 
   def recent_time_read

lib/bookmark_manager.rb

@@ -27,6 +27,11 @@ 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
+
     BookmarkReminderNotificationHandler.cache_pending_at_desktop_reminder(@user)
     bookmark
   end
@@ -49,6 +54,8 @@ class BookmarkManager
         raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_delete?(bookmark)
         bookmark.destroy
       end
+
+      update_topic_user_bookmarked(topic: topic, bookmarked: false)
     end
 
     clear_at_desktop_cache_if_required
@@ -69,4 +76,8 @@ class BookmarkManager
   def user_has_any_pending_at_desktop_reminders?
     Bookmark.at_desktop_reminders_for_user(@user).any?
   end
+
+  def update_topic_user_bookmarked(topic:, bookmarked:)
+    TopicUser.change(@user.id, topic, bookmarked: bookmarked)
+  end
 end

lib/search.rb

@@ -364,17 +364,28 @@ class Search
     end
   end
 
-  advanced_filter(/^in:(likes|bookmarks)$/) do |posts, match|
-    if @guardian.user
-      post_action_type = PostActionType.types[:like] if match == "likes"
-      post_action_type = PostActionType.types[:bookmark] if match == "bookmarks"
+  def post_action_type_filter(posts, post_action_type)
+    posts.where("posts.id IN (
+      SELECT pa.post_id FROM post_actions pa
+      WHERE pa.user_id = #{@guardian.user.id} AND
+            pa.post_action_type_id = #{post_action_type} AND
+            deleted_at IS NULL
+    )")
+  end
 
-      posts.where("posts.id IN (
-                            SELECT pa.post_id FROM post_actions pa
-                            WHERE pa.user_id = #{@guardian.user.id} AND
-                                  pa.post_action_type_id = #{post_action_type} AND
-                                  deleted_at IS NULL
-                         )")
+  advanced_filter(/^in:(likes)$/) do |posts, match|
+    if @guardian.user
+      post_action_type_filter(posts, PostActionType.types[:like])
+    end
+  end
+
+  advanced_filter(/^in:(bookmarks)$/) do |posts, match|
+    if @guardian.user
+      if SiteSetting.enable_bookmarks_with_reminders
+        posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})")
+      else
+        post_action_type_filter(posts, PostActionType.types[:bookmark])
+      end
     end
   end
 

lib/tasks/bookmarks.rake

@@ -30,34 +30,43 @@ task "bookmarks:sync_to_table", [:sync_limit] => :environment do |_t, args|
   post_action_bookmark_count = post_action_bookmarks.count('post_actions.id')
   i = 0
 
+  bookmarks_to_create = []
   post_action_bookmarks.find_each(batch_size: 2000) do |pab|
-    # clear at start of each batch to only insert X at a time
-    bookmarks_to_create = []
+    RakeHelpers.print_status_with_label("Creating post new bookmarks.......", i, post_action_bookmark_count)
+    now = Time.zone.now
+    bookmarks_to_create << {
+      topic_id: pab.topic_id,
+      post_id: pab.post_id,
+      user_id: pab.user_id,
+      created_at: now,
+      updated_at: now
+    }
 
-    Bookmark.transaction do
-      RakeHelpers.print_status_with_label("Creating post new bookmarks.......", i, post_action_bookmark_count)
-      now = Time.zone.now
-      bookmarks_to_create << {
-        topic_id: pab.topic_id,
-        post_id: pab.post_id,
-        user_id: pab.user_id,
-        created_at: now,
-        updated_at: now
-      }
+    i += 1
 
-      i += 1
-
-      # this will ignore conflicts in the bookmarks table so
-      # if the user already has a post bookmarked in the new way,
-      # then we don't error and keep on truckin'
-      #
-      # we shouldn't have duplicates here at any rate because of
-      # the above LEFT JOIN but best to be safe knowing this
-      # won't blow up
-      Bookmark.insert_all(bookmarks_to_create)
+    # once we get to 2000 records to create, insert them all and reset
+    # the array and counter to make sure we don't have too many in memory
+    if i >= 2000
+      create_bookmarks(bookmarks_to_create)
+      i = 0
+      bookmarks_to_create = []
     end
   end
 
+  # if there was < 2000 bookmarks this finishes off the last ones
+  create_bookmarks(bookmarks_to_create)
+
   RakeHelpers.print_status_with_label("Bookmark creation complete!            ", i, post_action_bookmark_count)
   puts ""
 end
+
+def create_bookmarks(bookmarks_to_create)
+  # this will ignore conflicts in the bookmarks table so
+  # if the user already has a post bookmarked in the new way,
+  # then we don't error and keep on truckin'
+  #
+  # we shouldn't have duplicates here at any rate because of
+  # the above LEFT JOIN but best to be safe knowing this
+  # won't blow up
+  Bookmark.insert_all(bookmarks_to_create)
+end

spec/lib/bookmark_manager_spec.rb

@@ -32,6 +32,14 @@ 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 }
@@ -188,6 +196,12 @@ RSpec.describe BookmarkManager do
       end
     end
 
+    it "updates the topic user bookmarked column to false" do
+      TopicUser.create(user: user, topic: topic, bookmarked: true)
+      subject.destroy_for_topic(topic)
+      tu = TopicUser.find_by(user: user)
+      expect(tu.bookmarked).to eq(false)
+    end
   end
 
   describe ".send_reminder_notification" do