DEV: Always create chat mention records (#20470)

Before this commit, we created a chat mention record only in case we wanted to send a notification about that mention to the user. Notifications were the only use case for the chat_mention db table. Now we want to use that table for other features, so we have to always create a chat_mention record.

plugins/chat/app/jobs/regular/chat_notify_mentioned.rb

@@ -100,7 +100,7 @@ module Jobs
       payload
     end
 
-    def create_notification!(membership, notification_data)
+    def create_notification!(membership, notification_data, mention)
       is_read = Chat::ChatNotifier.user_has_seen_message?(membership, @chat_message.id)
 
       notification =
@@ -111,15 +111,15 @@ module Jobs
           data: notification_data.to_json,
           read: is_read,
         )
-      ChatMention.create!(
-        notification: notification,
-        user: membership.user,
-        chat_message: @chat_message,
-      )
+
+      mention.update!(notification: notification)
     end
 
     def send_notifications(membership, notification_data, os_payload)
-      create_notification!(membership, notification_data)
+      mention = ChatMention.find_by(user: membership.user, chat_message: @chat_message)
+      return if mention.blank?
+
+      create_notification!(membership, notification_data, mention)
 
       if !membership.desktop_notifications_never? && !membership.muted?
         MessageBus.publish(

plugins/chat/app/models/chat_message.rb

@@ -226,8 +226,36 @@ class ChatMessage < ActiveRecord::Base
     "/chat/c/-/#{self.chat_channel_id}/#{self.id}"
   end
 
+  def create_mentions(user_ids)
+    return if user_ids.empty?
+
+    now = Time.zone.now
+    mentions = []
+    User
+      .where(id: user_ids)
+      .find_each do |user|
+        mentions << { chat_message_id: self.id, user_id: user.id, created_at: now, updated_at: now }
+      end
+
+    ChatMention.insert_all(mentions)
+  end
+
+  def update_mentions(mentioned_user_ids)
+    old_mentions = chat_mentions.pluck(:user_id)
+    updated_mentions = mentioned_user_ids
+    mentioned_user_ids_to_drop = old_mentions - updated_mentions
+    mentioned_user_ids_to_add = updated_mentions - old_mentions
+
+    delete_mentions(mentioned_user_ids_to_drop)
+    create_mentions(mentioned_user_ids_to_add)
+  end
+
   private
 
+  def delete_mentions(user_ids)
+    chat_mentions.where(user_id: user_ids).destroy_all
+  end
+
   def message_too_short?
     message.length < SiteSetting.chat_minimum_message_length
   end

plugins/chat/db/post_migrate/20230227172543_make_chat_mention_notification_id_nullable.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class MakeChatMentionNotificationIdNullable < ActiveRecord::Migration[7.0]
+  def up
+    change_column_null :chat_mentions, :notification_id, true
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/lib/chat_message_mentions.rb

@@ -18,6 +18,18 @@ class Chat::ChatMessageMentions
                 :parsed_direct_mentions,
                 :parsed_group_mentions
 
+  def all_mentioned_users_ids
+    @all_mentioned_users_ids ||=
+      begin
+        user_ids = global_mentions.pluck(:id)
+        user_ids.concat(direct_mentions.pluck(:id))
+        user_ids.concat(group_mentions.pluck(:id))
+        user_ids.concat(here_mentions.pluck(:id))
+        user_ids.uniq!
+        user_ids
+      end
+  end
+
   def global_mentions
     return User.none unless @has_global_mention
     channel_members.where.not(username_lower: @parsed_direct_mentions)
@@ -67,7 +79,6 @@ class Chat::ChatMessageMentions
       .joins("LEFT OUTER JOIN user_chat_channel_memberships uccm ON uccm.user_id = users.id")
       .joins(:user_option)
       .real
-      .not_suspended
       .where(user_options: { chat_enabled: true })
       .where.not(username_lower: @message.user.username.downcase)
   end

plugins/chat/lib/chat_notifier.rb

@@ -66,6 +66,10 @@ class Chat::ChatNotifier
   ### Public API
 
   def notify_new
+    if @mentions.all_mentioned_users_ids.present?
+      @chat_message.create_mentions(@mentions.all_mentioned_users_ids)
+    end
+
     to_notify = list_users_to_notify
     mentioned_user_ids = to_notify.extract!(:all_mentioned_user_ids)[:all_mentioned_user_ids]
 
@@ -82,6 +86,8 @@ class Chat::ChatNotifier
   end
 
   def notify_edit
+    @chat_message.update_mentions(@mentions.all_mentioned_users_ids)
+
     existing_notifications =
       ChatMention.includes(:user, :notification).where(chat_message: @chat_message)
     already_notified_user_ids = existing_notifications.map(&:user_id)
@@ -139,6 +145,7 @@ class Chat::ChatNotifier
     if has_all_mention && @chat_channel.allow_channel_wide_mentions && !skip
       to_notify[:global_mentions] = @mentions
         .global_mentions
+        .not_suspended
         .where(user_options: { ignore_channel_wide_mention: [false, nil] })
         .where.not(id: already_covered_ids)
         .pluck(:id)
@@ -155,6 +162,7 @@ class Chat::ChatNotifier
     if has_here_mention && @chat_channel.allow_channel_wide_mentions && !skip
       to_notify[:here_mentions] = @mentions
         .here_mentions
+        .not_suspended
         .where(user_options: { ignore_channel_wide_mention: [false, nil] })
         .where.not(id: already_covered_ids)
         .pluck(:id)
@@ -192,7 +200,7 @@ class Chat::ChatNotifier
     if skip
       direct_mentions = []
     else
-      direct_mentions = @mentions.direct_mentions.where.not(id: already_covered_ids)
+      direct_mentions = @mentions.direct_mentions.not_suspended.where.not(id: already_covered_ids)
     end
 
     grouped = group_users_to_notify(direct_mentions)
@@ -209,6 +217,7 @@ class Chat::ChatNotifier
     reached_by_group =
       @mentions
         .group_mentions
+        .not_suspended
         .where("user_count <= ?", SiteSetting.max_users_notified_per_group_mention)
         .where.not(id: already_covered_ids)
 

plugins/chat/spec/components/chat_message_creator_spec.rb

@@ -140,16 +140,29 @@ describe Chat::ChatMessageCreator do
       expect(events.map { _1[:event_name] }).to include(:chat_message_created)
     end
 
-    it "creates mention notifications for public chat" do
-      expect {
+    it "creates mentions and mention notifications for public chat" do
+      message =
         Chat::ChatMessageCreator.create(
           chat_channel: public_chat_channel,
           user: user1,
           content:
             "this is a @#{user1.username} message with @system @mentions @#{user2.username} and @#{user3.username}",
-        )
-        # Only 2 mentions are created because user mentioned themselves, system, and an invalid username.
-      }.to change { ChatMention.count }.by(2).and not_change { user1.chat_mentions.count }
+        ).chat_message
+
+      # a mention for the user himself wasn't created
+      user1_mention = user1.chat_mentions.where(chat_message: message).first
+      expect(user1_mention).to be_nil
+
+      system_user_mention = Discourse.system_user.chat_mentions.where(chat_message: message).first
+      expect(system_user_mention).to be_nil
+
+      user2_mention = user2.chat_mentions.where(chat_message: message).first
+      expect(user2_mention).to be_present
+      expect(user2_mention.notification).to be_present
+
+      user3_mention = user3.chat_mentions.where(chat_message: message).first
+      expect(user3_mention).to be_present
+      expect(user3_mention.notification).to be_present
     end
 
     it "mentions are case insensitive" do
@@ -225,58 +238,88 @@ describe Chat::ChatMessageCreator do
     end
 
     it "doesn't create mention notifications for users without a membership record" do
-      expect {
+      message =
         Chat::ChatMessageCreator.create(
           chat_channel: public_chat_channel,
           user: user1,
           content: "hello @#{user_without_memberships.username}",
-        )
-      }.not_to change { ChatMention.count }
+        ).chat_message
+
+      mention = user_without_memberships.chat_mentions.where(chat_message: message).first
+      expect(mention.notification).to be_nil
     end
 
     it "doesn't create mention notifications for users who cannot chat" do
       new_group = Group.create
       SiteSetting.chat_allowed_groups = new_group.id
-      expect {
+
+      message =
         Chat::ChatMessageCreator.create(
           chat_channel: public_chat_channel,
           user: user1,
           content: "hi @#{user2.username} @#{user3.username}",
-        )
-      }.not_to change { ChatMention.count }
+        ).chat_message
+
+      user2_mention = user2.chat_mentions.where(chat_message: message).first
+      expect(user2_mention.notification).to be_nil
+
+      user3_mention = user2.chat_mentions.where(chat_message: message).first
+      expect(user3_mention.notification).to be_nil
     end
 
-    it "doesn't create mention notifications for users with chat disabled" do
+    it "doesn't create mentions for users with chat disabled" do
       user2.user_option.update(chat_enabled: false)
-      expect {
+
+      message =
         Chat::ChatMessageCreator.create(
           chat_channel: public_chat_channel,
           user: user1,
           content: "hi @#{user2.username}",
-        )
-      }.not_to change { ChatMention.count }
+        ).chat_message
+
+      mention = user2.chat_mentions.where(chat_message: message).first
+      expect(mention).to be_nil
     end
 
     it "creates only mention notifications for users with access in private chat" do
-      expect {
+      message =
         Chat::ChatMessageCreator.create(
           chat_channel: direct_message_channel,
           user: user1,
           content: "hello there @#{user2.username} and @#{user3.username}",
-        )
-        # Only user2 should be notified
-      }.to change { user2.chat_mentions.count }.by(1).and not_change { user3.chat_mentions.count }
+        ).chat_message
+
+      # Only user2 should be notified
+      user2_mention = user2.chat_mentions.where(chat_message: message).first
+      expect(user2_mention.notification).to be_present
+
+      user3_mention = user3.chat_mentions.where(chat_message: message).first
+      expect(user3_mention.notification).to be_nil
     end
 
-    it "creates a mention notifications for group users that are participating in private chat" do
+    it "creates a mention for group users even if they're not participating in private chat" do
       expect {
         Chat::ChatMessageCreator.create(
           chat_channel: direct_message_channel,
           user: user1,
           content: "hello there @#{user_group.name}",
         )
-        # Only user2 should be notified
-      }.to change { user2.chat_mentions.count }.by(1).and not_change { user3.chat_mentions.count }
+      }.to change { user2.chat_mentions.count }.by(1).and change { user3.chat_mentions.count }.by(1)
+    end
+
+    it "creates a mention notifications only for group users that are participating in private chat" do
+      message =
+        Chat::ChatMessageCreator.create(
+          chat_channel: direct_message_channel,
+          user: user1,
+          content: "hello there @#{user_group.name}",
+        ).chat_message
+
+      user2_mention = user2.chat_mentions.where(chat_message: message).first
+      expect(user2_mention.notification).to be_present
+
+      user3_mention = user3.chat_mentions.where(chat_message: message).first
+      expect(user3_mention.notification).to be_nil
     end
 
     it "publishes inaccessible mentions when user isn't aren't a part of the channel" do
@@ -307,51 +350,64 @@ describe Chat::ChatMessageCreator do
       )
     end
 
-    it "does not create mentions for suspended users" do
+    it "creates mentions for suspended users" do
       user2.update(suspended_till: Time.now + 10.years)
-      expect {
+
+      message =
         Chat::ChatMessageCreator.create(
           chat_channel: direct_message_channel,
           user: user1,
           content: "hello @#{user2.username}",
-        )
-      }.not_to change { user2.chat_mentions.count }
+        ).chat_message
+
+      mention = user2.chat_mentions.where(chat_message: message).first
+      expect(mention).to be_present
     end
 
-    it "does not create @all mentions for users when ignore_channel_wide_mention is enabled" do
-      expect {
-        Chat::ChatMessageCreator.create(
-          chat_channel: public_chat_channel,
-          user: user1,
-          content: "@all",
-        )
-      }.to change { ChatMention.count }.by(4)
+    it "does not create mention notifications for suspended users" do
+      user2.update(suspended_till: Time.now + 10.years)
 
-      user2.user_option.update(ignore_channel_wide_mention: true)
-      expect {
+      message =
         Chat::ChatMessageCreator.create(
-          chat_channel: public_chat_channel,
+          chat_channel: direct_message_channel,
           user: user1,
-          content: "hi! @all",
-        )
-      }.to change { ChatMention.count }.by(3)
+          content: "hello @#{user2.username}",
+        ).chat_message
+
+      mention = user2.chat_mentions.where(chat_message: message).first
+      expect(mention.notification).to be_nil
     end
 
-    it "does not create @here mentions for users when ignore_channel_wide_mention is enabled" do
-      admin1.update(last_seen_at: 1.year.ago)
-      admin2.update(last_seen_at: 1.year.ago)
-      user1.update(last_seen_at: Time.now)
-      user2.update(last_seen_at: Time.now)
-      user2.user_option.update(ignore_channel_wide_mention: true)
-      user3.update(last_seen_at: Time.now)
+    context "when ignore_channel_wide_mention is enabled" do
+      before { user2.user_option.update(ignore_channel_wide_mention: true) }
 
-      expect {
-        Chat::ChatMessageCreator.create(
-          chat_channel: public_chat_channel,
-          user: user1,
-          content: "@here",
-        )
-      }.to change { ChatMention.count }.by(1)
+      it "when mentioning @all creates a mention without notification" do
+        message =
+          Chat::ChatMessageCreator.create(
+            chat_channel: public_chat_channel,
+            user: user1,
+            content: "hi! @all",
+          ).chat_message
+
+        mention = user2.chat_mentions.where(chat_message: message).first
+        expect(mention).to be_present
+        expect(mention.notification).to be_nil
+      end
+
+      it "when mentioning @here creates a mention without notification" do
+        user2.update(last_seen_at: Time.now)
+
+        message =
+          Chat::ChatMessageCreator.create(
+            chat_channel: public_chat_channel,
+            user: user1,
+            content: "@here",
+          ).chat_message
+
+        mention = user2.chat_mentions.where(chat_message: message).first
+        expect(mention).to be_present
+        expect(mention.notification).to be_nil
+      end
     end
 
     describe "replies" do

plugins/chat/spec/components/chat_message_updater_spec.rb

@@ -150,17 +150,18 @@ describe Chat::ChatMessageUpdater do
     }.not_to change { ChatMention.count }
   end
 
-  it "doesn't create mentions for users without access" do
+  it "doesn't create mention notification for users without access" do
     message = "ping"
     chat_message = create_chat_message(user1, message, public_chat_channel)
 
-    expect {
-      Chat::ChatMessageUpdater.update(
-        guardian: guardian,
-        chat_message: chat_message,
-        new_content: message + " @#{user_without_memberships.username}",
-      )
-    }.not_to change { ChatMention.count }
+    Chat::ChatMessageUpdater.update(
+      guardian: guardian,
+      chat_message: chat_message,
+      new_content: message + " @#{user_without_memberships.username}",
+    )
+
+    mention = user_without_memberships.chat_mentions.where(chat_message: chat_message).first
+    expect(mention.notification).to be_nil
   end
 
   it "destroys mention notifications that should be removed" do
@@ -189,15 +190,17 @@ describe Chat::ChatMessageUpdater do
     expect(user4.chat_mentions.where(chat_message: chat_message)).to be_present
   end
 
-  it "does not create new mentions in direct message for users who don't have access" do
-    chat_message = create_chat_message(user1, "ping nobody", @direct_message_channel)
-    expect {
-      Chat::ChatMessageUpdater.update(
-        guardian: guardian,
-        chat_message: chat_message,
-        new_content: "ping @#{admin1.username}",
-      )
-    }.not_to change { ChatMention.count }
+  it "doesn't create mention notification in direct message for users without access" do
+    message = create_chat_message(user1, "ping nobody", @direct_message_channel)
+
+    Chat::ChatMessageUpdater.update(
+      guardian: guardian,
+      chat_message: message,
+      new_content: "ping @#{admin1.username}",
+    )
+
+    mention = admin1.chat_mentions.where(chat_message: message).first
+    expect(mention.notification).to be_nil
   end
 
   describe "group mentions" do

plugins/chat/spec/jobs/regular/chat_notify_mentioned_spec.rb

@@ -21,8 +21,11 @@ describe Jobs::ChatNotifyMentioned do
     end
   end
 
-  def create_chat_message(channel: public_channel, user: user_1)
-    Fabricate(:chat_message, chat_channel: channel, user: user, created_at: 10.minutes.ago)
+  def create_chat_message(channel: public_channel, author: user_1, mentioned_user: user_2)
+    message =
+      Fabricate(:chat_message, chat_channel: channel, user: author, created_at: 10.minutes.ago)
+    Fabricate(:chat_mention, chat_message: message, user: mentioned_user)
+    message
   end
 
   def track_desktop_notification(

plugins/chat/spec/lib/chat_message_mentions_spec.rb

@@ -139,7 +139,18 @@ RSpec.describe Chat::ChatMessageMentions do
     end
 
     it "returns an empty list if no group was mentioned" do
-      message = create_message("not mentioning anybody")
+      message = create_message("not mentioning anyone")
+
+      mentions = Chat::ChatMessageMentions.new(message)
+      result = mentions.group_mentions.pluck(:username)
+
+      expect(result).to be_empty
+    end
+
+    it "returns an empty list when mentioning an unmentionable group" do
+      group1.mentionable_level = Group::ALIAS_LEVELS[:nobody]
+      group1.save!
+      message = create_message("mentioning @#{group1.name}")
 
       mentions = Chat::ChatMessageMentions.new(message)
       result = mentions.group_mentions.pluck(:username)

plugins/chat/spec/lib/chat_notifier_spec.rb

@@ -23,7 +23,7 @@ describe Chat::ChatNotifier do
     end
 
     def build_cooked_msg(message_body, user, chat_channel: channel)
-      ChatMessage.new(
+      ChatMessage.create(
         chat_channel: chat_channel,
         user: user,
         message: message_body,

plugins/chat/spec/models/chat_message_spec.rb

@@ -570,6 +570,68 @@ describe ChatMessage do
     end
   end
 
+  describe "#create_mentions" do
+    fab!(:message) { Fabricate(:chat_message) }
+    fab!(:user1) { Fabricate(:user) }
+    fab!(:user2) { Fabricate(:user) }
+
+    it "creates mentions for passed user ids" do
+      mentioned_user_ids = [user1.id, user2.id]
+      message.create_mentions(mentioned_user_ids)
+      message.reload
+
+      expect(message.chat_mentions.pluck(:user_id)).to match_array(mentioned_user_ids)
+    end
+
+    it "ignores duplicates in passed user ids" do
+      mentioned_user_ids = [user1.id, user1.id, user1.id, user1.id, user1.id]
+      message.create_mentions(mentioned_user_ids)
+      message.reload
+
+      expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id)
+    end
+  end
+
+  describe "#update_mentions" do
+    fab!(:message) { Fabricate(:chat_message) }
+    fab!(:user1) { Fabricate(:user) }
+    fab!(:user2) { Fabricate(:user) }
+    fab!(:user3) { Fabricate(:user) }
+    fab!(:user4) { Fabricate(:user) }
+    let(:already_mentioned) { [user1.id, user2.id] }
+
+    before { message.create_mentions(already_mentioned) }
+
+    it "creates newly added mentions" do
+      existing_mention_ids = message.chat_mentions.pluck(:id)
+
+      mentioned_user_ids = [*already_mentioned, user3.id, user4.id]
+      message.update_mentions(mentioned_user_ids)
+      message.reload
+
+      expect(message.chat_mentions.pluck(:user_id)).to match_array(mentioned_user_ids)
+      expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # existing mentions weren't recreated
+    end
+
+    it "drops removed mentions" do
+      message.update_mentions([user1.id]) # user 2 is not mentioned anymore
+      message.reload
+
+      expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id)
+    end
+
+    it "changes nothing if passed mentions are identical to existing mentions" do
+      existing_mention_ids = message.chat_mentions.pluck(:id)
+
+      mentioned_user_ids = [*already_mentioned]
+      message.update_mentions(mentioned_user_ids)
+      message.reload
+
+      expect(message.chat_mentions.pluck(:user_id)).to match_array(mentioned_user_ids)
+      expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated
+    end
+  end
+
   # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01
   def chat_upload_count(uploads = nil)
     return DB.query_single("SELECT COUNT(*) FROM chat_uploads").first if !uploads