diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 75cd9c60b79..ecf965cf256 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -55,7 +55,8 @@ class NotificationsController < ApplicationController end end - notifications = filter_inaccessible_notifications(notifications) + notifications = + Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications) json = { notifications: serialize_data(notifications, NotificationSerializer), @@ -82,7 +83,8 @@ class NotificationsController < ApplicationController total_rows = notifications.dup.count notifications = notifications.offset(offset).limit(60) - notifications = filter_inaccessible_notifications(notifications) + notifications = + Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications) render_json_dump( notifications: serialize_data(notifications, NotificationSerializer), total_rows_notifications: total_rows, @@ -155,10 +157,4 @@ class NotificationsController < ApplicationController def render_notification render_json_dump(NotificationSerializer.new(@notification, scope: guardian, root: false)) end - - def filter_inaccessible_notifications(notifications) - topic_ids = notifications.map { |n| n.topic_id }.compact.uniq - accessible_topic_ids = guardian.can_see_topic_ids(topic_ids: topic_ids) - notifications.select { |n| n.topic_id.blank? || accessible_topic_ids.include?(n.topic_id) } - end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 968911f2a01..238ec7d9639 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1878,11 +1878,7 @@ class UsersController < ApplicationController end reminder_notifications = - Notification - .for_user_menu(current_user.id, limit: USER_MENU_LIST_LIMIT) - .unread - .where(notification_type: Notification.types[:bookmark_reminder]) - + BookmarkQuery.new(user: current_user).unread_notifications(limit: USER_MENU_LIST_LIMIT) if reminder_notifications.size < USER_MENU_LIST_LIMIT exclude_bookmark_ids = reminder_notifications.filter_map { |notification| notification.data_hash[:bookmark_id] } diff --git a/app/models/notification.rb b/app/models/notification.rb index 3673a3798f8..91c506a28dd 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -227,6 +227,12 @@ class Notification < ActiveRecord::Base Notification.where(user_id: user_id, topic_id: topic_id).delete_all end + def self.filter_inaccessible_topic_notifications(guardian, notifications) + topic_ids = notifications.map { |n| n.topic_id }.compact.uniq + accessible_topic_ids = guardian.can_see_topic_ids(topic_ids: topic_ids) + notifications.select { |n| n.topic_id.blank? || accessible_topic_ids.include?(n.topic_id) } + end + # Be wary of calling this frequently. O(n) JSON parsing can suck. def data_hash @data_hash ||= diff --git a/app/services/post_bookmarkable.rb b/app/services/post_bookmarkable.rb index 97086d9ba40..4769b4224f2 100644 --- a/app/services/post_bookmarkable.rb +++ b/app/services/post_bookmarkable.rb @@ -12,7 +12,7 @@ class PostBookmarkable < BaseBookmarkable end def self.preload_associations - [{ topic: [:tags] }, :user] + [{ topic: %i[tags category] }, :user] end def self.list_query(user, guardian) @@ -54,7 +54,8 @@ class PostBookmarkable < BaseBookmarkable end def self.reminder_conditions(bookmark) - bookmark.bookmarkable.present? && bookmark.bookmarkable.topic.present? + bookmark.bookmarkable.present? && bookmark.bookmarkable.topic.present? && + self.can_see?(bookmark.user.guardian, bookmark) end def self.can_see?(guardian, bookmark) diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index f79af4c07c4..02e092584a9 100644 --- a/app/services/topic_bookmarkable.rb +++ b/app/services/topic_bookmarkable.rb @@ -12,7 +12,7 @@ class TopicBookmarkable < BaseBookmarkable end def self.preload_associations - [:tags, { first_post: :user }] + [:category, :tags, { first_post: :user }] end def self.perform_custom_preload!(topic_bookmarks, guardian) @@ -58,7 +58,7 @@ class TopicBookmarkable < BaseBookmarkable end def self.reminder_conditions(bookmark) - bookmark.bookmarkable.present? + bookmark.bookmarkable.present? && self.can_see?(bookmark.user.guardian, bookmark) end def self.can_see?(guardian, bookmark) diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 7624f011369..dd066f6bd2a 100644 --- a/lib/bookmark_query.rb +++ b/lib/bookmark_query.rb @@ -88,4 +88,28 @@ class BookmarkQuery BookmarkQuery.preload(results, self) results end + + def unread_notifications(limit: 20) + reminder_notifications = + Notification + .for_user_menu(@user.id, limit: [limit, 100].min) + .unread + .where(notification_type: Notification.types[:bookmark_reminder]) + + # 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, + ) + 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) + end + end end diff --git a/plugins/chat/lib/chat/message_bookmarkable.rb b/plugins/chat/lib/chat/message_bookmarkable.rb index a759aef1209..4a9b2025679 100644 --- a/plugins/chat/lib/chat/message_bookmarkable.rb +++ b/plugins/chat/lib/chat/message_bookmarkable.rb @@ -11,7 +11,7 @@ module Chat end def self.preload_associations - [:chat_channel] + [{ chat_channel: :chatable }] end def self.list_query(user, guardian) @@ -58,7 +58,8 @@ module Chat end def self.reminder_conditions(bookmark) - bookmark.bookmarkable.present? && bookmark.bookmarkable.chat_channel.present? + bookmark.bookmarkable.present? && bookmark.bookmarkable.chat_channel.present? && + self.can_see?(bookmark.user.guardian, bookmark) end def self.can_see?(guardian, bookmark) diff --git a/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb b/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb index f5d050dcf77..d2030004322 100644 --- a/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb +++ b/plugins/chat/spec/lib/chat/message_bookmarkable_spec.rb @@ -119,6 +119,13 @@ describe Chat::MessageBookmarkable do bookmark1.reload expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false) end + + it "cannot send reminder if the user cannot access the channel" do + expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(true) + bookmark1.bookmarkable.update!(chat_channel: Fabricate(:private_category_channel)) + bookmark1.reload + expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false) + end end describe "#reminder_handler" do diff --git a/plugins/chat/spec/requests/core_ext/users_controller_spec.rb b/plugins/chat/spec/requests/core_ext/users_controller_spec.rb index 8e9d060589d..b7ff5445fd2 100644 --- a/plugins/chat/spec/requests/core_ext/users_controller_spec.rb +++ b/plugins/chat/spec/requests/core_ext/users_controller_spec.rb @@ -22,4 +22,33 @@ describe UsersController do expect(membership.following).to eq(true) end end + + describe "#user_menu_bookmarks" do + fab!(:chatters) { Fabricate(:group) } + let(:current_user) { Fabricate(:user, group_ids: [chatters.id]) } + let(:bookmark_message) { Fabricate(:chat_message) } + let(:bookmark_user) { current_user } + + before do + register_test_bookmarkable(Chat::MessageBookmarkable) + SiteSetting.chat_allowed_groups = [chatters] + sign_in(current_user) + end + + it "does not return any unread notifications for chat bookmarks that the user no longer has access to" do + bookmark_with_reminder = + Fabricate(:bookmark, user: current_user, bookmarkable: bookmark_message) + BookmarkReminderNotificationHandler.new(bookmark_with_reminder).send_notification + + bookmark_with_reminder.bookmarkable.update!( + chat_channel: Fabricate(:private_category_channel), + ) + + get "/u/#{current_user.username}/user-menu-bookmarks" + expect(response.status).to eq(200) + + notifications = response.parsed_body["notifications"] + expect(notifications.size).to eq(0) + end + end end diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index d54c10d07d2..3bac9c55ca3 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/spec/lib/bookmark_query_spec.rb @@ -105,6 +105,8 @@ RSpec.describe BookmarkQuery do context "with custom bookmarkable fitering" do before { register_test_bookmarkable } + after { DiscoursePluginRegistry.reset! } + let!(:bookmark5) do Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkking")) end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index caa11464358..60a40f49fed 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -6922,8 +6922,7 @@ RSpec.describe UsersController do expect(response.status).to eq(200) notifications = response.parsed_body["notifications"] - expect(notifications.size).to eq(1) - expect(notifications.first["data"]["bookmark_id"]).to be_nil + expect(notifications.size).to eq(0) bookmarks = response.parsed_body["bookmarks"] expect(bookmarks.map { |bookmark| bookmark["id"] }).to contain_exactly( @@ -6970,6 +6969,24 @@ RSpec.describe UsersController do bookmarks = response.parsed_body["bookmarks"] expect(bookmarks.size).to eq(1) end + + it "does not return any unread notifications for bookmarks that the user no longer has access to" do + bookmark_with_reminder2 = Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) + TopicUser.change(user.id, bookmark_with_reminder2.bookmarkable.topic, total_msecs_viewed: 1) + BookmarkReminderNotificationHandler.new(bookmark_with_reminder2).send_notification + + bookmark_with_reminder2.bookmarkable.topic.update!( + archetype: Archetype.private_message, + category: nil, + ) + + 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 end end diff --git a/spec/services/post_bookmarkable_spec.rb b/spec/services/post_bookmarkable_spec.rb index dbd1d8855ca..234a80eb193 100644 --- a/spec/services/post_bookmarkable_spec.rb +++ b/spec/services/post_bookmarkable_spec.rb @@ -101,6 +101,13 @@ RSpec.describe PostBookmarkable do bookmark1.reload expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false) end + + it "cannot send reminder if the user cannot access the topic" do + expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(true) + bookmark1.bookmarkable.topic.update!(category: private_category) + bookmark1.reload + expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false) + end end describe "#reminder_handler" do diff --git a/spec/services/topic_bookmarkable_spec.rb b/spec/services/topic_bookmarkable_spec.rb index b5aa7685974..594bc6a1182 100644 --- a/spec/services/topic_bookmarkable_spec.rb +++ b/spec/services/topic_bookmarkable_spec.rb @@ -97,6 +97,13 @@ RSpec.describe TopicBookmarkable do bookmark1.reload expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false) end + + it "cannot send reminder if the user cannot access the topic" do + expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(true) + bookmark1.bookmarkable.update!(category: private_category) + bookmark1.reload + expect(registered_bookmarkable.can_send_reminder?(bookmark1)).to eq(false) + end end describe "#reminder_handler" do