fix: Disable automations on auto-reply emails (#12101)
The term "sorcerer’s apprentice mode" is defined as a bug in a protocol where, under some circumstances, the receipt of a message causes multiple messages to be sent, each of which, when received, triggers the same bug. - RFC3834 Reference: https://github.com/chatwoot/chatwoot/pull/9606 This PR: - Adds an auto_reply attribute to message. - Adds an auto_reply attribute to conversation. - Disable conversation_created / conversation_opened event if auto_reply is set. - Disable message_created event if auto_reply is set. --------- Co-authored-by: Muhsin Keloth <muhsinkeramam@gmail.com>
This commit is contained in:
@@ -17,7 +17,7 @@ class AutomationRuleListener < BaseListener
|
|||||||
end
|
end
|
||||||
|
|
||||||
def conversation_created(event)
|
def conversation_created(event)
|
||||||
return if performed_by_automation?(event)
|
return if performed_by_automation?(event) || ignore_auto_reply_event?(event)
|
||||||
|
|
||||||
conversation = event.data[:conversation]
|
conversation = event.data[:conversation]
|
||||||
account = conversation.account
|
account = conversation.account
|
||||||
@@ -34,7 +34,7 @@ class AutomationRuleListener < BaseListener
|
|||||||
end
|
end
|
||||||
|
|
||||||
def conversation_opened(event)
|
def conversation_opened(event)
|
||||||
return if performed_by_automation?(event)
|
return if performed_by_automation?(event) || ignore_auto_reply_event?(event)
|
||||||
|
|
||||||
conversation = event.data[:conversation]
|
conversation = event.data[:conversation]
|
||||||
account = conversation.account
|
account = conversation.account
|
||||||
@@ -87,8 +87,13 @@ class AutomationRuleListener < BaseListener
|
|||||||
event.data[:performed_by].present? && event.data[:performed_by].instance_of?(AutomationRule)
|
event.data[:performed_by].present? && event.data[:performed_by].instance_of?(AutomationRule)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def ignore_auto_reply_event?(event)
|
||||||
|
conversation = event.data[:conversation]
|
||||||
|
conversation.additional_attributes['auto_reply'].present?
|
||||||
|
end
|
||||||
|
|
||||||
def ignore_message_created_event?(event)
|
def ignore_message_created_event?(event)
|
||||||
message = event.data[:message]
|
message = event.data[:message]
|
||||||
performed_by_automation?(event) || message.activity?
|
performed_by_automation?(event) || message.activity? || message.auto_reply_email?
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -84,6 +84,7 @@ class Imap::ImapMailbox
|
|||||||
additional_attributes: {
|
additional_attributes: {
|
||||||
source: 'email',
|
source: 'email',
|
||||||
in_reply_to: in_reply_to,
|
in_reply_to: in_reply_to,
|
||||||
|
auto_reply: @processed_mail.auto_reply?,
|
||||||
mail_subject: @processed_mail.subject,
|
mail_subject: @processed_mail.subject,
|
||||||
initiated_at: {
|
initiated_at: {
|
||||||
timestamp: Time.now.utc
|
timestamp: Time.now.utc
|
||||||
|
|||||||
@@ -8,13 +8,6 @@ module IncomingEmailValidityHelper
|
|||||||
# This can happen in cases like bounce emails for invalid contact email address
|
# This can happen in cases like bounce emails for invalid contact email address
|
||||||
return false unless Devise.email_regexp.match?(@processed_mail.original_sender)
|
return false unless Devise.email_regexp.match?(@processed_mail.original_sender)
|
||||||
|
|
||||||
# Process bounced emails, as regular emails
|
|
||||||
return true if @processed_mail.bounced?
|
|
||||||
|
|
||||||
# we skip processing auto reply emails like delivery status notifications
|
|
||||||
# out of office replies, etc.
|
|
||||||
return false if auto_reply_email?
|
|
||||||
|
|
||||||
true
|
true
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -24,13 +17,4 @@ module IncomingEmailValidityHelper
|
|||||||
|
|
||||||
true
|
true
|
||||||
end
|
end
|
||||||
|
|
||||||
def auto_reply_email?
|
|
||||||
if @processed_mail.auto_reply?
|
|
||||||
Rails.logger.info "is_auto_reply? : #{processed_mail.auto_reply?}"
|
|
||||||
true
|
|
||||||
else
|
|
||||||
false
|
|
||||||
end
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -70,6 +70,7 @@ class SupportMailbox < ApplicationMailbox
|
|||||||
additional_attributes: {
|
additional_attributes: {
|
||||||
in_reply_to: in_reply_to,
|
in_reply_to: in_reply_to,
|
||||||
source: 'email',
|
source: 'email',
|
||||||
|
auto_reply: @processed_mail.auto_reply?,
|
||||||
mail_subject: @processed_mail.subject,
|
mail_subject: @processed_mail.subject,
|
||||||
initiated_at: {
|
initiated_at: {
|
||||||
timestamp: Time.now.utc
|
timestamp: Time.now.utc
|
||||||
|
|||||||
@@ -195,6 +195,12 @@ class Message < ApplicationRecord
|
|||||||
true
|
true
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def auto_reply_email?
|
||||||
|
return false unless incoming_email? || inbox.email?
|
||||||
|
|
||||||
|
content_attributes.dig(:email, :auto_reply) == true
|
||||||
|
end
|
||||||
|
|
||||||
def valid_first_reply?
|
def valid_first_reply?
|
||||||
return false unless human_response? && !private?
|
return false unless human_response? && !private?
|
||||||
return false if conversation.first_reply_created_at.present?
|
return false if conversation.first_reply_created_at.present?
|
||||||
|
|||||||
@@ -103,7 +103,8 @@ class MailPresenter < SimpleDelegator
|
|||||||
references: references,
|
references: references,
|
||||||
subject: subject,
|
subject: subject,
|
||||||
text_content: text_content,
|
text_content: text_content,
|
||||||
to: to
|
to: to,
|
||||||
|
auto_reply: auto_reply?
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ class MessageTemplates::HookExecutionService
|
|||||||
def perform
|
def perform
|
||||||
return if conversation.campaign.present?
|
return if conversation.campaign.present?
|
||||||
return if conversation.last_incoming_message.blank?
|
return if conversation.last_incoming_message.blank?
|
||||||
|
return if message.auto_reply_email?
|
||||||
|
|
||||||
trigger_templates
|
trigger_templates
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -48,6 +48,13 @@ describe AutomationRuleListener do
|
|||||||
listener.conversation_created(event)
|
listener.conversation_created(event)
|
||||||
expect(AutomationRules::ActionService).not_to have_received(:new).with(automation_rule, account, conversation)
|
expect(AutomationRules::ActionService).not_to have_received(:new).with(automation_rule, account, conversation)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'does not call AutomationRules::ActionService if conversation has auto_reply in additional_attributes' do
|
||||||
|
conversation.additional_attributes = { 'auto_reply' => true }
|
||||||
|
allow(condition_match).to receive(:present?).and_return(true)
|
||||||
|
listener.conversation_created(event)
|
||||||
|
expect(AutomationRules::ActionService).not_to have_received(:new).with(automation_rule, account, conversation)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -165,6 +172,18 @@ describe AutomationRuleListener do
|
|||||||
expect(AutomationRules::ActionService).not_to have_received(:new).with(automation_rule, account, conversation)
|
expect(AutomationRules::ActionService).not_to have_received(:new).with(automation_rule, account, conversation)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'does not call AutomationRules::ActionService if message is auto reply email' do
|
||||||
|
email_channel = create(:channel_email, account: account)
|
||||||
|
email_inbox = create(:inbox, channel: email_channel, account: account)
|
||||||
|
email_conversation = create(:conversation, inbox: email_inbox, account: account)
|
||||||
|
email_message = create(:message, conversation: email_conversation, account: account, content_attributes: { email: { auto_reply: true } })
|
||||||
|
email_event = Events::Base.new('message_created', Time.zone.now, { message: email_message })
|
||||||
|
allow(condition_match).to receive(:present?).and_return(true)
|
||||||
|
|
||||||
|
listener.message_created(email_event)
|
||||||
|
expect(AutomationRules::ActionService).not_to have_received(:new)
|
||||||
|
end
|
||||||
|
|
||||||
it 'does not call AutomationRules::ActionService if conditions do not match based on content' do
|
it 'does not call AutomationRules::ActionService if conditions do not match based on content' do
|
||||||
message.update!(processed_message_content: 'hi', content: "hi\n\nhello")
|
message.update!(processed_message_content: 'hi', content: "hi\n\nhello")
|
||||||
allow(condition_match).to receive(:present?).and_return(false)
|
allow(condition_match).to receive(:present?).and_return(false)
|
||||||
|
|||||||
@@ -111,7 +111,8 @@ RSpec.describe Imap::ImapMailbox do
|
|||||||
let(:auto_reply_mail) { create_inbound_email_from_fixture('auto_reply.eml') }
|
let(:auto_reply_mail) { create_inbound_email_from_fixture('auto_reply.eml') }
|
||||||
|
|
||||||
it 'does not create a new conversation' do
|
it 'does not create a new conversation' do
|
||||||
expect { class_instance.process(auto_reply_mail.mail, channel) }.not_to change(Conversation, :count)
|
expect { class_instance.process(auto_reply_mail.mail, channel) }.to change(Conversation, :count)
|
||||||
|
expect(Conversation.last.additional_attributes['auto_reply']).to be true
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -120,6 +121,8 @@ RSpec.describe Imap::ImapMailbox do
|
|||||||
|
|
||||||
it 'processes the bounced email' do
|
it 'processes the bounced email' do
|
||||||
expect { class_instance.process(bounced_mail.mail, channel) }.to change(Message, :count)
|
expect { class_instance.process(bounced_mail.mail, channel) }.to change(Message, :count)
|
||||||
|
expect(Message.last.content_attributes['email']['auto_reply']).to be true
|
||||||
|
expect(Conversation.last.additional_attributes['auto_reply']).to be true
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
@@ -12,7 +12,8 @@ RSpec.describe ReplyMailbox do
|
|||||||
let(:conversation) { create(:conversation, assignee: agent, inbox: create(:inbox, account: account, greeting_enabled: false), account: account) }
|
let(:conversation) { create(:conversation, assignee: agent, inbox: create(:inbox, account: account, greeting_enabled: false), account: account) }
|
||||||
let(:described_subject) { described_class.receive reply_mail }
|
let(:described_subject) { described_class.receive reply_mail }
|
||||||
let(:serialized_attributes) do
|
let(:serialized_attributes) do
|
||||||
%w[bcc cc content_type date from html_content in_reply_to message_id multipart number_of_attachments references subject text_content to]
|
%w[bcc cc content_type date from html_content in_reply_to message_id multipart number_of_attachments references subject text_content to
|
||||||
|
auto_reply]
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'with reply uuid present' do
|
context 'with reply uuid present' do
|
||||||
|
|||||||
@@ -56,7 +56,7 @@ RSpec.describe SupportMailbox do
|
|||||||
let(:described_subject) { described_class.receive support_mail }
|
let(:described_subject) { described_class.receive support_mail }
|
||||||
let(:serialized_attributes) do
|
let(:serialized_attributes) do
|
||||||
%w[bcc cc content_type date from html_content in_reply_to message_id multipart number_of_attachments references subject
|
%w[bcc cc content_type date from html_content in_reply_to message_id multipart number_of_attachments references subject
|
||||||
text_content to]
|
text_content to auto_reply]
|
||||||
end
|
end
|
||||||
let(:conversation) { Conversation.where(inbox_id: channel_email.inbox).last }
|
let(:conversation) { Conversation.where(inbox_id: channel_email.inbox).last }
|
||||||
|
|
||||||
|
|||||||
@@ -548,4 +548,69 @@ RSpec.describe Message do
|
|||||||
expect(presenter).to have_received(:outgoing_content)
|
expect(presenter).to have_received(:outgoing_content)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe '#auto_reply_email?' do
|
||||||
|
context 'when message is not an incoming email and inbox is not email' do
|
||||||
|
let(:conversation) { create(:conversation) }
|
||||||
|
let(:message) { create(:message, conversation: conversation, message_type: :outgoing) }
|
||||||
|
|
||||||
|
it 'returns false' do
|
||||||
|
expect(message.auto_reply_email?).to be false
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when message is an incoming email' do
|
||||||
|
let(:email_channel) { create(:channel_email) }
|
||||||
|
let(:email_inbox) { create(:inbox, channel: email_channel) }
|
||||||
|
let(:conversation) { create(:conversation, inbox: email_inbox) }
|
||||||
|
|
||||||
|
it 'returns false when auto_reply is not set to true' do
|
||||||
|
message = create(
|
||||||
|
:message,
|
||||||
|
conversation: conversation,
|
||||||
|
message_type: :incoming,
|
||||||
|
content_type: 'incoming_email',
|
||||||
|
content_attributes: {}
|
||||||
|
)
|
||||||
|
expect(message.auto_reply_email?).to be false
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns true when auto_reply is set to true' do
|
||||||
|
message = create(
|
||||||
|
:message,
|
||||||
|
conversation: conversation,
|
||||||
|
message_type: :incoming,
|
||||||
|
content_type: 'incoming_email',
|
||||||
|
content_attributes: { email: { auto_reply: true } }
|
||||||
|
)
|
||||||
|
expect(message.auto_reply_email?).to be true
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when inbox is email' do
|
||||||
|
let(:email_channel) { create(:channel_email) }
|
||||||
|
let(:email_inbox) { create(:inbox, channel: email_channel) }
|
||||||
|
let(:conversation) { create(:conversation, inbox: email_inbox) }
|
||||||
|
|
||||||
|
it 'returns false when auto_reply is not set to true' do
|
||||||
|
message = create(
|
||||||
|
:message,
|
||||||
|
conversation: conversation,
|
||||||
|
message_type: :outgoing,
|
||||||
|
content_attributes: {}
|
||||||
|
)
|
||||||
|
expect(message.auto_reply_email?).to be false
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns true when auto_reply is set to true' do
|
||||||
|
message = create(
|
||||||
|
:message,
|
||||||
|
conversation: conversation,
|
||||||
|
message_type: :outgoing,
|
||||||
|
content_attributes: { email: { auto_reply: true } }
|
||||||
|
)
|
||||||
|
expect(message.auto_reply_email?).to be true
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -49,13 +49,15 @@ RSpec.describe MailPresenter do
|
|||||||
:references,
|
:references,
|
||||||
:subject,
|
:subject,
|
||||||
:text_content,
|
:text_content,
|
||||||
:to
|
:to,
|
||||||
|
:auto_reply
|
||||||
])
|
])
|
||||||
expect(data[:content_type]).to include('multipart/alternative')
|
expect(data[:content_type]).to include('multipart/alternative')
|
||||||
expect(data[:date].to_s).to eq('2020-04-20T04:20:20-04:00')
|
expect(data[:date].to_s).to eq('2020-04-20T04:20:20-04:00')
|
||||||
expect(data[:message_id]).to eq(mail.message_id)
|
expect(data[:message_id]).to eq(mail.message_id)
|
||||||
expect(data[:multipart]).to be(true)
|
expect(data[:multipart]).to be(true)
|
||||||
expect(data[:subject]).to eq(decorated_mail.subject)
|
expect(data[:subject]).to eq(decorated_mail.subject)
|
||||||
|
expect(data[:auto_reply]).to eq(decorated_mail.auto_reply?)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'give email from in downcased format' do
|
it 'give email from in downcased format' do
|
||||||
@@ -136,6 +138,11 @@ RSpec.describe MailPresenter do
|
|||||||
expect(decorated_auto_reply_mail.auto_reply?).to be true
|
expect(decorated_auto_reply_mail.auto_reply?).to be true
|
||||||
expect(decorated_auto_reply_with_auto_submitted_mail.auto_reply?).to be true
|
expect(decorated_auto_reply_with_auto_submitted_mail.auto_reply?).to be true
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'includes auto_reply status in serialized_data' do
|
||||||
|
expect(decorated_auto_reply_mail.serialized_data[:auto_reply]).to be true
|
||||||
|
expect(decorated_mail.serialized_data[:auto_reply]).to be_falsey
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -111,6 +111,35 @@ describe MessageTemplates::HookExecutionService do
|
|||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'when message is an auto reply email' do
|
||||||
|
it 'does not call any template hooks' do
|
||||||
|
contact = create(:contact)
|
||||||
|
conversation = create(:conversation, contact: contact)
|
||||||
|
conversation.inbox.update(greeting_enabled: true, enable_email_collect: true, greeting_message: 'Hi, this is a greeting message')
|
||||||
|
|
||||||
|
message = create(:message, conversation: conversation, content_type: :incoming_email)
|
||||||
|
message.content_attributes = { email: { auto_reply: true } }
|
||||||
|
message.save!
|
||||||
|
|
||||||
|
greeting_service = double
|
||||||
|
email_collect_service = double
|
||||||
|
out_of_office_service = double
|
||||||
|
|
||||||
|
allow(MessageTemplates::Template::Greeting).to receive(:new).and_return(greeting_service)
|
||||||
|
allow(greeting_service).to receive(:perform).and_return(true)
|
||||||
|
allow(MessageTemplates::Template::EmailCollect).to receive(:new).and_return(email_collect_service)
|
||||||
|
allow(email_collect_service).to receive(:perform).and_return(true)
|
||||||
|
allow(MessageTemplates::Template::OutOfOffice).to receive(:new).and_return(out_of_office_service)
|
||||||
|
allow(out_of_office_service).to receive(:perform).and_return(true)
|
||||||
|
|
||||||
|
described_class.new(message: message).perform
|
||||||
|
|
||||||
|
expect(MessageTemplates::Template::Greeting).not_to have_received(:new)
|
||||||
|
expect(MessageTemplates::Template::EmailCollect).not_to have_received(:new)
|
||||||
|
expect(MessageTemplates::Template::OutOfOffice).not_to have_received(:new)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context 'when it is after working hours' do
|
context 'when it is after working hours' do
|
||||||
it 'calls ::MessageTemplates::Template::OutOfOffice' do
|
it 'calls ::MessageTemplates::Template::OutOfOffice' do
|
||||||
contact = create(:contact)
|
contact = create(:contact)
|
||||||
|
|||||||
Reference in New Issue
Block a user