From 62f423da15edbbfbe4e37d218544a3e1ca427e6a Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Wed, 17 Jan 2024 15:24:01 +0400 Subject: [PATCH] DEV: Redesign chat mentions (#24752) At the moment, when someone is mentioning a group, or using here or all mention, we create a chat_mention record per user. What we want instead is to have special kinds of mentions, so we can create only one chat_mention record in such cases. This PR implements that. Note, that such mentions will still have N related notifications, one notification per a user. We don't expect we'll have performance problems on the notifications side, but if at some point we do, we should be able to solve them on the side of notifications (notifications are handled in jobs, also some little delays with the notifications are acceptable, so we can make sure notifications are properly queued, and that processing of every notification is fast enough to make delays small enough). The preparation work for this PR was done in fbd24fa, where we make it possible for one mention to have several related notifications. A pretty tricky part of this PR is schema and data migration, I've explained related details inline on the migration files. --- .../app/jobs/regular/chat/notify_mentioned.rb | 32 +++- plugins/chat/app/models/chat/all_mention.rb | 6 + plugins/chat/app/models/chat/group_mention.rb | 7 + plugins/chat/app/models/chat/here_mention.rb | 6 + plugins/chat/app/models/chat/mention.rb | 10 +- plugins/chat/app/models/chat/message.rb | 82 +++++--- plugins/chat/app/models/chat/user_mention.rb | 7 + .../chat/app/queries/chat/messages_query.rb | 4 +- .../serializers/chat/message_serializer.rb | 2 +- .../thread_original_message_serializer.rb | 2 +- ...add_type_and_target_id_to_chat_mentions.rb | 15 ++ ...set_type_and_target_id_on_chat_mentions.rb | 24 +++ ...add_and_remove_indexes_on_chat_mentions.rb | 23 +++ ...target_id_on_chat_mentions_post_migrate.rb | 27 +++ ...make_type_on_chat_mentions_non_nullable.rb | 11 ++ plugins/chat/lib/chat/group_extension.rb | 11 ++ plugins/chat/lib/chat/mailer.rb | 6 +- plugins/chat/lib/chat/notifier.rb | 13 +- plugins/chat/lib/chat/parsed_mentions.rb | 12 -- plugins/chat/lib/chat/user_extension.rb | 2 +- .../lib/chat/user_notifications_extension.rb | 3 +- plugins/chat/plugin.rb | 1 + .../chat/spec/components/chat/mailer_spec.rb | 33 +++- .../chat/spec/fabricators/chat_fabricator.rb | 15 +- .../jobs/regular/chat/channel_delete_spec.rb | 2 +- .../regular/chat/notify_mentioned_spec.rb | 24 ++- .../chat/spec/lib/chat/message_mover_spec.rb | 2 +- .../spec/mailers/user_notifications_spec.rb | 36 ++-- plugins/chat/spec/models/chat/message_spec.rb | 179 +++++++++++++++--- .../chat/channel_unreads_query_spec.rb | 2 +- .../chat/api/reads_controller_spec.rb | 2 +- .../chat/api/thread_reads_controller_spec.rb | 2 +- .../chat/chat_message_serializer_spec.rb | 4 +- ...thread_original_message_serializer_spec.rb | 2 +- .../chat/mark_all_user_channels_read_spec.rb | 4 +- .../spec/services/chat/trash_message_spec.rb | 3 +- .../spec/services/chat/update_message_spec.rb | 144 ++++++++++---- .../chat/update_user_last_read_spec.rb | 2 +- .../chat/update_user_thread_last_read_spec.rb | 4 +- plugins/chat/spec/system/chat_channel_spec.rb | 4 +- 40 files changed, 613 insertions(+), 157 deletions(-) create mode 100644 plugins/chat/app/models/chat/all_mention.rb create mode 100644 plugins/chat/app/models/chat/group_mention.rb create mode 100644 plugins/chat/app/models/chat/here_mention.rb create mode 100644 plugins/chat/app/models/chat/user_mention.rb create mode 100644 plugins/chat/db/migrate/20231227160001_add_type_and_target_id_to_chat_mentions.rb create mode 100644 plugins/chat/db/migrate/20231227160002_set_type_and_target_id_on_chat_mentions.rb create mode 100644 plugins/chat/db/migrate/20231227160003_add_and_remove_indexes_on_chat_mentions.rb create mode 100644 plugins/chat/db/post_migrate/20231227160004_set_type_and_target_id_on_chat_mentions_post_migrate.rb create mode 100644 plugins/chat/db/post_migrate/20231227160005_make_type_on_chat_mentions_non_nullable.rb create mode 100644 plugins/chat/lib/chat/group_extension.rb 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)