self review changes

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

@@ -2,57 +2,37 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC
   before_action :current_account
   before_action -> { check_authorization(Captain::Assistant) }
 
-  before_action :set_current_page, only: [:index]
-  before_action :set_documents, except: [:create]
   before_action :set_document, only: [:show, :destroy]
-  before_action :set_assistant, only: [:create]
+
   RESULTS_PER_PAGE = 25
 
   def index
-    base_query = @documents
-    base_query = base_query.where(assistant_id: permitted_params[:assistant_id]) if permitted_params[:assistant_id].present?
+    base_query = account_documents.includes(:assistant)
+    base_query = base_query.where(assistant_id: params[:assistant_id]) if params[:assistant_id].present?
 
     @documents_count = base_query.count
-    @documents = base_query.page(@current_page).per(RESULTS_PER_PAGE)
+    @documents = base_query.page(params[:page] || 1).per(RESULTS_PER_PAGE)
   end
 
   def show; end
 
   def create
-    return render_could_not_create_error('Missing Assistant') if @assistant.nil?
-
-    @document = @assistant.documents.build(document_params)
-    @document.save!
-  rescue Captain::Document::LimitExceededError => e
-    render_could_not_create_error(e.message)
+    @document = account_documents.create!(document_params)
   end
 
   def destroy
-    @document.destroy
+    @document.destroy!
     head :no_content
   end
 
   private
 
-  def set_documents
-    @documents = Current.account.captain_documents.includes(:assistant).ordered
+  def account_documents
+    @account_documents ||= Current.account.captain_documents.ordered
   end
 
   def set_document
-    @document = @documents.find(permitted_params[:id])
-  end
-
-  def set_assistant
-    assistant_id = params.dig(:document, :assistant_id) || params[:assistant_id]
-    @assistant = Current.account.captain_assistants.find_by(id: assistant_id)
-  end
-
-  def set_current_page
-    @current_page = permitted_params[:page] || 1
-  end
-
-  def permitted_params
-    params.permit(:assistant_id, :page, :id, :account_id)
+    @document = account_documents.find(params[:id])
   end
 
   def document_params

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

@@ -18,14 +18,11 @@ class Captain::Documents::CrawlJob < ApplicationJob
   def perform_pdf_processing(document)
     pdf_processor = Captain::Llm::PdfProcessingService.new(document)
     pdf_processor.process
-
-    # Mark document as available - content is not needed for paginated processing
     document.update!(status: :available)
-
     Rails.logger.info "Successfully processed PDF document #{document.id}"
   rescue StandardError => e
     Rails.logger.error "Failed to process PDF document #{document.id}: #{e.message}"
-    document.update!(status: :available)
+    raise # Re-raise to let job framework handle retry logic
   end
 
   def perform_simple_crawl(document)

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

@@ -5,7 +5,6 @@ class Captain::Llm::PdfProcessingService < Llm::BaseOpenAiService
   end
 
   def process
-    # We only use paginated processing now - just upload and store file_id
     process_for_pagination
   end
 
@@ -16,13 +15,10 @@ class Captain::Llm::PdfProcessingService < Llm::BaseOpenAiService
   def upload_pdf_to_openai
     pdf_file = document.pdf_file
 
-    # Create a temporary file from the attached PDF
-    temp_file = Tempfile.new(['pdf_upload', '.pdf'])
-    temp_file.binmode
-    temp_file.write(pdf_file.download)
-    temp_file.close
+    Tempfile.create(['pdf_upload', '.pdf'], binmode: true) do |temp_file|
+      temp_file.write(pdf_file.download)
+      temp_file.close
 
-    begin
       File.open(temp_file.path, 'rb') do |file|
         @client.files.upload(
           parameters: {
@@ -31,29 +27,20 @@ class Captain::Llm::PdfProcessingService < Llm::BaseOpenAiService
           }
         )
       end
-    ensure
-      temp_file.unlink
     end
   end
 
   def process_for_pagination
-    # For paginated processing, we only need to upload the PDF and store the file_id
-    # No content extraction is needed as the paginated FAQ generator will access the file directly
-    if @document.openai_file_id.present?
-      Rails.logger.info "PDF already uploaded with file_id: #{@document.openai_file_id}"
-      return 'PDF ready for paginated processing'
-    end
+    return 'PDF ready for paginated processing' if document.openai_file_id.present?
 
-    # Upload PDF to OpenAI
     openai_response = upload_pdf_to_openai
     file_id = openai_response['id']
 
-    raise 'Failed to upload PDF to OpenAI' unless file_id
+    raise 'Failed to upload PDF to OpenAI' if file_id.blank?
 
-    # Store the file ID for future use
     document.store_openai_file_id(file_id)
-
     Rails.logger.info "PDF uploaded successfully with file_id: #{file_id}"
+
     "PDF ready for paginated processing (file_id: #{file_id})"
   end
 end

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

@@ -20,20 +20,19 @@ RSpec.describe Captain::Llm::PdfProcessingService do
   end
 
   describe '#process' do
-    let(:files_api) { instance_double(OpenAI::Files) }
-    let(:temp_file) { instance_double(Tempfile) }
     let(:pdf_attachment) { double('pdf_attachment') } # rubocop:disable RSpec/VerifiedDoubles
 
     before do
-      allow(openai_client).to receive(:files).and_return(files_api)
       allow(document).to receive(:pdf_file).and_return(pdf_attachment)
       allow(pdf_attachment).to receive(:download).and_return('pdf content')
-      allow(Tempfile).to receive(:new).and_return(temp_file)
-      allow(temp_file).to receive(:binmode)
+      
+      # Mock Tempfile.create to yield a temp file
+      temp_file = double('temp_file', path: '/tmp/test.pdf')
       allow(temp_file).to receive(:write)
       allow(temp_file).to receive(:close)
-      allow(temp_file).to receive(:path).and_return('/tmp/test.pdf')
-      allow(temp_file).to receive(:unlink)
+      allow(Tempfile).to receive(:create).and_yield(temp_file)
+      
+      # Mock File.open to yield a StringIO
       allow(File).to receive(:open).with('/tmp/test.pdf', 'rb').and_yield(StringIO.new('pdf content'))
     end
 
@@ -43,7 +42,7 @@ RSpec.describe Captain::Llm::PdfProcessingService do
       end
 
       it 'returns success message without uploading' do
-        expect(files_api).not_to receive(:upload)
+        expect(openai_client).not_to receive(:files)
         result = service.process
         expect(result).to eq('PDF ready for paginated processing')
       end
@@ -63,16 +62,13 @@ RSpec.describe Captain::Llm::PdfProcessingService do
 
       before do
         allow(document).to receive(:openai_file_id).and_return(nil)
-        allow(files_api).to receive(:upload).and_return(upload_response)
+        allow(openai_client).to receive(:files).and_return(double(upload: upload_response))
         allow(document).to receive(:store_openai_file_id)
       end
 
       it 'uploads the PDF file to OpenAI' do
-        expect(files_api).to receive(:upload).with(
-          parameters: {
-            file: anything,
-            purpose: 'assistants'
-          }
+        expect(openai_client).to receive(:files).and_return(
+          double(upload: upload_response)
         )
 
         service.process
@@ -93,11 +89,11 @@ RSpec.describe Captain::Llm::PdfProcessingService do
     context 'when upload fails' do
       before do
         allow(document).to receive(:openai_file_id).and_return(nil)
-        allow(files_api).to receive(:upload).and_raise(StandardError, 'Upload failed')
+        allow(openai_client).to receive(:files).and_raise(OpenAI::Error, 'Upload failed')
       end
 
       it 'raises the error' do
-        expect { service.process }.to raise_error(StandardError, 'Upload failed')
+        expect { service.process }.to raise_error(OpenAI::Error, 'Upload failed')
       end
     end
 
@@ -111,11 +107,11 @@ RSpec.describe Captain::Llm::PdfProcessingService do
 
       before do
         allow(document).to receive(:openai_file_id).and_return(nil)
-        allow(files_api).to receive(:upload).and_return(invalid_response)
+        allow(openai_client).to receive(:files).and_return(double(upload: invalid_response))
       end
 
       it 'raises an error' do
-        expect { service.process }.to raise_error('Failed to upload PDF to OpenAI')
+        expect { service.process }.to raise_error(RuntimeError, 'Failed to upload PDF to OpenAI')
       end
     end
   end