From ce7847364b71002c95bde76d7b87d636c24b4515 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma Date: Wed, 20 Aug 2025 13:46:57 +0530 Subject: [PATCH] self review changes --- config/locales/en.yml | 7 +++++++ .../accounts/captain/documents_controller.rb | 17 +++++++++++---- .../app/jobs/captain/documents/crawl_job.rb | 4 ++-- .../captain/documents/response_builder_job.rb | 2 +- enterprise/app/models/captain/document.rb | 10 ++++----- .../captain/llm/pdf_processing_service.rb | 2 +- .../jobs/captain/documents/crawl_job_spec.rb | 21 ++++++++++++------- 7 files changed, 43 insertions(+), 20 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index e55132709..b7eb17ada 100644 --- a/config/locales/en.yml +++ b/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... 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 737ebf3eb..16fe0edc4 100644 --- a/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb +++ b/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 diff --git a/enterprise/app/jobs/captain/documents/crawl_job.rb b/enterprise/app/jobs/captain/documents/crawl_job.rb index 1da54d32f..dd0091df6 100644 --- a/enterprise/app/jobs/captain/documents/crawl_job.rb +++ b/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 diff --git a/enterprise/app/jobs/captain/documents/response_builder_job.rb b/enterprise/app/jobs/captain/documents/response_builder_job.rb index e1b457a0c..9971b6063 100644 --- a/enterprise/app/jobs/captain/documents/response_builder_job.rb +++ b/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, diff --git a/enterprise/app/models/captain/document.rb b/enterprise/app/models/captain/document.rb index 9d40fc73e..944686c26 100644 --- a/enterprise/app/models/captain/document.rb +++ b/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 diff --git a/enterprise/app/services/captain/llm/pdf_processing_service.rb b/enterprise/app/services/captain/llm/pdf_processing_service.rb index b12209171..ab7af212d 100644 --- a/enterprise/app/services/captain/llm/pdf_processing_service.rb +++ b/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}" diff --git a/spec/enterprise/jobs/captain/documents/crawl_job_spec.rb b/spec/enterprise/jobs/captain/documents/crawl_job_spec.rb index 722602f9c..b7ab1a7f1 100644 --- a/spec/enterprise/jobs/captain/documents/crawl_job_spec.rb +++ b/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