refactor: refactor OpenaiMultimodalContentService and ResponseBuilderJob

app/services/llm_formatter/conversation_llm_formatter.rb

@@ -5,29 +5,22 @@ class LlmFormatter::ConversationLlmFormatter < LlmFormatter::DefaultLlmFormatter
     sections << "Channel: #{@record.inbox.channel.name}"
     sections << 'Message History:'
     sections << if @record.messages.any?
-                  build_messages(config)
+                  build_messages
                 else
                   'No messages in this conversation'
                 end
 
     sections << "Contact Details: #{@record.contact.to_llm_text}" if config[:include_contact_details]
-    sections << 'Conversation Attributes:'
-    sections << build_attributes
     sections.join("\n")
   end
 
   private
 
-  def build_messages(config = {})
+  def build_messages
     return "No messages in this conversation\n" if @record.messages.empty?
 
     message_text = ''
-    messages = @record.messages.where.not(message_type: :activity).order(created_at: :asc)
-
-    messages.each do |message|
-      # Skip private messages unless explicitly included in config
-      next if message.private? && !config[:include_private_messages]
-
+    @record.messages.chat.order(created_at: :asc).each do |message|
       message_text << format_message(message)
     end
     message_text
@@ -35,14 +28,6 @@ class LlmFormatter::ConversationLlmFormatter < LlmFormatter::DefaultLlmFormatter
 
   def format_message(message)
     sender = message.message_type == 'incoming' ? 'User' : 'Support agent'
-    sender = "[Private Note] #{sender}" if message.private?
     "#{sender}: #{message.content}\n"
   end
-
-  def build_attributes
-    attributes = @record.account.custom_attribute_definitions.with_attribute_model('conversation_attribute').map do |attribute|
-      "#{attribute.attribute_display_name}: #{@record.custom_attributes[attribute.attribute_key]}"
-    end
-    attributes.join("\n")
-  end
 end

app/services/openai_multimodal_content_service.rb

@@ -1,74 +0,0 @@
-class OpenaiMultimodalContentService
-  def initialize(message)
-    @message = message
-  end
-
-  def generate_content
-    parts = []
-
-    parts << text_part(@message.content) if @message.content.present?
-    parts.concat(attachment_parts(@message.attachments)) if @message.attachments.any?
-
-    finalize_content_parts(parts)
-  end
-
-  private
-
-  def text_part(text)
-    { type: 'text', text: text }
-  end
-
-  def attachment_parts(attachments)
-    [].tap do |parts|
-      parts.concat(image_parts(attachments.where(file_type: :image)))
-
-      transcription = extract_audio_transcriptions(attachments)
-      parts << text_part(transcription) if transcription.present?
-
-      parts << text_part('User has shared an attachment') if attachments.where.not(file_type: %i[image audio]).exists?
-    end
-  end
-
-  def image_parts(image_attachments)
-    image_attachments.each_with_object([]) do |attachment, parts|
-      url = get_attachment_url(attachment)
-      next if url.blank?
-
-      parts << {
-        type: 'image_url',
-        image_url: { url: url }
-      }
-    end
-  end
-
-  def finalize_content_parts(parts)
-    return 'Message without content' if parts.blank?
-    return parts.first[:text] if single_text_part?(parts)
-
-    parts
-  end
-
-  def single_text_part?(parts)
-    parts.one? && parts.first[:type] == 'text'
-  end
-
-  def get_attachment_url(attachment)
-    return attachment.external_url if attachment.external_url.present?
-
-    return unless attachment.file.attached?
-
-    attachment.file_url
-  end
-
-  def extract_audio_transcriptions(attachments)
-    audio_attachments = attachments.where(file_type: :audio)
-    return '' if audio_attachments.blank?
-
-    transcriptions = ''
-    audio_attachments.each do |attachment|
-      result = Messages::AudioTranscriptionService.new(attachment).perform
-      transcriptions += result[:transcriptions] if result[:success]
-    end
-    transcriptions
-  end
-end

enterprise/app/jobs/captain/conversation/response_builder_job.rb

@@ -45,7 +45,7 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
       .where(private: false)
       .map do |message|
       {
-        content: message_content_multimodal(message),
+        content: prepare_multimodal_message_content(message),
         role: determine_role(message)
       }
     end
@@ -57,8 +57,8 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
     message.message_type == 'incoming' ? 'user' : 'system'
   end
 
-  def message_content_multimodal(message)
-    OpenaiMultimodalContentService.new(message).generate_content
+  def prepare_multimodal_message_content(message)
+    OpenAiMessageBuilderService.new(message: message).generate_content
   end
 
   def handoff_requested?

enterprise/app/services/captain/open_ai_message_builder_service.rb

@@ -0,0 +1,59 @@
+class Captain::OpenAiMessageBuilderService
+  pattr_initialize [:message!]
+
+  def generate_content
+    parts = []
+    parts << text_part(@message.content) if @message.content.present?
+    parts.concat(attachment_parts(@message.attachments)) if @message.attachments.any?
+
+    return 'Message without content' if parts.blank?
+    return parts.first[:text] if parts.one? && parts.first[:type] == 'text'
+
+    parts
+  end
+
+  private
+
+  def text_part(text)
+    { type: 'text', text: text }
+  end
+
+  def image_part(image_url)
+    { type: 'image_url', image_url: { url: image_url } }
+  end
+
+  def attachment_parts(attachments)
+    image_attachments = attachments.where(file_type: :image)
+    image_content = image_parts(image_attachments)
+
+    transcription = extract_audio_transcriptions(attachments)
+    transcription_part = text_part(transcription) if transcription.present?
+
+    attachment_part = text_part('User has shared an attachment') if attachments.where.not(file_type: %i[image audio]).exists?
+
+    [image_content, transcription_part, attachment_part].flatten.compact
+  end
+
+  def image_parts(image_attachments)
+    image_attachments.each_with_object([]) do |attachment, parts|
+      url = get_attachment_url(attachment)
+      parts << image_part(url) if url.present?
+    end
+  end
+
+  def get_attachment_url(attachment)
+    return attachment.external_url if attachment.external_url.present?
+
+    attachment.file.attached? ? attachment.file_url : nil
+  end
+
+  def extract_audio_transcriptions(attachments)
+    audio_attachments = attachments.where(file_type: :audio)
+    return '' if audio_attachments.blank?
+
+    audio_attachments.map do |attachment|
+      result = Messages::AudioTranscriptionService.new(attachment).perform
+      result[:success] ? result[:transcriptions] : ''
+    end.join
+  end
+end

enterprise/app/services/captain/tools/copilot/get_conversation_service.rb

@@ -30,7 +30,7 @@ class Captain::Tools::Copilot::GetConversationService < Captain::Tools::BaseServ
     conversation = Conversation.find_by(display_id: conversation_id, account_id: @assistant.account_id)
     return 'Conversation not found' if conversation.blank?
 
-    conversation.to_llm_text(include_private_messages: true)
+    conversation.to_llm_text
   end
 
   def active?

spec/enterprise/services/captain/open_ai_message_builder_service_spec.rb

@@ -0,0 +1,309 @@
+require 'rails_helper'
+
+RSpec.describe Captain::OpenAiMessageBuilderService do
+  subject(:service) { described_class.new(message: message) }
+
+  let(:message) { create(:message, content: 'Hello world') }
+
+  describe '#generate_content' do
+    context 'when message has only text content' do
+      it 'returns the text content directly' do
+        expect(service.generate_content).to eq('Hello world')
+      end
+    end
+
+    context 'when message has no content and no attachments' do
+      let(:message) { create(:message, content: nil) }
+
+      it 'returns default message' do
+        expect(service.generate_content).to eq('Message without content')
+      end
+    end
+
+    context 'when message has text content and attachments' do
+      before do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :image, external_url: 'https://example.com/image.jpg')
+        attachment.save!
+      end
+
+      it 'returns an array of content parts' do
+        result = service.generate_content
+        expect(result).to be_an(Array)
+        expect(result).to include({ type: 'text', text: 'Hello world' })
+        expect(result).to include({ type: 'image_url', image_url: { url: 'https://example.com/image.jpg' } })
+      end
+    end
+
+    context 'when message has only non-text attachments' do
+      let(:message) { create(:message, content: nil) }
+
+      before do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :image, external_url: 'https://example.com/image.jpg')
+        attachment.save!
+      end
+
+      it 'returns an array of content parts without text' do
+        result = service.generate_content
+        expect(result).to be_an(Array)
+        expect(result).to include({ type: 'image_url', image_url: { url: 'https://example.com/image.jpg' } })
+        expect(result).not_to include(hash_including(type: 'text', text: 'Hello world'))
+      end
+    end
+  end
+
+  describe '#attachment_parts' do
+    let(:message) { create(:message, content: nil) }
+    let(:attachments) { message.attachments }
+
+    context 'with image attachments' do
+      before do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :image, external_url: 'https://example.com/image.jpg')
+        attachment.save!
+      end
+
+      it 'includes image parts' do
+        result = service.send(:attachment_parts, attachments)
+        expect(result).to include({ type: 'image_url', image_url: { url: 'https://example.com/image.jpg' } })
+      end
+    end
+
+    context 'with audio attachments' do
+      let(:audio_attachment) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :audio)
+        attachment.save!
+        attachment
+      end
+
+      before do
+        allow(Messages::AudioTranscriptionService).to receive(:new).with(audio_attachment).and_return(
+          instance_double(Messages::AudioTranscriptionService, perform: { success: true, transcriptions: 'Audio transcription text' })
+        )
+      end
+
+      it 'includes transcription text part' do
+        audio_attachment # trigger creation
+        result = service.send(:attachment_parts, attachments)
+        expect(result).to include({ type: 'text', text: 'Audio transcription text' })
+      end
+    end
+
+    context 'with other file types' do
+      before do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :file)
+        attachment.save!
+      end
+
+      it 'includes generic attachment message' do
+        result = service.send(:attachment_parts, attachments)
+        expect(result).to include({ type: 'text', text: 'User has shared an attachment' })
+      end
+    end
+
+    context 'with mixed attachment types' do
+      let(:image_attachment) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :image, external_url: 'https://example.com/image.jpg')
+        attachment.save!
+        attachment
+      end
+
+      let(:audio_attachment) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :audio)
+        attachment.save!
+        attachment
+      end
+
+      let(:document_attachment) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :file)
+        attachment.save!
+        attachment
+      end
+
+      before do
+        allow(Messages::AudioTranscriptionService).to receive(:new).with(audio_attachment).and_return(
+          instance_double(Messages::AudioTranscriptionService, perform: { success: true, transcriptions: 'Audio text' })
+        )
+      end
+
+      it 'includes all relevant parts' do
+        image_attachment    # trigger creation
+        audio_attachment    # trigger creation
+        document_attachment # trigger creation
+
+        result = service.send(:attachment_parts, attachments)
+        expect(result).to include({ type: 'image_url', image_url: { url: 'https://example.com/image.jpg' } })
+        expect(result).to include({ type: 'text', text: 'Audio text' })
+        expect(result).to include({ type: 'text', text: 'User has shared an attachment' })
+      end
+    end
+  end
+
+  describe '#image_parts' do
+    let(:message) { create(:message, content: nil) }
+
+    context 'with valid image attachments' do
+      let(:image1) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :image, external_url: 'https://example.com/image1.jpg')
+        attachment.save!
+        attachment
+      end
+
+      let(:image2) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :image, external_url: 'https://example.com/image2.jpg')
+        attachment.save!
+        attachment
+      end
+
+      it 'returns image parts for all valid images' do
+        image1 # trigger creation
+        image2 # trigger creation
+
+        image_attachments = message.attachments.where(file_type: :image)
+        result = service.send(:image_parts, image_attachments)
+
+        expect(result).to include({ type: 'image_url', image_url: { url: 'https://example.com/image1.jpg' } })
+        expect(result).to include({ type: 'image_url', image_url: { url: 'https://example.com/image2.jpg' } })
+      end
+    end
+
+    context 'with image attachments without URLs' do
+      let(:image_attachment) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :image, external_url: nil)
+        attachment.save!
+        attachment
+      end
+
+      before do
+        allow(image_attachment).to receive(:file).and_return(instance_double(ActiveStorage::Attached::One, attached?: false))
+      end
+
+      it 'skips images without valid URLs' do
+        image_attachment # trigger creation
+
+        image_attachments = message.attachments.where(file_type: :image)
+        result = service.send(:image_parts, image_attachments)
+
+        expect(result).to be_empty
+      end
+    end
+  end
+
+  describe '#get_attachment_url' do
+    let(:attachment) do
+      attachment = message.attachments.build(account_id: message.account_id, file_type: :image)
+      attachment.save!
+      attachment
+    end
+
+    context 'when attachment has external_url' do
+      before { attachment.update(external_url: 'https://example.com/image.jpg') }
+
+      it 'returns external_url' do
+        expect(service.send(:get_attachment_url, attachment)).to eq('https://example.com/image.jpg')
+      end
+    end
+
+    context 'when attachment has attached file' do
+      before do
+        attachment.update(external_url: nil)
+        allow(attachment).to receive(:file).and_return(instance_double(ActiveStorage::Attached::One, attached?: true))
+        allow(attachment).to receive(:file_url).and_return('https://local.com/file.jpg')
+      end
+
+      it 'returns file_url' do
+        expect(service.send(:get_attachment_url, attachment)).to eq('https://local.com/file.jpg')
+      end
+    end
+
+    context 'when attachment has no URL or file' do
+      before do
+        attachment.update(external_url: nil)
+        allow(attachment).to receive(:file).and_return(instance_double(ActiveStorage::Attached::One, attached?: false))
+      end
+
+      it 'returns nil' do
+        expect(service.send(:get_attachment_url, attachment)).to be_nil
+      end
+    end
+  end
+
+  describe '#extract_audio_transcriptions' do
+    let(:message) { create(:message, content: nil) }
+
+    context 'with no audio attachments' do
+      it 'returns empty string' do
+        result = service.send(:extract_audio_transcriptions, message.attachments)
+        expect(result).to eq('')
+      end
+    end
+
+    context 'with successful audio transcriptions' do
+      let(:audio1) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :audio)
+        attachment.save!
+        attachment
+      end
+
+      let(:audio2) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :audio)
+        attachment.save!
+        attachment
+      end
+
+      before do
+        allow(Messages::AudioTranscriptionService).to receive(:new).with(audio1).and_return(
+          instance_double(Messages::AudioTranscriptionService, perform: { success: true, transcriptions: 'First audio text. ' })
+        )
+        allow(Messages::AudioTranscriptionService).to receive(:new).with(audio2).and_return(
+          instance_double(Messages::AudioTranscriptionService, perform: { success: true, transcriptions: 'Second audio text.' })
+        )
+      end
+
+      it 'concatenates all successful transcriptions' do
+        audio1 # trigger creation
+        audio2 # trigger creation
+
+        attachments = message.attachments
+        result = service.send(:extract_audio_transcriptions, attachments)
+        expect(result).to eq('First audio text. Second audio text.')
+      end
+    end
+
+    context 'with failed audio transcriptions' do
+      let(:audio_attachment) do
+        attachment = message.attachments.build(account_id: message.account_id, file_type: :audio)
+        attachment.save!
+        attachment
+      end
+
+      before do
+        allow(Messages::AudioTranscriptionService).to receive(:new).with(audio_attachment).and_return(
+          instance_double(Messages::AudioTranscriptionService, perform: { success: false, transcriptions: nil })
+        )
+      end
+
+      it 'returns empty string for failed transcriptions' do
+        audio_attachment # trigger creation
+
+        attachments = message.attachments
+        result = service.send(:extract_audio_transcriptions, attachments)
+        expect(result).to eq('')
+      end
+    end
+  end
+
+  describe 'private helper methods' do
+    describe '#text_part' do
+      it 'returns correct text part format' do
+        result = service.send(:text_part, 'Hello world')
+        expect(result).to eq({ type: 'text', text: 'Hello world' })
+      end
+    end
+
+    describe '#image_part' do
+      it 'returns correct image part format' do
+        result = service.send(:image_part, 'https://example.com/image.jpg')
+        expect(result).to eq({ type: 'image_url', image_url: { url: 'https://example.com/image.jpg' } })
+      end
+    end
+  end
+end