From 4bbc2b7abe23d22fc9cae37faeb31ac07813aabd Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Wed, 17 Dec 2025 18:35:02 +0530 Subject: [PATCH] refactor: better delegation of captain tasks --- config/routes.rb | 5 +- .../v1/accounts/captain/editor_controller.rb | 59 +++++++++----- lib/captain/base_editor_service.rb | 10 +-- lib/captain/label_suggestion_service.rb | 8 +- lib/captain/reply_suggestion_service.rb | 10 ++- lib/captain/rewrite_service.rb | 76 +++++++++++-------- lib/captain/summary_service.rb | 10 ++- spec/lib/captain/base_editor_service_spec.rb | 31 ++++---- .../captain/label_suggestion_service_spec.rb | 38 ++++------ .../captain/reply_suggestion_service_spec.rb | 20 ++--- spec/lib/captain/rewrite_service_spec.rb | 56 ++++++-------- spec/lib/captain/summary_service_spec.rb | 18 ++--- 12 files changed, 181 insertions(+), 160 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index b9eb8088a..9f2dda3bf 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -70,7 +70,10 @@ Rails.application.routes.draw do resources :custom_tools resources :documents, only: [:index, :show, :create, :destroy] resource :editor, only: [], controller: 'editor' do - post :process_event + post :rewrite + post :summarize + post :reply_suggestion + post :label_suggestion end end resource :saml_settings, only: [:show, :create, :update, :destroy] diff --git a/enterprise/app/controllers/api/v1/accounts/captain/editor_controller.rb b/enterprise/app/controllers/api/v1/accounts/captain/editor_controller.rb index 59e67792b..231f355de 100644 --- a/enterprise/app/controllers/api/v1/accounts/captain/editor_controller.rb +++ b/enterprise/app/controllers/api/v1/accounts/captain/editor_controller.rb @@ -1,28 +1,47 @@ class Api::V1::Accounts::Captain::EditorController < Api::V1::Accounts::BaseController before_action :check_authorization - EVENT_SERVICE_MAP = { - 'fix_spelling_grammar' => Captain::RewriteService, - 'casual' => Captain::RewriteService, - 'professional' => Captain::RewriteService, - 'friendly' => Captain::RewriteService, - 'confident' => Captain::RewriteService, - 'straightforward' => Captain::RewriteService, - 'improve' => Captain::RewriteService, - 'summarize' => Captain::SummaryService, - 'reply_suggestion' => Captain::ReplySuggestionService, - 'label_suggestion' => Captain::LabelSuggestionService - }.freeze - - def process_event - service_class = EVENT_SERVICE_MAP[params[:event]['name']] - return render json: { error: 'Unknown event' }, status: :unprocessable_entity unless service_class - - result = service_class.new( + def rewrite + result = Captain::RewriteService.new( account: Current.account, - event: params[:event] + content: params[:content], + action: params[:action], + conversation_display_id: params[:conversation_display_id] ).perform + render_result(result) + end + + def summarize + result = Captain::SummaryService.new( + account: Current.account, + conversation_display_id: params[:conversation_display_id] + ).perform + + render_result(result) + end + + def reply_suggestion + result = Captain::ReplySuggestionService.new( + account: Current.account, + conversation_display_id: params[:conversation_display_id] + ).perform + + render_result(result) + end + + def label_suggestion + result = Captain::LabelSuggestionService.new( + account: Current.account, + conversation_display_id: params[:conversation_display_id] + ).perform + + render_result(result) + end + + private + + def render_result(result) if result.nil? render json: { message: nil } elsif result[:error] @@ -32,8 +51,6 @@ class Api::V1::Accounts::Captain::EditorController < Api::V1::Accounts::BaseCont end end - private - def check_authorization authorize(:'captain/editor') end diff --git a/lib/captain/base_editor_service.rb b/lib/captain/base_editor_service.rb index 486d5a309..cc5421e46 100644 --- a/lib/captain/base_editor_service.rb +++ b/lib/captain/base_editor_service.rb @@ -8,20 +8,16 @@ class Captain::BaseEditorService TOKEN_LIMIT = 400_000 GPT_MODEL = Llm::Config::DEFAULT_MODEL - pattr_initialize [:account!, :event!] - - def perform - send("#{event_name}_message") - end + pattr_initialize [:account!, { conversation_display_id: nil }] private def event_name - event['name'] + raise NotImplementedError, "#{self.class} must implement #event_name" end def conversation - @conversation ||= account.conversations.find_by(display_id: event['data']['conversation_display_id']) + @conversation ||= account.conversations.find_by(display_id: conversation_display_id) end def api_base diff --git a/lib/captain/label_suggestion_service.rb b/lib/captain/label_suggestion_service.rb index b06514721..b9b1bddec 100644 --- a/lib/captain/label_suggestion_service.rb +++ b/lib/captain/label_suggestion_service.rb @@ -1,5 +1,7 @@ class Captain::LabelSuggestionService < Captain::BaseEditorService - def label_suggestion_message + pattr_initialize [:account!, :conversation_display_id!] + + def perform # Check cache first cached_response = read_from_cache return cached_response if cached_response.present? @@ -80,4 +82,8 @@ class Captain::LabelSuggestionService < Captain::BaseEditorService true end + + def event_name + 'label_suggestion' + end end diff --git a/lib/captain/reply_suggestion_service.rb b/lib/captain/reply_suggestion_service.rb index ceea19172..1bc66948c 100644 --- a/lib/captain/reply_suggestion_service.rb +++ b/lib/captain/reply_suggestion_service.rb @@ -1,5 +1,7 @@ class Captain::ReplySuggestionService < Captain::BaseEditorService - def reply_suggestion_message + pattr_initialize [:account!, :conversation_display_id!] + + def perform make_api_call( model: GPT_MODEL, messages: [ @@ -7,4 +9,10 @@ class Captain::ReplySuggestionService < Captain::BaseEditorService ].concat(conversation_messages) ) end + + private + + def event_name + 'reply_suggestion' + end end diff --git a/lib/captain/rewrite_service.rb b/lib/captain/rewrite_service.rb index 8ed2a8257..ca2b8f2fb 100644 --- a/lib/captain/rewrite_service.rb +++ b/lib/captain/rewrite_service.rb @@ -1,42 +1,48 @@ class Captain::RewriteService < Captain::BaseEditorService - def fix_spelling_grammar_message - call_llm_with_prompt(prompt_from_file('fix_spelling_grammar')) - end + pattr_initialize [:account!, :content!, :action!, { conversation_display_id: nil }] - def confident_message - call_llm_with_prompt(tone_rewrite_prompt('confident')) - end - - def straightforward_message - call_llm_with_prompt(tone_rewrite_prompt('straightforward')) - end - - def casual_message - call_llm_with_prompt(tone_rewrite_prompt('casual')) - end - - def friendly_message - call_llm_with_prompt(tone_rewrite_prompt('friendly')) - end - - def professional_message - call_llm_with_prompt(tone_rewrite_prompt('professional')) - end - - def improve_message - template = prompt_from_file('improve') - - system_prompt = render_liquid_template(template, { - 'conversation_context' => conversation.to_llm_text(include_contact_details: true), - 'draft_message' => event['data']['content'] - }) - - call_llm_with_prompt(system_prompt, event['data']['content']) + def perform + send(action) end private - def call_llm_with_prompt(system_content, user_content = event['data']['content']) + def fix_spelling_grammar + call_llm_with_prompt(prompt_from_file('fix_spelling_grammar')) + end + + def casual + call_llm_with_prompt(tone_rewrite_prompt('casual')) + end + + def professional + call_llm_with_prompt(tone_rewrite_prompt('professional')) + end + + def friendly + call_llm_with_prompt(tone_rewrite_prompt('friendly')) + end + + def confident + call_llm_with_prompt(tone_rewrite_prompt('confident')) + end + + def straightforward + call_llm_with_prompt(tone_rewrite_prompt('straightforward')) + end + + def improve + template = prompt_from_file('improve') + + system_prompt = render_liquid_template(template, { + 'conversation_context' => conversation.to_llm_text(include_contact_details: true), + 'draft_message' => content + }) + + call_llm_with_prompt(system_prompt, content) + end + + def call_llm_with_prompt(system_content, user_content = content) make_api_call( model: GPT_MODEL, messages: [ @@ -54,4 +60,8 @@ class Captain::RewriteService < Captain::BaseEditorService template = prompt_from_file('tone_rewrite') render_liquid_template(template, 'tone' => tone) end + + def event_name + action + end end diff --git a/lib/captain/summary_service.rb b/lib/captain/summary_service.rb index 1c2f94aac..46c45af45 100644 --- a/lib/captain/summary_service.rb +++ b/lib/captain/summary_service.rb @@ -1,5 +1,7 @@ class Captain::SummaryService < Captain::BaseEditorService - def summarize_message + pattr_initialize [:account!, :conversation_display_id!] + + def perform make_api_call( model: GPT_MODEL, messages: [ @@ -8,4 +10,10 @@ class Captain::SummaryService < Captain::BaseEditorService ] ) end + + private + + def event_name + 'summarize' + end end diff --git a/spec/lib/captain/base_editor_service_spec.rb b/spec/lib/captain/base_editor_service_spec.rb index 98bd2bf33..1cc3db873 100644 --- a/spec/lib/captain/base_editor_service_spec.rb +++ b/spec/lib/captain/base_editor_service_spec.rb @@ -4,39 +4,44 @@ RSpec.describe Captain::BaseEditorService do let(:account) { create(:account) } let(:inbox) { create(:inbox, account: account) } let(:conversation) { create(:conversation, account: account, inbox: inbox) } - let(:event) do - { - 'name' => 'test_event', - 'data' => { - 'conversation_display_id' => conversation.display_id, - 'content' => 'Test content' - } - } - end # Create a concrete test service class since BaseEditorService is abstract let(:test_service_class) do Class.new(described_class) do - def test_event_message + def perform { message: 'Test response' } end + + def event_name + 'test_event' + end end end - let(:service) { test_service_class.new(account: account, event: event) } + let(:service) { test_service_class.new(account: account, conversation_display_id: conversation.display_id) } before do create(:installation_config, name: 'CAPTAIN_OPEN_AI_API_KEY', value: 'test-key') end describe '#perform' do - it 'calls the correct service method based on event name' do - expect(service).to receive(:test_event_message).and_call_original + it 'returns the expected result' do result = service.perform expect(result).to eq({ message: 'Test response' }) end end + describe '#event_name' do + it 'raises NotImplementedError for base class' do + base_service = described_class.new(account: account, conversation_display_id: conversation.display_id) + expect { base_service.send(:event_name) }.to raise_error(NotImplementedError, /must implement #event_name/) + end + + it 'returns custom event name in subclass' do + expect(service.send(:event_name)).to eq('test_event') + end + end + describe '#conversation' do it 'finds conversation by display_id' do expect(service.send(:conversation)).to eq(conversation) diff --git a/spec/lib/captain/label_suggestion_service_spec.rb b/spec/lib/captain/label_suggestion_service_spec.rb index dd36bd665..f99864288 100644 --- a/spec/lib/captain/label_suggestion_service_spec.rb +++ b/spec/lib/captain/label_suggestion_service_spec.rb @@ -6,15 +6,7 @@ RSpec.describe Captain::LabelSuggestionService do let(:conversation) { create(:conversation, account: account, inbox: inbox) } let(:label1) { create(:label, account: account, title: 'bug') } let(:label2) { create(:label, account: account, title: 'feature-request') } - let(:event) do - { - 'name' => 'label_suggestion', - 'data' => { - 'conversation_display_id' => conversation.display_id - } - } - end - let(:service) { described_class.new(account: account, event: event) } + let(:service) { described_class.new(account: account, conversation_display_id: conversation.display_id) } let(:mock_chat) { instance_double(RubyLLM::Chat) } let(:mock_context) { instance_double(RubyLLM::Context, chat: mock_chat) } let(:mock_response) { instance_double(RubyLLM::Message, content: 'bug, feature-request', input_tokens: 100, output_tokens: 20) } @@ -39,7 +31,7 @@ RSpec.describe Captain::LabelSuggestionService do end it 'returns label suggestions' do - result = service.label_suggestion_message + result = service.perform expect(result[:message]).to eq('bug, feature-request') end @@ -47,7 +39,7 @@ RSpec.describe Captain::LabelSuggestionService do it 'removes "Labels:" prefix from response' do allow(mock_response).to receive(:content).and_return('Labels: bug, feature-request') - result = service.label_suggestion_message + result = service.perform expect(result[:message]).to eq(' bug, feature-request') end @@ -55,7 +47,7 @@ RSpec.describe Captain::LabelSuggestionService do it 'removes "Label:" prefix (singular) from response' do allow(mock_response).to receive(:content).and_return('label: bug') - result = service.label_suggestion_message + result = service.perform expect(result[:message]).to eq(' bug') end @@ -70,7 +62,7 @@ RSpec.describe Captain::LabelSuggestionService do { message: 'bug' } end - service.label_suggestion_message + service.perform end end @@ -79,7 +71,7 @@ RSpec.describe Captain::LabelSuggestionService do create(:message, conversation: conversation, message_type: :incoming, content: 'Message 1') create(:message, conversation: conversation, message_type: :incoming, content: 'Message 2') - result = service.label_suggestion_message + result = service.perform expect(result).to be_nil end @@ -89,7 +81,7 @@ RSpec.describe Captain::LabelSuggestionService do create(:message, conversation: conversation, message_type: :incoming, content: "Message #{i}") end - result = service.label_suggestion_message + result = service.perform expect(result).to be_nil end @@ -100,7 +92,7 @@ RSpec.describe Captain::LabelSuggestionService do end create(:message, conversation: conversation, message_type: :outgoing, content: 'Agent reply') - result = service.label_suggestion_message + result = service.perform expect(result).to be_nil end @@ -116,13 +108,13 @@ RSpec.describe Captain::LabelSuggestionService do it 'reads from cache on cache hit' do # Warm up cache - service.label_suggestion_message + service.perform # Create new service instance to test cache read - new_service = described_class.new(account: account, event: event) + new_service = described_class.new(account: account, conversation_display_id: conversation.display_id) expect(new_service).not_to receive(:make_api_call) - result = new_service.label_suggestion_message + result = new_service.perform expect(result[:message]).to eq('bug, feature-request') end @@ -130,7 +122,7 @@ RSpec.describe Captain::LabelSuggestionService do it 'writes to cache on cache miss' do expect(Redis::Alfred).to receive(:setex).and_call_original - service.label_suggestion_message + service.perform end it 'returns nil for invalid cached JSON' do @@ -138,7 +130,7 @@ RSpec.describe Captain::LabelSuggestionService do cache_key = service.send(:cache_key) Redis::Alfred.set(cache_key, 'invalid json') - result = service.label_suggestion_message + result = service.perform # Should make API call since cache read failed expect(result[:message]).to eq('bug, feature-request') @@ -150,7 +142,7 @@ RSpec.describe Captain::LabelSuggestionService do expect(Redis::Alfred).not_to receive(:setex) - service.label_suggestion_message + service.perform end end @@ -164,7 +156,7 @@ RSpec.describe Captain::LabelSuggestionService do end it 'returns nil' do - result = service.label_suggestion_message + result = service.perform expect(result).to be_nil end diff --git a/spec/lib/captain/reply_suggestion_service_spec.rb b/spec/lib/captain/reply_suggestion_service_spec.rb index c5ce45ea2..a331f5412 100644 --- a/spec/lib/captain/reply_suggestion_service_spec.rb +++ b/spec/lib/captain/reply_suggestion_service_spec.rb @@ -4,15 +4,7 @@ RSpec.describe Captain::ReplySuggestionService do let(:account) { create(:account) } let(:inbox) { create(:inbox, account: account) } let(:conversation) { create(:conversation, account: account, inbox: inbox) } - let(:event) do - { - 'name' => 'reply_suggestion', - 'data' => { - 'conversation_display_id' => conversation.display_id - } - } - end - let(:service) { described_class.new(account: account, event: event) } + let(:service) { described_class.new(account: account, conversation_display_id: conversation.display_id) } let(:mock_chat) { instance_double(RubyLLM::Chat) } let(:mock_context) { instance_double(RubyLLM::Context, chat: mock_chat) } let(:mock_response) { instance_double(RubyLLM::Message, content: 'Suggested reply', input_tokens: 100, output_tokens: 50) } @@ -25,7 +17,7 @@ RSpec.describe Captain::ReplySuggestionService do allow(mock_chat).to receive(:ask).and_return(mock_response) end - describe '#reply_suggestion_message' do + describe '#perform' do let(:message1) { create(:message, conversation: conversation, message_type: :incoming, content: 'Hello') } let(:message2) { create(:message, conversation: conversation, message_type: :outgoing, content: 'Hi there') } @@ -36,7 +28,7 @@ RSpec.describe Captain::ReplySuggestionService do it 'uses conversation_messages to build message history' do expect(service).to receive(:conversation_messages).and_call_original - service.reply_suggestion_message + service.perform end it 'concatenates system prompt with conversation history' do @@ -53,7 +45,7 @@ RSpec.describe Captain::ReplySuggestionService do { message: 'Suggested reply' } end - service.reply_suggestion_message + service.perform end it 'passes correct model to API' do @@ -61,11 +53,11 @@ RSpec.describe Captain::ReplySuggestionService do hash_including(model: Captain::BaseEditorService::GPT_MODEL) ).and_call_original - service.reply_suggestion_message + service.perform end it 'returns formatted response' do - result = service.reply_suggestion_message + result = service.perform expect(result[:message]).to eq('Suggested reply') expect(result[:usage]['prompt_tokens']).to eq(100) diff --git a/spec/lib/captain/rewrite_service_spec.rb b/spec/lib/captain/rewrite_service_spec.rb index 8364481c4..c62f7b491 100644 --- a/spec/lib/captain/rewrite_service_spec.rb +++ b/spec/lib/captain/rewrite_service_spec.rb @@ -5,16 +5,8 @@ RSpec.describe Captain::RewriteService do let(:inbox) { create(:inbox, account: account) } let(:conversation) { create(:conversation, account: account, inbox: inbox) } let(:content) { 'I need help with my order' } - let(:event) do - { - 'name' => event_name, - 'data' => { - 'conversation_display_id' => conversation.display_id, - 'content' => content - } - } - end - let(:service) { described_class.new(account: account, event: event) } + let(:action) { 'fix_spelling_grammar' } + let(:service) { described_class.new(account: account, content: content, action: action, conversation_display_id: conversation.display_id) } let(:mock_chat) { instance_double(RubyLLM::Chat) } let(:mock_context) { instance_double(RubyLLM::Context, chat: mock_chat) } let(:mock_response) { instance_double(RubyLLM::Message, content: 'Rewritten text', input_tokens: 10, output_tokens: 5) } @@ -26,8 +18,8 @@ RSpec.describe Captain::RewriteService do allow(mock_chat).to receive(:ask).and_return(mock_response) end - describe '#fix_spelling_grammar_message' do - let(:event_name) { 'fix_spelling_grammar' } + describe '#perform with fix_spelling_grammar action' do + let(:action) { 'fix_spelling_grammar' } it 'uses fix_spelling_grammar prompt' do expect(service).to receive(:prompt_from_file).with('fix_spelling_grammar').and_return('Fix errors') @@ -38,7 +30,7 @@ RSpec.describe Captain::RewriteService do { message: 'Fixed' } end - service.fix_spelling_grammar_message + service.perform end end @@ -49,8 +41,8 @@ RSpec.describe Captain::RewriteService do allow(service).to receive(:prompt_from_file).with('tone_rewrite').and_return(tone_prompt_template) end - describe '#casual_message' do - let(:event_name) { 'casual' } + describe '#perform with casual action' do + let(:action) { 'casual' } it 'uses casual tone' do expect(service).to receive(:make_api_call) do |args| @@ -58,12 +50,12 @@ RSpec.describe Captain::RewriteService do { message: 'Hey, need help?' } end - service.casual_message + service.perform end end - describe '#professional_message' do - let(:event_name) { 'professional' } + describe '#perform with professional action' do + let(:action) { 'professional' } it 'uses professional tone' do expect(service).to receive(:make_api_call) do |args| @@ -71,12 +63,12 @@ RSpec.describe Captain::RewriteService do { message: 'Professional text' } end - service.professional_message + service.perform end end - describe '#friendly_message' do - let(:event_name) { 'friendly' } + describe '#perform with friendly action' do + let(:action) { 'friendly' } it 'uses friendly tone' do expect(service).to receive(:make_api_call) do |args| @@ -84,12 +76,12 @@ RSpec.describe Captain::RewriteService do { message: 'Friendly text' } end - service.friendly_message + service.perform end end - describe '#confident_message' do - let(:event_name) { 'confident' } + describe '#perform with confident action' do + let(:action) { 'confident' } it 'uses confident tone' do expect(service).to receive(:make_api_call) do |args| @@ -97,12 +89,12 @@ RSpec.describe Captain::RewriteService do { message: 'Confident text' } end - service.confident_message + service.perform end end - describe '#straightforward_message' do - let(:event_name) { 'straightforward' } + describe '#perform with straightforward action' do + let(:action) { 'straightforward' } it 'uses straightforward tone' do expect(service).to receive(:make_api_call) do |args| @@ -110,13 +102,13 @@ RSpec.describe Captain::RewriteService do { message: 'Straightforward text' } end - service.straightforward_message + service.perform end end end - describe '#improve_message' do - let(:event_name) { 'improve' } + describe '#perform with improve action' do + let(:action) { 'improve' } let(:improve_template) { 'Context: {{ conversation_context }}\nDraft: {{ draft_message }}' } before do @@ -134,11 +126,11 @@ RSpec.describe Captain::RewriteService do { message: 'Improved text' } end - service.improve_message + service.perform end it 'returns formatted response' do - result = service.improve_message + result = service.perform expect(result[:message]).to eq('Rewritten text') end diff --git a/spec/lib/captain/summary_service_spec.rb b/spec/lib/captain/summary_service_spec.rb index 56ef0b8e2..0a4a6e545 100644 --- a/spec/lib/captain/summary_service_spec.rb +++ b/spec/lib/captain/summary_service_spec.rb @@ -4,15 +4,7 @@ RSpec.describe Captain::SummaryService do let(:account) { create(:account) } let(:inbox) { create(:inbox, account: account) } let(:conversation) { create(:conversation, account: account, inbox: inbox) } - let(:event) do - { - 'name' => 'summarize', - 'data' => { - 'conversation_display_id' => conversation.display_id - } - } - end - let(:service) { described_class.new(account: account, event: event) } + let(:service) { described_class.new(account: account, conversation_display_id: conversation.display_id) } let(:mock_chat) { instance_double(RubyLLM::Chat) } let(:mock_context) { instance_double(RubyLLM::Context, chat: mock_chat) } let(:mock_response) { instance_double(RubyLLM::Message, content: 'Summary of conversation', input_tokens: 100, output_tokens: 50) } @@ -24,13 +16,13 @@ RSpec.describe Captain::SummaryService do allow(mock_chat).to receive(:ask).and_return(mock_response) end - describe '#summarize_message' do + describe '#perform' do it 'passes correct model to API' do expect(service).to receive(:make_api_call).with( hash_including(model: Captain::BaseEditorService::GPT_MODEL) ).and_call_original - service.summarize_message + service.perform end it 'passes system prompt and conversation text as messages' do @@ -45,11 +37,11 @@ RSpec.describe Captain::SummaryService do { message: 'Summary' } end - service.summarize_message + service.perform end it 'returns formatted response' do - result = service.summarize_message + result = service.perform expect(result[:message]).to eq('Summary of conversation') expect(result[:usage]['prompt_tokens']).to eq(100)