diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index acb91a57e51..2e800484490 100644 --- a/app/models/bookmark.rb +++ b/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) diff --git a/plugins/chat/spec/system/bookmark_message_spec.rb b/plugins/chat/spec/system/bookmark_message_spec.rb index ec5c403930f..a7870cdb486 100644 --- a/plugins/chat/spec/system/bookmark_message_spec.rb +++ b/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 diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index a82f0cd4007..12b1f5274f4 100644 --- a/spec/models/bookmark_spec.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 667ca4e76bf..f4a19f2e6e6 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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.first["data"]["bookmark_id"]).to eq(bookmark_with_reminder.id) + expect(notifications.size).to eq(0) end it "does not show unread notifications if the bookmark has been deleted if they only have the bookmark_id data" do