From 615e81731c2797b108e5ced9c4b0b285611b03a9 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Mon, 10 Nov 2025 20:47:18 +0530 Subject: [PATCH 01/66] refactor: strategy pattern for mailbox conversation finding (#12766) Co-authored-by: Pranav Co-authored-by: Muhsin Keloth --- app/mailboxes/application_mailbox.rb | 31 +- app/mailboxes/imap/imap_mailbox.rb | 23 +- app/mailboxes/reply_mailbox.rb | 96 +--- app/mailboxes/support_mailbox.rb | 94 ---- app/services/mailbox/conversation_finder.rb | 29 ++ .../base_strategy.rb | 13 + .../in_reply_to_strategy.rb | 48 ++ .../new_conversation_strategy.rb | 83 ++++ .../receiver_uuid_strategy.rb | 26 + .../references_strategy.rb | 59 +++ spec/mailboxes/application_mailbox_spec.rb | 15 +- spec/mailboxes/imap/imap_mailbox_spec.rb | 49 ++ spec/mailboxes/reply_mailbox_spec.rb | 443 ++++++++++++++++++ spec/mailboxes/support_mailbox_spec.rb | 352 -------------- .../mailbox/conversation_finder_spec.rb | 133 ++++++ .../in_reply_to_strategy_spec.rb | 118 +++++ .../new_conversation_strategy_spec.rb | 161 +++++++ .../receiver_uuid_strategy_spec.rb | 99 ++++ .../references_strategy_spec.rb | 208 ++++++++ 19 files changed, 1527 insertions(+), 553 deletions(-) delete mode 100644 app/mailboxes/support_mailbox.rb create mode 100644 app/services/mailbox/conversation_finder.rb create mode 100644 app/services/mailbox/conversation_finder_strategies/base_strategy.rb create mode 100644 app/services/mailbox/conversation_finder_strategies/in_reply_to_strategy.rb create mode 100644 app/services/mailbox/conversation_finder_strategies/new_conversation_strategy.rb create mode 100644 app/services/mailbox/conversation_finder_strategies/receiver_uuid_strategy.rb create mode 100644 app/services/mailbox/conversation_finder_strategies/references_strategy.rb delete mode 100644 spec/mailboxes/support_mailbox_spec.rb create mode 100644 spec/services/mailbox/conversation_finder_spec.rb create mode 100644 spec/services/mailbox/conversation_finder_strategies/in_reply_to_strategy_spec.rb create mode 100644 spec/services/mailbox/conversation_finder_strategies/new_conversation_strategy_spec.rb create mode 100644 spec/services/mailbox/conversation_finder_strategies/receiver_uuid_strategy_spec.rb create mode 100644 spec/services/mailbox/conversation_finder_strategies/references_strategy_spec.rb diff --git a/app/mailboxes/application_mailbox.rb b/app/mailboxes/application_mailbox.rb index 9fc7a435d..a77f5ea43 100644 --- a/app/mailboxes/application_mailbox.rb +++ b/app/mailboxes/application_mailbox.rb @@ -4,38 +4,21 @@ class ApplicationMailbox < ActionMailbox::Base # Last part is the regex for the UUID # Eg: email should be something like : reply+6bdc3f4d-0bec-4515-a284-5d916fdde489@domain.com REPLY_EMAIL_UUID_PATTERN = /^reply\+([0-9a-f]{8}\b-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-\b[0-9a-f]{12})$/i - CONVERSATION_MESSAGE_ID_PATTERN = %r{conversation/([a-zA-Z0-9-]*?)/messages/(\d+?)@(\w+\.\w+)} - # routes as a reply to existing conversations + # Route all emails to verified channels to the unified reply mailbox + # The ConversationFinder will determine if it's a reply or new conversation routing( - ->(inbound_mail) { valid_to_address?(inbound_mail) && (reply_uuid_mail?(inbound_mail) || in_reply_to_mail?(inbound_mail)) } => :reply - ) - - # routes as a new conversation in email channel - routing( - ->(inbound_mail) { valid_to_address?(inbound_mail) && EmailChannelFinder.new(inbound_mail.mail).perform.present? } => :support + lambda { |inbound_mail| + valid_to_address?(inbound_mail) && + (reply_uuid_mail?(inbound_mail) || EmailChannelFinder.new(inbound_mail.mail).perform.present?) + } => :reply ) # catchall routing(all: :default) class << self - # checks if follow this pattern then send it to reply_mailbox - # - def in_reply_to_mail?(inbound_mail) - in_reply_to = inbound_mail.mail.in_reply_to - - in_reply_to.present? && ( - in_reply_to_matches?(in_reply_to) || Message.exists?(source_id: in_reply_to) - ) - end - - def in_reply_to_matches?(in_reply_to) - Array.wrap(in_reply_to).any? { it.match?(CONVERSATION_MESSAGE_ID_PATTERN) } - end - - # checks if follow this pattern send it to reply_mailbox - # reply+@ + # checks if follows this pattern: reply+@ def reply_uuid_mail?(inbound_mail) inbound_mail.mail.to&.any? do |email| conversation_uuid = email.split('@')[0] diff --git a/app/mailboxes/imap/imap_mailbox.rb b/app/mailboxes/imap/imap_mailbox.rb index 5fea49722..27bc88e06 100644 --- a/app/mailboxes/imap/imap_mailbox.rb +++ b/app/mailboxes/imap/imap_mailbox.rb @@ -3,6 +3,8 @@ class Imap::ImapMailbox include IncomingEmailValidityHelper attr_accessor :channel, :account, :inbox, :conversation, :processed_mail + FALLBACK_CONVERSATION_PATTERN = %r{account/(\d+)/conversation/([a-zA-Z0-9-]+)@} + def process(mail, channel) @inbound_mail = mail @channel = channel @@ -49,19 +51,32 @@ class Imap::ImapMailbox end def find_conversation_by_reference_ids - return if @inbound_mail.references.blank? && in_reply_to.present? + return if @inbound_mail.references.blank? message = find_message_by_references + if message.present? + conversation = @inbox.conversations.find_by(id: message.conversation_id) + return conversation if conversation.present? + end - return if message.nil? - - @inbox.conversations.find(message.conversation_id) + # FALLBACK_PATTERN use to find a conversation that is started by an agent (no incoming message yet) + conversation_id = find_conversation_by_references + @inbox.conversations.find_by(uuid: conversation_id) if conversation_id.present? end def in_reply_to @processed_mail.in_reply_to end + def find_conversation_by_references + references = Array.wrap(@inbound_mail.references) + references.each do |message_id| + match = FALLBACK_CONVERSATION_PATTERN.match(message_id) + + return match[2] if match.present? + end + end + def find_message_by_references message_to_return = nil diff --git a/app/mailboxes/reply_mailbox.rb b/app/mailboxes/reply_mailbox.rb index 0b1efd09a..6e766f83e 100644 --- a/app/mailboxes/reply_mailbox.rb +++ b/app/mailboxes/reply_mailbox.rb @@ -1,88 +1,38 @@ class ReplyMailbox < ApplicationMailbox - attr_accessor :conversation_uuid, :processed_mail + attr_accessor :conversation, :processed_mail - # Last part is the regex for the UUID - # Eg: email should be something like : reply+6bdc3f4d-0bec-4515-a284-5d916fdde489@domain.com - EMAIL_PART_PATTERN = /^reply\+([0-9a-f]{8}\b-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-\b[0-9a-f]{12})$/i - - before_processing :conversation_uuid_from_to_address, - :find_relative_conversation + before_processing :find_conversation def process - return if @conversation.blank? + # Return early if no conversation was found (e.g., notification emails, suspended accounts) + return unless @conversation - decorate_mail - create_message - add_attachments_to_message + # Wrap everything in a transaction to ensure atomicity + # This prevents orphan conversations if message/attachment creation fails + # and ensures idempotency on job retry (conversation won't be duplicated) + ActiveRecord::Base.transaction do + persist_conversation_if_needed + decorate_mail + create_message + add_attachments_to_message + end end private - def find_relative_conversation - if @conversation_uuid - find_conversation_with_uuid - elsif mail.in_reply_to.present? - find_conversation_with_in_reply_to - end + def find_conversation + @conversation = Mailbox::ConversationFinder.new(mail).find + # Log when email is rejected + Rails.logger.info "Email #{mail.message_id} rejected - no conversation found" unless @conversation end - def conversation_uuid_from_to_address - @mail = MailPresenter.new(mail) + def persist_conversation_if_needed + # Save the conversation if it's a new record (from NewConversationStrategy) + # We persist here instead of in the strategy to maintain transaction integrity + return unless @conversation.new_record? - return if @mail.mail_receiver.blank? - - @mail.mail_receiver.each do |email| - username = email.split('@')[0] - match_result = username.match(ApplicationMailbox::REPLY_EMAIL_UUID_PATTERN) - if match_result - @conversation_uuid = match_result.captures - break - end - end - @conversation_uuid - end - - # find conversation uuid from below pattern - # reply+@ - def find_conversation_with_uuid - @conversation = Conversation.find_by(uuid: conversation_uuid) - validate_resource @conversation - end - - def find_conversation_by_uuid(match_result) - @conversation_uuid = match_result.captures[0] - - find_conversation_with_uuid - end - - def find_conversation_by_message_id(in_reply_to) - @message = Message.find_by(source_id: in_reply_to) - @conversation = @message.conversation if @message.present? - @conversation_uuid = @conversation.uuid if @conversation.present? - end - - # find conversation uuid from below pattern - # - def find_conversation_with_in_reply_to - match_result = nil - in_reply_to_addresses = mail.in_reply_to - in_reply_to_addresses = [in_reply_to_addresses] if in_reply_to_addresses.is_a?(String) - in_reply_to_addresses.each do |in_reply_to| - match_result = in_reply_to.match(::ApplicationMailbox::CONVERSATION_MESSAGE_ID_PATTERN) - break if match_result - end - find_by_in_reply_to_addresses(match_result, in_reply_to_addresses) - end - - def find_by_in_reply_to_addresses(match_result, in_reply_to_addresses) - find_conversation_by_uuid(match_result) if match_result - find_conversation_by_message_id(in_reply_to_addresses) if @conversation.blank? - end - - def validate_resource(resource) - Rails.logger.error "[App::Mailboxes::ReplyMailbox] Email conversation with uuid: #{conversation_uuid} not found" if resource.nil? - - resource + @conversation.save! + Rails.logger.info "Created new conversation #{@conversation.id} for email #{mail.message_id}" end def decorate_mail diff --git a/app/mailboxes/support_mailbox.rb b/app/mailboxes/support_mailbox.rb deleted file mode 100644 index 6279293b2..000000000 --- a/app/mailboxes/support_mailbox.rb +++ /dev/null @@ -1,94 +0,0 @@ -class SupportMailbox < ApplicationMailbox - include IncomingEmailValidityHelper - attr_accessor :channel, :account, :inbox, :conversation, :processed_mail - - before_processing :find_channel, - :load_account, - :load_inbox, - :decorate_mail - - def process - Rails.logger.info "Processing email #{mail.message_id} from #{original_sender_email} to #{mail.to} with subject #{mail.subject}" - - # Skip processing email if it belongs to any of the edge cases - return unless incoming_email_from_valid_email? - - ActiveRecord::Base.transaction do - find_or_create_contact - find_or_create_conversation - create_message - add_attachments_to_message - end - end - - private - - def find_channel - find_channel_with_to_mail if @channel.blank? - - raise 'Email channel/inbox not found' if @channel.nil? - - @channel - end - - def find_channel_with_to_mail - @channel = EmailChannelFinder.new(mail).perform - end - - def load_account - @account = @channel.account - end - - def load_inbox - @inbox = @channel.inbox - end - - def decorate_mail - @processed_mail = MailPresenter.new(mail, @account) - end - - def find_conversation_by_in_reply_to - return if in_reply_to.blank? - - @account.conversations.where("additional_attributes->>'in_reply_to' = ?", in_reply_to).first - end - - def in_reply_to - mail['In-Reply-To'].try(:value) - end - - def original_sender_email - @processed_mail.original_sender&.downcase - end - - def find_or_create_conversation - @conversation = find_conversation_by_in_reply_to || ::Conversation.create!({ - account_id: @account.id, - inbox_id: @inbox.id, - contact_id: @contact.id, - contact_inbox_id: @contact_inbox.id, - additional_attributes: { - in_reply_to: in_reply_to, - source: 'email', - auto_reply: @processed_mail.auto_reply?, - mail_subject: @processed_mail.subject, - initiated_at: { - timestamp: Time.now.utc - } - } - }) - end - - def find_or_create_contact - @contact = @inbox.contacts.from_email(original_sender_email) - if @contact.present? - @contact_inbox = ContactInbox.find_by(inbox: @inbox, contact: @contact) - else - create_contact - end - end - - def identify_contact_name - processed_mail.sender_name || processed_mail.from.first.split('@').first - end -end diff --git a/app/services/mailbox/conversation_finder.rb b/app/services/mailbox/conversation_finder.rb new file mode 100644 index 000000000..2f8128ecd --- /dev/null +++ b/app/services/mailbox/conversation_finder.rb @@ -0,0 +1,29 @@ +class Mailbox::ConversationFinder + DEFAULT_STRATEGIES = [ + Mailbox::ConversationFinderStrategies::ReceiverUuidStrategy, + Mailbox::ConversationFinderStrategies::InReplyToStrategy, + Mailbox::ConversationFinderStrategies::ReferencesStrategy, + Mailbox::ConversationFinderStrategies::NewConversationStrategy + ].freeze + + def initialize(mail, strategies: DEFAULT_STRATEGIES) + @mail = mail + @strategies = strategies + end + + def find + @strategies.each do |strategy_class| + conversation = strategy_class.new(@mail).find + + next unless conversation + + strategy_name = strategy_class.name.demodulize.underscore + Rails.logger.info "Conversation found via #{strategy_name} strategy" + return conversation + end + + # Should not reach here if NewConversationStrategy is in the chain + Rails.logger.error 'No conversation found via any strategy (NewConversationStrategy missing?)' + nil + end +end diff --git a/app/services/mailbox/conversation_finder_strategies/base_strategy.rb b/app/services/mailbox/conversation_finder_strategies/base_strategy.rb new file mode 100644 index 000000000..c1e738805 --- /dev/null +++ b/app/services/mailbox/conversation_finder_strategies/base_strategy.rb @@ -0,0 +1,13 @@ +class Mailbox::ConversationFinderStrategies::BaseStrategy + attr_reader :mail + + def initialize(mail) + @mail = mail + end + + # Returns Conversation or nil + # Subclasses must implement this method + def find + raise NotImplementedError, "#{self.class} must implement #find" + end +end diff --git a/app/services/mailbox/conversation_finder_strategies/in_reply_to_strategy.rb b/app/services/mailbox/conversation_finder_strategies/in_reply_to_strategy.rb new file mode 100644 index 000000000..b14f851f5 --- /dev/null +++ b/app/services/mailbox/conversation_finder_strategies/in_reply_to_strategy.rb @@ -0,0 +1,48 @@ +class Mailbox::ConversationFinderStrategies::InReplyToStrategy < Mailbox::ConversationFinderStrategies::BaseStrategy + # Patterns from ApplicationMailbox + MESSAGE_PATTERN = %r{conversation/([a-zA-Z0-9-]+)/messages/(\d+)@} + + # FALLBACK_PATTERN is used when building In-Reply-To headers in ConversationReplyMailer + # when there's no actual message to reply to (see app/mailers/conversation_reply_mailer.rb#in_reply_to_email). + # This happens when: + # - A conversation is started by an agent (no incoming message yet) + # - The conversation originated from a non-email channel (widget, WhatsApp, etc.) but is now using email + # - The incoming message doesn't have email metadata with a message_id + # In these cases, we use a conversation-level identifier instead of a message-level one. + FALLBACK_PATTERN = %r{account/(\d+)/conversation/([a-zA-Z0-9-]+)@} + + def find + return nil if mail.in_reply_to.blank? + + in_reply_to_addresses = Array.wrap(mail.in_reply_to) + + in_reply_to_addresses.each do |in_reply_to| + # Try extracting UUID from patterns + uuid = extract_uuid_from_patterns(in_reply_to) + if uuid + conversation = Conversation.find_by(uuid: uuid) + return conversation if conversation + end + + # Try finding by message source_id + message = Message.find_by(source_id: in_reply_to) + return message.conversation if message&.conversation + end + + nil + end + + private + + def extract_uuid_from_patterns(message_id) + # Try message-specific pattern first + match = MESSAGE_PATTERN.match(message_id) + return match[1] if match + + # Try conversation fallback pattern + match = FALLBACK_PATTERN.match(message_id) + return match[2] if match + + nil + end +end diff --git a/app/services/mailbox/conversation_finder_strategies/new_conversation_strategy.rb b/app/services/mailbox/conversation_finder_strategies/new_conversation_strategy.rb new file mode 100644 index 000000000..a21fcebe5 --- /dev/null +++ b/app/services/mailbox/conversation_finder_strategies/new_conversation_strategy.rb @@ -0,0 +1,83 @@ +class Mailbox::ConversationFinderStrategies::NewConversationStrategy < Mailbox::ConversationFinderStrategies::BaseStrategy + include MailboxHelper + include IncomingEmailValidityHelper + + attr_accessor :processed_mail, :account, :inbox, :contact, :contact_inbox, :conversation, :channel + + def initialize(mail) + super(mail) + @channel = EmailChannelFinder.new(mail).perform + return unless @channel + + @account = @channel.account + @inbox = @channel.inbox + @processed_mail = MailPresenter.new(mail, @account) + end + + # This strategy prepares a new conversation but doesn't persist it yet. + # Why we don't use create! here: + # - Avoids orphan conversations if message/attachment creation fails later + # - Prevents duplicate conversations on job retry (no idempotency issue) + # - Follows the pattern from old SupportMailbox where everything was in one transaction + # The actual persistence happens in ReplyMailbox within a transaction that includes message creation. + def find + return nil unless @channel # No valid channel found + return nil unless incoming_email_from_valid_email? # Skip edge cases + + # Check if conversation already exists by in_reply_to + existing_conversation = find_conversation_by_in_reply_to + return existing_conversation if existing_conversation + + # Prepare contact (persisted) and build conversation (not persisted) + find_or_create_contact + build_conversation + end + + private + + def find_or_create_contact + @contact = @inbox.contacts.from_email(original_sender_email) + if @contact.present? + @contact_inbox = ContactInbox.find_by(inbox: @inbox, contact: @contact) + else + create_contact + end + end + + def original_sender_email + @processed_mail.original_sender&.downcase + end + + def identify_contact_name + @processed_mail.sender_name || @processed_mail.from.first.split('@').first + end + + def build_conversation + # Build but don't persist - ReplyMailbox will save in transaction with message + @conversation = ::Conversation.new( + account_id: @account.id, + inbox_id: @inbox.id, + contact_id: @contact.id, + contact_inbox_id: @contact_inbox.id, + additional_attributes: { + in_reply_to: in_reply_to, + source: 'email', + auto_reply: @processed_mail.auto_reply?, + mail_subject: @processed_mail.subject, + initiated_at: { + timestamp: Time.now.utc + } + } + ) + end + + def in_reply_to + mail['In-Reply-To'].try(:value) + end + + def find_conversation_by_in_reply_to + return if in_reply_to.blank? + + @account.conversations.where("additional_attributes->>'in_reply_to' = ?", in_reply_to).first + end +end diff --git a/app/services/mailbox/conversation_finder_strategies/receiver_uuid_strategy.rb b/app/services/mailbox/conversation_finder_strategies/receiver_uuid_strategy.rb new file mode 100644 index 000000000..39b155cf8 --- /dev/null +++ b/app/services/mailbox/conversation_finder_strategies/receiver_uuid_strategy.rb @@ -0,0 +1,26 @@ +class Mailbox::ConversationFinderStrategies::ReceiverUuidStrategy < Mailbox::ConversationFinderStrategies::BaseStrategy + # Pattern from ApplicationMailbox::REPLY_EMAIL_UUID_PATTERN + UUID_PATTERN = /^reply\+([0-9a-f]{8}\b-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-\b[0-9a-f]{12})$/i + + def find + uuid = extract_uuid_from_receivers + return nil unless uuid + + Conversation.find_by(uuid: uuid) + end + + private + + def extract_uuid_from_receivers + mail_presenter = MailPresenter.new(mail) + return nil if mail_presenter.mail_receiver.blank? + + mail_presenter.mail_receiver.each do |email| + username = email.split('@').first + match = username.match(UUID_PATTERN) + return match[1] if match + end + + nil + end +end diff --git a/app/services/mailbox/conversation_finder_strategies/references_strategy.rb b/app/services/mailbox/conversation_finder_strategies/references_strategy.rb new file mode 100644 index 000000000..86a0aa3c5 --- /dev/null +++ b/app/services/mailbox/conversation_finder_strategies/references_strategy.rb @@ -0,0 +1,59 @@ +class Mailbox::ConversationFinderStrategies::ReferencesStrategy < Mailbox::ConversationFinderStrategies::BaseStrategy + # Patterns from ApplicationMailbox + MESSAGE_PATTERN = %r{conversation/([a-zA-Z0-9-]+)/messages/(\d+)@} + + # FALLBACK_PATTERN is used when building References headers in ConversationReplyMailer + # when there's no actual message to reply to (see app/mailers/conversation_reply_mailer.rb#in_reply_to_email). + # This happens when: + # - A conversation is started by an agent (no incoming message yet) + # - The conversation originated from a non-email channel (widget, WhatsApp, etc.) but is now using email + # - The incoming message doesn't have email metadata with a message_id + # In these cases, we use a conversation-level identifier instead of a message-level one. + FALLBACK_PATTERN = %r{account/(\d+)/conversation/([a-zA-Z0-9-]+)@} + + def initialize(mail) + super(mail) + # Get channel once upfront to use for scoped queries + @channel = EmailChannelFinder.new(mail).perform + end + + def find + return nil if mail.references.blank? + return nil unless @channel # No valid channel found + + references = Array.wrap(mail.references) + + references.each do |reference| + conversation = find_conversation_from_reference(reference) + return conversation if conversation + end + + nil + end + + private + + def find_conversation_from_reference(reference) + # Try extracting UUID from patterns + uuid = extract_uuid_from_patterns(reference) + if uuid + # Query scoped to inbox - prevents cross-account/cross-inbox matches at database level + conversation = Conversation.find_by(uuid: uuid, inbox_id: @channel.inbox.id) + return conversation if conversation + end + + # We scope to the inbox, that way we filter out messages and conversations that don't belong to the channel + message = Message.find_by(source_id: reference, inbox_id: @channel.inbox.id) + message&.conversation + end + + def extract_uuid_from_patterns(message_id) + match = MESSAGE_PATTERN.match(message_id) + return match[1] if match + + match = FALLBACK_PATTERN.match(message_id) + return match[2] if match + + nil + end +end diff --git a/spec/mailboxes/application_mailbox_spec.rb b/spec/mailboxes/application_mailbox_spec.rb index 33bbf9de8..1bc4d0fd1 100644 --- a/spec/mailboxes/application_mailbox_spec.rb +++ b/spec/mailboxes/application_mailbox_spec.rb @@ -41,28 +41,31 @@ RSpec.describe ApplicationMailbox do describe 'Support' do let!(:channel_email) { create(:channel_email) } - it 'routes support emails to Support Mailbox when mail is to channel email' do + it 'routes support emails to Reply Mailbox when mail is to channel email' do # this email is hardcoded in the support.eml, that's why we are updating this + # With NewConversationStrategy, all channel emails route to ReplyMailbox channel_email.update(email: 'care@example.com') dbl = double - expect(SupportMailbox).to receive(:new).and_return(dbl) + expect(ReplyMailbox).to receive(:new).and_return(dbl) expect(dbl).to receive(:perform_processing).and_return(true) described_class.route support_mail end - it 'routes support emails to Support Mailbox when mail is to channel forward to email' do + it 'routes support emails to Reply Mailbox when mail is to channel forward to email' do # this email is hardcoded in the support.eml, that's why we are updating this + # With NewConversationStrategy, all channel emails route to ReplyMailbox channel_email.update(forward_to_email: 'care@example.com') dbl = double - expect(SupportMailbox).to receive(:new).and_return(dbl) + expect(ReplyMailbox).to receive(:new).and_return(dbl) expect(dbl).to receive(:perform_processing).and_return(true) described_class.route support_mail end - it 'routes support emails to Support Mailbox with cc email' do + it 'routes support emails to Reply Mailbox with cc email' do + # With NewConversationStrategy, all channel emails route to ReplyMailbox channel_email.update(email: 'test@example.com') dbl = double - expect(SupportMailbox).to receive(:new).and_return(dbl) + expect(ReplyMailbox).to receive(:new).and_return(dbl) expect(dbl).to receive(:perform_processing).and_return(true) described_class.route reply_cc_mail end diff --git a/spec/mailboxes/imap/imap_mailbox_spec.rb b/spec/mailboxes/imap/imap_mailbox_spec.rb index 4535899cf..309a38a65 100644 --- a/spec/mailboxes/imap/imap_mailbox_spec.rb +++ b/spec/mailboxes/imap/imap_mailbox_spec.rb @@ -264,5 +264,54 @@ RSpec.describe Imap::ImapMailbox do expect(conversation.additional_attributes['in_reply_to']).to eq(multiple_in_reply_to_mail.in_reply_to.first) end end + + context 'when a reply to a conversation started by an agent' do + let(:agent_conversation) { create(:conversation, account: account, inbox: channel.inbox, assignee: agent) } + let(:reply_mail_with_fallback_reference) do + # Simulate an email reply with a reference that matches FALLBACK_PATTERN + reference_id = "account/#{account.id}/conversation/#{agent_conversation.uuid}@chatwoot.com" + create_inbound_email_from_mail( + from: 'email@gmail.com', + to: 'imap@gmail.com', + subject: 'Re: Agent started conversation', + references: [reference_id] + ) + end + + it 'appends email to the existing conversation using FALLBACK_PATTERN' do + expect(agent_conversation.messages.size).to eq(0) + + class_instance.process(reply_mail_with_fallback_reference.mail, channel) + + agent_conversation.reload + expect(agent_conversation.messages.size).to eq(1) + expect(agent_conversation.messages.last.content_attributes['email']['from']).to eq(reply_mail_with_fallback_reference.mail.from) + end + end + + context 'when references contain both message and fallback patterns' do + let(:agent_conversation) { create(:conversation, account: account, inbox: channel.inbox, assignee: agent) } + let(:reply_mail_with_multiple_references) do + # Multiple references including both patterns + fallback_reference = "account/#{account.id}/conversation/#{agent_conversation.uuid}@chatwoot.com" + other_reference = 'some-other-message-id@example.com' + create_inbound_email_from_mail( + from: 'email@gmail.com', + to: 'imap@gmail.com', + subject: 'Re: Multiple references', + references: [other_reference, fallback_reference] + ) + end + + it 'finds conversation using fallback pattern when message lookup fails' do + expect(agent_conversation.messages.size).to eq(0) + + class_instance.process(reply_mail_with_multiple_references.mail, channel) + + agent_conversation.reload + expect(agent_conversation.messages.size).to eq(1) + expect(agent_conversation.messages.last.content_attributes['email']['from']).to eq(reply_mail_with_multiple_references.mail.from) + end + end end end diff --git a/spec/mailboxes/reply_mailbox_spec.rb b/spec/mailboxes/reply_mailbox_spec.rb index 167777218..2658731bc 100644 --- a/spec/mailboxes/reply_mailbox_spec.rb +++ b/spec/mailboxes/reply_mailbox_spec.rb @@ -248,5 +248,448 @@ RSpec.describe ReplyMailbox do ) end end + + context 'with references header' do + let(:reply_mail_with_references) { create_inbound_email_from_fixture('reply_mail_without_uuid.eml') } + let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } + let(:conversation_1) do + create( + :conversation, + assignee: agent, + inbox: email_channel.inbox, + account: account, + additional_attributes: { mail_subject: "Discussion: Let's debate these attachments" } + ) + end + + before do + conversation_1.update!(uuid: '6bdc3f4d-0bec-4515-a284-5d916fdde489') + end + + context 'with message-specific pattern in references' do + before do + reply_mail_with_references.mail['References'] = '' + end + + it 'finds conversation from references header with message pattern' do + described_class.receive reply_mail_with_references + expect(conversation_1.messages.last.content).to include("Let's talk about these images:") + end + end + + context 'with conversation fallback pattern in references' do + before do + reply_mail_with_references.mail['References'] = "" + end + + it 'finds conversation from references header with fallback pattern' do + described_class.receive reply_mail_with_references + expect(conversation_1.messages.last.content).to include("Let's talk about these images:") + end + end + + context 'with multiple references including conversation pattern' do + before do + reply_mail_with_references.mail['References'] = [ + '', + "", + '' + ].join("\r\n ") + end + + it 'finds conversation from any reference in the chain' do + described_class.receive reply_mail_with_references + expect(conversation_1.messages.last.content).to include("Let's talk about these images:") + end + end + + context 'with message source_id in references' do + before do + conversation_1.messages.create!( + source_id: 'original-message-id@test.com', + account_id: account.id, + message_type: 'outgoing', + inbox_id: email_channel.inbox.id, + content: 'Original message' + ) + reply_mail_with_references.mail['References'] = '' + end + + it 'finds conversation from message source_id in references' do + described_class.receive reply_mail_with_references + expect(conversation_1.messages.last.content).to include("Let's talk about these images:") + end + end + + context 'with conversation from different channel in references' do + let(:other_email_channel) { create(:channel_email, email: 'other@example.com', account: account) } + let(:other_conversation) do + create( + :conversation, + assignee: agent, + inbox: other_email_channel.inbox, + account: account + ) + end + + before do + other_conversation.update!(uuid: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee') + reply_mail_with_references.mail['References'] = '' + end + + it 'does not use conversation from different channel' do + described_class.receive reply_mail_with_references + expect(other_conversation.messages.count).to eq(0) + end + end + end + end + + describe 'when a chatwoot notification email is received' do + let(:account) { create(:account) } + let!(:channel_email) { create(:channel_email, email: 'sojan@chatwoot.com', account: account) } + let(:notification_mail) { create_inbound_email_from_fixture('notification.eml') } + let(:described_subject) { described_class.receive notification_mail } + let(:conversation) { Conversation.where(inbox_id: channel_email.inbox).last } + + it 'shouldnt create a conversation in the channel' do + described_subject + expect(conversation.present?).to be(false) + end + end + + describe 'when bounced email with out a sender is recieved' do + let(:account) { create(:account) } + let(:bounced_email) { create_inbound_email_from_fixture('bounced_with_no_from.eml') } + let(:described_subject) { described_class.receive bounced_email } + + it 'shouldnt throw an error' do + create(:channel_email, email: 'support@example.com', account: account) + expect { described_subject }.not_to raise_error + end + end + + describe 'when an account is suspended' do + let(:account) { create(:account, status: :suspended) } + let(:agent) { create(:user, email: 'agent1@example.com', account: account) } + let!(:channel_email) { create(:channel_email, account: account) } + let(:support_mail) { create_inbound_email_from_fixture('support.eml') } + let(:described_subject) { described_class.receive support_mail } + let(:conversation) { Conversation.where(inbox_id: channel_email.inbox).last } + + before do + # this email is hardcoded in the support.eml, that's why we are updating this + channel_email.email = 'care@example.com' + channel_email.save! + end + + it 'shouldnt create a conversation in the channel' do + described_subject + expect(conversation.present?).to be(false) + end + end + + describe 'add mail as a new ticket in the email inbox' do + let(:account) { create(:account) } + let(:agent) { create(:user, email: 'agent1@example.com', account: account) } + let!(:channel_email) { create(:channel_email, account: account) } + let(:support_mail) { create_inbound_email_from_fixture('support.eml') } + let(:support_in_reply_to_mail) { create_inbound_email_from_fixture('support_in_reply_to.eml') } + let(:described_subject) { described_class.receive support_mail } + 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 auto_reply] + end + let(:conversation) { Conversation.where(inbox_id: channel_email.inbox).last } + + before do + # this email is hardcoded in the support.eml, that's why we are updating this + channel_email.email = 'care@example.com' + channel_email.save! + end + + describe 'covers email address format' do + before do + described_class.receive support_in_reply_to_mail + end + + it 'creates contact with proper email address' do + expect(support_in_reply_to_mail.mail['reply_to'].try(:value)).to eq('Sony Mathew ') + expect(conversation.contact.email).to eq('sony@chatwoot.com') + end + end + + describe 'covers basic ticket creation' do + before do + described_subject + end + + it 'create the conversation in the inbox of the email channel' do + expect(conversation.inbox.id).to eq(channel_email.inbox.id) + expect(conversation.additional_attributes['source']).to eq('email') + expect(conversation.contact.email).to eq(support_mail.mail.from.first) + end + + it 'create a new contact as the sender of the email' do + email_sender = Mail::Address.new(support_mail.mail[:from].value).name + expect(conversation.messages.last.sender.email).to eq(support_mail.mail.from.first) + expect(conversation.contact.name).to eq(email_sender) + end + + it 'add the mail content as new message on the conversation' do + expect(conversation.messages.last.content).to eq("Let's talk about these images:") + end + + it 'add the attachments' do + expect(conversation.messages.last.attachments.count).to eq(2) + end + + it 'have proper content_attributes with details of email' do + expect(conversation.messages.last.content_attributes[:email].keys).to eq(serialized_attributes) + end + + it 'set proper content_type' do + expect(conversation.messages.last.content_type).to eq('incoming_email') + end + end + + describe 'email with references header' do + let(:mail_with_references) { create_inbound_email_from_fixture('mail_with_references.eml') } + let(:described_subject) { described_class.receive mail_with_references } + + before do + # reuse the existing channel_email that's already set to 'care@example.com' + described_subject + end + + it 'includes references in the message content_attributes' do + message = conversation.messages.last + email_attributes = message.content_attributes['email'] + + expect(email_attributes['references']).to be_present + expect(email_attributes['references']).to eq(['4e6e35f5a38b4_479f13bb90078178@small-app-01.mail', 'test-reference-id']) + end + + it 'includes references in serialized email attributes' do + message = conversation.messages.last + expect(message.content_attributes['email'].keys).to include('references') + end + end + + describe 'Sender without name' do + let(:support_mail_without_sender_name) { create_inbound_email_from_fixture('support_without_sender_name.eml') } + let(:described_subject) { described_class.receive support_mail_without_sender_name } + + it 'create a new contact with the email' do + described_subject + email_sender = support_mail_without_sender_name.mail.from.first.split('@').first + expect(conversation.messages.last.sender.email).to eq(support_mail.mail.from.first) + expect(conversation.contact.name).to eq(email_sender) + end + end + + describe 'Sender with upcase mail address' do + let(:support_mail_without_sender_name) { create_inbound_email_from_fixture('support_without_sender_name.eml') } + let(:described_subject) { described_class.receive support_mail_without_sender_name } + + it 'create a new inbox with the email case insensitive' do + described_subject + expect(conversation.inbox.id).to eq(channel_email.inbox.id) + end + end + + describe 'handle inbox contacts' do + let!(:contact) { create(:contact, account: account, email: support_mail.mail.from.first) } + let!(:contact_inbox) { create(:contact_inbox, inbox: channel_email.inbox, contact: contact) } + + it 'does not create new contact if that contact exists in the inbox' do + expect do + described_subject + end + .to(not_change { Contact.count } + .and(not_change { ContactInbox.count })) + + expect(conversation.messages.last.sender.id).to eq(contact.id) + expect(conversation.contact_inbox).to eq(contact_inbox) + end + + context 'with uppercase reply-to' do + let(:support_mail) { create_inbound_email_from_fixture('support_uppercase.eml') } + let!(:contact) { create(:contact, account: account, email: support_mail.mail.from.first) } + let!(:contact_inbox) { create(:contact_inbox, inbox: channel_email.inbox, contact: contact) } + + it 'does not create new contact if that contact exists in the inbox' do + expect do + described_subject + end + .to(not_change { Contact.count } + .and(not_change { ContactInbox.count })) + + expect(conversation.messages.last.sender.id).to eq(contact.id) + expect(conversation.contact_inbox).to eq(contact_inbox) + end + end + end + + describe 'group email sender' do + let(:group_sender_support_mail) { create_inbound_email_from_fixture('group_sender_support.eml') } + let(:described_subject) { described_class.receive group_sender_support_mail } + + before do + # this email is hardcoded eml fixture file that's why we are updating this + channel_email.email = 'support@chatwoot.com' + channel_email.save! + end + + it 'create new contact with original sender' do + described_subject + email_sender = Mail::Address.new(group_sender_support_mail.mail[:from].value).name + + expect(conversation.contact.email).to eq(group_sender_support_mail.mail['X-Original-Sender'].value) + expect(conversation.contact.name).to eq(email_sender) + end + end + + describe 'when mail has in reply to email' do + let(:reply_mail_without_uuid) { create_inbound_email_from_fixture('reply_mail_without_uuid.eml') } + let(:described_subject) { described_class.receive reply_mail_without_uuid } + let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } + + before do + email_channel + reply_mail_without_uuid.mail['In-Reply-To'] = 'conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123' + end + + it 'create channel with reply to mail' do + described_subject + conversation_1 = Conversation.last + + expect(conversation_1.messages.last.content).to eq("Let's talk about these images:") + expect(conversation_1.additional_attributes['in_reply_to']).to eq('conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123') + end + + it 'append message to email conversation with same in reply to' do + described_subject + conversation_1 = Conversation.last + + expect(conversation_1.messages.last.content).to eq("Let's talk about these images:") + expect(conversation_1.additional_attributes['in_reply_to']).to eq('conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123') + expect(conversation_1.messages.count).to eq(1) + + reply_mail_without_uuid.mail['In-Reply-To'] = 'conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123' + reply_mail_without_uuid.mail['Message-Id'] = '0CB459E0-0336-41DA-BC88-E6E28C697SFC@chatwoot.com' + + described_class.receive reply_mail_without_uuid + + expect(conversation_1.messages.last.content).to eq("Let's talk about these images:") + expect(conversation_1.additional_attributes['in_reply_to']).to eq('conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123') + expect(conversation_1.messages.count).to eq(2) + end + end + + describe 'Sender with reply_to email address' do + let(:reply_to_mail) { create_inbound_email_from_fixture('reply_to.eml') } + let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } + + it 'prefer reply-to over from address' do + email_channel + described_class.receive reply_to_mail + + conversation_1 = Conversation.last + email = conversation_1.messages.last.content_attributes['email'] + + expect(reply_to_mail.mail['From'].value).to be_present + expect(conversation_1.messages.last.content).to eq("Let's talk about these images:") + expect(reply_to_mail.mail['Reply-To'].value).to include(email['from'][0]) + expect(reply_to_mail.mail['Reply-To'].value).to include(conversation_1.contact.email) + expect(reply_to_mail.mail['From'].value).not_to include(conversation_1.contact.email) + end + end + + describe 'when mail part is not present' do + let(:support_mail) { create_inbound_email_from_fixture('support_1.eml') } + let(:only_text) { create_inbound_email_from_fixture('only_text.eml') } + let(:only_html) { create_inbound_email_from_fixture('only_html.eml') } + let(:only_attachments) { create_inbound_email_from_fixture('only_attachments.eml') } + let(:html_and_attachments) { create_inbound_email_from_fixture('html_and_attachments.eml') } + let(:described_subject) { described_class.receive support_mail } + + it 'Considers raw html mail body' do + described_subject + expect(conversation.inbox.id).to eq(channel_email.inbox.id) + + expect(conversation.messages.last.content_attributes['email']['html_content']['reply']).to include( + <<~BODY.chomp + Hi, + + We are providing you platform from here you can sell paid posts on your website. + + Chatwoot | CS team | [C](https://d33wubrfki0l68.cloudfront.net/973467c532160fd8b940300a43fa85fa2d060307/dc9a0/static/brand-73f58cdefae282ae74cebfa74c1d7003.svg) + + Skype: live:.cid.something + + [] + BODY + ) + expect(conversation.messages.last.content_attributes['email']['subject']).to eq('Get Paid to post an article') + end + + it 'Considers only text body' do + described_class.receive only_text + + expect(conversation.inbox.id).to eq(channel_email.inbox.id) + + expect(conversation.messages.last.content).to eq('text only mail') + expect(conversation.messages.last.content_attributes['email']['subject']).to eq('test text only mail') + end + + it 'Considers only html body' do + described_class.receive only_html + + expect(conversation.inbox.id).to eq(channel_email.inbox.id) + + expect(conversation.messages.last.content).to eq( + <<~BODY.chomp + This is html only mail + BODY + ) + expect(conversation.messages.last.content_attributes['email']['subject']).to eq('test html only mail') + end + + it 'Considers only attachments' do + described_class.receive only_attachments + + expect(conversation.inbox.id).to eq(channel_email.inbox.id) + + expect(conversation.messages.last.content).to be_nil + expect(conversation.messages.last.attachments.count).to eq(1) + expect(conversation.messages.last.content_attributes['email']['subject']).to eq('only attachments') + end + + it 'Considers html and attachments' do + described_class.receive html_and_attachments + + expect(conversation.inbox.id).to eq(channel_email.inbox.id) + + expect(conversation.messages.last.content).to eq('This is html and attachments only mail') + expect(conversation.messages.last.attachments.count).to eq(1) + expect(conversation.messages.last.content_attributes['email']['subject']).to eq('attachment with html') + end + end + + describe 'when BCC processing is disabled for account' do + before do + allow(GlobalConfigService).to receive(:load).with('SKIP_INCOMING_BCC_PROCESSING', '').and_return(account.id.to_s) + end + + it 'does not process BCC-only emails' do + bcc_mail = create_inbound_email_from_fixture('support.eml') + bcc_mail.mail['to'] = nil + bcc_mail.mail['bcc'] = 'care@example.com' + + described_class.receive bcc_mail + expect(conversation.present?).to be(false) + end + end end end diff --git a/spec/mailboxes/support_mailbox_spec.rb b/spec/mailboxes/support_mailbox_spec.rb deleted file mode 100644 index 0dbfbbe3b..000000000 --- a/spec/mailboxes/support_mailbox_spec.rb +++ /dev/null @@ -1,352 +0,0 @@ -require 'rails_helper' - -RSpec.describe SupportMailbox do - include ActionMailbox::TestHelper - - describe 'when a chatwoot notification email is received' do - let(:account) { create(:account) } - let!(:channel_email) { create(:channel_email, email: 'sojan@chatwoot.com', account: account) } - let(:notification_mail) { create_inbound_email_from_fixture('notification.eml') } - let(:described_subject) { described_class.receive notification_mail } - let(:conversation) { Conversation.where(inbox_id: channel_email.inbox).last } - - it 'shouldnt create a conversation in the channel' do - described_subject - expect(conversation.present?).to be(false) - end - end - - describe 'when bounced email with out a sender is recieved' do - let(:account) { create(:account) } - let(:bounced_email) { create_inbound_email_from_fixture('bounced_with_no_from.eml') } - let(:described_subject) { described_class.receive bounced_email } - - it 'shouldnt throw an error' do - create(:channel_email, email: 'support@example.com', account: account) - expect { described_subject }.not_to raise_error - end - end - - describe 'when an account is suspended' do - let(:account) { create(:account, status: :suspended) } - let(:agent) { create(:user, email: 'agent1@example.com', account: account) } - let!(:channel_email) { create(:channel_email, account: account) } - let(:support_mail) { create_inbound_email_from_fixture('support.eml') } - let(:described_subject) { described_class.receive support_mail } - let(:conversation) { Conversation.where(inbox_id: channel_email.inbox).last } - - before do - # this email is hardcoded in the support.eml, that's why we are updating this - channel_email.email = 'care@example.com' - channel_email.save! - end - - it 'shouldnt create a conversation in the channel' do - described_subject - expect(conversation.present?).to be(false) - end - end - - describe 'add mail as a new ticket in the email inbox' do - let(:account) { create(:account) } - let(:agent) { create(:user, email: 'agent1@example.com', account: account) } - let!(:channel_email) { create(:channel_email, account: account) } - let(:support_mail) { create_inbound_email_from_fixture('support.eml') } - let(:support_in_reply_to_mail) { create_inbound_email_from_fixture('support_in_reply_to.eml') } - let(:described_subject) { described_class.receive support_mail } - 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 auto_reply] - end - let(:conversation) { Conversation.where(inbox_id: channel_email.inbox).last } - - before do - # this email is hardcoded in the support.eml, that's why we are updating this - channel_email.email = 'care@example.com' - channel_email.save! - end - - describe 'covers email address format' do - before do - described_class.receive support_in_reply_to_mail - end - - it 'creates contact with proper email address' do - expect(support_in_reply_to_mail.mail['reply_to'].try(:value)).to eq('Sony Mathew ') - expect(conversation.contact.email).to eq('sony@chatwoot.com') - end - end - - describe 'covers basic ticket creation' do - before do - described_subject - end - - it 'create the conversation in the inbox of the email channel' do - expect(conversation.inbox.id).to eq(channel_email.inbox.id) - expect(conversation.additional_attributes['source']).to eq('email') - expect(conversation.contact.email).to eq(support_mail.mail.from.first) - end - - it 'create a new contact as the sender of the email' do - email_sender = Mail::Address.new(support_mail.mail[:from].value).name - expect(conversation.messages.last.sender.email).to eq(support_mail.mail.from.first) - expect(conversation.contact.name).to eq(email_sender) - end - - it 'add the mail content as new message on the conversation' do - expect(conversation.messages.last.content).to eq("Let's talk about these images:") - end - - it 'add the attachments' do - expect(conversation.messages.last.attachments.count).to eq(2) - end - - it 'have proper content_attributes with details of email' do - expect(conversation.messages.last.content_attributes[:email].keys).to eq(serialized_attributes) - end - - it 'set proper content_type' do - expect(conversation.messages.last.content_type).to eq('incoming_email') - end - end - - describe 'email with references header' do - let(:mail_with_references) { create_inbound_email_from_fixture('mail_with_references.eml') } - let(:described_subject) { described_class.receive mail_with_references } - - before do - # reuse the existing channel_email that's already set to 'care@example.com' - described_subject - end - - it 'includes references in the message content_attributes' do - message = conversation.messages.last - email_attributes = message.content_attributes['email'] - - expect(email_attributes['references']).to be_present - expect(email_attributes['references']).to eq(['4e6e35f5a38b4_479f13bb90078178@small-app-01.mail', 'test-reference-id']) - end - - it 'includes references in serialized email attributes' do - message = conversation.messages.last - expect(message.content_attributes['email'].keys).to include('references') - end - end - - describe 'Sender without name' do - let(:support_mail_without_sender_name) { create_inbound_email_from_fixture('support_without_sender_name.eml') } - let(:described_subject) { described_class.receive support_mail_without_sender_name } - - it 'create a new contact with the email' do - described_subject - email_sender = support_mail_without_sender_name.mail.from.first.split('@').first - expect(conversation.messages.last.sender.email).to eq(support_mail.mail.from.first) - expect(conversation.contact.name).to eq(email_sender) - end - end - - describe 'Sender with upcase mail address' do - let(:support_mail_without_sender_name) { create_inbound_email_from_fixture('support_without_sender_name.eml') } - let(:described_subject) { described_class.receive support_mail_without_sender_name } - - it 'create a new inbox with the email case insensitive' do - described_subject - expect(conversation.inbox.id).to eq(channel_email.inbox.id) - end - end - - describe 'handle inbox contacts' do - let!(:contact) { create(:contact, account: account, email: support_mail.mail.from.first) } - let!(:contact_inbox) { create(:contact_inbox, inbox: channel_email.inbox, contact: contact) } - - it 'does not create new contact if that contact exists in the inbox' do - expect do - described_subject - end - .to(not_change { Contact.count } - .and(not_change { ContactInbox.count })) - - expect(conversation.messages.last.sender.id).to eq(contact.id) - expect(conversation.contact_inbox).to eq(contact_inbox) - end - - context 'with uppercase reply-to' do - let(:support_mail) { create_inbound_email_from_fixture('support_uppercase.eml') } - let!(:contact) { create(:contact, account: account, email: support_mail.mail.from.first) } - let!(:contact_inbox) { create(:contact_inbox, inbox: channel_email.inbox, contact: contact) } - - it 'does not create new contact if that contact exists in the inbox' do - expect do - described_subject - end - .to(not_change { Contact.count } - .and(not_change { ContactInbox.count })) - - expect(conversation.messages.last.sender.id).to eq(contact.id) - expect(conversation.contact_inbox).to eq(contact_inbox) - end - end - end - - describe 'group email sender' do - let(:group_sender_support_mail) { create_inbound_email_from_fixture('group_sender_support.eml') } - let(:described_subject) { described_class.receive group_sender_support_mail } - - before do - # this email is hardcoded eml fixture file that's why we are updating this - channel_email.email = 'support@chatwoot.com' - channel_email.save! - end - - it 'create new contact with original sender' do - described_subject - email_sender = Mail::Address.new(group_sender_support_mail.mail[:from].value).name - - expect(conversation.contact.email).to eq(group_sender_support_mail.mail['X-Original-Sender'].value) - expect(conversation.contact.name).to eq(email_sender) - end - end - - describe 'when mail has in reply to email' do - let(:reply_mail_without_uuid) { create_inbound_email_from_fixture('reply_mail_without_uuid.eml') } - let(:described_subject) { described_class.receive reply_mail_without_uuid } - let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } - - before do - email_channel - reply_mail_without_uuid.mail['In-Reply-To'] = 'conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123' - end - - it 'create channel with reply to mail' do - described_subject - conversation_1 = Conversation.last - - expect(conversation_1.messages.last.content).to eq("Let's talk about these images:") - expect(conversation_1.additional_attributes['in_reply_to']).to eq('conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123') - end - - it 'append message to email conversation with same in reply to' do - described_subject - conversation_1 = Conversation.last - - expect(conversation_1.messages.last.content).to eq("Let's talk about these images:") - expect(conversation_1.additional_attributes['in_reply_to']).to eq('conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123') - expect(conversation_1.messages.count).to eq(1) - - reply_mail_without_uuid.mail['In-Reply-To'] = 'conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123' - reply_mail_without_uuid.mail['Message-Id'] = '0CB459E0-0336-41DA-BC88-E6E28C697SFC@chatwoot.com' - - described_class.receive reply_mail_without_uuid - - expect(conversation_1.messages.last.content).to eq("Let's talk about these images:") - expect(conversation_1.additional_attributes['in_reply_to']).to eq('conversation/6bdc3f4d-0bec-4515-a284-5d916fdde489/messages/123') - expect(conversation_1.messages.count).to eq(2) - end - end - - describe 'Sender with reply_to email address' do - let(:reply_to_mail) { create_inbound_email_from_fixture('reply_to.eml') } - let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } - - it 'prefer reply-to over from address' do - email_channel - described_class.receive reply_to_mail - - conversation_1 = Conversation.last - email = conversation_1.messages.last.content_attributes['email'] - - expect(reply_to_mail.mail['From'].value).to be_present - expect(conversation_1.messages.last.content).to eq("Let's talk about these images:") - expect(reply_to_mail.mail['Reply-To'].value).to include(email['from'][0]) - expect(reply_to_mail.mail['Reply-To'].value).to include(conversation_1.contact.email) - expect(reply_to_mail.mail['From'].value).not_to include(conversation_1.contact.email) - end - end - - describe 'when mail part is not present' do - let(:support_mail) { create_inbound_email_from_fixture('support_1.eml') } - let(:only_text) { create_inbound_email_from_fixture('only_text.eml') } - let(:only_html) { create_inbound_email_from_fixture('only_html.eml') } - let(:only_attachments) { create_inbound_email_from_fixture('only_attachments.eml') } - let(:html_and_attachments) { create_inbound_email_from_fixture('html_and_attachments.eml') } - let(:described_subject) { described_class.receive support_mail } - - it 'Considers raw html mail body' do - described_subject - expect(conversation.inbox.id).to eq(channel_email.inbox.id) - - expect(conversation.messages.last.content_attributes['email']['html_content']['reply']).to include( - <<~BODY.chomp - Hi, - - We are providing you platform from here you can sell paid posts on your website. - - Chatwoot | CS team | [C](https://d33wubrfki0l68.cloudfront.net/973467c532160fd8b940300a43fa85fa2d060307/dc9a0/static/brand-73f58cdefae282ae74cebfa74c1d7003.svg) - - Skype: live:.cid.something - - [] - BODY - ) - expect(conversation.messages.last.content_attributes['email']['subject']).to eq('Get Paid to post an article') - end - - it 'Considers only text body' do - described_class.receive only_text - - expect(conversation.inbox.id).to eq(channel_email.inbox.id) - - expect(conversation.messages.last.content).to eq('text only mail') - expect(conversation.messages.last.content_attributes['email']['subject']).to eq('test text only mail') - end - - it 'Considers only html body' do - described_class.receive only_html - - expect(conversation.inbox.id).to eq(channel_email.inbox.id) - - expect(conversation.messages.last.content).to eq( - <<~BODY.chomp - This is html only mail - BODY - ) - expect(conversation.messages.last.content_attributes['email']['subject']).to eq('test html only mail') - end - - it 'Considers only attachments' do - described_class.receive only_attachments - - expect(conversation.inbox.id).to eq(channel_email.inbox.id) - - expect(conversation.messages.last.content).to be_nil - expect(conversation.messages.last.attachments.count).to eq(1) - expect(conversation.messages.last.content_attributes['email']['subject']).to eq('only attachments') - end - - it 'Considers html and attachments' do - described_class.receive html_and_attachments - - expect(conversation.inbox.id).to eq(channel_email.inbox.id) - - expect(conversation.messages.last.content).to eq('This is html and attachments only mail') - expect(conversation.messages.last.attachments.count).to eq(1) - expect(conversation.messages.last.content_attributes['email']['subject']).to eq('attachment with html') - end - end - - describe 'when BCC processing is disabled for account' do - before do - allow(GlobalConfigService).to receive(:load).with('SKIP_INCOMING_BCC_PROCESSING', '').and_return(account.id.to_s) - end - - it 'does not process BCC-only emails' do - bcc_mail = create_inbound_email_from_fixture('support.eml') - bcc_mail.mail['to'] = nil - bcc_mail.mail['bcc'] = 'care@example.com' - - expect { described_class.receive bcc_mail }.to raise_error('Email channel/inbox not found') - end - end - end -end diff --git a/spec/services/mailbox/conversation_finder_spec.rb b/spec/services/mailbox/conversation_finder_spec.rb new file mode 100644 index 000000000..313ca409a --- /dev/null +++ b/spec/services/mailbox/conversation_finder_spec.rb @@ -0,0 +1,133 @@ +require 'rails_helper' + +RSpec.describe Mailbox::ConversationFinder do + let(:account) { create(:account) } + let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } + let(:conversation) { create(:conversation, inbox: email_channel.inbox, account: account) } + let(:mail) { Mail.new } + + describe '#find' do + context 'when receiver uuid strategy finds conversation' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'reply+12345678-1234-1234-1234-123456789012@example.com' + end + + it 'returns the conversation' do + finder = described_class.new(mail) + expect(finder.find).to eq(conversation) + end + + it 'logs which strategy succeeded' do + allow(Rails.logger).to receive(:info) + finder = described_class.new(mail) + finder.find + + expect(Rails.logger).to have_received(:info).with('Conversation found via receiver_uuid_strategy strategy') + end + end + + context 'when in_reply_to strategy finds conversation' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.in_reply_to = 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' + end + + it 'returns the conversation' do + finder = described_class.new(mail) + expect(finder.find).to eq(conversation) + end + + it 'logs which strategy succeeded' do + allow(Rails.logger).to receive(:info) + finder = described_class.new(mail) + finder.find + + expect(Rails.logger).to have_received(:info).with('Conversation found via in_reply_to_strategy strategy') + end + end + + context 'when references strategy finds conversation' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'test@example.com' + mail.references = 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' + end + + it 'returns the conversation' do + finder = described_class.new(mail) + expect(finder.find).to eq(conversation) + end + + it 'logs which strategy succeeded' do + allow(Rails.logger).to receive(:info) + finder = described_class.new(mail) + finder.find + + expect(Rails.logger).to have_received(:info).with('Conversation found via references_strategy strategy') + end + end + + context 'when no strategy finds conversation' do + # With NewConversationStrategy in default strategies, this scenario only happens + # when using custom strategies that exclude NewConversationStrategy + let(:finding_strategies) do + [ + Mailbox::ConversationFinderStrategies::ReceiverUuidStrategy, + Mailbox::ConversationFinderStrategies::InReplyToStrategy, + Mailbox::ConversationFinderStrategies::ReferencesStrategy + ] + end + + it 'returns nil' do + finder = described_class.new(mail, strategies: finding_strategies) + expect(finder.find).to be_nil + end + + it 'logs that no conversation was found' do + allow(Rails.logger).to receive(:error) + finder = described_class.new(mail, strategies: finding_strategies) + finder.find + + expect(Rails.logger).to have_received(:error).with('No conversation found via any strategy (NewConversationStrategy missing?)') + end + end + + context 'with custom strategies' do + let(:custom_strategy_class) do + Class.new(Mailbox::ConversationFinderStrategies::BaseStrategy) do + def find + # Always return nil for testing + nil + end + end + end + + it 'uses provided strategies instead of defaults' do + finder = described_class.new(mail, strategies: [custom_strategy_class]) + expect(finder.find).to be_nil + end + end + + context 'with strategy execution order' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + + # Set up mail so all strategies could match + mail.to = 'reply+12345678-1234-1234-1234-123456789012@example.com' + mail.in_reply_to = 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' + mail.references = 'conversation/12345678-1234-1234-1234-123456789012/messages/456@example.com' + end + + it 'returns conversation from first matching strategy' do + allow(Rails.logger).to receive(:info) + finder = described_class.new(mail) + result = finder.find + + expect(result).to eq(conversation) + # Should only log the first strategy that succeeded (ReceiverUuidStrategy) + expect(Rails.logger).to have_received(:info).once.with('Conversation found via receiver_uuid_strategy strategy') + end + end + end +end diff --git a/spec/services/mailbox/conversation_finder_strategies/in_reply_to_strategy_spec.rb b/spec/services/mailbox/conversation_finder_strategies/in_reply_to_strategy_spec.rb new file mode 100644 index 000000000..3fca8c3c9 --- /dev/null +++ b/spec/services/mailbox/conversation_finder_strategies/in_reply_to_strategy_spec.rb @@ -0,0 +1,118 @@ +require 'rails_helper' + +RSpec.describe Mailbox::ConversationFinderStrategies::InReplyToStrategy do + let(:account) { create(:account) } + let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } + let(:conversation) { create(:conversation, inbox: email_channel.inbox, account: account) } + let(:mail) { Mail.new } + + describe '#find' do + context 'when in_reply_to has message-specific pattern' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.in_reply_to = 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' + end + + it 'extracts UUID and returns conversation' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when in_reply_to has conversation fallback pattern' do + before do + conversation.update!(uuid: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee') + mail.in_reply_to = "account/#{account.id}/conversation/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee@example.com" + end + + it 'extracts UUID and returns conversation' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when in_reply_to matches message source_id' do + let(:message) do + conversation.messages.create!( + source_id: 'original-message-id@example.com', + account_id: account.id, + message_type: 'outgoing', + inbox_id: email_channel.inbox.id, + content: 'Original message' + ) + end + + before do + message # Create the message + mail.in_reply_to = 'original-message-id@example.com' + end + + it 'finds conversation from message source_id' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when in_reply_to has multiple values' do + let(:message) do + conversation.messages.create!( + source_id: 'message-123@example.com', + account_id: account.id, + message_type: 'outgoing', + inbox_id: email_channel.inbox.id, + content: 'Test message' + ) + end + + before do + message # Create the message + mail.in_reply_to = ['some-other-id@example.com', 'message-123@example.com'] + end + + it 'finds conversation from any in_reply_to value' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when in_reply_to is blank' do + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when in_reply_to does not match any pattern or source_id' do + before do + mail.in_reply_to = 'random-message-id@gmail.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when UUID exists but conversation does not' do + before do + mail.in_reply_to = 'conversation/99999999-9999-9999-9999-999999999999/messages/123@example.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'with malformed in_reply_to pattern' do + before do + mail.in_reply_to = 'conversation/not-a-uuid/messages/123@example.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + end +end diff --git a/spec/services/mailbox/conversation_finder_strategies/new_conversation_strategy_spec.rb b/spec/services/mailbox/conversation_finder_strategies/new_conversation_strategy_spec.rb new file mode 100644 index 000000000..8609d0133 --- /dev/null +++ b/spec/services/mailbox/conversation_finder_strategies/new_conversation_strategy_spec.rb @@ -0,0 +1,161 @@ +require 'rails_helper' + +RSpec.describe Mailbox::ConversationFinderStrategies::NewConversationStrategy do + let(:account) { create(:account) } + let(:email_channel) { create(:channel_email, account: account) } + let(:mail) { Mail.new } + + before do + mail.to = [email_channel.email] + mail.from = 'sender@example.com' + mail.subject = 'Test Subject' + mail.message_id = '' + end + + describe '#find' do + context 'when channel is found' do + context 'with new contact' do + it 'builds a new conversation with new contact' do + strategy = described_class.new(mail) + + expect do + conversation = strategy.find + expect(conversation).to be_a(Conversation) + expect(conversation.new_record?).to be(true) # Not persisted yet + expect(conversation.inbox).to eq(email_channel.inbox) + expect(conversation.account).to eq(account) + end.to not_change(Conversation, :count) # No conversation created yet + .and change(Contact, :count).by(1) # Contact is created + .and change(ContactInbox, :count).by(1) + end + + it 'sets conversation attributes correctly' do + strategy = described_class.new(mail) + conversation = strategy.find + + expect(conversation.additional_attributes['source']).to eq('email') + expect(conversation.additional_attributes['mail_subject']).to eq('Test Subject') + expect(conversation.additional_attributes['initiated_at']).to have_key('timestamp') + end + + it 'sets contact attributes correctly' do + strategy = described_class.new(mail) + conversation = strategy.find + + expect(conversation.contact.email).to eq('sender@example.com') + expect(conversation.contact.name).to eq('sender') + end + end + + context 'with existing contact' do + let!(:existing_contact) { create(:contact, email: 'sender@example.com', account: account) } + + before do + create(:contact_inbox, contact: existing_contact, inbox: email_channel.inbox) + end + + it 'builds conversation with existing contact' do + strategy = described_class.new(mail) + + expect do + conversation = strategy.find + expect(conversation).to be_a(Conversation) + expect(conversation.new_record?).to be(true) # Not persisted yet + expect(conversation.contact).to eq(existing_contact) + end.to not_change(Conversation, :count) # No conversation created yet + .and not_change(Contact, :count) + .and not_change(ContactInbox, :count) + end + end + + context 'when mail has In-Reply-To header' do + before do + mail['In-Reply-To'] = '' + end + + it 'stores in_reply_to in additional_attributes' do + strategy = described_class.new(mail) + conversation = strategy.find + + expect(conversation.additional_attributes['in_reply_to']).to eq('') + end + end + + context 'when mail is auto reply' do + before do + mail['X-Autoreply'] = 'yes' + end + + it 'marks conversation as auto_reply' do + strategy = described_class.new(mail) + conversation = strategy.find + + expect(conversation.additional_attributes['auto_reply']).to be true + end + end + + context 'when sender has name in From header' do + before do + mail.from = 'John Doe ' + end + + it 'uses sender name from mail' do + strategy = described_class.new(mail) + conversation = strategy.find + + expect(conversation.contact.name).to eq('John Doe') + end + end + end + + context 'when channel is not found' do + before do + mail.to = ['nonexistent@example.com'] + end + + it 'returns nil' do + strategy = described_class.new(mail) + + expect do + result = strategy.find + expect(result).to be_nil + end.not_to change(Conversation, :count) + end + end + + context 'when contact creation fails' do + before do + builder = instance_double(ContactInboxWithContactBuilder) + allow(ContactInboxWithContactBuilder).to receive(:new).and_return(builder) + allow(builder).to receive(:perform).and_raise(ActiveRecord::RecordInvalid) + end + + it 'rolls back the transaction' do + strategy = described_class.new(mail) + + expect do + strategy.find + end.to raise_error(ActiveRecord::RecordInvalid) + .and not_change(Conversation, :count) + .and not_change(Contact, :count) + .and not_change(ContactInbox, :count) + end + end + + context 'when conversation creation fails' do + before do + # Make conversation build fail with invalid attributes + allow(Conversation).to receive(:new).and_return(Conversation.new) + end + + it 'returns invalid conversation object' do + strategy = described_class.new(mail) + + conversation = strategy.find + expect(conversation).to be_a(Conversation) + expect(conversation.new_record?).to be(true) + expect(conversation.valid?).to be(false) + end + end + end +end diff --git a/spec/services/mailbox/conversation_finder_strategies/receiver_uuid_strategy_spec.rb b/spec/services/mailbox/conversation_finder_strategies/receiver_uuid_strategy_spec.rb new file mode 100644 index 000000000..d45f6f4fe --- /dev/null +++ b/spec/services/mailbox/conversation_finder_strategies/receiver_uuid_strategy_spec.rb @@ -0,0 +1,99 @@ +require 'rails_helper' + +RSpec.describe Mailbox::ConversationFinderStrategies::ReceiverUuidStrategy do + let(:account) { create(:account) } + let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } + let(:conversation) { create(:conversation, inbox: email_channel.inbox, account: account) } + let(:mail) { Mail.new } + + describe '#find' do + context 'when mail has valid reply+uuid format' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'reply+12345678-1234-1234-1234-123456789012@example.com' + end + + it 'returns the conversation' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when mail has uppercase UUID' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'reply+12345678-1234-1234-1234-123456789012@EXAMPLE.COM' + end + + it 'returns the conversation (case-insensitive matching)' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when mail has multiple recipients with valid UUID' do + before do + conversation.update!(uuid: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee') + mail.to = ['other@example.com', 'reply+aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee@example.com'] + end + + it 'extracts UUID from any recipient' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when UUID does not exist in database' do + before do + mail.to = 'reply+99999999-9999-9999-9999-999999999999@example.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when mail has no recipients' do + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when mail recipient has malformed UUID' do + before do + mail.to = 'reply+not-a-valid-uuid@example.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when mail recipient has no reply+ prefix' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'test+12345678-1234-1234-1234-123456789012@example.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when mail recipient has additional text after UUID' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'reply+12345678-1234-1234-1234-123456789012-extra@example.com' + end + + it 'returns nil (UUID must be exact)' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + end +end diff --git a/spec/services/mailbox/conversation_finder_strategies/references_strategy_spec.rb b/spec/services/mailbox/conversation_finder_strategies/references_strategy_spec.rb new file mode 100644 index 000000000..6ef61a58f --- /dev/null +++ b/spec/services/mailbox/conversation_finder_strategies/references_strategy_spec.rb @@ -0,0 +1,208 @@ +require 'rails_helper' + +RSpec.describe Mailbox::ConversationFinderStrategies::ReferencesStrategy do + let(:account) { create(:account) } + let(:email_channel) { create(:channel_email, email: 'test@example.com', account: account) } + let(:conversation) { create(:conversation, inbox: email_channel.inbox, account: account) } + let(:mail) { Mail.new } + + describe '#find' do + context 'when references has message-specific pattern' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'test@example.com' + mail.references = 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' + end + + it 'extracts UUID and returns conversation' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when references has conversation fallback pattern' do + before do + conversation.update!(uuid: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee') + mail.to = 'test@example.com' + mail.references = "account/#{account.id}/conversation/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee@example.com" + end + + it 'extracts UUID and returns conversation' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when references matches message source_id' do + let(:message) do + conversation.messages.create!( + source_id: 'original-message-id@example.com', + account_id: account.id, + message_type: 'outgoing', + inbox_id: email_channel.inbox.id, + content: 'Original message' + ) + end + + before do + message # Create the message + mail.to = 'test@example.com' + mail.references = 'original-message-id@example.com' + end + + it 'finds conversation from message source_id' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when references has multiple values' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'test@example.com' + mail.references = [ + 'some-random-message@gmail.com', + 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com', + 'another-message@outlook.com' + ] + end + + it 'finds conversation from any reference' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when references is blank' do + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when references does not match any pattern or source_id' do + before do + mail.references = 'random-message-id@gmail.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'with channel validation' do + context 'when conversation belongs to the correct channel' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'test@example.com' + mail.references = 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' + end + + it 'returns the conversation' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + + context 'when conversation belongs to a different channel' do + let(:other_email_channel) { create(:channel_email, email: 'other@example.com', account: account) } + let(:other_conversation) do + create( + :conversation, + inbox: other_email_channel.inbox, + account: account + ) + end + + before do + other_conversation.update!(uuid: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee') + # Mail is addressed to test@example.com but references conversation from other@example.com + mail.to = 'test@example.com' + mail.references = 'conversation/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/messages/456@example.com' + end + + it 'returns nil (prevents cross-channel hijacking)' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when channel cannot be determined from mail' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = 'unknown@example.com' # Email not associated with any channel + mail.references = 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when mail has multiple recipients including correct channel' do + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + mail.to = ['other@example.com', 'test@example.com'] + mail.references = 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' + end + + it 'finds the correct channel and returns conversation' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + end + + context 'when UUID exists but conversation does not' do + before do + mail.to = 'test@example.com' + mail.references = 'conversation/99999999-9999-9999-9999-999999999999/messages/123@example.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'with malformed references pattern' do + before do + mail.references = 'conversation/not-a-uuid/messages/123@example.com' + end + + it 'returns nil' do + strategy = described_class.new(mail) + expect(strategy.find).to be_nil + end + end + + context 'when first reference fails channel validation but second succeeds' do + let(:other_email_channel) { create(:channel_email, email: 'other@example.com', account: account) } + let(:other_conversation) do + create( + :conversation, + inbox: other_email_channel.inbox, + account: account + ) + end + + before do + conversation.update!(uuid: '12345678-1234-1234-1234-123456789012') + other_conversation.update!(uuid: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee') + + mail.to = 'test@example.com' + mail.references = [ + 'conversation/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/messages/456@example.com', # Wrong channel + 'conversation/12345678-1234-1234-1234-123456789012/messages/123@example.com' # Correct channel + ] + end + + it 'skips invalid reference and returns conversation from valid reference' do + strategy = described_class.new(mail) + expect(strategy.find).to eq(conversation) + end + end + end +end From e81152608d991e7024733b84ec9c01bfef2e5012 Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Mon, 10 Nov 2025 20:50:02 +0530 Subject: [PATCH 02/66] fix: Issue with processing variables in outgoing email content (#12799) Co-authored-by: Shivam Mishra Co-authored-by: Vinay Keerthi <11478411+stonecharioteer@users.noreply.github.com> Co-authored-by: Sojan Jose --- app/builders/messages/message_builder.rb | 75 ++++++++----------- app/helpers/data_helper.rb | 24 ++++++ app/helpers/email_helper.rb | 22 +++++- .../builders/messages/message_builder_spec.rb | 63 ++++++++++++++++ 4 files changed, 140 insertions(+), 44 deletions(-) create mode 100644 app/helpers/data_helper.rb diff --git a/app/builders/messages/message_builder.rb b/app/builders/messages/message_builder.rb index 857d901e5..af31a0728 100644 --- a/app/builders/messages/message_builder.rb +++ b/app/builders/messages/message_builder.rb @@ -1,5 +1,8 @@ class Messages::MessageBuilder include ::FileTypeHelper + include ::EmailHelper + include ::DataHelper + attr_reader :message def initialize(user, conversation, params) @@ -38,30 +41,12 @@ class Messages::MessageBuilder params = convert_to_hash(@params) content_attributes = params.fetch(:content_attributes, {}) - return parse_json(content_attributes) if content_attributes.is_a?(String) + return safe_parse_json(content_attributes) if content_attributes.is_a?(String) return content_attributes if content_attributes.is_a?(Hash) {} end - # Converts the given object to a hash. - # If it's an instance of ActionController::Parameters, converts it to an unsafe hash. - # Otherwise, returns the object as-is. - def convert_to_hash(obj) - return obj.to_unsafe_h if obj.instance_of?(ActionController::Parameters) - - obj - end - - # Attempts to parse a string as JSON. - # If successful, returns the parsed hash with symbolized names. - # If unsuccessful, returns nil. - def parse_json(content) - JSON.parse(content, symbolize_names: true) - rescue JSON::ParserError - {} - end - def process_attachments return if @attachments.blank? @@ -110,12 +95,6 @@ class Messages::MessageBuilder email_string.gsub(/\s+/, '').split(',') end - def validate_email_addresses(all_emails) - all_emails&.each do |email| - raise StandardError, 'Invalid email address' unless email.match?(URI::MailTo::EMAIL_REGEXP) - end - end - def message_type if @conversation.inbox.channel_type != 'Channel::Api' && @message_type == 'incoming' raise StandardError, 'Incoming messages are only allowed in Api inboxes' @@ -178,14 +157,17 @@ class Messages::MessageBuilder email_attributes = ensure_indifferent_access(@message.content_attributes[:email] || {}) normalized_content = normalize_email_body(@message.content) + # Process liquid templates in normalized content with code block protection + processed_content = process_liquid_in_email_body(normalized_content) + # Use custom HTML content if provided, otherwise generate from message content email_attributes[:html_content] = if custom_email_content_provided? build_custom_html_content else - build_html_content(normalized_content) + build_html_content(processed_content) end - email_attributes[:text_content] = build_text_content(normalized_content) + email_attributes[:text_content] = build_text_content(processed_content) email_attributes end @@ -204,22 +186,6 @@ class Messages::MessageBuilder text_content end - def ensure_indifferent_access(hash) - return {} if hash.blank? - - hash.respond_to?(:with_indifferent_access) ? hash.with_indifferent_access : hash - end - - def normalize_email_body(content) - content.to_s.gsub("\r\n", "\n") - end - - def render_email_html(content) - return '' if content.blank? - - ChatwootMarkdownRenderer.new(content).render_message.to_s - end - def custom_email_content_provided? @params[:email_html_content].present? end @@ -232,4 +198,27 @@ class Messages::MessageBuilder html_content end + + # Liquid processing methods for email content + def process_liquid_in_email_body(content) + return content if content.blank? + return content unless should_process_liquid? + + # Protect code blocks from liquid processing + modified_content = modified_liquid_content(content) + template = Liquid::Template.parse(modified_content) + template.render(drops_with_sender) + rescue Liquid::Error + content + end + + def should_process_liquid? + @message_type == 'outgoing' || @message_type == 'template' + end + + def drops_with_sender + message_drops(@conversation).merge({ + 'agent' => UserDrop.new(sender) + }) + end end diff --git a/app/helpers/data_helper.rb b/app/helpers/data_helper.rb new file mode 100644 index 000000000..66f5a9508 --- /dev/null +++ b/app/helpers/data_helper.rb @@ -0,0 +1,24 @@ +# Provides utility methods for data transformation, hash manipulation, and JSON parsing. +# This module contains helper methods for converting between different data types, +# normalizing hashes, and safely handling JSON operations. +module DataHelper + # Ensures a hash supports indifferent access (string or symbol keys). + # Returns an empty hash if the input is blank. + def ensure_indifferent_access(hash) + return {} if hash.blank? + + hash.respond_to?(:with_indifferent_access) ? hash.with_indifferent_access : hash + end + + def convert_to_hash(obj) + return obj.to_unsafe_h if obj.instance_of?(ActionController::Parameters) + + obj + end + + def safe_parse_json(content) + JSON.parse(content, symbolize_names: true) + rescue JSON::ParserError + {} + end +end diff --git a/app/helpers/email_helper.rb b/app/helpers/email_helper.rb index 05b6a53e3..fcc8b463d 100644 --- a/app/helpers/email_helper.rb +++ b/app/helpers/email_helper.rb @@ -4,6 +4,19 @@ module EmailHelper domain.split('.').first end + def render_email_html(content) + return '' if content.blank? + + ChatwootMarkdownRenderer.new(content).render_message.to_s + end + + # Raise a standard error if any email address is invalid + def validate_email_addresses(emails_to_test) + emails_to_test&.each do |email| + raise StandardError, 'Invalid email address' unless email.match?(URI::MailTo::EMAIL_REGEXP) + end + end + # ref: https://www.rfc-editor.org/rfc/rfc5233.html # This is not a mandatory requirement for email addresses, but it is a common practice. # john+test@xyc.com is the same as john@xyc.com @@ -21,6 +34,10 @@ module EmailHelper end end + def normalize_email_body(content) + content.to_s.gsub("\r\n", "\n") + end + def modified_liquid_content(email) # This regex is used to match the code blocks in the content # We don't want to process liquid in code blocks @@ -29,7 +46,10 @@ module EmailHelper def message_drops(conversation) { - 'contact' => ContactDrop.new(conversation.contact) + 'contact' => ContactDrop.new(conversation.contact), + 'conversation' => ConversationDrop.new(conversation), + 'inbox' => InboxDrop.new(conversation.inbox), + 'account' => AccountDrop.new(conversation.account) } end end diff --git a/spec/builders/messages/message_builder_spec.rb b/spec/builders/messages/message_builder_spec.rb index 64a1aad62..71b3cbf5a 100644 --- a/spec/builders/messages/message_builder_spec.rb +++ b/spec/builders/messages/message_builder_spec.rb @@ -223,6 +223,69 @@ describe Messages::MessageBuilder do expect(message.content_attributes.dig('email', 'text_content', 'full')).to eq 'Regular **markdown** content' end end + + context 'when liquid templates are present in email content' do + let(:contact) { create(:contact, name: 'John', email: 'john@example.com') } + let(:conversation) { create(:conversation, inbox: channel_email.inbox, account: account, contact: contact) } + + it 'processes liquid variables in email content' do + params = ActionController::Parameters.new({ + content: 'Hello {{contact.name}}, your email is {{contact.email}}' + }) + + message = described_class.new(user, conversation, params).perform + + expect(message.content_attributes.dig('email', 'html_content', 'full')).to include('Hello John') + expect(message.content_attributes.dig('email', 'html_content', 'full')).to include('john@example.com') + expect(message.content_attributes.dig('email', 'text_content', 'full')).to eq 'Hello John, your email is john@example.com' + end + + it 'does not process liquid in code blocks' do + params = ActionController::Parameters.new({ + content: 'Hello {{contact.name}}, use this code: `{{contact.email}}`' + }) + + message = described_class.new(user, conversation, params).perform + + expect(message.content_attributes.dig('email', 'text_content', 'full')).to eq 'Hello John, use this code: `{{contact.email}}`' + end + + it 'handles broken liquid syntax gracefully' do + params = ActionController::Parameters.new({ + content: 'Hello {{contact.name} {{invalid}}' + }) + + message = described_class.new(user, conversation, params).perform + + expect(message.content_attributes.dig('email', 'text_content', 'full')).to eq 'Hello {{contact.name} {{invalid}}' + end + + it 'does not process liquid for incoming messages' do + params = ActionController::Parameters.new({ + content: 'Hello {{contact.name}}', + message_type: 'incoming' + }) + + api_channel = create(:channel_api, account: account) + api_conversation = create(:conversation, inbox: api_channel.inbox, account: account, contact: contact) + + message = described_class.new(user, api_conversation, params).perform + + expect(message.content).to eq 'Hello {{contact.name}}' + end + + it 'does not process liquid for private messages' do + params = ActionController::Parameters.new({ + content: 'Hello {{contact.name}}', + private: true + }) + + message = described_class.new(user, conversation, params).perform + + expect(message.content_attributes.dig('email', 'html_content')).to be_nil + expect(message.content_attributes.dig('email', 'text_content')).to be_nil + end + end end end end From c07d03e4e5cc0e4f443ea0f779ba0829ae4d4851 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Tue, 11 Nov 2025 21:21:24 +0530 Subject: [PATCH 03/66] fix: hide pdf citations in captain faq responses (#12839) --- enterprise/lib/captain/tools/faq_lookup_tool.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/enterprise/lib/captain/tools/faq_lookup_tool.rb b/enterprise/lib/captain/tools/faq_lookup_tool.rb index d09b7c2d7..93dd90259 100644 --- a/enterprise/lib/captain/tools/faq_lookup_tool.rb +++ b/enterprise/lib/captain/tools/faq_lookup_tool.rb @@ -28,7 +28,7 @@ class Captain::Tools::FaqLookupTool < Captain::Tools::BasePublicTool Question: #{response.question} Answer: #{response.answer} " - if response.documentable.present? && response.documentable.try(:external_link) + if should_show_source?(response) formatted_response += " Source: #{response.documentable.external_link} " @@ -36,4 +36,13 @@ class Captain::Tools::FaqLookupTool < Captain::Tools::BasePublicTool formatted_response end + + def should_show_source?(response) + return false if response.documentable.blank? + return false unless response.documentable.try(:external_link) + + # Don't show source if it's a PDF placeholder + external_link = response.documentable.external_link + !external_link.start_with?('PDF:') + end end From d6af182f82e8e082950b33e14fd0b909b7e9b7e0 Mon Sep 17 00:00:00 2001 From: Vishnu Narayanan Date: Wed, 12 Nov 2025 16:56:52 +0530 Subject: [PATCH 04/66] fix: Use contact_id instead of sender_id for Instagram message locks (#12841) Previously, the lock key for Instagram used sender_id, which for echo messages (outgoing) would be the account's own ID. This caused all outgoing messages to compete for the same lock, creating a bottleneck during bulk messaging. The fix introduces contact_instagram_id method that correctly identifies the contact's ID regardless of message direction: - For echo messages (outgoing): uses recipient.id (the contact) - For incoming messages: uses sender.id (the contact) This ensures each conversation has a unique lock, allowing parallel processing of webhooks while maintaining race condition protection within individual conversations. Fixes lock acquisition errors in Sidekiq when processing bulk Instagram messages. Fixes https://linear.app/chatwoot/issue/CW-5931/p0-mutexapplicationjoblockacquisitionerror-failed-to-acquire-lock-for ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) --- app/jobs/webhooks/instagram_events_job.rb | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/app/jobs/webhooks/instagram_events_job.rb b/app/jobs/webhooks/instagram_events_job.rb index 3db8b6ba9..6383daff8 100644 --- a/app/jobs/webhooks/instagram_events_job.rb +++ b/app/jobs/webhooks/instagram_events_job.rb @@ -8,7 +8,7 @@ class Webhooks::InstagramEventsJob < MutexApplicationJob def perform(entries) @entries = entries - key = format(::Redis::Alfred::IG_MESSAGE_MUTEX, sender_id: sender_id, ig_account_id: ig_account_id) + key = format(::Redis::Alfred::IG_MESSAGE_MUTEX, sender_id: contact_instagram_id, ig_account_id: ig_account_id) with_lock(key) do process_entries(entries) end @@ -77,6 +77,23 @@ class Webhooks::InstagramEventsJob < MutexApplicationJob @entries&.first&.dig(:id) end + def contact_instagram_id + entry = @entries&.first + return nil unless entry + + # Handle both messaging and standby arrays + messaging = (entry[:messaging].presence || entry[:standby] || []).first + return nil unless messaging + + # For echo messages (outgoing from our account), use recipient's ID (the contact) + # For incoming messages (from contact), use sender's ID (the contact) + if messaging.dig(:message, :is_echo) + messaging.dig(:recipient, :id) + else + messaging.dig(:sender, :id) + end + end + def sender_id @entries&.dig(0, :messaging, 0, :sender, :id) end From 3c9c0298bac938470571b34c67fed5fb53aa2ac5 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Wed, 12 Nov 2025 18:31:09 +0530 Subject: [PATCH 05/66] fix: label tags for contactable inboxes (#12838) --- .../application/contact_inboxes/create.yml | 2 +- .../application/contactable_inboxes/get.yml | 2 +- swagger/swagger.json | 4 +- swagger/tag_groups/application_swagger.json | 146 ++++++++++++++++++ 4 files changed, 150 insertions(+), 4 deletions(-) diff --git a/swagger/paths/application/contact_inboxes/create.yml b/swagger/paths/application/contact_inboxes/create.yml index 64ebeb21c..80984c49f 100644 --- a/swagger/paths/application/contact_inboxes/create.yml +++ b/swagger/paths/application/contact_inboxes/create.yml @@ -1,6 +1,6 @@ post: tags: - - Contact + - Contacts operationId: contactInboxCreation description: Create a contact inbox record for an inbox summary: Create contact inbox diff --git a/swagger/paths/application/contactable_inboxes/get.yml b/swagger/paths/application/contactable_inboxes/get.yml index 357c84597..8eea02bbf 100644 --- a/swagger/paths/application/contactable_inboxes/get.yml +++ b/swagger/paths/application/contactable_inboxes/get.yml @@ -1,6 +1,6 @@ get: tags: - - Contact + - Contacts operationId: contactableInboxesGet description: Get List of contactable Inboxes summary: Get Contactable Inboxes diff --git a/swagger/swagger.json b/swagger/swagger.json index 39022bf84..9bb9ff95a 100644 --- a/swagger/swagger.json +++ b/swagger/swagger.json @@ -3281,7 +3281,7 @@ "/api/v1/accounts/{account_id}/contacts/{id}/contact_inboxes": { "post": { "tags": [ - "Contact" + "Contacts" ], "operationId": "contactInboxCreation", "description": "Create a contact inbox record for an inbox", @@ -3366,7 +3366,7 @@ "/api/v1/accounts/{account_id}/contacts/{id}/contactable_inboxes": { "get": { "tags": [ - "Contact" + "Contacts" ], "operationId": "contactableInboxesGet", "description": "Get List of contactable Inboxes", diff --git a/swagger/tag_groups/application_swagger.json b/swagger/tag_groups/application_swagger.json index b26b913bc..600df4597 100644 --- a/swagger/tag_groups/application_swagger.json +++ b/swagger/tag_groups/application_swagger.json @@ -1821,6 +1821,152 @@ } } }, + "/api/v1/accounts/{account_id}/contacts/{id}/contact_inboxes": { + "post": { + "tags": [ + "Contacts" + ], + "operationId": "contactInboxCreation", + "description": "Create a contact inbox record for an inbox", + "summary": "Create contact inbox", + "parameters": [ + { + "$ref": "#/components/parameters/account_id" + }, + { + "name": "id", + "in": "path", + "schema": { + "type": "number" + }, + "description": "ID of the contact", + "required": true + } + ], + "security": [ + { + "userApiKey": [] + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "inbox_id" + ], + "properties": { + "inbox_id": { + "type": "number", + "description": "The ID of the inbox", + "example": 1 + }, + "source_id": { + "type": "string", + "description": "Contact Inbox Source Id" + } + } + } + } + } + }, + "responses": { + "200": { + "description": "Success", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/contact_inboxes" + } + } + } + }, + "401": { + "description": "Authentication error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/bad_request_error" + } + } + } + }, + "422": { + "description": "Incorrect payload", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/bad_request_error" + } + } + } + } + } + } + }, + "/api/v1/accounts/{account_id}/contacts/{id}/contactable_inboxes": { + "get": { + "tags": [ + "Contacts" + ], + "operationId": "contactableInboxesGet", + "description": "Get List of contactable Inboxes", + "summary": "Get Contactable Inboxes", + "security": [ + { + "userApiKey": [] + } + ], + "parameters": [ + { + "$ref": "#/components/parameters/account_id" + }, + { + "name": "id", + "in": "path", + "schema": { + "type": "number" + }, + "description": "ID of the contact", + "required": true + } + ], + "responses": { + "200": { + "description": "Success", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/contactable_inboxes_response" + } + } + } + }, + "401": { + "description": "Authentication error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/bad_request_error" + } + } + } + }, + "422": { + "description": "Incorrect payload", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/bad_request_error" + } + } + } + } + } + } + }, "/api/v1/accounts/{account_id}/automation_rules": { "parameters": [ { From fb0be60ae21c6c34bde588cce331d41ccee4aeed Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Wed, 12 Nov 2025 18:31:17 +0530 Subject: [PATCH 06/66] chore: Improve captain layout (#12820) --- .../components-next/captain/PageLayout.vue | 90 +++++++++---------- .../captain/assistant/AssistantPlayground.vue | 9 +- .../captain/assistant/MessageList.vue | 22 +++-- .../captain/assistants/playground/Index.vue | 4 +- .../captain/assistants/settings/Settings.vue | 1 - .../dashboard/captain/responses/Index.vue | 5 +- 6 files changed, 70 insertions(+), 61 deletions(-) diff --git a/app/javascript/dashboard/components-next/captain/PageLayout.vue b/app/javascript/dashboard/components-next/captain/PageLayout.vue index 4b6760f98..19303ba01 100644 --- a/app/javascript/dashboard/components-next/captain/PageLayout.vue +++ b/app/javascript/dashboard/components-next/captain/PageLayout.vue @@ -4,7 +4,6 @@ import { OnClickOutside } from '@vueuse/components'; import { useRoute } from 'vue-router'; import { useMapGetter } from 'dashboard/composables/store.js'; import { usePolicy } from 'dashboard/composables/usePolicy'; -import Icon from 'dashboard/components-next/icon/Icon.vue'; import Button from 'dashboard/components-next/button/Button.vue'; import BackButton from 'dashboard/components/widgets/BackButton.vue'; import PaginationFooter from 'dashboard/components-next/pagination/PaginationFooter.vue'; @@ -117,58 +116,57 @@ const handleCreateAssistant = () => {
-
+
- -
-
- +
+ + {{ activeAssistantName }} + +
+ - {{ activeAssistantName }} - -
- -
- - - {{ headerTitle }} - + +
- +
+
+
+ {{ headerTitle }} - -
-
- +
+
+ +
diff --git a/app/javascript/dashboard/components-next/captain/assistant/AssistantPlayground.vue b/app/javascript/dashboard/components-next/captain/assistant/AssistantPlayground.vue index 5fef2d6c1..69cbb9be5 100644 --- a/app/javascript/dashboard/components-next/captain/assistant/AssistantPlayground.vue +++ b/app/javascript/dashboard/components-next/captain/assistant/AssistantPlayground.vue @@ -75,9 +75,9 @@ const sendMessage = async () => {