diff --git a/plugins/chat/lib/chat/duplicate_message_validator.rb b/plugins/chat/lib/chat/duplicate_message_validator.rb index fa9175b8ed7..2f0eb68f30b 100644 --- a/plugins/chat/lib/chat/duplicate_message_validator.rb +++ b/plugins/chat/lib/chat/duplicate_message_validator.rb @@ -2,45 +2,26 @@ module Chat class DuplicateMessageValidator - attr_reader :chat_message + attr_reader :chat_message, :chat_channel def initialize(chat_message) @chat_message = chat_message + @chat_channel = chat_message&.chat_channel end def validate - return if SiteSetting.chat_duplicate_message_sensitivity.zero? - matrix = - DuplicateMessageValidator.sensitivity_matrix(SiteSetting.chat_duplicate_message_sensitivity) + return if chat_message.nil? || chat_channel.nil? + return if chat_message.user.bot? + return if chat_channel.direct_message_channel? && chat_channel.user_count <= 2 - # Check if the length of the message is too short to check for a duplicate message - return if chat_message.message.length < matrix[:min_message_length] - - # Check if there are enough users in the channel to check for a duplicate message - return if (chat_message.chat_channel.user_count || 0) < matrix[:min_user_count] - - # Check if the same duplicate message has been posted in the last N seconds by any user - if !chat_message - .chat_channel + if chat_channel .chat_messages - .where("created_at > ?", matrix[:min_past_seconds].seconds.ago) - .where(message: chat_message.message) + .where(created_at: 10.seconds.ago..) + .where("LOWER(message) = ?", chat_message.message.strip.downcase) + .where(user: chat_message.user) .exists? - return + chat_message.errors.add(:base, I18n.t("chat.errors.duplicate_message")) end - - chat_message.errors.add(:base, I18n.t("chat.errors.duplicate_message")) - end - - def self.sensitivity_matrix(sensitivity) - { - # 0.1 sensitivity = 100 users and 1.0 sensitivity = 5 users. - min_user_count: (-1.0 * 105.5 * sensitivity + 110.55).to_i, - # 0.1 sensitivity = 30 chars and 1.0 sensitivity = 10 chars. - min_message_length: (-1.0 * 22.2 * sensitivity + 32.22).to_i, - # 0.1 sensitivity = 10 seconds and 1.0 sensitivity = 60 seconds. - min_past_seconds: (55.55 * sensitivity + 4.5).to_i, - } end end end diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index c9841af6b95..81d88fcf684 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -54,6 +54,10 @@ Fabricator(:direct_message_channel, from: :chat_channel) do end end +def fake_chat_message + Faker::Alphanumeric.alpha(number: [15, SiteSetting.chat_minimum_message_length].max) +end + Fabricator(:chat_message, class_name: "Chat::Message") do transient use_service: false @@ -68,7 +72,7 @@ end Fabricator(:chat_message_without_service, class_name: "Chat::Message") do user chat_channel - message { Faker::Alphanumeric.alpha(number: SiteSetting.chat_minimum_message_length) } + message { fake_chat_message } after_build { |message, attrs| message.cook } after_create { |message, attrs| message.upsert_mentions } @@ -96,9 +100,7 @@ Fabricator(:chat_message_with_service, class_name: "Chat::CreateMessage") do resolved_class.call( params: { chat_channel_id: channel.id, - message: - transients[:message] || - Faker::Alphanumeric.alpha(number: SiteSetting.chat_minimum_message_length), + message: transients[:message].presence || fake_chat_message, thread_id: transients[:thread]&.id, in_reply_to_id: transients[:in_reply_to]&.id, upload_ids: transients[:upload_ids], diff --git a/plugins/chat/spec/lib/chat/duplicate_message_validator_spec.rb b/plugins/chat/spec/lib/chat/duplicate_message_validator_spec.rb index 62141957fee..9e50e899a3c 100644 --- a/plugins/chat/spec/lib/chat/duplicate_message_validator_spec.rb +++ b/plugins/chat/spec/lib/chat/duplicate_message_validator_spec.rb @@ -1,121 +1,108 @@ # frozen_string_literal: true describe Chat::DuplicateMessageValidator do - let(:chat_channel) { Fabricate(:chat_channel) } + let(:message) { "goal!" } + fab!(:category_channel) { Fabricate(:chat_channel) } + fab!(:dm_channel) { Fabricate(:direct_message_channel) } + fab!(:user) - def message_blocked?(message) - chat_message = Fabricate.build(:chat_message, message: message, chat_channel: chat_channel) + def message_blocked?(message:, chat_channel:, user:) + chat_message = Fabricate.build(:chat_message, user:, message:, chat_channel:) described_class.new(chat_message).validate chat_message.errors.full_messages.include?(I18n.t("chat.errors.duplicate_message")) end - it "adds no errors when chat_duplicate_message_sensitivity is 0" do - SiteSetting.chat_duplicate_message_sensitivity = 0 - expect(message_blocked?("test")).to eq(false) + it "blocks a message if it was posted in a category channel in the last 10 seconds by the same user" do + Fabricate( + :chat_message, + created_at: 1.second.ago, + user:, + message:, + chat_channel: category_channel, + ) + + expect(message_blocked?(message:, user:, chat_channel: category_channel)).to eq(true) end - skip "errors if the message meets the requirements for sensitivity 0.1" do - SiteSetting.chat_duplicate_message_sensitivity = 0.1 + it "doesn't block a message if it's different" do + Fabricate( + :chat_message, + created_at: 1.second.ago, + user:, + message:, + chat_channel: category_channel, + ) - chat_channel.update!(user_count: 100) - message = "this is a 30 char message for test" - dupe = - Fabricate( - :chat_message, - created_at: 1.second.ago, - message: message, - chat_channel: chat_channel, - ) - expect(message_blocked?(message)).to eq(true) - - expect(message_blocked?("blah")).to eq(false) - - dupe.update!(created_at: 11.seconds.ago) - expect(message_blocked?(message)).to eq(false) + expect(message_blocked?(message: "BUT!", user:, chat_channel: category_channel)).to eq(false) end - skip "errors if the message meets the requirements for sensitivity 0.5" do - SiteSetting.chat_duplicate_message_sensitivity = 0.5 - chat_channel.update!(user_count: 57) - message = "this is a 21 char msg" - dupe = - Fabricate( - :chat_message, - created_at: 1.second.ago, - message: message, - chat_channel: chat_channel, - ) - expect(message_blocked?(message)).to eq(true) + it "doesn't block a message if it was posted more than 10 seconds ago" do + Fabricate( + :chat_message, + created_at: 11.seconds.ago, + user:, + message:, + chat_channel: category_channel, + ) - expect(message_blocked?("blah")).to eq(false) - - dupe.update!(created_at: 33.seconds.ago) - expect(message_blocked?(message)).to eq(false) + expect(message_blocked?(message:, user:, chat_channel: category_channel)).to eq(false) end - skip "errors if the message meets the requirements for sensitivity 1.0" do - SiteSetting.chat_duplicate_message_sensitivity = 1.0 - chat_channel.update!(user_count: 5) - message = "10 char msg" - dupe = - Fabricate( - :chat_message, - created_at: 1.second.ago, - message: message, - chat_channel: chat_channel, - ) - expect(message_blocked?(message)).to eq(true) + it "blocks a message case insensitively" do + Fabricate( + :chat_message, + created_at: 1.second.ago, + user:, + message:, + chat_channel: category_channel, + ) - expect(message_blocked?("blah")).to eq(false) - - dupe.update!(created_at: 61.seconds.ago) - expect(message_blocked?(message)).to eq(false) + expect(message_blocked?(message: message.upcase, user:, chat_channel: category_channel)).to eq( + true, + ) end - describe "#sensitivity_matrix" do - describe "#min_user_count" do - it "calculates correctly for each of the major points from 0.1 to 1.0" do - expect(described_class.sensitivity_matrix(0.1)[:min_user_count]).to eq(100) - expect(described_class.sensitivity_matrix(0.2)[:min_user_count]).to eq(89) - expect(described_class.sensitivity_matrix(0.3)[:min_user_count]).to eq(78) - expect(described_class.sensitivity_matrix(0.4)[:min_user_count]).to eq(68) - expect(described_class.sensitivity_matrix(0.5)[:min_user_count]).to eq(57) - expect(described_class.sensitivity_matrix(0.6)[:min_user_count]).to eq(47) - expect(described_class.sensitivity_matrix(0.7)[:min_user_count]).to eq(36) - expect(described_class.sensitivity_matrix(0.8)[:min_user_count]).to eq(26) - expect(described_class.sensitivity_matrix(0.9)[:min_user_count]).to eq(15) - expect(described_class.sensitivity_matrix(1.0)[:min_user_count]).to eq(5) - end - end + it "doesn't block a message if it was posted by a different user" do + Fabricate( + :chat_message, + created_at: 1.second.ago, + user: Fabricate(:user), + message:, + chat_channel: category_channel, + ) - describe "#min_message_length" do - it "calculates correctly for each of the major points from 0.1 to 1.0" do - expect(described_class.sensitivity_matrix(0.1)[:min_message_length]).to eq(30) - expect(described_class.sensitivity_matrix(0.2)[:min_message_length]).to eq(27) - expect(described_class.sensitivity_matrix(0.3)[:min_message_length]).to eq(25) - expect(described_class.sensitivity_matrix(0.4)[:min_message_length]).to eq(23) - expect(described_class.sensitivity_matrix(0.5)[:min_message_length]).to eq(21) - expect(described_class.sensitivity_matrix(0.6)[:min_message_length]).to eq(18) - expect(described_class.sensitivity_matrix(0.7)[:min_message_length]).to eq(16) - expect(described_class.sensitivity_matrix(0.8)[:min_message_length]).to eq(14) - expect(described_class.sensitivity_matrix(0.9)[:min_message_length]).to eq(12) - expect(described_class.sensitivity_matrix(1.0)[:min_message_length]).to eq(10) - end - end + expect(message_blocked?(message:, user:, chat_channel: category_channel)).to eq(false) + end - describe "#min_past_seconds" do - it "calculates correctly for each of the major points from 0.1 to 1.0" do - expect(described_class.sensitivity_matrix(0.1)[:min_past_seconds]).to eq(10) - expect(described_class.sensitivity_matrix(0.2)[:min_past_seconds]).to eq(15) - expect(described_class.sensitivity_matrix(0.3)[:min_past_seconds]).to eq(21) - expect(described_class.sensitivity_matrix(0.4)[:min_past_seconds]).to eq(26) - expect(described_class.sensitivity_matrix(0.5)[:min_past_seconds]).to eq(32) - expect(described_class.sensitivity_matrix(0.6)[:min_past_seconds]).to eq(37) - expect(described_class.sensitivity_matrix(0.7)[:min_past_seconds]).to eq(43) - expect(described_class.sensitivity_matrix(0.8)[:min_past_seconds]).to eq(48) - expect(described_class.sensitivity_matrix(0.9)[:min_past_seconds]).to eq(54) - expect(described_class.sensitivity_matrix(1.0)[:min_past_seconds]).to eq(60) - end - end + it "doesn't block a message if it was posted in a different channel" do + Fabricate( + :chat_message, + created_at: 1.second.ago, + user:, + message:, + chat_channel: Fabricate(:chat_channel), + ) + + expect(message_blocked?(message:, user:, chat_channel: category_channel)).to eq(false) + end + + it "doesn't block a message if it was posted by a bot" do + bot = Fabricate(:bot) + + Fabricate( + :chat_message, + created_at: 1.second.ago, + user: bot, + message:, + chat_channel: category_channel, + ) + + expect(message_blocked?(message:, user: bot, chat_channel: category_channel)).to eq(false) + end + + it "doesn't block a message if it was posted in a 1:1 DM" do + Fabricate(:chat_message, created_at: 1.second.ago, user:, message:, chat_channel: dm_channel) + + expect(message_blocked?(message:, user:, chat_channel: dm_channel)).to eq(false) end end diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index bb2bd529c88..adea9ca9794 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/plugins/chat/spec/models/chat/message_spec.rb @@ -616,18 +616,15 @@ describe Chat::Message do end describe "blocking duplicate messages" do - fab!(:channel) { Fabricate(:chat_channel, user_count: 10) } - fab!(:user1) { Fabricate(:user) } - fab!(:user2) { Fabricate(:user) } + let(:message) { "this is duplicate" } + fab!(:chat_channel) + fab!(:user) - before { SiteSetting.chat_duplicate_message_sensitivity = 1 } - - it "blocks duplicate messages for the message, channel user, and message age requirements" do - Fabricate(:chat_message, message: "this is duplicate", chat_channel: channel, user: user1) - message = - described_class.new(message: "this is duplicate", chat_channel: channel, user: user2) - message.valid? - expect(message.errors.full_messages).to include(I18n.t("chat.errors.duplicate_message")) + it "blocks duplicate messages" do + Fabricate(:chat_message, message:, chat_channel:, user:) + msg = described_class.new(message:, chat_channel:, user:) + msg.valid? + expect(msg.errors.full_messages).to include(I18n.t("chat.errors.duplicate_message")) end end diff --git a/plugins/chat/spec/plugin_helper.rb b/plugins/chat/spec/plugin_helper.rb index 9e1c2f88264..2adf5399e2b 100644 --- a/plugins/chat/spec/plugin_helper.rb +++ b/plugins/chat/spec/plugin_helper.rb @@ -43,7 +43,7 @@ module ChatSystemHelpers chat_channel_id: channel.id, in_reply_to_id: in_reply_to, thread_id: thread_id, - message: Faker::Alphanumeric.alpha(number: SiteSetting.chat_minimum_message_length), + message: fake_chat_message, }, ) @@ -68,6 +68,10 @@ module ChatSpecHelpers ) end + def fake_chat_message + Faker::Alphanumeric.alpha(number: [15, SiteSetting.chat_minimum_message_length].max) + end + def update_message!(message, text: nil, user: Discourse.system_user, upload_ids: nil) Chat::UpdateMessage.call( guardian: user.guardian, diff --git a/plugins/chat/spec/requests/chat/api/channels_archives_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_archives_controller_spec.rb index 8582f3944d3..4ff346602ff 100644 --- a/plugins/chat/spec/requests/chat/api/channels_archives_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channels_archives_controller_spec.rb @@ -22,7 +22,6 @@ RSpec.describe Chat::Api::ChannelsArchivesController do before do SiteSetting.chat_enabled = true SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] - SiteSetting.chat_duplicate_message_sensitivity = 0 end describe "#create" do diff --git a/plugins/chat/spec/requests/chat/api/channels_status_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_status_controller_spec.rb index 0765690e79b..9d1363fdc5a 100644 --- a/plugins/chat/spec/requests/chat/api/channels_status_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channels_status_controller_spec.rb @@ -6,7 +6,6 @@ RSpec.describe Chat::Api::ChannelsStatusController do before do SiteSetting.chat_enabled = true SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] - SiteSetting.chat_duplicate_message_sensitivity = 0 end def status(status) diff --git a/plugins/chat/spec/services/chat/update_message_spec.rb b/plugins/chat/spec/services/chat/update_message_spec.rb index 625be4852c9..83a3e0c8efc 100644 --- a/plugins/chat/spec/services/chat/update_message_spec.rb +++ b/plugins/chat/spec/services/chat/update_message_spec.rb @@ -40,7 +40,6 @@ RSpec.describe Chat::UpdateMessage do before do SiteSetting.chat_enabled = true SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] - SiteSetting.chat_duplicate_message_sensitivity = 0 Jobs.run_immediately! [admin1, admin2, user1, user2, user3, user4].each { |user| public_chat_channel.add(user) } @@ -580,23 +579,14 @@ RSpec.describe Chat::UpdateMessage do fab!(:upload1) { Fabricate(:upload, user: user1) } fab!(:upload2) { Fabricate(:upload, user: user1) } - before do - SiteSetting.chat_duplicate_message_sensitivity = 1.0 - public_chat_channel.update!(user_count: 50) - end - it "errors when editing the message to be the same as one that was posted recently" do chat_message_1 = create_chat_message(user1, "this is some chat message", public_chat_channel) chat_message_2 = - create_chat_message( - Fabricate(:user), - "another different chat message here", - public_chat_channel, - ) + create_chat_message(user1, "another different chat message here", public_chat_channel) - chat_message_1.update!(created_at: 30.seconds.ago) - chat_message_2.update!(created_at: 20.seconds.ago) + chat_message_1.update!(created_at: 15.seconds.ago) + chat_message_2.update!(created_at: 5.seconds.ago) expect do described_class.call( @@ -619,7 +609,7 @@ RSpec.describe Chat::UpdateMessage do public_chat_channel, upload_ids: [upload1.id, upload2.id], ) - chat_message.update!(created_at: 30.seconds.ago) + chat_message.update!(created_at: 5.seconds.ago) updater = described_class.call( diff --git a/plugins/chat/spec/system/message_notifications_mobile_spec.rb b/plugins/chat/spec/system/message_notifications_mobile_spec.rb index 3e00008ef3c..398378528c9 100644 --- a/plugins/chat/spec/system/message_notifications_mobile_spec.rb +++ b/plugins/chat/spec/system/message_notifications_mobile_spec.rb @@ -12,8 +12,8 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do chat_system_bootstrap end - def create_message(channel, text: "this is fine", user: Fabricate(:user)) - Fabricate(:chat_message_with_service, chat_channel: channel, message: text, user: user) + def create_message(chat_channel, message: nil, user: Fabricate(:user)) + Fabricate(:chat_message_with_service, chat_channel:, message:, user:) end context "as a user" do @@ -98,7 +98,7 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do create_message( channel_1, user: user_1, - text: "hello @#{current_user.username} what's up?", + message: "hello @#{current_user.username} what's up?", ) expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator") @@ -113,7 +113,7 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do create_message( channel_1, user: user_1, - text: "Are you busy @#{current_user.username}?", + message: "Are you busy @#{current_user.username}?", ) 3.times { create_message(channel_1, user: user_1) } diff --git a/plugins/chat/spec/system/page_objects/chat/chat_channel.rb b/plugins/chat/spec/system/page_objects/chat/chat_channel.rb index bd426a38476..efbf7475e36 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_channel.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_channel.rb @@ -116,7 +116,7 @@ module PageObjects end def send_message(text = nil) - text ||= Faker::Lorem.characters(number: SiteSetting.chat_minimum_message_length) + text ||= fake_chat_message text = text.chomp if text.present? # having \n on the end of the string counts as an Enter keypress composer.fill_in(with: text) click_send_message diff --git a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb index d23aab5e5c6..86b2f282a19 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb @@ -96,7 +96,7 @@ module PageObjects end def send_message(text = nil) - text ||= Faker::Lorem.characters(number: SiteSetting.chat_minimum_message_length) + text ||= fake_chat_message text = text.chomp if text.present? # having \n on the end of the string counts as an Enter keypress composer.fill_in(with: text) click_send_message