DEV: Create and update chat message mentions earlier (#21388)

We need to create and update `chat_mentions` records for messages earlier. They should be created or updated before we  call `Chat::Publisher.publish_new!` `Chat::Publisher.publish_edit!` to send the message to message bus subscribers).

This logic is covered with tests in `message_creator_spec.rb`, `message_updater_spec.rb`, `notifier_spec.rb` and `notify_mentioned_spec.rb`.

See the commits history for steps of refactoring.

plugins/chat/app/models/chat/message.rb

@@ -157,6 +157,8 @@ module Chat
 
       self.cooked = self.class.cook(self.message, user_id: self.last_editor_id)
       self.cooked_version = BAKED_VERSION
+
+      invalidate_parsed_mentions
     end
 
     def rebake!(invalidate_oneboxes: false, priority: nil)
@@ -258,7 +260,49 @@ module Chat
       "/chat/c/-/#{self.chat_channel_id}/#{self.id}"
     end
 
-    def create_mentions(user_ids)
+    def create_mentions
+      insert_mentions(parsed_mentions.all_mentioned_users_ids)
+    end
+
+    def update_mentions
+      mentioned_user_ids = parsed_mentions.all_mentioned_users_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)
+      insert_mentions(mentioned_user_ids_to_add)
+    end
+
+    def in_thread?
+      self.thread_id.present?
+    end
+
+    def thread_reply?
+      in_thread? && !thread_om?
+    end
+
+    def thread_om?
+      in_thread? && self.thread.original_message_id == self.id
+    end
+
+    def parsed_mentions
+      @parsed_mentions ||= Chat::ParsedMentions.new(self)
+    end
+
+    def invalidate_parsed_mentions
+      @parsed_mentions = nil
+    end
+
+    private
+
+    def delete_mentions(user_ids)
+      chat_mentions.where(user_id: user_ids).destroy_all
+    end
+
+    def insert_mentions(user_ids)
       return if user_ids.empty?
 
       now = Time.zone.now
@@ -277,34 +321,6 @@ module Chat
       Chat::Mention.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
-
-    def in_thread?
-      self.thread_id.present?
-    end
-
-    def thread_reply?
-      in_thread? && !thread_om?
-    end
-
-    def thread_om?
-      in_thread? && self.thread.original_message_id == self.id
-    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/lib/chat/message_creator.rb

@@ -52,9 +52,12 @@ module Chat
         validate_message!(has_uploads: uploads.any?)
         validate_reply_chain!
         validate_existing_thread!
+
         @chat_message.thread_id = @existing_thread&.id
         @chat_message.cook
         @chat_message.save!
+        @chat_message.create_mentions
+
         create_chat_webhook_event
         create_thread
         @chat_message.attach_uploads(uploads)

plugins/chat/lib/chat/message_updater.rb

@@ -31,8 +31,11 @@ module Chat
         validate_message!(has_uploads: upload_info[:uploads].any?)
         @chat_message.cook
         @chat_message.save!
+
+        @chat_message.update_mentions
         update_uploads(upload_info)
         revision = save_revision!
+
         @chat_message.reload
         Chat::Publisher.publish_edit!(@chat_channel, @chat_message)
         Jobs.enqueue(Jobs::Chat::ProcessMessage, { chat_message_id: @chat_message.id })

plugins/chat/lib/chat/notifier.rb

@@ -61,16 +61,11 @@ module Chat
       @timestamp = timestamp
       @chat_channel = @chat_message.chat_channel
       @user = @chat_message.user
-      @mentions = Chat::MessageMentions.new(chat_message)
     end
 
     ### 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]
 
@@ -87,8 +82,6 @@ module Chat
     end
 
     def notify_edit
-      @chat_message.update_mentions(@mentions.all_mentioned_users_ids)
-
       already_notified_user_ids =
         Chat::Mention
           .where(chat_message: @chat_message)
@@ -108,7 +101,8 @@ module Chat
     private
 
     def list_users_to_notify
-      skip_notifications = @mentions.count > SiteSetting.max_mentions_per_chat_message
+      skip_notifications =
+        @chat_message.parsed_mentions.count > SiteSetting.max_mentions_per_chat_message
 
       {}.tap do |to_notify|
         # The order of these methods is the precedence
@@ -130,10 +124,11 @@ module Chat
     end
 
     def expand_global_mention(to_notify, already_covered_ids)
-      has_all_mention = @mentions.has_global_mention
+      has_all_mention = @chat_message.parsed_mentions.has_global_mention
 
       if has_all_mention && @chat_channel.allow_channel_wide_mentions
-        to_notify[:global_mentions] = @mentions
+        to_notify[:global_mentions] = @chat_message
+          .parsed_mentions
           .global_mentions
           .not_suspended
           .where(user_options: { ignore_channel_wide_mention: [false, nil] })
@@ -147,10 +142,11 @@ module Chat
     end
 
     def expand_here_mention(to_notify, already_covered_ids)
-      has_here_mention = @mentions.has_here_mention
+      has_here_mention = @chat_message.parsed_mentions.has_here_mention
 
       if has_here_mention && @chat_channel.allow_channel_wide_mentions
-        to_notify[:here_mentions] = @mentions
+        to_notify[:here_mentions] = @chat_message
+          .parsed_mentions
           .here_mentions
           .not_suspended
           .where(user_options: { ignore_channel_wide_mention: [false, nil] })
@@ -190,7 +186,10 @@ module Chat
       if skip
         direct_mentions = []
       else
-        direct_mentions = @mentions.direct_mentions.not_suspended.where.not(id: already_covered_ids)
+        direct_mentions =
+          @chat_message.parsed_mentions.direct_mentions.not_suspended.where.not(
+            id: already_covered_ids,
+          )
       end
 
       grouped = group_users_to_notify(direct_mentions)
@@ -202,21 +201,24 @@ module Chat
     end
 
     def expand_group_mentions(to_notify, already_covered_ids)
-      return if @mentions.visible_groups.empty?
+      return if @chat_message.parsed_mentions.visible_groups.empty?
 
       reached_by_group =
-        @mentions
+        @chat_message
+          .parsed_mentions
           .group_mentions
           .not_suspended
           .where("user_count <= ?", SiteSetting.max_users_notified_per_group_mention)
           .where.not(id: already_covered_ids)
 
       too_many_members, mentionable =
-        @mentions.mentionable_groups.partition do |group|
+        @chat_message.parsed_mentions.mentionable_groups.partition do |group|
           group.user_count > SiteSetting.max_users_notified_per_group_mention
         end
 
-      mentions_disabled = @mentions.visible_groups - @mentions.mentionable_groups
+      mentions_disabled =
+        @chat_message.parsed_mentions.visible_groups -
+          @chat_message.parsed_mentions.mentionable_groups
       to_notify[:group_mentions_disabled] = mentions_disabled
       to_notify[:too_many_members] = too_many_members
       mentionable.each { |g| to_notify[g.name.downcase] = [] }
@@ -226,7 +228,8 @@ module Chat
         # When a user is a member of multiple mentioned groups,
         # the most far to the left should take precedence.
         ordered_group_names =
-          @mentions.parsed_group_mentions & mentionable.map { |mg| mg.name.downcase }
+          @chat_message.parsed_mentions.parsed_group_mentions &
+            mentionable.map { |mg| mg.name.downcase }
         user_group_names = user.groups.map { |ug| ug.name.downcase }
         group_name = ordered_group_names.detect { |gn| user_group_names.include?(gn) }
 

plugins/chat/lib/chat/parsed_mentions.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 module Chat
-  class MessageMentions
+  class ParsedMentions
     def initialize(message)
       @message = message
 

plugins/chat/spec/lib/chat/parsed_mentions_spec.rb

@@ -2,7 +2,7 @@
 
 require "rails_helper"
 
-RSpec.describe Chat::MessageMentions do
+RSpec.describe Chat::ParsedMentions do
   fab!(:channel_member_1) { Fabricate(:user) }
   fab!(:channel_member_2) { Fabricate(:user) }
   fab!(:channel_member_3) { Fabricate(:user) }

plugins/chat/spec/models/chat/message_spec.rb

@@ -552,17 +552,22 @@ describe Chat::Message do
     fab!(:user1) { Fabricate(:user) }
     fab!(:user2) { Fabricate(:user) }
 
-    it "creates mentions for passed user ids" do
+    it "creates mentions for mentioned  usernames" do
-      mentioned_user_ids = [user1.id, user2.id]
+      message.message = "Mentioning @#{user1.username} and @#{user2.username}"
-      message.create_mentions(mentioned_user_ids)
+      message.cook
+
+      message.create_mentions
       message.reload
 
-      expect(message.chat_mentions.pluck(:user_id)).to match_array(mentioned_user_ids)
+      expect(message.chat_mentions.pluck(:user_id)).to match_array([user1.id, user2.id])
     end
 
-    it "ignores duplicates in passed user ids" do
+    it "ignores duplicated mentions" do
-      mentioned_user_ids = [user1.id, user1.id, user1.id, user1.id, user1.id]
+      message.message =
-      message.create_mentions(mentioned_user_ids)
+        "Mentioning @#{user1.username} @#{user1.username} @#{user1.username} @#{user1.username}"
+      message.cook
+
+      message.create_mentions
       message.reload
 
       expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id)
@@ -570,28 +575,36 @@ describe Chat::Message do
   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) }
+    fab!(:message) do
+      Fabricate(:chat_message, message: "Hey @#{user1.username} and @#{user2.username}")
+    end
     let(:already_mentioned) { [user1.id, user2.id] }
 
-    before { message.create_mentions(already_mentioned) }
+    before { message.create_mentions }
 
     it "creates newly added mentions" do
       existing_mention_ids = message.chat_mentions.pluck(:id)
+      message.message = message.message + " @#{user3.username} @#{user4.username} "
+      message.cook
 
-      mentioned_user_ids = [*already_mentioned, user3.id, user4.id]
+      message.update_mentions
-      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(: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
     end
 
     it "drops removed mentions" do
-      message.update_mentions([user1.id]) # user 2 is not mentioned anymore
+      message.message = "Hey @#{user1.username}" # user 2 is not mentioned anymore
+      message.cook
+
+      message.update_mentions
       message.reload
 
       expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id)
@@ -599,12 +612,13 @@ describe Chat::Message do
 
     it "changes nothing if passed mentions are identical to existing mentions" do
       existing_mention_ids = message.chat_mentions.pluck(:id)
+      message.message = message.message
+      message.cook
 
-      mentioned_user_ids = [*already_mentioned]
+      message.update_mentions
-      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(:user_id)).to match_array(already_mentioned)
       expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated
     end
   end