diff --git a/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb b/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb index bfbd925ff5d..9e101aa53b4 100644 --- a/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb +++ b/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb @@ -145,13 +145,43 @@ module Jobs memberships = get_memberships(user_ids) memberships.each do |membership| - mention = ::Chat::Mention.find_by(user: membership.user, chat_message: @chat_message) + mention = find_mention(@chat_message, mention_type, membership.user.id) if mention.present? create_notification!(membership, mention, mention_type) send_notifications(membership, mention_type) end end end + + def find_mention(chat_message, mention_type, user_id) + mention_klass = resolve_mention_klass(mention_type) + + target_id = nil + if mention_klass == ::Chat::UserMention + target_id = user_id + elsif mention_klass == ::Chat::GroupMention + begin + target_id = Group.where("LOWER(name) = ?", "#{mention_type}").first.id + rescue => e + Discourse.warn_exception(e, message: "Mentioned group doesn't exist") + end + end + + mention_klass.find_by(chat_message: chat_message, target_id: target_id) + end + + def resolve_mention_klass(mention_type) + case mention_type + when :global_mentions + ::Chat::AllMention + when :here_mentions + ::Chat::HereMention + when :direct_mentions + ::Chat::UserMention + else + ::Chat::GroupMention + end + end end end end diff --git a/plugins/chat/app/models/chat/all_mention.rb b/plugins/chat/app/models/chat/all_mention.rb new file mode 100644 index 00000000000..01a1c4c7295 --- /dev/null +++ b/plugins/chat/app/models/chat/all_mention.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +module Chat + class AllMention < Mention + end +end diff --git a/plugins/chat/app/models/chat/group_mention.rb b/plugins/chat/app/models/chat/group_mention.rb new file mode 100644 index 00000000000..5a64237035a --- /dev/null +++ b/plugins/chat/app/models/chat/group_mention.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module Chat + class GroupMention < Mention + belongs_to :group, foreign_key: :target_id + end +end diff --git a/plugins/chat/app/models/chat/here_mention.rb b/plugins/chat/app/models/chat/here_mention.rb new file mode 100644 index 00000000000..ca41d58ec6b --- /dev/null +++ b/plugins/chat/app/models/chat/here_mention.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +module Chat + class HereMention < Mention + end +end diff --git a/plugins/chat/app/models/chat/mention.rb b/plugins/chat/app/models/chat/mention.rb index 9e89dd23b3e..51fc6450724 100644 --- a/plugins/chat/app/models/chat/mention.rb +++ b/plugins/chat/app/models/chat/mention.rb @@ -3,9 +3,8 @@ module Chat class Mention < ActiveRecord::Base self.table_name = "chat_mentions" - self.ignored_columns = ["notification_id"] + self.ignored_columns = %w[notification_id user_id] - belongs_to :user belongs_to :chat_message, class_name: "Chat::Message" has_many :mention_notifications, class_name: "Chat::MentionNotification", @@ -20,12 +19,15 @@ end # # id :bigint not null, primary key # chat_message_id :integer not null -# user_id :integer not null +# user_id :integer # notification_id :integer not null +# target_id :integer +# type :integer not null # created_at :datetime not null # updated_at :datetime not null # # Indexes # -# chat_mentions_index (chat_message_id,user_id,notification_id) UNIQUE +# index_chat_mentions_on_chat_message_id (chat_message_id) +# index_chat_mentions_on_target_id (target_id) # diff --git a/plugins/chat/app/models/chat/message.rb b/plugins/chat/app/models/chat/message.rb index 5fa0eaa987e..8fd52263378 100644 --- a/plugins/chat/app/models/chat/message.rb +++ b/plugins/chat/app/models/chat/message.rb @@ -51,6 +51,22 @@ module Chat dependent: :destroy, class_name: "Chat::Mention", foreign_key: :chat_message_id + has_many :user_mentions, + dependent: :destroy, + class_name: "Chat::UserMention", + foreign_key: :chat_message_id + has_many :group_mentions, + dependent: :destroy, + class_name: "Chat::GroupMention", + foreign_key: :chat_message_id + has_one :all_mention, + dependent: :destroy, + class_name: "Chat::AllMention", + foreign_key: :chat_message_id + has_one :here_mention, + dependent: :destroy, + class_name: "Chat::HereMention", + foreign_key: :chat_message_id scope :in_public_channel, -> do @@ -248,14 +264,10 @@ module Chat end def upsert_mentions - mentioned_user_ids = parsed_mentions.all_mentioned_users_ids - old_mentions = chat_mentions.pluck(:user_id) - - mentioned_user_ids_to_drop = old_mentions - mentioned_user_ids - delete_mentions(mentioned_user_ids_to_drop) - - mentioned_user_ids_to_add = mentioned_user_ids - old_mentions - insert_mentions(mentioned_user_ids_to_add) + upsert_user_mentions + upsert_group_mentions + create_or_delete_all_mention + create_or_delete_here_mention end def in_thread? @@ -280,24 +292,16 @@ module Chat private - def delete_mentions(user_ids) - chat_mentions.where(user_id: user_ids).destroy_all + def delete_mentions(mention_type, target_ids) + chat_mentions.where(type: mention_type, target_id: target_ids).destroy_all end - def insert_mentions(user_ids) - return if user_ids.empty? + def insert_mentions(type, target_ids) + return if target_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, - } + mentions = + target_ids.map do |target_id| + { chat_message_id: self.id, target_id: target_id, type: type } end Chat::Mention.insert_all(mentions) @@ -314,6 +318,38 @@ module Chat def ensure_last_editor_id self.last_editor_id ||= self.user_id end + + def create_or_delete_all_mention + if !parsed_mentions.has_global_mention && all_mention.present? + all_mention.destroy! + association(:all_mention).reload + elsif parsed_mentions.has_global_mention && all_mention.blank? + build_all_mention.save! + end + end + + def create_or_delete_here_mention + if !parsed_mentions.has_here_mention && here_mention.present? + here_mention.destroy! + association(:here_mention).reload + elsif parsed_mentions.has_here_mention && here_mention.blank? + build_here_mention.save! + end + end + + def upsert_group_mentions + old_mentions = group_mentions.pluck(:target_id) + new_mentions = parsed_mentions.groups_to_mention.pluck(:id) + delete_mentions("Chat::GroupMention", old_mentions - new_mentions) + insert_mentions("Chat::GroupMention", new_mentions - old_mentions) + end + + def upsert_user_mentions + old_mentions = user_mentions.pluck(:target_id) + new_mentions = parsed_mentions.direct_mentions.pluck(:id) + delete_mentions("Chat::UserMention", old_mentions - new_mentions) + insert_mentions("Chat::UserMention", new_mentions - old_mentions) + end end end diff --git a/plugins/chat/app/models/chat/user_mention.rb b/plugins/chat/app/models/chat/user_mention.rb new file mode 100644 index 00000000000..b1aa75a2b57 --- /dev/null +++ b/plugins/chat/app/models/chat/user_mention.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module Chat + class UserMention < Mention + belongs_to :user, foreign_key: :target_id + end +end diff --git a/plugins/chat/app/queries/chat/messages_query.rb b/plugins/chat/app/queries/chat/messages_query.rb index 8a888ec6a22..55b34889faf 100644 --- a/plugins/chat/app/queries/chat/messages_query.rb +++ b/plugins/chat/app/queries/chat/messages_query.rb @@ -87,9 +87,9 @@ module Chat if SiteSetting.enable_user_status query = query.includes(user: :user_status) - query = query.includes(chat_mentions: { user: :user_status }) + query = query.includes(user_mentions: { user: :user_status }) else - query = query.includes(chat_mentions: :user) + query = query.includes(user_mentions: :user) end query diff --git a/plugins/chat/app/serializers/chat/message_serializer.rb b/plugins/chat/app/serializers/chat/message_serializer.rb index b2fc876f8d5..75ebbe0ce96 100644 --- a/plugins/chat/app/serializers/chat/message_serializer.rb +++ b/plugins/chat/app/serializers/chat/message_serializer.rb @@ -36,7 +36,7 @@ module Chat def mentioned_users object - .chat_mentions + .user_mentions .limit(SiteSetting.max_mentions_per_chat_message) .map(&:user) .compact diff --git a/plugins/chat/app/serializers/chat/thread_original_message_serializer.rb b/plugins/chat/app/serializers/chat/thread_original_message_serializer.rb index cef872f1425..1afcca20799 100644 --- a/plugins/chat/app/serializers/chat/thread_original_message_serializer.rb +++ b/plugins/chat/app/serializers/chat/thread_original_message_serializer.rb @@ -17,7 +17,7 @@ module Chat def mentioned_users object - .chat_mentions + .user_mentions .limit(SiteSetting.max_mentions_per_chat_message) .map(&:user) .compact diff --git a/plugins/chat/db/migrate/20231227160001_add_type_and_target_id_to_chat_mentions.rb b/plugins/chat/db/migrate/20231227160001_add_type_and_target_id_to_chat_mentions.rb new file mode 100644 index 00000000000..9530e2222e2 --- /dev/null +++ b/plugins/chat/db/migrate/20231227160001_add_type_and_target_id_to_chat_mentions.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddTypeAndTargetIdToChatMentions < ActiveRecord::Migration[7.0] + def up + add_column :chat_mentions, :type, :string, null: true + add_column :chat_mentions, :target_id, :integer, null: true + change_column_null :chat_mentions, :user_id, true + end + + def down + change_column_null :chat_mentions, :user_id, false + remove_column :chat_mentions, :target_id + remove_column :chat_mentions, :type + end +end diff --git a/plugins/chat/db/migrate/20231227160002_set_type_and_target_id_on_chat_mentions.rb b/plugins/chat/db/migrate/20231227160002_set_type_and_target_id_on_chat_mentions.rb new file mode 100644 index 00000000000..778485c7f01 --- /dev/null +++ b/plugins/chat/db/migrate/20231227160002_set_type_and_target_id_on_chat_mentions.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class SetTypeAndTargetIdOnChatMentions < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + BATCH_SIZE = 5000 + + def up + begin + updated_count = DB.exec(<<~SQL, batch_size: BATCH_SIZE) + WITH cte AS (SELECT id, user_id + FROM chat_mentions + WHERE type IS NULL AND target_id IS NULL + LIMIT :batch_size) + UPDATE chat_mentions + SET type = 'Chat::UserMention', target_id = cte.user_id + FROM cte + WHERE chat_mentions.id = cte.id; + SQL + end while updated_count > 0 + end + + def down + end +end diff --git a/plugins/chat/db/migrate/20231227160003_add_and_remove_indexes_on_chat_mentions.rb b/plugins/chat/db/migrate/20231227160003_add_and_remove_indexes_on_chat_mentions.rb new file mode 100644 index 00000000000..f16e2736357 --- /dev/null +++ b/plugins/chat/db/migrate/20231227160003_add_and_remove_indexes_on_chat_mentions.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class AddAndRemoveIndexesOnChatMentions < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + def up + remove_index :chat_mentions, + name: :chat_mentions_index, + algorithm: :concurrently, + if_exists: true + add_index :chat_mentions, %i[chat_message_id], algorithm: :concurrently + add_index :chat_mentions, %i[target_id], algorithm: :concurrently + end + + def down + remove_index :chat_mentions, %i[target_id], algorithm: :concurrently, if_exists: true + remove_index :chat_mentions, %i[chat_message_id], algorithm: :concurrently, if_exists: true + add_index :chat_mentions, + %i[chat_message_id user_id notification_id], + unique: true, + name: "chat_mentions_index", + algorithm: :concurrently + end +end diff --git a/plugins/chat/db/post_migrate/20231227160004_set_type_and_target_id_on_chat_mentions_post_migrate.rb b/plugins/chat/db/post_migrate/20231227160004_set_type_and_target_id_on_chat_mentions_post_migrate.rb new file mode 100644 index 00000000000..f0825fe0912 --- /dev/null +++ b/plugins/chat/db/post_migrate/20231227160004_set_type_and_target_id_on_chat_mentions_post_migrate.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class SetTypeAndTargetIdOnChatMentionsPostMigrate < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + BATCH_SIZE = 5000 + + def up + # we're setting it again in post-migration + # in case some mentions have been created after we run + # this query the first time in the regular migration + begin + updated_count = DB.exec(<<~SQL, batch_size: BATCH_SIZE) + WITH cte AS (SELECT id, user_id + FROM chat_mentions + WHERE type IS NULL AND target_id IS NULL + LIMIT :batch_size) + UPDATE chat_mentions + SET type = 'Chat::UserMention', target_id = cte.user_id + FROM cte + WHERE chat_mentions.id = cte.id; + SQL + end while updated_count > 0 + end + + def down + end +end diff --git a/plugins/chat/db/post_migrate/20231227160005_make_type_on_chat_mentions_non_nullable.rb b/plugins/chat/db/post_migrate/20231227160005_make_type_on_chat_mentions_non_nullable.rb new file mode 100644 index 00000000000..ffef281519f --- /dev/null +++ b/plugins/chat/db/post_migrate/20231227160005_make_type_on_chat_mentions_non_nullable.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class MakeTypeOnChatMentionsNonNullable < ActiveRecord::Migration[7.0] + def up + change_column_null :chat_mentions, :type, false + end + + def down + change_column_null :chat_mentions, :type, true + end +end diff --git a/plugins/chat/lib/chat/group_extension.rb b/plugins/chat/lib/chat/group_extension.rb new file mode 100644 index 00000000000..b2af954de8e --- /dev/null +++ b/plugins/chat/lib/chat/group_extension.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Chat + module GroupExtension + extend ActiveSupport::Concern + + prepended do + has_many :chat_mentions, class_name: "Chat::GroupMention", foreign_key: "target_id" + end + end +end diff --git a/plugins/chat/lib/chat/mailer.rb b/plugins/chat/lib/chat/mailer.rb index f6a2579af1e..1be1ae3259e 100644 --- a/plugins/chat/lib/chat/mailer.rb +++ b/plugins/chat/lib/chat/mailer.rb @@ -52,13 +52,17 @@ module Chat .joins("INNER JOIN chat_channels cc ON cc.id = uccm.chat_channel_id") .joins("INNER JOIN chat_messages c_msg ON c_msg.chat_channel_id = uccm.chat_channel_id") .joins("LEFT OUTER JOIN chat_mentions c_mentions ON c_mentions.chat_message_id = c_msg.id") + .joins( + "LEFT OUTER JOIN chat_mention_notifications cmn ON cmn.chat_mention_id = c_mentions.id", + ) + .joins("LEFT OUTER JOIN notifications n ON cmn.notification_id = n.id") .where("c_msg.deleted_at IS NULL AND c_msg.user_id <> users.id") .where("c_msg.created_at > ?", 1.week.ago) .where(<<~SQL) (uccm.last_read_message_id IS NULL OR c_msg.id > uccm.last_read_message_id) AND (uccm.last_unread_mention_when_emailed_id IS NULL OR c_msg.id > uccm.last_unread_mention_when_emailed_id) AND ( - (uccm.user_id = c_mentions.user_id AND uccm.following IS true AND cc.chatable_type = 'Category') OR + (uccm.user_id = n.user_id AND uccm.following IS true AND cc.chatable_type = 'Category') OR (cc.chatable_type = 'DirectMessage') ) SQL diff --git a/plugins/chat/lib/chat/notifier.rb b/plugins/chat/lib/chat/notifier.rb index fae9241230c..a671b43e835 100644 --- a/plugins/chat/lib/chat/notifier.rb +++ b/plugins/chat/lib/chat/notifier.rb @@ -83,10 +83,15 @@ module Chat def notify_edit already_notified_user_ids = - Chat::Mention - .where(chat_message: @chat_message) - .left_outer_joins(:notifications) - .where.not(notifications: { id: nil }) + Notification + .where(notification_type: Notification.types[:chat_mention]) + .joins( + "INNER JOIN chat_mention_notifications ON chat_mention_notifications.notification_id = notifications.id", + ) + .joins( + "INNER JOIN chat_mentions ON chat_mentions.id = chat_mention_notifications.chat_mention_id", + ) + .where("chat_mentions.chat_message_id = ?", @chat_message.id) .pluck(:user_id) to_notify, inaccessible, all_mentioned_user_ids = list_users_to_notify diff --git a/plugins/chat/lib/chat/parsed_mentions.rb b/plugins/chat/lib/chat/parsed_mentions.rb index b3f80ce6eb9..20b04f1e2c3 100644 --- a/plugins/chat/lib/chat/parsed_mentions.rb +++ b/plugins/chat/lib/chat/parsed_mentions.rb @@ -19,18 +19,6 @@ module Chat :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 count @count ||= begin diff --git a/plugins/chat/lib/chat/user_extension.rb b/plugins/chat/lib/chat/user_extension.rb index 366b5b26243..80734442878 100644 --- a/plugins/chat/lib/chat/user_extension.rb +++ b/plugins/chat/lib/chat/user_extension.rb @@ -12,7 +12,7 @@ module Chat class_name: "Chat::UserChatThreadMembership", dependent: :destroy has_many :chat_message_reactions, class_name: "Chat::MessageReaction", dependent: :destroy - has_many :chat_mentions, class_name: "Chat::Mention" + has_many :chat_mentions, class_name: "Chat::UserMention", foreign_key: "target_id" has_many :direct_message_users, class_name: "Chat::DirectMessageUser" has_many :direct_messages, through: :direct_message_users, class_name: "Chat::DirectMessage" end diff --git a/plugins/chat/lib/chat/user_notifications_extension.rb b/plugins/chat/lib/chat/user_notifications_extension.rb index ec1907feec4..e9070f96ef3 100644 --- a/plugins/chat/lib/chat/user_notifications_extension.rb +++ b/plugins/chat/lib/chat/user_notifications_extension.rb @@ -13,6 +13,7 @@ module Chat .where("chat_messages.created_at > ?", 1.week.ago) .joins("LEFT OUTER JOIN chat_mentions cm ON cm.chat_message_id = chat_messages.id") .joins("LEFT OUTER JOIN chat_mention_notifications cmn ON cmn.chat_mention_id = cm.id") + .joins("LEFT OUTER JOIN notifications n ON cmn.notification_id = n.id") .joins( "INNER JOIN user_chat_channel_memberships uccm ON uccm.chat_channel_id = chat_channels.id", ) @@ -21,7 +22,7 @@ module Chat (uccm.last_read_message_id IS NULL OR chat_messages.id > uccm.last_read_message_id) AND (uccm.last_unread_mention_when_emailed_id IS NULL OR chat_messages.id > uccm.last_unread_mention_when_emailed_id) AND ( - (cm.user_id = :user_id AND cmn.notification_id IS NOT NULL AND uccm.following IS true AND chat_channels.chatable_type = 'Category') OR + (n.user_id = :user_id AND cmn.notification_id IS NOT NULL AND uccm.following IS true AND chat_channels.chatable_type = 'Category') OR (chat_channels.chatable_type = 'DirectMessage') ) SQL diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 0013b7f5e92..d9ff7bf0365 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -66,6 +66,7 @@ after_initialize do Reviewable.prepend Chat::ReviewableExtension Bookmark.prepend Chat::BookmarkExtension User.prepend Chat::UserExtension + Group.prepend Chat::GroupExtension Jobs::UserEmail.prepend Chat::UserEmailExtension Plugin::Instance.prepend Chat::PluginInstanceExtension Jobs::ExportCsvFile.class_eval { prepend Chat::MessagesExporter } diff --git a/plugins/chat/spec/components/chat/mailer_spec.rb b/plugins/chat/spec/components/chat/mailer_spec.rb index dec8390e20a..0d0e8add08a 100644 --- a/plugins/chat/spec/components/chat/mailer_spec.rb +++ b/plugins/chat/spec/components/chat/mailer_spec.rb @@ -46,7 +46,12 @@ describe Chat::Mailer do end describe "for chat mentions" do - fab!(:mention) { Fabricate(:chat_mention, user: user_1, chat_message: chat_message) } + fab!(:notification) do + Fabricate(:notification, notification_type: Notification.types[:chat_mention], user: user_1) + end + fab!(:mention) do + Fabricate(:user_chat_mention, chat_message: chat_message, notifications: [notification]) + end it "skips users without chat access" do chatters_group.remove(user_1) @@ -157,7 +162,14 @@ describe Chat::Mailer do last_unread_mention_when_emailed_id: chat_message.id, ) unread_message = Fabricate(:chat_message, chat_channel: chat_channel, user: sender) - Fabricate(:chat_mention, user: user_1, chat_message: unread_message) + notification_2 = + Fabricate(:notification, notification_type: Notification.types[:chat_mention], user: user_1) + Fabricate( + :user_chat_mention, + user: user_1, + chat_message: unread_message, + notifications: [notification_2], + ) described_class.send_unread_mentions_summary @@ -183,7 +195,14 @@ describe Chat::Mailer do last_read_message_id: nil, ) new_message = Fabricate(:chat_message, chat_channel: chat_channel, user: sender) - Fabricate(:chat_mention, user: user_2, chat_message: new_message) + notification_2 = + Fabricate(:notification, notification_type: Notification.types[:chat_mention], user: user_2) + Fabricate( + :user_chat_mention, + user: user_2, + chat_message: new_message, + notifications: [notification_2], + ) described_class.send_unread_mentions_summary @@ -222,7 +241,7 @@ describe Chat::Mailer do ) another_channel_message = Fabricate(:chat_message, chat_channel: chat_channel, user: sender) - Fabricate(:chat_mention, user: user_1, chat_message: another_channel_message) + Fabricate(:user_chat_mention, user: user_1, chat_message: another_channel_message) expect { described_class.send_unread_mentions_summary }.not_to change( Jobs::UserEmail.jobs, @@ -234,7 +253,7 @@ describe Chat::Mailer do another_channel = Fabricate(:category_channel) another_channel_message = Fabricate(:chat_message, chat_channel: another_channel, user: sender) - Fabricate(:chat_mention, user: user_1, chat_message: another_channel_message) + Fabricate(:user_chat_mention, user: user_1, chat_message: another_channel_message) another_channel_membership = Fabricate( :user_chat_channel_membership, @@ -264,7 +283,7 @@ describe Chat::Mailer do end it "only queues the job once when the user has mentions and private messages" do - Fabricate(:chat_mention, user: user_1, chat_message: chat_message) + Fabricate(:user_chat_mention, user: user_1, chat_message: chat_message) described_class.send_unread_mentions_summary @@ -280,7 +299,7 @@ describe Chat::Mailer do chat_channel: chat_channel, last_read_message_id: chat_message.id, ) - Fabricate(:chat_mention, user: user_2, chat_message: chat_message) + Fabricate(:user_chat_mention, user: user_2, chat_message: chat_message) described_class.send_unread_mentions_summary diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index ff1e9a2f74d..d2fb91c3c6e 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -115,7 +115,7 @@ Fabricator(:chat_message_with_service, class_name: "Chat::CreateMessage") do end end -Fabricator(:chat_mention, class_name: "Chat::Mention") do +Fabricator(:user_chat_mention, class_name: "Chat::UserMention") do transient read: false transient high_priority: true transient identifier: :direct_mentions @@ -124,6 +124,19 @@ Fabricator(:chat_mention, class_name: "Chat::Mention") do chat_message { Fabricate(:chat_message) } end +Fabricator(:group_chat_mention, class_name: "Chat::GroupMention") do + chat_message { Fabricate(:chat_message) } + group { Fabricate(:group) } +end + +Fabricator(:all_chat_mention, class_name: "Chat::AllMention") do + chat_message { Fabricate(:chat_message) } +end + +Fabricator(:here_chat_mention, class_name: "Chat::HereMention") do + chat_message { Fabricate(:chat_message) } +end + Fabricator(:chat_message_reaction, class_name: "Chat::MessageReaction") do chat_message { Fabricate(:chat_message) } user { Fabricate(:user) } diff --git a/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb b/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb index 6e48a4e0f92..0b441276dac 100644 --- a/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/channel_delete_spec.rb @@ -23,7 +23,7 @@ describe Jobs::Chat::ChannelDelete do UploadReference.create(target: message, upload: upload) end - Chat::Mention.create( + Chat::UserMention.create( user: user2, chat_message: messages.sample, notifications: [Fabricate(:notification)], 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 3cde7483515..878e9e07494 100644 --- a/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb @@ -44,7 +44,7 @@ describe Jobs::Chat::NotifyMentioned do created_at: 10.minutes.ago, thread: thread, ) - Fabricate(:chat_mention, chat_message: message, user: mentioned_user) + Fabricate(:user_chat_mention, chat_message: message, user: mentioned_user) message end @@ -226,6 +226,9 @@ describe Jobs::Chat::NotifyMentioned do it "works for desktop notifications" do message = create_chat_message + Fabricate(:all_chat_mention, chat_message: message) + Fabricate(:here_chat_mention, chat_message: message) + Fabricate(:group_chat_mention, group: @chat_group, chat_message: message) desktop_notification = track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -244,6 +247,9 @@ describe Jobs::Chat::NotifyMentioned do it "works for push notifications" do message = create_chat_message + Fabricate(:all_chat_mention, chat_message: message) + Fabricate(:here_chat_mention, chat_message: message) + Fabricate(:group_chat_mention, group: @chat_group, chat_message: message) PostAlerter.expects(:push_notification).with( user_2, @@ -266,6 +272,9 @@ describe Jobs::Chat::NotifyMentioned do it "works for core notifications" do message = create_chat_message + Fabricate(:all_chat_mention, chat_message: message) + Fabricate(:here_chat_mention, chat_message: message) + Fabricate(:group_chat_mention, group: @chat_group, chat_message: message) created_notification = track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -302,6 +311,7 @@ describe Jobs::Chat::NotifyMentioned do it "includes global mention specific data to core notifications" do message = create_chat_message + Fabricate(:all_chat_mention, chat_message: message) created_notification = track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -313,6 +323,7 @@ describe Jobs::Chat::NotifyMentioned do it "includes global mention specific data to desktop notifications" do message = create_chat_message + Fabricate(:all_chat_mention, chat_message: message) desktop_notification = track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -323,6 +334,7 @@ describe Jobs::Chat::NotifyMentioned do context "with private channels" do it "users a different translated title" do message = create_chat_message(channel: @personal_chat_channel) + Fabricate(:all_chat_mention, chat_message: message) desktop_notification = track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -355,6 +367,7 @@ describe Jobs::Chat::NotifyMentioned do it "includes here mention specific data to core notifications" do message = create_chat_message + Fabricate(:here_chat_mention, chat_message: message) created_notification = track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -365,6 +378,7 @@ describe Jobs::Chat::NotifyMentioned do it "includes here mention specific data to desktop notifications" do message = create_chat_message + Fabricate(:here_chat_mention, chat_message: message) desktop_notification = track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -373,8 +387,9 @@ describe Jobs::Chat::NotifyMentioned do end context "with private channels" do - it "users a different translated title" do + it "uses a different translated title" do message = create_chat_message(channel: @personal_chat_channel) + Fabricate(:here_chat_mention, chat_message: message) desktop_notification = track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -479,6 +494,7 @@ describe Jobs::Chat::NotifyMentioned do it "includes here mention specific data to core notifications" do message = create_chat_message + Fabricate(:group_chat_mention, group: @chat_group, chat_message: message) created_notification = track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -490,6 +506,7 @@ describe Jobs::Chat::NotifyMentioned do it "includes here mention specific data to desktop notifications" do message = create_chat_message + Fabricate(:group_chat_mention, group: @chat_group, chat_message: message) desktop_notification = track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) @@ -498,8 +515,9 @@ describe Jobs::Chat::NotifyMentioned do end context "with private channels" do - it "users a different translated title" do + it "uses a different translated title" do message = create_chat_message(channel: @personal_chat_channel) + Fabricate(:group_chat_mention, group: @chat_group, chat_message: message) desktop_notification = track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map) diff --git a/plugins/chat/spec/lib/chat/message_mover_spec.rb b/plugins/chat/spec/lib/chat/message_mover_spec.rb index 41a2f3e59d2..754d6370c82 100644 --- a/plugins/chat/spec/lib/chat/message_mover_spec.rb +++ b/plugins/chat/spec/lib/chat/message_mover_spec.rb @@ -116,7 +116,7 @@ describe Chat::MessageMover do it "updates references for reactions, uploads, revisions, mentions, etc." do reaction = Fabricate(:chat_message_reaction, chat_message: message1) upload = Fabricate(:upload_reference, target: message1) - mention = Fabricate(:chat_mention, chat_message: message2, user: acting_user) + mention = Fabricate(:user_chat_mention, chat_message: message2, user: acting_user) revision = Fabricate(:chat_message_revision, chat_message: message3) webhook_event = Fabricate(:chat_webhook_event, chat_message: message3) move! diff --git a/plugins/chat/spec/mailers/user_notifications_spec.rb b/plugins/chat/spec/mailers/user_notifications_spec.rb index 80e2f914d9e..d5d37661d7f 100644 --- a/plugins/chat/spec/mailers/user_notifications_spec.rb +++ b/plugins/chat/spec/mailers/user_notifications_spec.rb @@ -252,9 +252,9 @@ describe UserNotifications do describe "email subject" do context "with regular mentions" do before do - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: chat_message, notifications: [notification], @@ -305,9 +305,9 @@ describe UserNotifications do user: user, last_read_message_id: another_chat_message.id - 2, ) - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: another_chat_message, notifications: [notification], @@ -345,9 +345,9 @@ describe UserNotifications do user: user, last_read_message_id: another_chat_message.id - 2, ) - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: another_chat_message, notifications: [notification], @@ -374,9 +374,9 @@ describe UserNotifications do refresh_auto_groups channel = create_dm_channel(sender, [sender, user]) Fabricate(:chat_message, user: sender, chat_channel: channel) - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: chat_message, notifications: [notification], @@ -401,9 +401,9 @@ describe UserNotifications do describe "When there are mentions" do before do - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: chat_message, notifications: [notification], @@ -508,9 +508,9 @@ describe UserNotifications do ) new_message = Fabricate(:chat_message, user: sender, chat_channel: channel) - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: new_message, notifications: [notification], @@ -652,9 +652,9 @@ describe UserNotifications do it "includes a view more link " do 2.times do msg = Fabricate(:chat_message, user: sender, chat_channel: channel) - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: msg, notifications: [notification], @@ -679,9 +679,9 @@ describe UserNotifications do it "has only a link to view all messages" do 2.times do msg = Fabricate(:chat_message, user: sender, chat_channel: channel) - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: msg, notifications: [notification], @@ -706,9 +706,9 @@ describe UserNotifications do new_message = Fabricate(:chat_message, user: sender, chat_channel: channel, cooked: "New message") - notification = Fabricate(:notification) + notification = Fabricate(:notification, user: user) Fabricate( - :chat_mention, + :user_chat_mention, user: user, chat_message: new_message, notifications: [notification], diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index 1601529ec8b..20d12d96ec1 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/plugins/chat/spec/models/chat/message_spec.rb @@ -450,7 +450,8 @@ describe Chat::Message do it "destroys chat_mention" do message_1 = Fabricate(:chat_message) notification = Fabricate(:notification, notification_type: Notification.types[:chat_mention]) - mention_1 = Fabricate(:chat_mention, chat_message: message_1, notifications: [notification]) + mention_1 = + Fabricate(:user_chat_mention, chat_message: message_1, notifications: [notification]) message_1.reload.destroy! @@ -527,42 +528,162 @@ describe Chat::Message do end describe "#upsert_mentions" do - fab!(:user1) { Fabricate(:user) } - fab!(:user2) { Fabricate(:user) } - fab!(:user3) { Fabricate(:user) } - fab!(:user4) { Fabricate(:user) } - fab!(:message) do - Fabricate(:chat_message, message: "Hey @#{user1.username} and @#{user2.username}") - end - let(:already_mentioned) { [user1.id, user2.id] } + context "with direct mentions" do + fab!(:user1) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + fab!(:user3) { Fabricate(:user) } + fab!(:user4) { Fabricate(:user) } + fab!(:message) do + Fabricate(:chat_message, message: "Hey @#{user1.username} and @#{user2.username}") + end + let(:already_mentioned) { [user1.id, user2.id] } - it "creates newly added mentions" do - existing_mention_ids = message.chat_mentions.pluck(:id) - update_message!( - message, - user: message.user, - text: message.message + " @#{user3.username} @#{user4.username} ", - ) + it "creates newly added mentions" do + existing_mention_ids = message.chat_mentions.pluck(:id) + message.message = message.message + " @#{user3.username} @#{user4.username} " + message.cook - expect(message.chat_mentions.pluck(:user_id)).to match_array( - [user1.id, user2.id, user3.id, user4.id], - ) - expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # existing mentions weren't recreated + message.upsert_mentions + + expect(message.user_mentions.pluck(:target_id)).to match_array( + [user1.id, user2.id, user3.id, user4.id], + ) + expect(message.user_mentions.pluck(:id)).to include(*existing_mention_ids) # existing mentions weren't recreated + end + + it "drops removed mentions" do + # user 2 is not mentioned anymore: + message.message = "Hey @#{user1.username}" + message.cook + + message.upsert_mentions + + expect(message.user_mentions.pluck(:target_id)).to contain_exactly(user1.id) + end + + it "changes nothing if message mentions has not been changed" do + existing_mention_ids = message.chat_mentions.pluck(:id) + + message.upsert_mentions + + expect(message.user_mentions.pluck(:target_id)).to match_array(already_mentioned) + expect(message.user_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated + end end - it "drops removed mentions" do - # user 2 is not mentioned anymore - update_message!(message, user: message.user, text: "Hey @#{user1.username}") + context "with group mentions" do + fab!(:group1) { Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:group2) { Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:group3) { Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:group4) { Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:message) do + Fabricate(:chat_message, message: "Hey @#{group1.name} and @#{group2.name}") + end + let(:already_mentioned) { [group1.id, group2.id] } - expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id) + it "creates newly added mentions" do + existing_mention_ids = message.chat_mentions.pluck(:id) + message.message = message.message + " @#{group3.name} @#{group4.name} " + message.cook + + message.upsert_mentions + + expect(message.group_mentions.pluck(:target_id)).to match_array( + [group1.id, group2.id, group3.id, group4.id], + ) + expect(message.group_mentions.pluck(:id)).to include(*existing_mention_ids) # existing mentions weren't recreated + end + + it "drops removed mentions" do + # group 2 is not mentioned anymore: + message.message = "Hey @#{group1.name}" + message.cook + + message.upsert_mentions + + expect(message.group_mentions.pluck(:target_id)).to contain_exactly(group1.id) + end + + it "changes nothing if message mentions has not been changed" do + existing_mention_ids = message.chat_mentions.pluck(:id) + + message.upsert_mentions + + expect(message.group_mentions.pluck(:target_id)).to match_array(already_mentioned) + expect(message.group_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated + end end - it "changes nothing if passed mentions are identical to existing mentions" do - existing_mention_ids = message.chat_mentions.pluck(:id) - update_message!(message, user: message.user, text: message.message) + context "with @here mentions" do + fab!(:message) { Fabricate(:chat_message, message: "There are no mentions yet") } - expect(message.chat_mentions.pluck(:user_id)).to match_array(already_mentioned) - expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated + it "creates @here mention" do + message.message = "Mentioning @here" + message.cook + + message.upsert_mentions + + expect(message.here_mention).to be_present + end + + it "creates only one mention even if @here present more than once in a message" do + message.message = "Mentioning several times: @here @here @here" + message.cook + + message.upsert_mentions + + expect(message.here_mention).to be_present + expect(message.chat_mentions.count).to be(1) + end + + it "drops @here mention when it's dropped from the message" do + message.message = "Mentioning @here" + message.cook + message.upsert_mentions + + message.message = "No mentions now" + message.cook + + message.upsert_mentions + + expect(message.here_mention).to be_blank + end + end + + context "with @all mentions" do + fab!(:message) { Fabricate(:chat_message, message: "There are no mentions yet") } + + it "creates @all mention" do + message.message = "Mentioning @all" + message.cook + + message.upsert_mentions + + expect(message.all_mention).to be_present + end + + it "creates only one mention even if @here present more than once in a message" do + message.message = "Mentioning several times: @all @all @all" + message.cook + + message.upsert_mentions + + expect(message.all_mention).to be_present + expect(message.chat_mentions.count).to be(1) + end + + it "drops @here mention when it's dropped from the message" do + message.message = "Mentioning @all" + message.cook + message.upsert_mentions + + message.message = "No mentions now" + message.cook + + message.upsert_mentions + + expect(message.all_mention).to be_blank + end end end diff --git a/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb b/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb index be57404be42..38da4fba08e 100644 --- a/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb +++ b/plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb @@ -128,7 +128,7 @@ describe Chat::ChannelUnreadsQuery do user_id: current_user.id, data: { chat_message_id: message.id, chat_channel_id: channel.id }.to_json, ) - Chat::Mention.create!( + Chat::UserMention.create!( notifications: [notification], user: current_user, chat_message: message, diff --git a/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb index 67b3b099b8b..85add0ad9c3 100644 --- a/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb @@ -199,7 +199,7 @@ RSpec.describe Chat::Api::ReadsController do }.to_json, ) .tap do |notification| - Chat::Mention.create!(user: user, chat_message: msg, notifications: [notification]) + Chat::UserMention.create!(user: user, chat_message: msg, notifications: [notification]) end end end diff --git a/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb index fe4bc4a7257..75f4a079bfe 100644 --- a/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb @@ -66,7 +66,7 @@ RSpec.describe Chat::Api::ThreadReadsController do }.to_json, ) .tap do |notification| - Chat::Mention.create!(user: user, chat_message: msg, notifications: [notification]) + Chat::UserMention.create!(user: user, chat_message: msg, notifications: [notification]) end end end diff --git a/plugins/chat/spec/serializer/chat/chat_message_serializer_spec.rb b/plugins/chat/spec/serializer/chat/chat_message_serializer_spec.rb index 7798601e060..e20a4ef573f 100644 --- a/plugins/chat/spec/serializer/chat/chat_message_serializer_spec.rb +++ b/plugins/chat/spec/serializer/chat/chat_message_serializer_spec.rb @@ -14,7 +14,7 @@ describe Chat::MessageSerializer do describe "#mentioned_users" do it "is limited by max_mentions_per_chat_message setting" do - Fabricate.times(2, :chat_mention, chat_message: message_1) + Fabricate.times(2, :user_chat_mention, chat_message: message_1) SiteSetting.max_mentions_per_chat_message = 1 expect(serializer.as_json[:mentioned_users].length).to eq(1) @@ -228,7 +228,7 @@ describe Chat::MessageSerializer do message: "here should be a mention, but since we're fabricating objects it doesn't matter", ) - Fabricate(:chat_mention, chat_message: message, user: mentioned_user) + Fabricate(:user_chat_mention, chat_message: message, user: mentioned_user) mentioned_user.destroy! message.reload diff --git a/plugins/chat/spec/serializer/chat/chat_thread_original_message_serializer_spec.rb b/plugins/chat/spec/serializer/chat/chat_thread_original_message_serializer_spec.rb index f873872d33f..9a9e37e85d6 100644 --- a/plugins/chat/spec/serializer/chat/chat_thread_original_message_serializer_spec.rb +++ b/plugins/chat/spec/serializer/chat/chat_thread_original_message_serializer_spec.rb @@ -12,7 +12,7 @@ describe Chat::ThreadOriginalMessageSerializer do describe "#mentioned_users" do it "is limited by max_mentions_per_chat_message setting" do - Fabricate.times(2, :chat_mention, chat_message: message_1) + Fabricate.times(2, :user_chat_mention, chat_message: message_1) SiteSetting.max_mentions_per_chat_message = 1 expect(serializer.as_json[:mentioned_users].length).to eq(1) diff --git a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb index 549e489c177..346de0648a1 100644 --- a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb +++ b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb @@ -84,12 +84,12 @@ RSpec.describe Chat::MarkAllUserChannelsRead do let(:messages) { MessageBus.track_publish { result } } before do - Chat::Mention.create!( + Chat::UserMention.create!( notifications: [notification_1], user: current_user, chat_message: message_1, ) - Chat::Mention.create!( + Chat::UserMention.create!( notifications: [notification_2], user: current_user, chat_message: message_3, diff --git a/plugins/chat/spec/services/chat/trash_message_spec.rb b/plugins/chat/spec/services/chat/trash_message_spec.rb index cb03dea8783..bac4357267e 100644 --- a/plugins/chat/spec/services/chat/trash_message_spec.rb +++ b/plugins/chat/spec/services/chat/trash_message_spec.rb @@ -49,7 +49,8 @@ RSpec.describe Chat::TrashMessage do it "destroys notifications for mentions" do notification = Fabricate(:notification) - mention = Fabricate(:chat_mention, chat_message: message, notifications: [notification]) + mention = + Fabricate(:user_chat_mention, chat_message: message, notifications: [notification]) result diff --git a/plugins/chat/spec/services/chat/update_message_spec.rb b/plugins/chat/spec/services/chat/update_message_spec.rb index 347675c8fd1..6897c9de19d 100644 --- a/plugins/chat/spec/services/chat/update_message_spec.rb +++ b/plugins/chat/spec/services/chat/update_message_spec.rb @@ -369,46 +369,126 @@ RSpec.describe Chat::UpdateMessage do end describe "with group mentions" do - it "creates group mentions on update" do + fab!(:group_1) do + Fabricate( + :public_group, + users: [user1, user2], + mentionable_level: Group::ALIAS_LEVELS[:everyone], + ) + end + fab!(:group_2) do + Fabricate( + :public_group, + users: [user3, user4], + mentionable_level: Group::ALIAS_LEVELS[:everyone], + ) + end + + it "creates a mention record when a group was mentioned on message update" do chat_message = create_chat_message(user1, "ping nobody", public_chat_channel) - expect { - described_class.call( - guardian: guardian, - message_id: chat_message.id, - message: "ping @#{admin_group.name}", - ) - }.to change { Chat::Mention.where(chat_message: chat_message).count }.by(2) - expect(admin1.chat_mentions.where(chat_message: chat_message)).to be_present - expect(admin2.chat_mentions.where(chat_message: chat_message)).to be_present + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping @#{group_1.name}", + ) + + expect(group_1.chat_mentions.where(chat_message: chat_message).count).to be(1) end - it "doesn't duplicate mentions when the user is already direct mentioned and then group mentioned" do - chat_message = create_chat_message(user1, "ping @#{admin2.username}", public_chat_channel) - expect { - described_class.call( - guardian: guardian, - message_id: chat_message.id, - message: "ping @#{admin_group.name} @#{admin2.username}", - ) - }.to change { admin1.chat_mentions.count }.by(1).and not_change { - admin2.chat_mentions.count - } + it "updates mention records when another group was mentioned on message update" do + chat_message = create_chat_message(user1, "ping @#{group_1.name}", public_chat_channel) + + expect(chat_message.group_mentions.map(&:target_id)).to contain_exactly(group_1.id) + + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping @#{group_2.name}", + ) + + expect(chat_message.reload.group_mentions.map(&:target_id)).to contain_exactly(group_2.id) end - it "deletes old mentions when group mention is removed" do + it "deletes a mention record when a group mention was removed on message update" do + chat_message = create_chat_message(user1, "ping @#{group_1.name}", public_chat_channel) + + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping nobody anymore!", + ) + + expect(group_1.chat_mentions.where(chat_message: chat_message).count).to be(0) + end + + it "doesn't notify the second time users that has already been notified when creating the message" do + group_user = Fabricate(:user) + Fabricate( + :user_chat_channel_membership, + chat_channel: public_chat_channel, + user: group_user, + ) + group = + Fabricate( + :public_group, + users: [group_user], + mentionable_level: Group::ALIAS_LEVELS[:everyone], + ) + chat_message = - create_chat_message(user1, "ping @#{admin_group.name}", public_chat_channel) - expect { - described_class.call( - guardian: guardian, - message_id: chat_message.id, - message: "ping nobody anymore!", - ) - }.to change { Chat::Mention.where(chat_message: chat_message).count }.by(-2) + create_chat_message(user1, "Mentioning @#{group.name}", public_chat_channel) + expect(group_user.notifications.count).to be(1) + notification_id = group_user.notifications.first.id - expect(admin1.chat_mentions.where(chat_message: chat_message)).not_to be_present - expect(admin2.chat_mentions.where(chat_message: chat_message)).not_to be_present + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "Update the message and still mention the same group @#{group.name}", + ) + + expect(group_user.notifications.count).to be(1) # no new notifications has been created + expect(group_user.notifications.first.id).to be(notification_id) # the existing notification hasn't been recreated + end + end + + describe "with @here mentions" do + it "doesn't notify the second time users that has already been notified when creating the message" do + user = Fabricate(:user) + Fabricate(:user_chat_channel_membership, chat_channel: public_chat_channel, user: user) + user.update!(last_seen_at: 4.minutes.ago) + + chat_message = create_chat_message(user1, "Mentioning @here", public_chat_channel) + expect(user.notifications.count).to be(1) + notification_id = user.notifications.first.id + + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "Update the message and still mention @here", + ) + + expect(user.notifications.count).to be(1) # no new notifications have been created + expect(user.notifications.first.id).to be(notification_id) # the existing notification haven't been recreated + end + end + + describe "with @all mentions" do + it "doesn't notify the second time users that has already been notified when creating the message" do + user = Fabricate(:user) + Fabricate(:user_chat_channel_membership, chat_channel: public_chat_channel, user: user) + + chat_message = create_chat_message(user1, "Mentioning @all", public_chat_channel) + notification_id = user.notifications.first.id + + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "Update the message and still mention @all", + ) + + expect(user.notifications.count).to be(1) # no new notifications have been created + expect(user.notifications.first.id).to be(notification_id) # the existing notification haven't been recreated end end end diff --git a/plugins/chat/spec/services/chat/update_user_last_read_spec.rb b/plugins/chat/spec/services/chat/update_user_last_read_spec.rb index bf3195b40bb..935430d18ec 100644 --- a/plugins/chat/spec/services/chat/update_user_last_read_spec.rb +++ b/plugins/chat/spec/services/chat/update_user_last_read_spec.rb @@ -84,7 +84,7 @@ RSpec.describe Chat::UpdateUserLastRead do before do Jobs.run_immediately! - Chat::Mention.create!( + Chat::UserMention.create!( notifications: [notification], user: current_user, chat_message: message_1, diff --git a/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb b/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb index c2bc3bf6a1f..39020398638 100644 --- a/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb +++ b/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb @@ -61,12 +61,12 @@ RSpec.describe Chat::UpdateUserThreadLastRead do before do Jobs.run_immediately! - Chat::Mention.create!( + Chat::UserMention.create!( notifications: [notification_1], user: current_user, chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread), ) - Chat::Mention.create!( + Chat::UserMention.create!( notifications: [notification_2], user: current_user, chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread), diff --git a/plugins/chat/spec/system/chat_channel_spec.rb b/plugins/chat/spec/system/chat_channel_spec.rb index adf5c0673fc..3983a90c1ff 100644 --- a/plugins/chat/spec/system/chat_channel_spec.rb +++ b/plugins/chat/spec/system/chat_channel_spec.rb @@ -202,8 +202,8 @@ RSpec.describe "Chat channel", type: :system do SiteSetting.enable_user_status = true current_user.set_status!("off to dentist", "tooth") other_user.set_status!("surfing", "surfing_man") - Fabricate(:chat_mention, user: current_user, chat_message: message) - Fabricate(:chat_mention, user: other_user, chat_message: message) + Fabricate(:user_chat_mention, user: current_user, chat_message: message) + Fabricate(:user_chat_mention, user: other_user, chat_message: message) chat_page.visit_channel(channel_1)