From 62b01fba6f569e32c2e971fa7d9728c33b473f7e Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma Date: Mon, 18 Aug 2025 21:18:59 +0530 Subject: [PATCH] self review changes --- .../accounts/captain/documents_controller.rb | 38 +++++-------------- .../app/jobs/captain/documents/crawl_job.rb | 5 +-- .../captain/llm/pdf_processing_service.rb | 25 +++--------- .../llm/pdf_processing_service_spec.rb | 32 +++++++--------- 4 files changed, 30 insertions(+), 70 deletions(-) diff --git a/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb b/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb index 67ac10515..737ebf3eb 100644 --- a/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb +++ b/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 diff --git a/enterprise/app/jobs/captain/documents/crawl_job.rb b/enterprise/app/jobs/captain/documents/crawl_job.rb index a4bd67d4f..1da54d32f 100644 --- a/enterprise/app/jobs/captain/documents/crawl_job.rb +++ b/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) diff --git a/enterprise/app/services/captain/llm/pdf_processing_service.rb b/enterprise/app/services/captain/llm/pdf_processing_service.rb index 3690ec016..b12209171 100644 --- a/enterprise/app/services/captain/llm/pdf_processing_service.rb +++ b/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 diff --git a/spec/enterprise/services/captain/llm/pdf_processing_service_spec.rb b/spec/enterprise/services/captain/llm/pdf_processing_service_spec.rb index b3647440c..d89c27ec3 100644 --- a/spec/enterprise/services/captain/llm/pdf_processing_service_spec.rb +++ b/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