mirror of
https://github.com/discourse/discourse.git
synced 2025-01-24 09:46:16 +08:00
FIX: remove complicated 'chat_duplicate_message_sensitivity' site setting (#30516)
And change the "formula" to check for duplicate messages to - no duplicate check in 1:1 DMs - only duplicate check in group DMs / channels, for posts made by the same user, in the past 10 seconds Internal ref - t/144262
This commit is contained in:
parent
d7aa13328d
commit
1f483f48a0
plugins/chat
|
@ -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
|
||||
|
|
|
@ -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],
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
|
@ -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,
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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(
|
||||
|
|
|
@ -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) }
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue
Block a user