From a56c4a5f8c5cbb7bc35f578ce7e4aa7e8db2f7d4 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma Date: Wed, 6 Aug 2025 12:44:39 +0530 Subject: [PATCH] self review changes for pdf support for captain --- config/routes.rb | 6 +--- ...00_add_pdf_support_to_captain_documents.rb | 8 ----- db/schema.rb | 3 -- .../accounts/captain/documents_controller.rb | 36 ++----------------- enterprise/app/models/captain/document.rb | 21 ++++++++--- .../v1/models/captain/_document.json.jbuilder | 2 ++ 6 files changed, 23 insertions(+), 53 deletions(-) delete mode 100644 db/migrate/20250804110000_add_pdf_support_to_captain_documents.rb diff --git a/config/routes.rb b/config/routes.rb index 6016fdd00..7fb348084 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -67,11 +67,7 @@ Rails.application.routes.draw do resources :copilot_threads, only: [:index, :create] do resources :copilot_messages, only: [:index, :create] end - resources :documents, only: [:index, :show, :create, :destroy] do - collection do - post :upload_pdf - end - end + resources :documents, only: [:index, :show, :create, :destroy] end resources :agent_bots, only: [:index, :create, :show, :update, :destroy] do delete :avatar, on: :member diff --git a/db/migrate/20250804110000_add_pdf_support_to_captain_documents.rb b/db/migrate/20250804110000_add_pdf_support_to_captain_documents.rb deleted file mode 100644 index 1fdffe34f..000000000 --- a/db/migrate/20250804110000_add_pdf_support_to_captain_documents.rb +++ /dev/null @@ -1,8 +0,0 @@ -class AddPdfSupportToCaptainDocuments < ActiveRecord::Migration[7.1] - def change - add_column :captain_documents, :content_type, :string - add_column :captain_documents, :file_size, :bigint - - add_index :captain_documents, :content_type - end -end diff --git a/db/schema.rb b/db/schema.rb index b3b53f967..9ad676b6f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -292,13 +292,10 @@ ActiveRecord::Schema[7.1].define(version: 2025_08_05_082345) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "status", default: 0, null: false - t.string "content_type" - t.bigint "file_size" t.jsonb "metadata", default: {} t.index ["account_id"], name: "index_captain_documents_on_account_id" t.index ["assistant_id", "external_link"], name: "index_captain_documents_on_assistant_id_and_external_link", unique: true t.index ["assistant_id"], name: "index_captain_documents_on_assistant_id" - t.index ["content_type"], name: "index_captain_documents_on_content_type" t.index ["status"], name: "index_captain_documents_on_status" end 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 ac9a5e6a7..acf3f44a1 100644 --- a/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb +++ b/enterprise/app/controllers/api/v1/accounts/captain/documents_controller.rb @@ -3,9 +3,9 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC before_action -> { check_authorization(Captain::Assistant) } before_action :set_current_page, only: [:index] - before_action :set_documents, except: [:create, :upload_pdf] + before_action :set_documents, except: [:create] before_action :set_document, only: [:show, :destroy] - before_action :set_assistant, only: [:create, :upload_pdf] + before_action :set_assistant, only: [:create] RESULTS_PER_PAGE = 25 def index @@ -34,23 +34,6 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC render_could_not_create_error('Failed to create document') end - def upload_pdf - pdf_file = params[:pdf_file] - - return render_could_not_create_error('PDF file is required') if pdf_file.blank? - return render_could_not_create_error('Invalid file type') unless valid_pdf?(pdf_file) - return render_could_not_create_error('File too large (max 512MB)') if pdf_file.size > 512.megabytes - return render_could_not_create_error('Missing Assistant') if @assistant.nil? - - create_pdf_document(pdf_file) - render :show - rescue Captain::Document::LimitExceededError => e - render_could_not_create_error(e.message) - rescue StandardError => e - Rails.logger.error "PDF upload error: #{e.message}" - render_could_not_create_error('Failed to upload PDF') - end - def destroy @document.destroy head :no_content @@ -67,7 +50,7 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC end def set_assistant - assistant_id = document_params[:assistant_id] || params[:assistant_id] + assistant_id = params.dig(:document, :assistant_id) || params[:assistant_id] @assistant = Current.account.captain_assistants.find_by(id: assistant_id) end @@ -82,17 +65,4 @@ class Api::V1::Accounts::Captain::DocumentsController < Api::V1::Accounts::BaseC def document_params params.require(:document).permit(:name, :external_link, :assistant_id, :pdf_file) end - - def valid_pdf?(file) - file.content_type == 'application/pdf' - end - - def create_pdf_document(pdf_file) - @document = @assistant.documents.build( - name: pdf_file.original_filename, - account: Current.account - ) - @document.pdf_file.attach(pdf_file) - @document.save! - end end diff --git a/enterprise/app/models/captain/document.rb b/enterprise/app/models/captain/document.rb index 89b30e667..0bc3fd585 100644 --- a/enterprise/app/models/captain/document.rb +++ b/enterprise/app/models/captain/document.rb @@ -36,6 +36,7 @@ class Captain::Document < ApplicationRecord validates :content, length: { maximum: 200_000 } validates :pdf_file, presence: true, if: :pdf_document? validate :validate_pdf_format, if: :pdf_document? + validate :validate_file_attachment, if: -> { pdf_file.attached? } before_validation :ensure_account_id before_validation :set_external_link_for_pdf @@ -55,7 +56,15 @@ class Captain::Document < ApplicationRecord scope :for_assistant, ->(assistant_id) { where(assistant_id: assistant_id) } def pdf_document? - (external_link&.ends_with?('.pdf')) || (pdf_file.attached? && pdf_file.content_type == 'application/pdf') + (external_link&.ends_with?('.pdf')) || (pdf_file.attached? && pdf_file.blob.content_type == 'application/pdf') + end + + def content_type + pdf_file.blob.content_type if pdf_file.attached? + end + + def file_size + pdf_file.blob.byte_size if pdf_file.attached? end def openai_file_id @@ -112,11 +121,15 @@ class Captain::Document < ApplicationRecord def validate_pdf_format return unless pdf_file.attached? - errors.add(:pdf_file, 'must be a PDF file') unless pdf_file.content_type == 'application/pdf' + errors.add(:pdf_file, 'must be a PDF file') unless pdf_file.blob.content_type == 'application/pdf' + end - return unless pdf_file.byte_size > 512.megabytes + def validate_file_attachment + return unless pdf_file.attached? - errors.add(:pdf_file, 'must be less than 512MB') + return unless pdf_file.blob.byte_size > 20.megabytes + + errors.add(:pdf_file, 'must be less than 20MB') end def set_external_link_for_pdf diff --git a/enterprise/app/views/api/v1/models/captain/_document.json.jbuilder b/enterprise/app/views/api/v1/models/captain/_document.json.jbuilder index 5f9e76e66..8064a5181 100644 --- a/enterprise/app/views/api/v1/models/captain/_document.json.jbuilder +++ b/enterprise/app/views/api/v1/models/captain/_document.json.jbuilder @@ -3,9 +3,11 @@ json.assistant do json.partial! 'api/v1/models/captain/assistant', formats: [:json], resource: resource.assistant end json.content resource.content +json.content_type resource.content_type json.created_at resource.created_at.to_i json.external_link resource.external_link json.display_url resource.display_url +json.file_size resource.file_size json.id resource.id json.name resource.name json.status resource.status