From 75b81b6854087842b53b4c9559ef5836d9516269 Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Thu, 16 Feb 2023 19:55:18 +0400 Subject: [PATCH] DEV: extract the logic for extracting and expanding mentions from ChatNotifier (#20290) Initially, the ChatMention model / db table was introduced to better support notifications (see discourse/discourse-chat@0801d10). That means that currently, we create a new chat_mention record only if a user will be notified about the mention. Now we plan to start using the ChatMention model in other scenarios (for example for implementing user status on mentions) so we need to always create a new record in the chat_mention table. This PR does the first step into that direction by decoupling the logic for extracting and expanding mentions from the code related to notifications. This doesn't change any behavior, only extracts code from ChatNotifier. --- plugins/chat/lib/chat_message_mentions.rb | 88 ++++++++++ plugins/chat/lib/chat_notifier.rb | 107 +++--------- plugins/chat/plugin.rb | 1 + .../spec/lib/chat_message_mentions_spec.rb | 154 ++++++++++++++++++ 4 files changed, 270 insertions(+), 80 deletions(-) create mode 100644 plugins/chat/lib/chat_message_mentions.rb create mode 100644 plugins/chat/spec/lib/chat_message_mentions_spec.rb diff --git a/plugins/chat/lib/chat_message_mentions.rb b/plugins/chat/lib/chat_message_mentions.rb new file mode 100644 index 00000000000..84516466b09 --- /dev/null +++ b/plugins/chat/lib/chat_message_mentions.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +class Chat::ChatMessageMentions + def initialize(message) + @message = message + + mentions = parse_mentions(message) + group_mentions = parse_group_mentions(message) + + @has_global_mention = mentions.include?("@all") + @has_here_mention = mentions.include?("@here") + @parsed_direct_mentions = normalize(mentions) + @parsed_group_mentions = normalize(group_mentions) + end + + attr_accessor :has_global_mention, + :has_here_mention, + :parsed_direct_mentions, + :parsed_group_mentions + + def global_mentions + return User.none unless @has_global_mention + channel_members.where.not(username_lower: @parsed_direct_mentions) + end + + def direct_mentions + chat_users.where(username_lower: @parsed_direct_mentions) + end + + def group_mentions + chat_users.includes(:groups).joins(:groups).where(groups: mentionable_groups) + end + + def here_mentions + return User.none unless @has_here_mention + + channel_members + .where("last_seen_at > ?", 5.minutes.ago) + .where.not(username_lower: @parsed_direct_mentions) + end + + def mentionable_groups + @mentionable_groups ||= + Group.mentionable(@message.user, include_public: false).where(id: visible_groups.map(&:id)) + end + + def visible_groups + @visible_groups ||= + Group.where("LOWER(name) IN (?)", @parsed_group_mentions).visible_groups(@message.user) + end + + private + + def channel_members + chat_users.where( + user_chat_channel_memberships: { + following: true, + chat_channel_id: @message.chat_channel.id, + }, + ) + end + + def chat_users + User + .includes(:user_chat_channel_memberships, :group_users) + .distinct + .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 + + def parse_mentions(message) + Nokogiri::HTML5.fragment(message.cooked).css(".mention").map(&:text) + end + + def parse_group_mentions(message) + Nokogiri::HTML5.fragment(message.cooked).css(".mention-group").map(&:text) + end + + def normalize(mentions) + mentions.reduce([]) do |memo, mention| + %w[@here @all].include?(mention.downcase) ? memo : (memo << mention[1..-1].downcase) + end + end +end diff --git a/plugins/chat/lib/chat_notifier.rb b/plugins/chat/lib/chat_notifier.rb index 4719b968019..7887cfade9a 100644 --- a/plugins/chat/lib/chat_notifier.rb +++ b/plugins/chat/lib/chat_notifier.rb @@ -60,6 +60,7 @@ class Chat::ChatNotifier @timestamp = timestamp @chat_channel = @chat_message.chat_channel @user = @chat_message.user + @mentions = Chat::ChatMessageMentions.new(chat_message) end ### Public API @@ -108,11 +109,12 @@ class Chat::ChatNotifier private def list_users_to_notify - direct_mentions_count = direct_mentions_from_cooked.length - group_mentions_count = group_name_mentions.length + mentions_count = + @mentions.parsed_direct_mentions.length + @mentions.parsed_group_mentions.length + mentions_count += 1 if @mentions.has_global_mention + mentions_count += 1 if @mentions.has_here_mention - skip_notifications = - (direct_mentions_count + group_mentions_count) > SiteSetting.max_mentions_per_chat_message + skip_notifications = mentions_count > SiteSetting.max_mentions_per_chat_message {}.tap do |to_notify| # The order of these methods is the precedence @@ -131,48 +133,13 @@ class Chat::ChatNotifier end end - def chat_users - User - .includes(:user_chat_channel_memberships, :group_users) - .distinct - .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: @user.username.downcase) - end - - def rest_of_the_channel - chat_users.where( - user_chat_channel_memberships: { - following: true, - chat_channel_id: @chat_channel.id, - }, - ) - end - - def members_accepting_channel_wide_notifications - rest_of_the_channel.where(user_options: { ignore_channel_wide_mention: [false, nil] }) - end - - def direct_mentions_from_cooked - @direct_mentions_from_cooked ||= - Nokogiri::HTML5.fragment(@chat_message.cooked).css(".mention").map(&:text) - end - - def normalized_mentions(mentions) - mentions.reduce([]) do |memo, mention| - %w[@here @all].include?(mention.downcase) ? memo : (memo << mention[1..-1].downcase) - end - end - def expand_global_mention(to_notify, already_covered_ids, skip) - typed_global_mention = direct_mentions_from_cooked.include?("@all") + has_all_mention = @mentions.has_global_mention - if typed_global_mention && @chat_channel.allow_channel_wide_mentions && !skip - to_notify[:global_mentions] = members_accepting_channel_wide_notifications - .where.not(username_lower: normalized_mentions(direct_mentions_from_cooked)) + if has_all_mention && @chat_channel.allow_channel_wide_mentions && !skip + to_notify[:global_mentions] = @mentions + .global_mentions + .where(user_options: { ignore_channel_wide_mention: [false, nil] }) .where.not(id: already_covered_ids) .pluck(:id) @@ -183,12 +150,12 @@ class Chat::ChatNotifier end def expand_here_mention(to_notify, already_covered_ids, skip) - typed_here_mention = direct_mentions_from_cooked.include?("@here") + has_here_mention = @mentions.has_here_mention - if typed_here_mention && @chat_channel.allow_channel_wide_mentions && !skip - to_notify[:here_mentions] = members_accepting_channel_wide_notifications - .where("last_seen_at > ?", 5.minutes.ago) - .where.not(username_lower: normalized_mentions(direct_mentions_from_cooked)) + if has_here_mention && @chat_channel.allow_channel_wide_mentions && !skip + to_notify[:here_mentions] = @mentions + .here_mentions + .where(user_options: { ignore_channel_wide_mention: [false, nil] }) .where.not(id: already_covered_ids) .pluck(:id) @@ -225,10 +192,7 @@ class Chat::ChatNotifier if skip direct_mentions = [] else - direct_mentions = - chat_users - .where(username_lower: normalized_mentions(direct_mentions_from_cooked)) - .where.not(id: already_covered_ids) + direct_mentions = @mentions.direct_mentions.where.not(id: already_covered_ids) end grouped = group_users_to_notify(direct_mentions) @@ -239,48 +203,31 @@ class Chat::ChatNotifier already_covered_ids.concat(to_notify[:direct_mentions]) end - def group_name_mentions - @group_mentions_from_cooked ||= - normalized_mentions( - Nokogiri::HTML5.fragment(@chat_message.cooked).css(".mention-group").map(&:text), - ) - end - - def visible_groups - @visible_groups ||= Group.where("LOWER(name) IN (?)", group_name_mentions).visible_groups(@user) - end - def expand_group_mentions(to_notify, already_covered_ids, skip) - return [] if skip || visible_groups.empty? + return [] if skip || @mentions.visible_groups.empty? - mentionable_groups = - Group.mentionable(@user, include_public: false).where(id: visible_groups.map(&:id)) - - mentions_disabled = visible_groups - mentionable_groups + reached_by_group = + @mentions + .group_mentions + .where("user_count <= ?", SiteSetting.max_users_notified_per_group_mention) + .where.not(id: already_covered_ids) too_many_members, mentionable = - mentionable_groups.partition do |group| + @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 to_notify[:group_mentions_disabled] = mentions_disabled to_notify[:too_many_members] = too_many_members - mentionable.each { |g| to_notify[g.name.downcase] = [] } - reached_by_group = - chat_users - .includes(:groups) - .joins(:groups) - .where(groups: mentionable) - .where.not(id: already_covered_ids) - grouped = group_users_to_notify(reached_by_group) - grouped[:already_participating].each do |user| # When a user is a member of multiple mentioned groups, # the most far to the left should take precedence. - ordered_group_names = group_name_mentions & mentionable.map { |mg| mg.name.downcase } + ordered_group_names = + @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/plugin.rb b/plugins/chat/plugin.rb index c5b2f5c6255..8a4792cb653 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -178,6 +178,7 @@ after_initialize do load File.expand_path("../lib/chat_message_updater.rb", __FILE__) load File.expand_path("../lib/chat_message_rate_limiter.rb", __FILE__) load File.expand_path("../lib/chat_message_reactor.rb", __FILE__) + load File.expand_path("../lib/chat_message_mentions.rb", __FILE__) load File.expand_path("../lib/chat_notifier.rb", __FILE__) load File.expand_path("../lib/chat_seeder.rb", __FILE__) load File.expand_path("../lib/chat_statistics.rb", __FILE__) diff --git a/plugins/chat/spec/lib/chat_message_mentions_spec.rb b/plugins/chat/spec/lib/chat_message_mentions_spec.rb new file mode 100644 index 00000000000..7ff25c873dd --- /dev/null +++ b/plugins/chat/spec/lib/chat_message_mentions_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Chat::ChatMessageMentions do + fab!(:channel_member_1) { Fabricate(:user) } + fab!(:channel_member_2) { Fabricate(:user) } + fab!(:channel_member_3) { Fabricate(:user) } + fab!(:not_a_channel_member) { Fabricate(:user) } + fab!(:chat_channel) { Fabricate(:chat_channel) } + + before do + chat_channel.add(channel_member_1) + chat_channel.add(channel_member_2) + chat_channel.add(channel_member_3) + end + + describe "#global_mentions" do + it "returns all members of the channel" do + message = create_message("mentioning @all") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.global_mentions.pluck(:username) + + expect(result).to contain_exactly( + channel_member_1.username, + channel_member_2.username, + channel_member_3.username, + ) + end + + it "doesn't include users that were also mentioned directly" do + message = create_message("mentioning @all and @#{channel_member_1.username}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.global_mentions.pluck(:username) + + expect(result).to contain_exactly(channel_member_2.username, channel_member_3.username) + end + + it "returns an empty list if there are no global mentions" do + message = create_message("not mentioning anybody") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.global_mentions.pluck(:username) + + expect(result).to be_empty + end + end + + describe "#here_mentions" do + before do + freeze_time + channel_member_1.update(last_seen_at: 2.minutes.ago) + channel_member_2.update(last_seen_at: 2.minutes.ago) + channel_member_3.update(last_seen_at: 5.minutes.ago) + end + + it "returns all members of the channel who were online in the last 5 minutes" do + message = create_message("mentioning @here") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.here_mentions.pluck(:username) + + expect(result).to contain_exactly(channel_member_1.username, channel_member_2.username) + end + + it "doesn't include users that were also mentioned directly" do + message = create_message("mentioning @here and @#{channel_member_1.username}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.here_mentions.pluck(:username) + + expect(result).to contain_exactly(channel_member_2.username) + end + + it "returns an empty list if there are no here mentions" do + message = create_message("not mentioning anybody") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.here_mentions.pluck(:username) + + expect(result).to be_empty + end + end + + describe "#direct_mentions" do + it "returns users who were mentioned directly" do + message = + create_message("mentioning @#{channel_member_1.username} and @#{channel_member_2.username}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.direct_mentions.pluck(:username) + + expect(result).to contain_exactly(channel_member_1.username, channel_member_2.username) + end + + it "returns a mentioned user even if he's not a member of the channel" do + message = create_message("mentioning @#{not_a_channel_member.username}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.direct_mentions.pluck(:username) + + expect(result).to contain_exactly(not_a_channel_member.username) + end + + it "returns an empty list if no one was mentioned directly" do + message = create_message("not mentioning anybody") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.direct_mentions.pluck(:username) + + expect(result).to be_empty + end + end + + describe "#group_mentions" do + fab!(:group1) { Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:group_member_1) { Fabricate(:user, group_ids: [group1.id]) } + fab!(:group_member_2) { Fabricate(:user, group_ids: [group1.id]) } + fab!(:group_member_3) { Fabricate(:user, group_ids: [group1.id]) } + + before do + chat_channel.add(group_member_1) + chat_channel.add(group_member_2) + end + + it "returns members of a mentioned group even if some of them is not members of the channel" do + message = create_message("mentioning @#{group1.name}") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.group_mentions.pluck(:username) + + expect(result).to contain_exactly( + group_member_1.username, + group_member_2.username, + group_member_3.username, + ) + end + + it "returns an empty list if no group was mentioned" do + message = create_message("not mentioning anybody") + + mentions = Chat::ChatMessageMentions.new(message) + result = mentions.group_mentions.pluck(:username) + + expect(result).to be_empty + end + end + + def create_message(text) + Fabricate(:chat_message, chat_channel: chat_channel, message: text) + end +end