self review changes

config/locales/en.yml

@@ -292,6 +292,13 @@ en:
       completed_tool_call: 'Completed %{function_name} tool call'
       invalid_tool_call: 'Invalid tool call'
       tool_not_available: 'Tool not available'
+    documents:
+      limit_exceeded: 'Document limit exceeded'
+      pdf_format_error: 'must be a PDF file'
+      pdf_size_error: 'must be less than 10MB'
+      pdf_upload_failed: 'Failed to upload PDF to OpenAI'
+      pdf_processing_failed: 'Failed to process PDF document %{document_id}: %{error}'
+      pdf_processing_success: 'Successfully processed PDF document %{document_id}'
   public_portal:
     search:
       search_placeholder: Search for article by title or body...

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

@@ -8,16 +8,21 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC
 
   def index
     base_query = account_documents.includes(:assistant)
-    base_query = base_query.where(assistant_id: params[:assistant_id]) if params[:assistant_id].present?
+    base_query = base_query.where(assistant_id: permitted_params[:assistant_id]) if permitted_params[:assistant_id].present?
 
     @documents_count = base_query.count
-    @documents = base_query.page(params[:page] || 1).per(RESULTS_PER_PAGE)
+    @documents = base_query.page(permitted_params[:page] || 1).per(RESULTS_PER_PAGE)
   end
 
   def show; end
 
   def create
-    @document = account_documents.create!(document_params)
+    @document = account_documents.build(document_params)
+    @document.save!
+  rescue Captain::Document::LimitExceededError => e
+    render_could_not_create_error(e.message)
+  rescue ActiveRecord::RecordInvalid => e
+    render_could_not_create_error(e.record.errors.full_messages.join(', '))
   end
 
   def destroy
@@ -32,7 +37,11 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC
   end
 
   def set_document
-    @document = account_documents.find(params[:id])
+    @document = account_documents.find(permitted_params[:id])
+  end
+
+  def permitted_params
+    params.permit(:assistant_id, :page, :id, :account_id)
   end
 
   def document_params

enterprise/app/jobs/captain/documents/crawl_job.rb

@@ -19,9 +19,9 @@ class Captain::Documents::CrawlJob < ApplicationJob
     pdf_processor = Captain::Llm::PdfProcessingService.new(document)
     pdf_processor.process
     document.update!(status: :available)
-    Rails.logger.info "Successfully processed PDF document #{document.id}"
+    Rails.logger.info I18n.t('captain.documents.pdf_processing_success', document_id: document.id)
   rescue StandardError => e
-    Rails.logger.error "Failed to process PDF document #{document.id}: #{e.message}"
+    Rails.logger.error I18n.t('captain.documents.pdf_processing_failed', document_id: document.id, error: e.message)
     raise # Re-raise to let job framework handle retry logic
   end
 

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

@@ -41,7 +41,7 @@ class Captain::Documents::ResponseBuilderJob < ApplicationJob
 
   def store_paginated_metadata(document, service)
     document.update!(
-      metadata: document.metadata.merge(
+      metadata: (document.metadata || {}).merge(
         'faq_generation' => {
           'method' => 'paginated',
           'pages_processed' => service.total_pages_processed,

enterprise/app/models/captain/document.rb

@@ -66,11 +66,11 @@ class Captain::Document < ApplicationRecord
   end
 
   def openai_file_id
-    metadata['openai_file_id']
+    metadata&.dig('openai_file_id')
   end
 
   def store_openai_file_id(file_id)
-    update!(metadata: metadata.merge('openai_file_id' => file_id))
+    update!(metadata: (metadata || {}).merge('openai_file_id' => file_id))
   end
 
   def display_url
@@ -113,13 +113,13 @@ class Captain::Document < ApplicationRecord
 
   def ensure_within_plan_limit
     limits = account.usage_limits[:captain][:documents]
-    raise LimitExceededError, 'Document limit exceeded' unless limits[:current_available].positive?
+    raise LimitExceededError, I18n.t('captain.documents.limit_exceeded') unless limits[:current_available].positive?
   end
 
   def validate_pdf_format
     return unless pdf_file.attached?
 
-    errors.add(:pdf_file, 'must be a PDF file') unless pdf_file.blob.content_type == 'application/pdf'
+    errors.add(:pdf_file, I18n.t('captain.documents.pdf_format_error')) unless pdf_file.blob.content_type == 'application/pdf'
   end
 
   def validate_file_attachment
@@ -127,7 +127,7 @@ class Captain::Document < ApplicationRecord
 
     return unless pdf_file.blob.byte_size > 10.megabytes
 
-    errors.add(:pdf_file, 'must be less than 10MB')
+    errors.add(:pdf_file, I18n.t('captain.documents.pdf_size_error'))
   end
 
   def set_external_link_for_pdf

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

@@ -36,7 +36,7 @@ class Captain::Llm::PdfProcessingService < Llm::BaseOpenAiService
     openai_response = upload_pdf_to_openai
     file_id = openai_response['id']
 
-    raise 'Failed to upload PDF to OpenAI' if file_id.blank?
+    raise I18n.t('captain.documents.pdf_upload_failed') if file_id.blank?
 
     document.store_openai_file_id(file_id)
     Rails.logger.info "PDF uploaded successfully with file_id: #{file_id}"

spec/enterprise/jobs/captain/documents/crawl_job_spec.rb

@@ -47,6 +47,7 @@ RSpec.describe Captain::Documents::CrawlJob, type: :job do
 
       it 'does not perform web crawling' do
         job = described_class.new
+        allow(job).to receive(:perform_pdf_processing)
         expect(job).not_to receive(:perform_simple_crawl)
         expect(job).not_to receive(:perform_firecrawl_crawl)
         job.perform(pdf_document)
@@ -77,19 +78,22 @@ RSpec.describe Captain::Documents::CrawlJob, type: :job do
 
         described_class.perform_now(pdf_document)
 
-        expect(Rails.logger).to have_received(:info).with("Successfully processed PDF document #{pdf_document.id}")
+        expect(Rails.logger).to have_received(:info).with(I18n.t('captain.documents.pdf_processing_success', document_id: pdf_document.id))
       end
 
       context 'when PDF processing fails' do
-        it 'logs error and still marks document as available' do
+        it 'logs error and re-raises' do
           pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
           allow(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
           allow(pdf_processing_service).to receive(:process).and_raise(StandardError, 'Processing failed')
 
-          expect(Rails.logger).to receive(:error).with("Failed to process PDF document #{pdf_document.id}: Processing failed")
+          allow(Rails.logger).to receive(:error)
 
           expect { described_class.perform_now(pdf_document) }
-            .to change { pdf_document.reload.status }.to('available')
+            .to raise_error(StandardError, 'Processing failed')
+
+          expect(Rails.logger).to have_received(:error).with(I18n.t('captain.documents.pdf_processing_failed', document_id: pdf_document.id,
+                                                                                                               error: 'Processing failed'))
         end
       end
     end
@@ -136,15 +140,18 @@ RSpec.describe Captain::Documents::CrawlJob, type: :job do
     end
 
     context 'when processing fails' do
-      it 'logs error and still marks document as available' do
+      it 'logs error and re-raises' do
         pdf_processing_service = instance_double(Captain::Llm::PdfProcessingService)
         allow(Captain::Llm::PdfProcessingService).to receive(:new).with(pdf_document).and_return(pdf_processing_service)
         allow(pdf_processing_service).to receive(:process).and_raise(StandardError, 'Test error')
 
-        expect(Rails.logger).to receive(:error).with("Failed to process PDF document #{pdf_document.id}: Test error")
+        allow(Rails.logger).to receive(:error)
 
         expect { job_instance.send(:perform_pdf_processing, pdf_document) }
-          .to change { pdf_document.reload.status }.to('available')
+          .to raise_error(StandardError, 'Test error')
+
+        expect(Rails.logger).to have_received(:error).with(I18n.t('captain.documents.pdf_processing_failed', document_id: pdf_document.id,
+                                                                                                             error: 'Test error'))
       end
     end
   end