From df4197c8b883a3aecabaf3d7cd8b362b6b4ea2b6 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 29 Feb 2024 09:03:49 +1000 Subject: [PATCH] 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 | 14 ++++ app/services/post_bookmarkable.rb | 6 +- app/services/registered_bookmarkable.rb | 4 ++ app/services/topic_bookmarkable.rb | 6 +- lib/bookmark_query.rb | 72 ++++++++++++++++--- plugins/chat/lib/chat/message_bookmarkable.rb | 6 +- .../lib/chat/message_bookmarkable_spec.rb | 2 + spec/requests/users_controller_spec.rb | 31 ++++++++ spec/services/base_bookmarkable_spec.rb | 2 + spec/services/post_bookmarkable_spec.rb | 2 + spec/services/topic_bookmarkable_spec.rb | 2 + 11 files changed, 134 insertions(+), 13 deletions(-) diff --git a/app/services/base_bookmarkable.rb b/app/services/base_bookmarkable.rb index a819a432fad..f68748c62b3 100644 --- a/app/services/base_bookmarkable.rb +++ b/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 diff --git a/app/services/post_bookmarkable.rb b/app/services/post_bookmarkable.rb index 4769b4224f2..c11385dd362 100644 --- a/app/services/post_bookmarkable.rb +++ b/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) diff --git a/app/services/registered_bookmarkable.rb b/app/services/registered_bookmarkable.rb index e71f45156b2..0c1b145a336 100644 --- a/app/services/registered_bookmarkable.rb +++ b/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 diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index 02e092584a9..ef299e87521 100644 --- a/app/services/topic_bookmarkable.rb +++ b/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) diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 692860c9440..b1afee13353 100644 --- a/lib/bookmark_query.rb +++ b/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: , + # 566: , + # }, + # "Topic": { + # 123: , + # 99: , + # } + # } + # + # 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 diff --git a/plugins/chat/lib/chat/message_bookmarkable.rb b/plugins/chat/lib/chat/message_bookmarkable.rb index 4a9b2025679..8a779469b70 100644 --- a/plugins/chat/lib/chat/message_bookmarkable.rb +++ b/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 diff --git a/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb b/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb index d2030004322..5ae37107ea0 100644 --- a/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index a5304777939..803f3770c41 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 } diff --git a/spec/services/base_bookmarkable_spec.rb b/spec/services/base_bookmarkable_spec.rb index 06c886bddc1..886a0fa0bf4 100644 --- a/spec/services/base_bookmarkable_spec.rb +++ b/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 diff --git a/spec/services/post_bookmarkable_spec.rb b/spec/services/post_bookmarkable_spec.rb index 36b42c2a2e1..fec2fc41e43 100644 --- a/spec/services/post_bookmarkable_spec.rb +++ b/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 diff --git a/spec/services/topic_bookmarkable_spec.rb b/spec/services/topic_bookmarkable_spec.rb index f9430619776..05dc11dbbc9 100644 --- a/spec/services/topic_bookmarkable_spec.rb +++ b/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