FIX: notification reminder is deleted with bookmark (#35069)

Prior to this fix a user could delete a chat bookmark but the
notification reminder if it had already fired would still show in
notifications.

app/models/bookmark.rb

@@ -28,6 +28,18 @@ class Bookmark < ActiveRecord::Base
   belongs_to :user
   belongs_to :bookmarkable, polymorphic: true
 
+  has_many :reminder_notifications,
+           ->(bookmark) do
+             where(notification_type: Notification.types[:bookmark_reminder]).where(
+               "data::jsonb->>'bookmark_id' = ?",
+               bookmark.id.to_s,
+             )
+           end,
+           class_name: "Notification",
+           foreign_key: :user_id,
+           primary_key: :user_id,
+           dependent: :destroy
+
   def self.auto_delete_preferences
     @auto_delete_preferences ||=
       Enum.new(never: 0, when_reminder_sent: 1, on_owner_reply: 2, clear_reminder: 3)

plugins/chat/spec/system/bookmark_message_spec.rb

@@ -103,6 +103,37 @@ RSpec.describe "Bookmark message", type: :system do
         )
       end
     end
+
+    context "with reminder notification cleanup" do
+      it "removes bookmark reminder notification when bookmark is deleted" do
+        chat_page.visit_channel(category_channel_1)
+        channel_page.bookmark_message(message_1)
+        bookmark_modal.fill_name("Check this out later")
+        bookmark_modal.select_preset_reminder(:tomorrow)
+
+        expect(channel_page).to have_bookmarked_message(message_1)
+
+        bookmark = Bookmark.find_by(bookmarkable: message_1, user: current_user)
+        Chat::MessageBookmarkable.send_reminder_notification(
+          bookmark,
+          data: {
+            title: bookmark.bookmarkable.chat_channel.title(current_user),
+            bookmarkable_url: bookmark.bookmarkable.url,
+          },
+        )
+
+        user_menu.open
+
+        expect(page).to have_css("#quick-access-all-notifications .bookmark-reminder")
+
+        channel_page.bookmark_message(message_1)
+        bookmark_modal.delete
+        bookmark_modal.confirm_delete
+        user_menu.open
+
+        expect(page).to have_no_css("#quick-access-all-notifications .bookmark-reminder")
+      end
+    end
   end
 
   context "when mobile", mobile: true do

spec/models/bookmark_spec.rb

@@ -178,4 +178,100 @@ RSpec.describe Bookmark do
       end
     end
   end
+
+  describe "reminder notifications cleanup" do
+    fab!(:user)
+
+    it "deletes reminder notifications when bookmark is destroyed" do
+      bookmark = Fabricate(:bookmark, user: user, bookmarkable: post)
+
+      notification =
+        Fabricate(
+          :bookmark_reminder_notification,
+          user:,
+          post:,
+          data: {
+            bookmark_id: bookmark.id,
+            bookmarkable_type: "Post",
+            bookmarkable_id: post.id,
+            title: post.topic.title,
+            bookmarkable_url: post.url,
+          }.to_json,
+        )
+
+      expect { bookmark.destroy }.to change { Notification.find_by(id: notification.id) }.from(
+        notification,
+      ).to(nil)
+    end
+
+    it "does not delete other user's reminder notifications" do
+      other_user = Fabricate(:user)
+      bookmark = Fabricate(:bookmark, user: user, bookmarkable: post)
+
+      user_notification =
+        Fabricate(
+          :bookmark_reminder_notification,
+          user:,
+          post:,
+          data: {
+            bookmark_id: bookmark.id,
+            bookmarkable_type: "Post",
+            bookmarkable_id: post.id,
+            title: post.topic.title,
+            bookmarkable_url: post.url,
+          }.to_json,
+        )
+
+      other_notification =
+        Fabricate(
+          :bookmark_reminder_notification,
+          user: other_user,
+          post: post,
+          data: {
+            bookmark_id: bookmark.id,
+            bookmarkable_type: "Post",
+            bookmarkable_id: post.id,
+            title: post.topic.title,
+            bookmarkable_url: post.url,
+          }.to_json,
+        )
+
+      expect { bookmark.destroy }.to change { Notification.find_by(id: user_notification.id) }
+        .from(user_notification)
+        .to(nil)
+        .and(not_change { Notification.find_by(id: other_notification.id) })
+    end
+
+    it "does not delete non-reminder notifications" do
+      bookmark = Fabricate(:bookmark, user: user, bookmarkable: post)
+
+      reminder_notification =
+        Fabricate(
+          :bookmark_reminder_notification,
+          user:,
+          post:,
+          data: {
+            bookmark_id: bookmark.id,
+            bookmarkable_type: "Post",
+            bookmarkable_id: post.id,
+            title: post.topic.title,
+            bookmarkable_url: post.url,
+          }.to_json,
+        )
+
+      other_notification =
+        Fabricate(
+          :notification,
+          user:,
+          post:,
+          notification_type: Notification.types[:liked],
+          data: { post_number: post.post_number }.to_json,
+        )
+
+      expect { bookmark.destroy }.to change { Notification.find_by(id: reminder_notification.id) }
+        .from(reminder_notification)
+        .to(nil)
+        .and(not_change { Notification.find_by(id: other_notification.id) })
+    end
+  end
 end

spec/requests/users_controller_spec.rb

@@ -7705,15 +7705,14 @@ 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
+      it "doesn’t show unread notifications when the bookmark has been deleted" 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.size).to eq(0)
-        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