diff --git a/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb b/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb index b10083f5ff0..aa8feafb221 100644 --- a/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb +++ b/plugins/chat/app/jobs/regular/chat_notify_mentioned.rb @@ -100,7 +100,7 @@ module Jobs payload end - def create_notification!(membership, notification_data) + def create_notification!(membership, notification_data, mention) is_read = Chat::ChatNotifier.user_has_seen_message?(membership, @chat_message.id) notification = @@ -111,15 +111,15 @@ module Jobs data: notification_data.to_json, read: is_read, ) - ChatMention.create!( - notification: notification, - user: membership.user, - chat_message: @chat_message, - ) + + mention.update!(notification: notification) end def send_notifications(membership, notification_data, os_payload) - create_notification!(membership, notification_data) + mention = ChatMention.find_by(user: membership.user, chat_message: @chat_message) + return if mention.blank? + + create_notification!(membership, notification_data, mention) if !membership.desktop_notifications_never? && !membership.muted? MessageBus.publish( diff --git a/plugins/chat/app/models/chat_message.rb b/plugins/chat/app/models/chat_message.rb index 7f087a3158e..13039d6421d 100644 --- a/plugins/chat/app/models/chat_message.rb +++ b/plugins/chat/app/models/chat_message.rb @@ -226,8 +226,36 @@ class ChatMessage < ActiveRecord::Base "/chat/c/-/#{self.chat_channel_id}/#{self.id}" end + def create_mentions(user_ids) + return if user_ids.empty? + + now = Time.zone.now + mentions = [] + User + .where(id: user_ids) + .find_each do |user| + mentions << { chat_message_id: self.id, user_id: user.id, created_at: now, updated_at: now } + end + + ChatMention.insert_all(mentions) + end + + def update_mentions(mentioned_user_ids) + old_mentions = chat_mentions.pluck(:user_id) + updated_mentions = mentioned_user_ids + mentioned_user_ids_to_drop = old_mentions - updated_mentions + mentioned_user_ids_to_add = updated_mentions - old_mentions + + delete_mentions(mentioned_user_ids_to_drop) + create_mentions(mentioned_user_ids_to_add) + end + private + def delete_mentions(user_ids) + chat_mentions.where(user_id: user_ids).destroy_all + end + def message_too_short? message.length < SiteSetting.chat_minimum_message_length end diff --git a/plugins/chat/db/post_migrate/20230227172543_make_chat_mention_notification_id_nullable.rb b/plugins/chat/db/post_migrate/20230227172543_make_chat_mention_notification_id_nullable.rb new file mode 100644 index 00000000000..b3bbcef23be --- /dev/null +++ b/plugins/chat/db/post_migrate/20230227172543_make_chat_mention_notification_id_nullable.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class MakeChatMentionNotificationIdNullable < ActiveRecord::Migration[7.0] + def up + change_column_null :chat_mentions, :notification_id, true + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/chat/lib/chat_message_mentions.rb b/plugins/chat/lib/chat_message_mentions.rb index 84516466b09..fd032301f89 100644 --- a/plugins/chat/lib/chat_message_mentions.rb +++ b/plugins/chat/lib/chat_message_mentions.rb @@ -18,6 +18,18 @@ class Chat::ChatMessageMentions :parsed_direct_mentions, :parsed_group_mentions + def all_mentioned_users_ids + @all_mentioned_users_ids ||= + begin + user_ids = global_mentions.pluck(:id) + user_ids.concat(direct_mentions.pluck(:id)) + user_ids.concat(group_mentions.pluck(:id)) + user_ids.concat(here_mentions.pluck(:id)) + user_ids.uniq! + user_ids + end + end + def global_mentions return User.none unless @has_global_mention channel_members.where.not(username_lower: @parsed_direct_mentions) @@ -67,7 +79,6 @@ class Chat::ChatMessageMentions .joins("LEFT OUTER JOIN user_chat_channel_memberships uccm ON uccm.user_id = users.id") .joins(:user_option) .real - .not_suspended .where(user_options: { chat_enabled: true }) .where.not(username_lower: @message.user.username.downcase) end diff --git a/plugins/chat/lib/chat_notifier.rb b/plugins/chat/lib/chat_notifier.rb index 7887cfade9a..0f8ffea4e11 100644 --- a/plugins/chat/lib/chat_notifier.rb +++ b/plugins/chat/lib/chat_notifier.rb @@ -66,6 +66,10 @@ class Chat::ChatNotifier ### Public API def notify_new + if @mentions.all_mentioned_users_ids.present? + @chat_message.create_mentions(@mentions.all_mentioned_users_ids) + end + to_notify = list_users_to_notify mentioned_user_ids = to_notify.extract!(:all_mentioned_user_ids)[:all_mentioned_user_ids] @@ -82,6 +86,8 @@ class Chat::ChatNotifier end def notify_edit + @chat_message.update_mentions(@mentions.all_mentioned_users_ids) + existing_notifications = ChatMention.includes(:user, :notification).where(chat_message: @chat_message) already_notified_user_ids = existing_notifications.map(&:user_id) @@ -139,6 +145,7 @@ class Chat::ChatNotifier if has_all_mention && @chat_channel.allow_channel_wide_mentions && !skip to_notify[:global_mentions] = @mentions .global_mentions + .not_suspended .where(user_options: { ignore_channel_wide_mention: [false, nil] }) .where.not(id: already_covered_ids) .pluck(:id) @@ -155,6 +162,7 @@ class Chat::ChatNotifier if has_here_mention && @chat_channel.allow_channel_wide_mentions && !skip to_notify[:here_mentions] = @mentions .here_mentions + .not_suspended .where(user_options: { ignore_channel_wide_mention: [false, nil] }) .where.not(id: already_covered_ids) .pluck(:id) @@ -192,7 +200,7 @@ class Chat::ChatNotifier if skip direct_mentions = [] else - direct_mentions = @mentions.direct_mentions.where.not(id: already_covered_ids) + direct_mentions = @mentions.direct_mentions.not_suspended.where.not(id: already_covered_ids) end grouped = group_users_to_notify(direct_mentions) @@ -209,6 +217,7 @@ class Chat::ChatNotifier reached_by_group = @mentions .group_mentions + .not_suspended .where("user_count <= ?", SiteSetting.max_users_notified_per_group_mention) .where.not(id: already_covered_ids) diff --git a/plugins/chat/spec/components/chat_message_creator_spec.rb b/plugins/chat/spec/components/chat_message_creator_spec.rb index 798149ece86..2c9e01de7e4 100644 --- a/plugins/chat/spec/components/chat_message_creator_spec.rb +++ b/plugins/chat/spec/components/chat_message_creator_spec.rb @@ -140,16 +140,29 @@ describe Chat::ChatMessageCreator do expect(events.map { _1[:event_name] }).to include(:chat_message_created) end - it "creates mention notifications for public chat" do - expect { + it "creates mentions and mention notifications for public chat" do + message = Chat::ChatMessageCreator.create( chat_channel: public_chat_channel, user: user1, content: "this is a @#{user1.username} message with @system @mentions @#{user2.username} and @#{user3.username}", - ) - # Only 2 mentions are created because user mentioned themselves, system, and an invalid username. - }.to change { ChatMention.count }.by(2).and not_change { user1.chat_mentions.count } + ).chat_message + + # a mention for the user himself wasn't created + user1_mention = user1.chat_mentions.where(chat_message: message).first + expect(user1_mention).to be_nil + + system_user_mention = Discourse.system_user.chat_mentions.where(chat_message: message).first + expect(system_user_mention).to be_nil + + user2_mention = user2.chat_mentions.where(chat_message: message).first + expect(user2_mention).to be_present + expect(user2_mention.notification).to be_present + + user3_mention = user3.chat_mentions.where(chat_message: message).first + expect(user3_mention).to be_present + expect(user3_mention.notification).to be_present end it "mentions are case insensitive" do @@ -225,58 +238,88 @@ describe Chat::ChatMessageCreator do end it "doesn't create mention notifications for users without a membership record" do - expect { + message = Chat::ChatMessageCreator.create( chat_channel: public_chat_channel, user: user1, content: "hello @#{user_without_memberships.username}", - ) - }.not_to change { ChatMention.count } + ).chat_message + + mention = user_without_memberships.chat_mentions.where(chat_message: message).first + expect(mention.notification).to be_nil end it "doesn't create mention notifications for users who cannot chat" do new_group = Group.create SiteSetting.chat_allowed_groups = new_group.id - expect { + + message = Chat::ChatMessageCreator.create( chat_channel: public_chat_channel, user: user1, content: "hi @#{user2.username} @#{user3.username}", - ) - }.not_to change { ChatMention.count } + ).chat_message + + user2_mention = user2.chat_mentions.where(chat_message: message).first + expect(user2_mention.notification).to be_nil + + user3_mention = user2.chat_mentions.where(chat_message: message).first + expect(user3_mention.notification).to be_nil end - it "doesn't create mention notifications for users with chat disabled" do + it "doesn't create mentions for users with chat disabled" do user2.user_option.update(chat_enabled: false) - expect { + + message = Chat::ChatMessageCreator.create( chat_channel: public_chat_channel, user: user1, content: "hi @#{user2.username}", - ) - }.not_to change { ChatMention.count } + ).chat_message + + mention = user2.chat_mentions.where(chat_message: message).first + expect(mention).to be_nil end it "creates only mention notifications for users with access in private chat" do - expect { + message = Chat::ChatMessageCreator.create( chat_channel: direct_message_channel, user: user1, content: "hello there @#{user2.username} and @#{user3.username}", - ) - # Only user2 should be notified - }.to change { user2.chat_mentions.count }.by(1).and not_change { user3.chat_mentions.count } + ).chat_message + + # Only user2 should be notified + user2_mention = user2.chat_mentions.where(chat_message: message).first + expect(user2_mention.notification).to be_present + + user3_mention = user3.chat_mentions.where(chat_message: message).first + expect(user3_mention.notification).to be_nil end - it "creates a mention notifications for group users that are participating in private chat" do + it "creates a mention for group users even if they're not participating in private chat" do expect { Chat::ChatMessageCreator.create( chat_channel: direct_message_channel, user: user1, content: "hello there @#{user_group.name}", ) - # Only user2 should be notified - }.to change { user2.chat_mentions.count }.by(1).and not_change { user3.chat_mentions.count } + }.to change { user2.chat_mentions.count }.by(1).and change { user3.chat_mentions.count }.by(1) + end + + it "creates a mention notifications only for group users that are participating in private chat" do + message = + Chat::ChatMessageCreator.create( + chat_channel: direct_message_channel, + user: user1, + content: "hello there @#{user_group.name}", + ).chat_message + + user2_mention = user2.chat_mentions.where(chat_message: message).first + expect(user2_mention.notification).to be_present + + user3_mention = user3.chat_mentions.where(chat_message: message).first + expect(user3_mention.notification).to be_nil end it "publishes inaccessible mentions when user isn't aren't a part of the channel" do @@ -307,51 +350,64 @@ describe Chat::ChatMessageCreator do ) end - it "does not create mentions for suspended users" do + it "creates mentions for suspended users" do user2.update(suspended_till: Time.now + 10.years) - expect { + + message = Chat::ChatMessageCreator.create( chat_channel: direct_message_channel, user: user1, content: "hello @#{user2.username}", - ) - }.not_to change { user2.chat_mentions.count } + ).chat_message + + mention = user2.chat_mentions.where(chat_message: message).first + expect(mention).to be_present end - it "does not create @all mentions for users when ignore_channel_wide_mention is enabled" do - expect { - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "@all", - ) - }.to change { ChatMention.count }.by(4) + it "does not create mention notifications for suspended users" do + user2.update(suspended_till: Time.now + 10.years) - user2.user_option.update(ignore_channel_wide_mention: true) - expect { + message = Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, + chat_channel: direct_message_channel, user: user1, - content: "hi! @all", - ) - }.to change { ChatMention.count }.by(3) + content: "hello @#{user2.username}", + ).chat_message + + mention = user2.chat_mentions.where(chat_message: message).first + expect(mention.notification).to be_nil end - it "does not create @here mentions for users when ignore_channel_wide_mention is enabled" do - admin1.update(last_seen_at: 1.year.ago) - admin2.update(last_seen_at: 1.year.ago) - user1.update(last_seen_at: Time.now) - user2.update(last_seen_at: Time.now) - user2.user_option.update(ignore_channel_wide_mention: true) - user3.update(last_seen_at: Time.now) + context "when ignore_channel_wide_mention is enabled" do + before { user2.user_option.update(ignore_channel_wide_mention: true) } - expect { - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "@here", - ) - }.to change { ChatMention.count }.by(1) + it "when mentioning @all creates a mention without notification" do + message = + Chat::ChatMessageCreator.create( + chat_channel: public_chat_channel, + user: user1, + content: "hi! @all", + ).chat_message + + mention = user2.chat_mentions.where(chat_message: message).first + expect(mention).to be_present + expect(mention.notification).to be_nil + end + + it "when mentioning @here creates a mention without notification" do + user2.update(last_seen_at: Time.now) + + message = + Chat::ChatMessageCreator.create( + chat_channel: public_chat_channel, + user: user1, + content: "@here", + ).chat_message + + mention = user2.chat_mentions.where(chat_message: message).first + expect(mention).to be_present + expect(mention.notification).to be_nil + end end describe "replies" do diff --git a/plugins/chat/spec/components/chat_message_updater_spec.rb b/plugins/chat/spec/components/chat_message_updater_spec.rb index 62de1bd9e3f..edb101a3afa 100644 --- a/plugins/chat/spec/components/chat_message_updater_spec.rb +++ b/plugins/chat/spec/components/chat_message_updater_spec.rb @@ -150,17 +150,18 @@ describe Chat::ChatMessageUpdater do }.not_to change { ChatMention.count } end - it "doesn't create mentions for users without access" do + it "doesn't create mention notification for users without access" do message = "ping" chat_message = create_chat_message(user1, message, public_chat_channel) - expect { - Chat::ChatMessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: message + " @#{user_without_memberships.username}", - ) - }.not_to change { ChatMention.count } + Chat::ChatMessageUpdater.update( + guardian: guardian, + chat_message: chat_message, + new_content: message + " @#{user_without_memberships.username}", + ) + + mention = user_without_memberships.chat_mentions.where(chat_message: chat_message).first + expect(mention.notification).to be_nil end it "destroys mention notifications that should be removed" do @@ -189,15 +190,17 @@ describe Chat::ChatMessageUpdater do expect(user4.chat_mentions.where(chat_message: chat_message)).to be_present end - it "does not create new mentions in direct message for users who don't have access" do - chat_message = create_chat_message(user1, "ping nobody", @direct_message_channel) - expect { - Chat::ChatMessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "ping @#{admin1.username}", - ) - }.not_to change { ChatMention.count } + it "doesn't create mention notification in direct message for users without access" do + message = create_chat_message(user1, "ping nobody", @direct_message_channel) + + Chat::ChatMessageUpdater.update( + guardian: guardian, + chat_message: message, + new_content: "ping @#{admin1.username}", + ) + + mention = admin1.chat_mentions.where(chat_message: message).first + expect(mention.notification).to be_nil end describe "group mentions" do diff --git a/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb b/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb index 99db2226233..e411d5b052a 100644 --- a/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb @@ -21,8 +21,11 @@ describe Jobs::ChatNotifyMentioned do end end - def create_chat_message(channel: public_channel, user: user_1) - Fabricate(:chat_message, chat_channel: channel, user: user, created_at: 10.minutes.ago) + def create_chat_message(channel: public_channel, author: user_1, mentioned_user: user_2) + message = + Fabricate(:chat_message, chat_channel: channel, user: author, created_at: 10.minutes.ago) + Fabricate(:chat_mention, chat_message: message, user: mentioned_user) + message end def track_desktop_notification( diff --git a/plugins/chat/spec/lib/chat_message_mentions_spec.rb b/plugins/chat/spec/lib/chat_message_mentions_spec.rb index 7ff25c873dd..dd2480d3e0a 100644 --- a/plugins/chat/spec/lib/chat_message_mentions_spec.rb +++ b/plugins/chat/spec/lib/chat_message_mentions_spec.rb @@ -139,7 +139,18 @@ RSpec.describe Chat::ChatMessageMentions do end it "returns an empty list if no group was mentioned" do - message = create_message("not mentioning anybody") + message = create_message("not mentioning anyone") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.group_mentions.pluck(:username) + + expect(result).to be_empty + end + + it "returns an empty list when mentioning an unmentionable group" do + group1.mentionable_level = Group::ALIAS_LEVELS[:nobody] + group1.save! + message = create_message("mentioning @#{group1.name}") mentions = Chat::ChatMessageMentions.new(message) result = mentions.group_mentions.pluck(:username) diff --git a/plugins/chat/spec/lib/chat_notifier_spec.rb b/plugins/chat/spec/lib/chat_notifier_spec.rb index e9f413cfd68..75b008a6209 100644 --- a/plugins/chat/spec/lib/chat_notifier_spec.rb +++ b/plugins/chat/spec/lib/chat_notifier_spec.rb @@ -23,7 +23,7 @@ describe Chat::ChatNotifier do end def build_cooked_msg(message_body, user, chat_channel: channel) - ChatMessage.new( + ChatMessage.create( chat_channel: chat_channel, user: user, message: message_body, diff --git a/plugins/chat/spec/models/chat_message_spec.rb b/plugins/chat/spec/models/chat_message_spec.rb index 7d16da0ebb2..5f2d5c21509 100644 --- a/plugins/chat/spec/models/chat_message_spec.rb +++ b/plugins/chat/spec/models/chat_message_spec.rb @@ -570,6 +570,68 @@ describe ChatMessage do end end + describe "#create_mentions" do + fab!(:message) { Fabricate(:chat_message) } + fab!(:user1) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + + it "creates mentions for passed user ids" do + mentioned_user_ids = [user1.id, user2.id] + message.create_mentions(mentioned_user_ids) + message.reload + + expect(message.chat_mentions.pluck(:user_id)).to match_array(mentioned_user_ids) + end + + it "ignores duplicates in passed user ids" do + mentioned_user_ids = [user1.id, user1.id, user1.id, user1.id, user1.id] + message.create_mentions(mentioned_user_ids) + message.reload + + expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id) + end + end + + describe "#update_mentions" do + fab!(:message) { Fabricate(:chat_message) } + fab!(:user1) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + fab!(:user3) { Fabricate(:user) } + fab!(:user4) { Fabricate(:user) } + let(:already_mentioned) { [user1.id, user2.id] } + + before { message.create_mentions(already_mentioned) } + + it "creates newly added mentions" do + existing_mention_ids = message.chat_mentions.pluck(:id) + + mentioned_user_ids = [*already_mentioned, user3.id, user4.id] + message.update_mentions(mentioned_user_ids) + message.reload + + expect(message.chat_mentions.pluck(:user_id)).to match_array(mentioned_user_ids) + expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # existing mentions weren't recreated + end + + it "drops removed mentions" do + message.update_mentions([user1.id]) # user 2 is not mentioned anymore + message.reload + + expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id) + end + + it "changes nothing if passed mentions are identical to existing mentions" do + existing_mention_ids = message.chat_mentions.pluck(:id) + + mentioned_user_ids = [*already_mentioned] + message.update_mentions(mentioned_user_ids) + message.reload + + expect(message.chat_mentions.pluck(:user_id)).to match_array(mentioned_user_ids) + expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated + end + end + # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01 def chat_upload_count(uploads = nil) return DB.query_single("SELECT COUNT(*) FROM chat_uploads").first if !uploads