From 35a414bb38b2ab990ff920f8c3adf2f877249f12 Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Fri, 5 May 2023 15:47:07 +0400 Subject: [PATCH] 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 | 74 +++++++++++-------- plugins/chat/lib/chat/message_creator.rb | 3 + plugins/chat/lib/chat/message_updater.rb | 3 + plugins/chat/lib/chat/notifier.rb | 39 +++++----- ...message_mentions.rb => parsed_mentions.rb} | 2 +- ...ntions_spec.rb => parsed_mentions_spec.rb} | 2 +- plugins/chat/spec/models/chat/message_spec.rb | 46 ++++++++---- 7 files changed, 104 insertions(+), 65 deletions(-) rename plugins/chat/lib/chat/{message_mentions.rb => parsed_mentions.rb} (99%) rename plugins/chat/spec/lib/chat/{message_mentions_spec.rb => parsed_mentions_spec.rb} (99%) diff --git a/plugins/chat/app/models/chat/message.rb b/plugins/chat/app/models/chat/message.rb index e5386c0cd03..3186bd40b5a 100644 --- a/plugins/chat/app/models/chat/message.rb +++ b/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 diff --git a/plugins/chat/lib/chat/message_creator.rb b/plugins/chat/lib/chat/message_creator.rb index 7acc4aceff9..2228c09cc19 100644 --- a/plugins/chat/lib/chat/message_creator.rb +++ b/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) diff --git a/plugins/chat/lib/chat/message_updater.rb b/plugins/chat/lib/chat/message_updater.rb index 4825045f4b0..91c1296fc4a 100644 --- a/plugins/chat/lib/chat/message_updater.rb +++ b/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 }) diff --git a/plugins/chat/lib/chat/notifier.rb b/plugins/chat/lib/chat/notifier.rb index 7d4e879a4f5..db1a95b36a5 100644 --- a/plugins/chat/lib/chat/notifier.rb +++ b/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) } diff --git a/plugins/chat/lib/chat/message_mentions.rb b/plugins/chat/lib/chat/parsed_mentions.rb similarity index 99% rename from plugins/chat/lib/chat/message_mentions.rb rename to plugins/chat/lib/chat/parsed_mentions.rb index cd8f3d78dc8..14745b0d0ca 100644 --- a/plugins/chat/lib/chat/message_mentions.rb +++ b/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 diff --git a/plugins/chat/spec/lib/chat/message_mentions_spec.rb b/plugins/chat/spec/lib/chat/parsed_mentions_spec.rb similarity index 99% rename from plugins/chat/spec/lib/chat/message_mentions_spec.rb rename to plugins/chat/spec/lib/chat/parsed_mentions_spec.rb index 4254c9c19ca..ed1d679dcd6 100644 --- a/plugins/chat/spec/lib/chat/message_mentions_spec.rb +++ b/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) } diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index d1eeb878cae..04695e4a49d 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/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 - mentioned_user_ids = [user1.id, user2.id] - message.create_mentions(mentioned_user_ids) + it "creates mentions for mentioned usernames" do + message.message = "Mentioning @#{user1.username} and @#{user2.username}" + 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 - mentioned_user_ids = [user1.id, user1.id, user1.id, user1.id, user1.id] - message.create_mentions(mentioned_user_ids) + it "ignores duplicated mentions" do + message.message = + "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(mentioned_user_ids) + message.update_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, 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(mentioned_user_ids) + message.update_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(already_mentioned) expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated end end