Merge remote-tracking branch 'origin/feature/cw-7495-api' into cook/pr-15017-review

enterprise/app/controllers/api/v1/accounts/captain/faq_suggestions_controller.rb

@@ -1,7 +1,7 @@
 class Api::V1::Accounts::Captain::FaqSuggestionsController < Api::V1::Accounts::BaseController
   before_action :current_account
-  before_action -> { check_authorization(Captain::Assistant) }
-  before_action :set_suggestions
+  before_action -> { check_authorization(Captain::FaqSuggestion) }
+  before_action :set_accessible_suggestions
   before_action :set_suggestion, except: [:index]
 
   RESULTS_PER_PAGE = 25
@@ -50,6 +50,7 @@ class Api::V1::Accounts::Captain::FaqSuggestionsController < Api::V1::Accounts::
     base_query = base_query.where(status: permitted_params[:status]) if permitted_params[:status].present?
 
     if permitted_params[:search].present?
+      # TODO: Move FAQ suggestion search to Elasticsearch when the records are indexed there.
       search_term = "%#{permitted_params[:search]}%"
       base_query = base_query.where('question ILIKE :search OR answer ILIKE :search', search: search_term)
     end
@@ -57,7 +58,7 @@ class Api::V1::Accounts::Captain::FaqSuggestionsController < Api::V1::Accounts::
     base_query
   end
 
-  def set_suggestions
+  def set_accessible_suggestions
     @suggestions = Captain::FaqSuggestionFinder.new(Current.user, Current.account).perform.includes(:assistant).ordered
   end
 

enterprise/app/policies/captain/faq_suggestion_policy.rb

@@ -0,0 +1,21 @@
+class Captain::FaqSuggestionPolicy < ApplicationPolicy
+  def index?
+    true
+  end
+
+  def show?
+    true
+  end
+
+  def update?
+    true
+  end
+
+  def approve?
+    true
+  end
+
+  def dismiss?
+    true
+  end
+end

enterprise/app/services/captain/faq_suggestion_approval_service.rb

@@ -9,14 +9,12 @@ class Captain::FaqSuggestionApprovalService
       raise ActiveRecord::RecordNotFound unless suggestion.open?
 
       suggestion.update!(attributes) if attributes.present?
-      validate_language!
 
       response = suggestion.assistant.responses.create!(
         question: suggestion.question,
         answer: suggestion.answer,
         status: :approved
       )
-      suggestion.observations.delete_all
       suggestion.approved!
       response
     end
@@ -25,15 +23,4 @@ class Captain::FaqSuggestionApprovalService
   private
 
   attr_reader :suggestion, :attributes
-
-  def validate_language!
-    return if base_language(suggestion.language) == base_language(suggestion.account.locale)
-
-    suggestion.errors.add(:language, 'must match the account locale before approval')
-    raise ActiveRecord::RecordInvalid, suggestion
-  end
-
-  def base_language(language)
-    language.to_s.tr('-', '_').split('_').first
-  end
 end

enterprise/app/services/captain/llm/conversation_faq_service.rb

@@ -1,6 +1,8 @@
 class Captain::Llm::ConversationFaqService < Llm::BaseAiService
   include Integrations::LlmInstrumentation
 
+  class SuggestionChangedError < StandardError; end
+
   DISTANCE_THRESHOLD = 0.3
   MATCH_LIMIT = 5
   LLM_FEATURE = 'conversation_faq_generation'.freeze
@@ -14,9 +16,6 @@ class Captain::Llm::ConversationFaqService < Llm::BaseAiService
     language.to_s.tr('-', '_').split('_').first.downcase
   end
 
-  def self.account_language_for(account)
-    normalize_language(account.locale.presence || I18n.default_locale.to_s)
-  end
   private_class_method :normalize_language
 
   def initialize(assistant, conversation)
@@ -44,10 +43,11 @@ class Captain::Llm::ConversationFaqService < Llm::BaseAiService
   def route_candidate(faq)
     embedding = embedding_service.get_embedding(candidate_text(faq))
 
-    return discard_observation(faq) if matching_record(approved_faqs_for_language, faq, embedding)
+    return discard_observation(faq) if matching_record(approved_faqs, faq, embedding)
     return discard_observation(faq) if matching_record(dismissed_suggestions_for_language, faq, embedding)
 
     suggestion = matching_record(open_suggestions_for_language, faq, embedding)
+    matched_content = suggestion&.slice('question', 'answer')
     suggestion ||= assistant.faq_suggestions.create!(
       question: faq.fetch('question'),
       answer: faq.fetch('answer'),
@@ -55,7 +55,7 @@ class Captain::Llm::ConversationFaqService < Llm::BaseAiService
       language: faq_language
     )
 
-    attach_observation(suggestion, faq)
+    attach_observation(suggestion, faq, matched_content)
   end
 
   def matching_record(relation, faq, embedding)
@@ -99,9 +99,10 @@ class Captain::Llm::ConversationFaqService < Llm::BaseAiService
     raise
   end
 
-  def attach_observation(suggestion, faq)
+  def attach_observation(suggestion, faq, matched_content)
     suggestion.with_lock do
       next unless suggestion.open?
+      raise SuggestionChangedError if matched_content && suggestion.slice('question', 'answer') != matched_content
 
       existing_observation = suggestion.observations.find_by(conversation: conversation)
       next existing_observation if existing_observation
@@ -136,10 +137,8 @@ class Captain::Llm::ConversationFaqService < Llm::BaseAiService
     assistant.faq_suggestions.where(account_id: conversation.account_id).dismissed.by_language(faq_language)
   end
 
-  def approved_faqs_for_language
-    return assistant.responses.approved if faq_language == account_language
-
-    assistant.responses.none
+  def approved_faqs
+    assistant.responses.approved
   end
 
   def candidate_text(faq)
@@ -199,10 +198,6 @@ class Captain::Llm::ConversationFaqService < Llm::BaseAiService
     @faq_language ||= self.class.language_for(conversation)
   end
 
-  def account_language
-    @account_language ||= self.class.account_language_for(conversation.account)
-  end
-
   def language_name(language)
     ISO_639.find(language)&.english_name&.downcase || 'english'
   end

spec/enterprise/controllers/api/v1/accounts/captain/faq_suggestions_controller_spec.rb

@@ -73,6 +73,24 @@ RSpec.describe 'Api::V1::Accounts::Captain::FaqSuggestions', type: :request do
         include('conversation' => include('id' => conversation.id, 'display_id' => conversation.display_id))
       )
     end
+
+    it 'returns only source conversations the agent can access' do
+      create(:inbox_member, user: agent, inbox: inbox)
+      hidden_conversation = create(:conversation, account: account, inbox: create(:inbox, account: account))
+      suggestion.observations.create!(
+        conversation: hidden_conversation,
+        generated_question: suggestion.question,
+        generated_answer: suggestion.answer,
+        language: suggestion.language
+      )
+
+      get "/api/v1/accounts/#{account.id}/captain/faq_suggestions/#{suggestion.id}",
+          headers: agent.create_new_auth_token,
+          as: :json
+
+      expect(response).to have_http_status(:success)
+      expect(response.parsed_body['observations'].pluck('conversation').pluck('id')).to contain_exactly(conversation.id)
+    end
   end
 
   describe 'PATCH /api/v1/accounts/:account_id/captain/faq_suggestions/:id' do
@@ -87,13 +105,25 @@ RSpec.describe 'Api::V1::Accounts::Captain::FaqSuggestions', type: :request do
       expect(suggestion.reload.question).to eq('Updated question')
     end
 
-    it 'does not let an agent edit a suggestion' do
+    it 'lets an agent edit an accessible suggestion' do
+      create(:inbox_member, user: agent, inbox: inbox)
+
       patch "/api/v1/accounts/#{account.id}/captain/faq_suggestions/#{suggestion.id}",
             params: { faq_suggestion: { question: 'Updated question' } },
             headers: agent.create_new_auth_token,
             as: :json
 
-      expect(response).to have_http_status(:unauthorized)
+      expect(response).to have_http_status(:success)
+      expect(suggestion.reload.question).to eq('Updated question')
+    end
+
+    it 'does not let an agent edit an inaccessible suggestion' do
+      patch "/api/v1/accounts/#{account.id}/captain/faq_suggestions/#{suggestion.id}",
+            params: { faq_suggestion: { question: 'Updated question' } },
+            headers: agent.create_new_auth_token,
+            as: :json
+
+      expect(response).to have_http_status(:not_found)
       expect(suggestion.reload.question).to eq('How do I enable the feature?')
     end
   end
@@ -110,17 +140,43 @@ RSpec.describe 'Api::V1::Accounts::Captain::FaqSuggestions', type: :request do
       expect(response).to have_http_status(:success)
       expect(response.parsed_body['answer']).to eq('Enable it in account settings.')
       expect(suggestion.reload).to be_approved
-      expect(suggestion.observations).to be_empty
+      expect(suggestion.observations.pluck(:conversation_id)).to contain_exactly(conversation.id)
     end
 
-    it 'does not let an agent approve a suggestion' do
+    it 'lets an agent approve an accessible suggestion' do
+      create(:inbox_member, user: agent, inbox: inbox)
+
+      expect do
+        post "/api/v1/accounts/#{account.id}/captain/faq_suggestions/#{suggestion.id}/approve",
+             headers: agent.create_new_auth_token,
+             as: :json
+      end.to change(assistant.responses.approved, :count).by(1)
+
+      expect(response).to have_http_status(:success)
+      expect(suggestion.reload).to be_approved
+    end
+
+    it 'approves a suggestion written in a language other than the account locale' do
+      suggestion.update!(language: 'pt')
+
+      expect do
+        post "/api/v1/accounts/#{account.id}/captain/faq_suggestions/#{suggestion.id}/approve",
+             headers: admin.create_new_auth_token,
+             as: :json
+      end.to change(assistant.responses.approved, :count).by(1)
+
+      expect(response).to have_http_status(:success)
+      expect(suggestion.reload).to be_approved
+    end
+
+    it 'does not let an agent approve an inaccessible suggestion' do
       expect do
         post "/api/v1/accounts/#{account.id}/captain/faq_suggestions/#{suggestion.id}/approve",
              headers: agent.create_new_auth_token,
              as: :json
       end.not_to change(assistant.responses, :count)
 
-      expect(response).to have_http_status(:unauthorized)
+      expect(response).to have_http_status(:not_found)
       expect(suggestion.reload).to be_open
     end
   end
@@ -136,5 +192,25 @@ RSpec.describe 'Api::V1::Accounts::Captain::FaqSuggestions', type: :request do
       expect(response).to have_http_status(:success)
       expect(suggestion.reload).to be_dismissed
     end
+
+    it 'lets an agent dismiss an accessible suggestion' do
+      create(:inbox_member, user: agent, inbox: inbox)
+
+      post "/api/v1/accounts/#{account.id}/captain/faq_suggestions/#{suggestion.id}/dismiss",
+           headers: agent.create_new_auth_token,
+           as: :json
+
+      expect(response).to have_http_status(:success)
+      expect(suggestion.reload).to be_dismissed
+    end
+
+    it 'does not let an agent dismiss an inaccessible suggestion' do
+      post "/api/v1/accounts/#{account.id}/captain/faq_suggestions/#{suggestion.id}/dismiss",
+           headers: agent.create_new_auth_token,
+           as: :json
+
+      expect(response).to have_http_status(:not_found)
+      expect(suggestion.reload).to be_open
+    end
   end
 end

spec/enterprise/services/captain/llm/conversation_faq_service_spec.rb

@@ -340,6 +340,23 @@ RSpec.describe Captain::Llm::ConversationFaqService do
         expect(existing_suggestion.reload.source_count).to eq(2)
         expect(captain_assistant.faq_suggestions.count).to eq(1)
       end
+
+      it 'does not attach the observation when the suggestion changes after classification' do
+        allow(mock_chat).to receive(:ask) do |input|
+          if input.start_with?('{')
+            existing_suggestion.update!(question: 'Edited after classification started')
+            match_response
+          else
+            mock_response
+          end
+        end
+
+        expect do
+          service.generate_suggestions
+        end.to raise_error(described_class::SuggestionChangedError)
+        expect(existing_suggestion.observations.count).to eq(1)
+        expect(existing_suggestion.reload.source_count).to eq(1)
+      end
     end
 
     context 'when a similar open suggestion uses another language' do
@@ -399,8 +416,9 @@ RSpec.describe Captain::Llm::ConversationFaqService do
       end
     end
 
-    context 'when a similar approved FAQ uses the account language' do
+    context 'when a similar approved FAQ uses another language' do
       let(:sample_faqs) { [{ 'question' => 'Como ativo o recurso?', 'answer' => 'Ative nas configuracoes.' }] }
+      let(:match_response) { instance_double(RubyLLM::Message, content: { same_faq: true }.to_json) }
 
       before do
         create(:captain_assistant_response, assistant: captain_assistant, account: captain_assistant.account,
@@ -408,14 +426,16 @@ RSpec.describe Captain::Llm::ConversationFaqService do
                                             embedding: embedding_one)
         conversation.update!(additional_attributes: { conversation_language: 'pt-BR' })
         allow(embedding_service).to receive(:get_embedding).and_return(embedding_one)
+        allow(mock_chat).to receive(:ask) do |input|
+          input.start_with?('{') ? match_response : mock_response
+        end
       end
 
-      it 'does not discard a candidate in another language' do
+      it 'deduplicates against the approved FAQ' do
         expect do
           service.generate_suggestions
-        end.to change(captain_assistant.faq_suggestions, :count).by(1)
-        expect(Captain::FaqObservation.discarded.count).to be_zero
-        expect(captain_assistant.faq_suggestions.last.language).to eq('pt')
+        end.to change(Captain::FaqObservation.discarded, :count).by(1)
+        expect(captain_assistant.faq_suggestions.count).to be_zero
       end
     end