FIX: Show deleted bookmark reminders in user bookmarks menu (#25905)

When we send a bookmark reminder, there is an option to delete
the underlying bookmark. The Notification record stays around.
However, if you want to filter your notifications user menu
to only bookmark-based notifications, we were not showing unread
bookmark notifications for deleted bookmarks.

This commit fixes the issue _going forward_ by adding the
bookmarkable_id and bookmarkable_type to the Notification data,
so we can look up the underlying Post/Topic/Chat::Message
for a deleted bookmark and check user access in this way. Then,
it doesn't matter if the bookmark was deleted.

app/services/base_bookmarkable.rb

@@ -130,6 +130,8 @@ class BaseBookmarkable
       display_username: bookmark.user.username,
       bookmark_name: bookmark.name,
       bookmark_id: bookmark.id,
+      bookmarkable_type: bookmark.bookmarkable_type,
+      bookmarkable_id: bookmark.bookmarkable_id,
     ).to_json
     notification_data[:notification_type] = Notification.types[:bookmark_reminder]
     bookmark.user.notifications.create!(notification_data)
@@ -147,6 +149,18 @@ class BaseBookmarkable
     raise NotImplementedError
   end
 
+  ##
+  # The can_see? method calls this one directly. can_see_bookmarkable? can be used
+  # in cases where you know the bookmarkable based on type but don't have a bookmark
+  # record to check against.
+  #
+  # @param [Guardian] guardian The guardian class for the user that we are performing the access check for.
+  # @param [Bookmark] bookmarkable The bookmarkable which we are checking access for (e.g. Post, Topic) which is an ActiveModel instance.
+  # @return [Boolean]
+  def self.can_see_bookmarkable?(guardian, bookmarkable)
+    raise NotImplementedError
+  end
+
   ##
   # Some additional information about the bookmark or the surrounding relations
   # may be required when the bookmark is created or destroyed. For example, when

app/services/post_bookmarkable.rb

@@ -59,7 +59,11 @@ class PostBookmarkable < BaseBookmarkable
   end
 
   def self.can_see?(guardian, bookmark)
-    guardian.can_see_post?(bookmark.bookmarkable)
+    can_see_bookmarkable?(guardian, bookmark.bookmarkable)
+  end
+
+  def self.can_see_bookmarkable?(guardian, bookmarkable)
+    guardian.can_see_post?(bookmarkable)
   end
 
   def self.bookmark_metadata(bookmark, user)

app/services/registered_bookmarkable.rb

@@ -86,6 +86,10 @@ class RegisteredBookmarkable
     bookmarkable_klass.can_see?(guardian, bookmark)
   end
 
+  def can_see_bookmarkable?(guardian, bookmarkable)
+    bookmarkable_klass.can_see_bookmarkable?(guardian, bookmarkable)
+  end
+
   def bookmark_metadata(bookmark, user)
     bookmarkable_klass.bookmark_metadata(bookmark, user)
   end

app/services/topic_bookmarkable.rb

@@ -62,7 +62,11 @@ class TopicBookmarkable < BaseBookmarkable
   end
 
   def self.can_see?(guardian, bookmark)
-    guardian.can_see_topic?(bookmark.bookmarkable)
+    can_see_bookmarkable?(guardian, bookmark.bookmarkable)
+  end
+
+  def self.can_see_bookmarkable?(guardian, bookmarkable)
+    guardian.can_see_topic?(bookmarkable)
   end
 
   def self.bookmark_metadata(bookmark, user)

lib/bookmark_query.rb

@@ -96,20 +96,72 @@ class BookmarkQuery
         .unread
         .where(notification_type: Notification.types[:bookmark_reminder])
 
+    reminder_bookmark_ids = reminder_notifications.map { |n| n.data_hash[:bookmark_id] }.compact
+
     # We preload associations like we do above for the list to avoid
     # N1s in the can_see? guardian calls for each bookmark.
-    bookmarks =
-      Bookmark.where(
-        id: reminder_notifications.map { |n| n.data_hash[:bookmark_id] }.compact,
-        user: @user,
-      )
+    bookmarks = Bookmark.where(user: @user, id: reminder_bookmark_ids)
     BookmarkQuery.preload(bookmarks, self)
 
-    reminder_notifications.select do |n|
-      bookmark = bookmarks.find { |bm| bm.id == n.data_hash[:bookmark_id] }
-      next if bookmark.blank?
-      bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmark.bookmarkable_type)
-      bookmarkable.can_see?(@guardian, bookmark)
+    # Any bookmarks that no longer exist, we need to find the associated
+    # records using bookmarkable details.
+    #
+    # First we want to group these by type into a hash to reduce queries:
+    #
+    # {
+    #   "Post": {
+    #     1234: <Post>,
+    #     566: <Post>,
+    #   },
+    #   "Topic": {
+    #     123: <Topic>,
+    #     99: <Topic>,
+    #   }
+    # }
+    #
+    # We may not need to do this most of the time. It depends mostly on
+    # a user's auto_delete_preference for bookmarks.
+    deleted_bookmark_ids = reminder_bookmark_ids - bookmarks.map(&:id)
+    deleted_bookmarkables =
+      reminder_notifications
+        .select do |notif|
+          deleted_bookmark_ids.include?(notif.data_hash[:bookmark_id]) &&
+            notif.data_hash[:bookmarkable_type].present?
+        end
+        .inject({}) do |hash, notif|
+          hash[notif.data_hash[:bookmarkable_type]] ||= {}
+          hash[notif.data_hash[:bookmarkable_type]][notif.data_hash[:bookmarkable_id]] = nil
+          hash
+        end
+
+    # Then, we can actually find the associated records for each type in the database.
+    deleted_bookmarkables.each do |type, bookmarkable|
+      records = Bookmark.registered_bookmarkable_from_type(type).model.where(id: bookmarkable.keys)
+      records.each { |record| deleted_bookmarkables[type][record.id] = record }
+    end
+
+    reminder_notifications.select do |notif|
+      bookmark = bookmarks.find { |bm| bm.id == notif.data_hash[:bookmark_id] }
+
+      # This is the happy path, it's easiest to look up using a bookmark
+      # that hasn't been deleted.
+      if bookmark.present?
+        bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmark.bookmarkable_type)
+        bookmarkable.can_see?(@guardian, bookmark)
+      else
+        # Otherwise, we have to use our cached records from the deleted
+        # bookmarks' related bookmarkable (e.g. Post, Topic) to determine
+        # secure access.
+        bookmarkable =
+          deleted_bookmarkables.dig(
+            notif.data_hash[:bookmarkable_type],
+            notif.data_hash[:bookmarkable_id],
+          )
+        bookmarkable.present? &&
+          Bookmark.registered_bookmarkable_from_type(
+            notif.data_hash[:bookmarkable_type],
+          ).can_see_bookmarkable?(@guardian, bookmarkable)
+      end
     end
   end
 end

plugins/chat/lib/chat/message_bookmarkable.rb

@@ -63,7 +63,11 @@ module Chat
     end
 
     def self.can_see?(guardian, bookmark)
-      guardian.can_join_chat_channel?(bookmark.bookmarkable.chat_channel)
+      can_see_bookmarkable?(guardian, bookmark.bookmarkable)
+    end
+
+    def self.can_see_bookmarkable?(guardian, bookmarkable)
+      guardian.can_join_chat_channel?(bookmarkable.chat_channel)
     end
 
     def self.cleanup_deleted

plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb

@@ -146,6 +146,8 @@ describe Chat::MessageBookmarkable do
           display_username: bookmark1.user.username,
           bookmark_name: bookmark1.name,
           bookmark_id: bookmark1.id,
+          bookmarkable_type: bookmark1.bookmarkable_type,
+          bookmarkable_id: bookmark1.bookmarkable_id,
         }.to_json,
       )
     end

spec/requests/users_controller_spec.rb

@@ -7087,6 +7087,37 @@ RSpec.describe UsersController do
         expect(notifications.first["data"]["bookmark_id"]).to eq(bookmark_with_reminder.id)
       end
 
+      it "shows unread notifications even if the bookmark has been deleted if they have bookmarkable data" do
+        bookmark_with_reminder.destroy!
+
+        get "/u/#{user.username}/user-menu-bookmarks"
+        expect(response.status).to eq(200)
+
+        notifications = response.parsed_body["notifications"]
+        expect(notifications.size).to eq(1)
+        expect(notifications.first["data"]["bookmark_id"]).to eq(bookmark_with_reminder.id)
+      end
+
+      it "does not show unread notifications if the bookmark has been deleted if they only have the bookmark_id data" do
+        notif =
+          Notification.find_by(
+            topic: bookmark_with_reminder.bookmarkable.topic,
+            post_number: bookmark_with_reminder.bookmarkable.post_number,
+          )
+        new_data = notif.data_hash
+        new_data.delete(:bookmarkable_type)
+        new_data.delete(:bookmarkable_id)
+        notif.update!(data: JSON.dump(new_data))
+
+        bookmark_with_reminder.destroy!
+
+        get "/u/#{user.username}/user-menu-bookmarks"
+        expect(response.status).to eq(200)
+
+        notifications = response.parsed_body["notifications"]
+        expect(notifications.size).to eq(0)
+      end
+
       context "with `show_user_menu_avatars` setting enabled" do
         before { SiteSetting.show_user_menu_avatars = true }
 

spec/services/base_bookmarkable_spec.rb

@@ -42,6 +42,8 @@ RSpec.describe BaseBookmarkable do
           display_username: bookmark.user.username,
           bookmark_name: bookmark.name,
           bookmark_id: bookmark.id,
+          bookmarkable_type: bookmark.bookmarkable_type,
+          bookmarkable_id: bookmark.bookmarkable_id,
         }.to_json,
       )
     end

spec/services/post_bookmarkable_spec.rb

@@ -126,6 +126,8 @@ RSpec.describe PostBookmarkable do
           display_username: bookmark1.user.username,
           bookmark_name: bookmark1.name,
           bookmark_id: bookmark1.id,
+          bookmarkable_type: bookmark1.bookmarkable_type,
+          bookmarkable_id: bookmark1.bookmarkable_id,
         }.to_json,
       )
     end

spec/services/topic_bookmarkable_spec.rb

@@ -122,6 +122,8 @@ RSpec.describe TopicBookmarkable do
           display_username: bookmark1.user.username,
           bookmark_name: bookmark1.name,
           bookmark_id: bookmark1.id,
+          bookmarkable_type: bookmark1.bookmarkable_type,
+          bookmarkable_id: bookmark1.bookmarkable_id,
         }.to_json,
       )
     end