From 7f5418386c23341d1f0ff4658d97125a6bc78062 Mon Sep 17 00:00:00 2001
From: Mason Ballengee <masaball@iu.edu>
Date: Thu, 6 Jun 2024 11:18:00 -0400
Subject: [PATCH 1/4] Add SupplementalFile ingest API

---
 app/controllers/application_controller.rb     |   2 +-
 .../supplemental_files_controller.rb          | 170 ++++++++---
 app/models/supplemental_file.rb               |  42 ++-
 config/routes.rb                              |   4 +-
 spec/models/supplemental_file_spec.rb         |  44 +++
 .../supplemental_files_controller_examples.rb | 288 ++++++++++++++----
 6 files changed, 454 insertions(+), 96 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 387711224b..d1893db102 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -29,7 +29,7 @@ class ApplicationController < ActionController::Base
 
   helper_method :render_bookmarks_control?
 
-  around_action :handle_api_request, if: proc{|c| request.format.json? || request.format.atom? }
+  around_action :handle_api_request, if: proc{|c| request.format.json? || request.format.atom? || request.headers['Avalon-Api-Key'].present? }
   before_action :rewrite_v4_ids, if: proc{|c| request.method_symbol == :get && [params[:id], params[:content]].flatten.compact.any? { |i| i =~ /^[a-z]+:[0-9]+$/}}
   before_action :set_no_cache_headers, if: proc{|c| request.xhr? }
   prepend_before_action :remove_zero_width_chars
diff --git a/app/controllers/supplemental_files_controller.rb b/app/controllers/supplemental_files_controller.rb
index bb20375b40..0fb3770732 100644
--- a/app/controllers/supplemental_files_controller.rb
+++ b/app/controllers/supplemental_files_controller.rb
@@ -14,6 +14,9 @@
 
 # frozen_string_literal: true
 class SupplementalFilesController < ApplicationController
+  include Rails::Pagination
+
+  before_action :authenticate_user!, except: [:show, :captions]
   before_action :set_object
   before_action :authorize_object
 
@@ -30,21 +33,39 @@ class SupplementalFilesController < ApplicationController
     handle_error(message: exception.full_message, status: 404)
   end
 
-  def create
-    # FIXME: move filedata to permanent location
-    raise Avalon::BadRequest, "Missing required parameters" unless supplemental_file_params[:file]
+  def index
+    files = paginate SupplementalFile.where("parent_id = ?", @object.id)
+    render json: files.to_a.collect { |f| f.as_json }
+  end
 
-    @supplemental_file = SupplementalFile.new(label: supplemental_file_params[:label], tags: supplemental_file_params[:tags], parent_id: @object.id)
-    begin
-      @supplemental_file.attach_file(supplemental_file_params[:file])
-    rescue StandardError, LoadError => e
-      raise Avalon::SaveError, "File could not be attached: #{e.full_message}"
+  def create
+    if metadata_upload? && !attachment
+      raise Avalon::BadRequest, "Missing required Content-type headers" unless request.headers["Content-Type"] == 'application/json'
     end
+    raise Avalon::BadRequest, "Missing required parameters" unless validate_params
+
+    @supplemental_file = SupplementalFile.new(**metadata_from_params)
+    
+    if attachment
+      begin
+        @supplemental_file.attach_file(attachment)
+      rescue StandardError, LoadError => e
+        raise Avalon::SaveError, "File could not be attached: #{e.full_message}"
+      end
 
-    # Raise errror if file wasn't attached
-    raise Avalon::SaveError, "File could not be attached." unless @supplemental_file.file.attached?
+      # Raise errror if file wasn't attached
+      raise Avalon::SaveError, "File could not be attached." unless @supplemental_file.file.attached?
 
-    raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save
+      raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save
+    else
+      # For multi-step API upload, we need to skip file type validation on the metadata portion of the save.
+      # Captions are validated as VTT or SRT which cannot be validated if there is no file attached.
+      @supplemental_file.skip_file_type = true
+      # Transcripts are automatically indexed on creation, so we need to skip indexing to avoid an error
+      # when saving without an uploaded file.
+      @supplemental_file.skip_index = true
+      raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save
+    end
 
     @object.supplemental_files += [@supplemental_file]
     raise Avalon::SaveError, @object.errors[:supplemental_files_json].full_messages unless @object.save
@@ -52,43 +73,65 @@ def create
     flash[:success] = "Supplemental file successfully added."
 
     respond_to do |format|
-      format.html { redirect_to edit_structure_path }
-      format.json { head :created, location: object_supplemental_file_path }
+      format.html { 
+        if request.headers['Avalon-Api-Key'].present?
+          render json: { supplemental_file: @supplemental_file.id }, status: :created
+        else
+          redirect_to edit_structure_path
+        end
+      }
+      format.json { render json: { supplemental_file: @supplemental_file.id }, status: :created }
     end
   end
 
   def show
     find_supplemental_file
 
-    # Redirect or proxy the content
-    if Settings.supplemental_files.proxy
-      send_data @supplemental_file.file.download, filename: @supplemental_file.file.filename.to_s, type: @supplemental_file.file.content_type, disposition: 'attachment'
-    else
-      redirect_to rails_blob_path(@supplemental_file.file, disposition: "attachment")
+    respond_to do |format|
+      format.html { 
+        # Redirect or proxy the content
+        if Settings.supplemental_files.proxy
+          send_data @supplemental_file.file.download, filename: @supplemental_file.file.filename.to_s, type: @supplemental_file.file.content_type, disposition: 'attachment'
+        else
+          redirect_to rails_blob_path(@supplemental_file.file, disposition: "attachment")
+        end
+      }
+      format.json { render json: @supplemental_file.as_json }
     end
   end
 
-  # Update the label and tags of the supplemental file
   def update
-    raise Avalon::NotFound, "Cannot update the supplemental file: #{params[:id]} not found" unless SupplementalFile.exists? params[:id].to_s
-    @supplemental_file = SupplementalFile.find(params[:id])
-    raise Avalon::NotFound, "Cannot update the supplemental file: #{@supplemental_file.id} not found" unless @object.supplemental_files.any? { |f| f.id == @supplemental_file.id }
-    raise Avalon::BadRequest, "Updating file contents not allowed" if supplemental_file_params[:file].present?
+    if metadata_upload?
+      raise Avalon::BadRequest, "Incorrect request format. Use HTML if updating attached file." if attachment
+      raise Avalon::BadRequest, "Missing required Content-type headers" unless request.headers["Content-Type"] == 'application/json'
+    elsif request.headers['Avalon-Api-Key'].present?
+      raise Avalon::BadRequest, "Incorrect request format. Use JSON if updating metadata." unless attachment
+    end
+    raise Avalon::BadRequest, "Missing required parameters" unless validate_params
+
+    find_supplemental_file
+
+    edit_file_information if !attachment
+
+    update_attached_file if attachment
 
-    edit_file_information
     raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save
 
     flash[:success] = "Supplemental file successfully updated."
     respond_to do |format|
-      format.html { redirect_to edit_structure_path }
-      format.json { head :ok, location: object_supplemental_file_path }
+      format.html { 
+        if request.headers['Avalon-Api-Key'].present?
+          render json: { supplemental_file: @supplemental_file.id }
+        else
+          redirect_to edit_structure_path
+        end
+      }
+      format.json { render json: { supplemental_file: @supplemental_file.id }, status: :ok  }
     end
   end
 
   def destroy
-    raise Avalon::NotFound, "Cannot delete the supplemental file: #{params[:id]} not found" unless SupplementalFile.exists? params[:id].to_s
-    @supplemental_file = SupplementalFile.find(params[:id])
-    raise Avalon::NotFound, "Cannot delete the supplemental file: #{@supplemental_file.id} not found" unless @object.supplemental_files.any? { |f| f.id == @supplemental_file.id }
+    find_supplemental_file
 
     @object.supplemental_files -= [@supplemental_file]
     raise Avalon::SaveError, "An error occurred when deleting the supplemental file: #{@object.errors[:supplemental_files_json].full_messages}" unless @object.save
@@ -117,9 +160,34 @@ def set_object
       @object = fetch_object params[:master_file_id] || params[:media_object_id]
     end
 
+    def validate_params
+      attachment.present? || [:label, :language, :tags].any? { |v| supplemental_file_params[v].present? }
+    end
+
     def supplemental_file_params
       # TODO: Add parameters for minio and s3
-      params.fetch(:supplemental_file, {}).permit(:label, :language, :file, tags: [])
+      sup_file_params = params.fetch(:supplemental_file, {}).permit(:label, :language, :file, tags: [])
+      return sup_file_params unless metadata_upload?
+
+      meta_params = params[:metadata].present? ? JSON.parse(params[:metadata]).symbolize_keys : params
+
+      type = case meta_params[:type]
+             when 'caption'
+               'caption'
+             when 'transcript'
+               'transcript'
+             else
+              nil
+             end
+      treat_as_transcript = 'transcript' if meta_params[:treat_as_transcript] == 1
+      machine_generated = 'machine_generated' if meta_params[:machine_generated] == 1
+
+      sup_file_params[:label] ||= meta_params[:label].presence
+      sup_file_params[:language] ||= meta_params[:language].presence
+      # The uniq is to prevent multiple instances of 'transcript' tag if an update is performed with
+      # `{ type: transcript, treat_as_transcript: 1}`
+      sup_file_params[:tags] ||= [type, treat_as_transcript, machine_generated].compact.uniq
+      sup_file_params
     end
 
     def find_supplemental_file
@@ -133,7 +201,7 @@ def find_supplemental_file
 
 
     def handle_error(message:, status:)
-      if request.format == :json
+      if request.format == :json || request.headers['Avalon-Api-Key'].present?
         render json: { errors: message }, status: status
       else
         flash[:error] = message
@@ -151,6 +219,22 @@ def edit_structure_path
     end
 
     def edit_file_information
+      update_tags
+
+      @supplemental_file.label = supplemental_file_params[:label]
+      return unless supplemental_file_params[:language].present?
+      @supplemental_file.language = LanguageTerm.find(supplemental_file_params[:language]).code
+    end
+
+    def update_tags
+      # The edit page only provides supplemental_file_params[:tags] on object creation.
+      # Thus, we need to provide individual handling for both updates triggered by page
+      # actions and updates through the JSON api.
+      if request.format == 'json'
+        @supplemental_file.tags = supplemental_file_params[:tags].presence
+        return
+      end
+
       file_params = [ 
         { param: "machine_generated_#{params[:id]}".to_sym, tag: "machine_generated", method: :machine_generated? },
         { param: "treat_as_transcript_#{params[:id]}".to_sym, tag: "transcript", method: :caption_transcript? }
@@ -166,9 +250,27 @@ def edit_file_information
           @supplemental_file.tags -= [tag]
         end
       end
-      @supplemental_file.label = supplemental_file_params[:label]
-      return unless supplemental_file_params[:language].present?
-      @supplemental_file.language = LanguageTerm.find(supplemental_file_params[:language]).code
+    end
+
+    def metadata_from_params
+      {
+        label: supplemental_file_params[:label],
+        tags: supplemental_file_params[:tags],
+        language: supplemental_file_params[:language].present? ? LanguageTerm.find(supplemental_file_params[:language]).code : Settings.caption_default.language,
+        parent_id: @object.id
+      }.compact
+    end
+
+    def metadata_upload?
+      params[:format] == 'json'
+    end
+
+    def attachment
+      params[:file] || supplemental_file_params[:file]
+    end
+
+    def update_attached_file
+      @supplemental_file.attach_file(attachment)
     end
 
     def object_supplemental_file_path
diff --git a/app/models/supplemental_file.rb b/app/models/supplemental_file.rb
index 25a2473740..2659f18707 100644
--- a/app/models/supplemental_file.rb
+++ b/app/models/supplemental_file.rb
@@ -19,29 +19,28 @@ class SupplementalFile < ApplicationRecord
 
   scope :with_tag, ->(tag_filter) { where("tags LIKE ?", "%\n- #{tag_filter}\n%") }
 
+  attr_accessor :skip_file_type
+  attr_accessor :skip_index
+
   # TODO: the empty tag should represent a generic supplemental file
   validates :tags, array_inclusion: ['transcript', 'caption', 'machine_generated', '', nil]
   validates :language, inclusion: { in: LanguageTerm.map.keys }
-  validate  :validate_file_type, if: :caption?
   validates :parent_id, presence: true
+  validate  :validate_file_type, if: :validate_caption?
 
   serialize :tags, Array
 
   # Need to prepend so this runs before the callback added by `has_one_attached` above
   # See https://github.com/rails/rails/issues/37304
-  after_create_commit :update_index, prepend: true
-  after_update :update_index
-
-  def validate_file_type
-    errors.add(:file_type, "Uploaded file is not a recognized captions file") unless ['text/vtt', 'text/srt'].include? file.content_type
-  end
+  after_create_commit :update_index, prepend: true, unless: :skip_index
+  after_update_commit :update_index, prepend: true
 
   def attach_file(new_file)
     file.attach(new_file)
     extension = File.extname(new_file.original_filename)
     self.file.content_type = Mime::Type.lookup_by_extension(extension.slice(1..-1)).to_s if extension == '.srt'
     self.label = file.filename.to_s if label.blank?
-    self.language = tags.include?('caption') ? Settings.caption_default.language : 'eng'
+    self.language ||= tags.include?('caption') ? Settings.caption_default.language : 'eng'
   end
 
   def mime_type
@@ -64,6 +63,25 @@ def caption_transcript?
     tags.include?('caption') && tags.include?('transcript')
   end
 
+  def as_json(options={})
+    type = if tags.include?('caption')
+             'caption'
+           elsif tags.include?('transcript')
+             'transcript'
+           else
+             'generic'
+           end
+
+    {
+      id: id,
+      type: type,
+      label: label,
+      language: LanguageTerm.find(language).text,
+      treat_as_transcript: caption_transcript? ? '1' : nil,
+      machine_generated: machine_generated? ? '1' : nil
+    }.compact
+  end
+
   # Adapted from https://github.com/opencoconut/webvtt-ruby/blob/e07d59220260fce33ba5a0c3b355e3ae88b99457/lib/webvtt/parser.rb#L11-L30
   def self.convert_from_srt(srt)
     # normalize timestamps in srt
@@ -111,6 +129,14 @@ def segment_transcript transcript
 
   private
 
+  def validate_file_type
+    errors.add(:file_type, "Uploaded file is not a recognized captions file") unless ['text/vtt', 'text/srt'].include? file.content_type
+  end
+
+  def validate_caption?
+    caption? && !skip_file_type
+  end
+
   def c_time
     created_at&.to_datetime || DateTime.now
   end
diff --git a/config/routes.rb b/config/routes.rb
index 333402d776..463beacd3b 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -139,7 +139,7 @@
     end
 
     # Supplemental Files
-    resources :supplemental_files, except: [:new, :index, :edit]
+    resources :supplemental_files, except: [:new, :edit]
   end
 
   resources :master_files, except: [:new, :index] do
@@ -168,7 +168,7 @@
     end
 
     # Supplemental Files
-    resources :supplemental_files, except: [:new, :index, :edit] do
+    resources :supplemental_files, except: [:new, :edit] do
       member do
         get 'captions'
         get 'transcripts', :to => redirect('/master_files/%{master_file_id}/supplemental_files/%{id}')
diff --git a/spec/models/supplemental_file_spec.rb b/spec/models/supplemental_file_spec.rb
index 5155ef9435..9ad8838fd9 100644
--- a/spec/models/supplemental_file_spec.rb
+++ b/spec/models/supplemental_file_spec.rb
@@ -126,6 +126,50 @@
     end
   end
 
+  describe '#as_json' do
+    subject { supplemental_file.as_json }
+    let(:supplemental_file) { FactoryBot.create(:supplemental_file, label: 'Test') }
+    
+    context 'generic supplemental file' do
+      it 'serializes the metadata' do
+        expect(subject[:id]).to eq supplemental_file.id
+        expect(subject[:type]).to eq 'generic'
+        expect(subject[:label]).to eq supplemental_file.label
+        expect(subject[:language]).to eq 'English'
+        expect(subject[:treat_as_transcript]).to_not be_present
+        expect(subject[:machine_generated]).to_not be_present
+      end
+    end
+
+    context 'machine generated file' do
+      let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_caption_file, tags: ['machine_generated'], label: 'Test') }
+      it 'includes machine_generated in json' do
+        expect(subject[:machine_generated]).to eq '1'
+      end
+    end
+
+    context 'caption file' do
+      let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_caption_file, :with_caption_tag, label: 'Test') }
+      it 'sets the type properly' do
+        expect(subject[:type]).to eq 'caption'
+      end
+
+      context 'as transcript' do
+        let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_caption_file, tags: ['caption', 'transcript'], label: 'Test') }
+        it 'includes treat_as_transcript in JSON' do
+          expect(subject[:treat_as_transcript]).to eq '1'
+        end
+      end
+    end
+
+    context 'transcript file' do
+      let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_transcript_file, :with_transcript_tag, label: 'Test') }
+      it 'sets the type properly' do
+        expect(subject[:type]).to eq 'transcript'
+      end
+    end
+  end
+
   describe '.convert_from_srt' do
     let(:input) { "1\n00:00:03,498 --> 00:00:05,000\n- Example Captions\n" }
     let(:output) { "WEBVTT\n\n1\n00:00:03.498 --> 00:00:05.000\n- Example Captions" }
diff --git a/spec/support/supplemental_files_controller_examples.rb b/spec/support/supplemental_files_controller_examples.rb
index b711ef6c0e..365ff25bd5 100644
--- a/spec/support/supplemental_files_controller_examples.rb
+++ b/spec/support/supplemental_files_controller_examples.rb
@@ -42,13 +42,11 @@
   }
 
   let(:invalid_update_attributes) {
-    { file: uploaded_file }
+    {}
   }
 
   let(:uploaded_file) { fixture_file_upload('/collection_poster.png', 'image/png') }
 
-  let(:supplemental_file) { FactoryBot.create(:supplemental_file) }
-
   # This should return the minimal set of values that should be in the session
   # in order to pass any filters (e.g. authentication) defined in
   # SupplementalFilesController. Be sure to keep this updated too.
@@ -64,27 +62,78 @@
   let(:manager) { media_object.collection.managers.first }
 
   describe 'security' do
-    context 'with unauthenticated user' do
-      it "all routes should return 401" do
-        expect(post :create, params: { class_id => object.id }).to have_http_status(401)
-        expect(put :update, params: { class_id =>  object.id, id: supplemental_file.id }).to have_http_status(401)
-        expect(delete :destroy, params: { class_id =>  object.id, id: supplemental_file.id }).to have_http_status(401)
-        expect(get :show, params: { class_id =>  object.id, id: supplemental_file.id }).to have_http_status(401)
+    describe 'ingest api' do
+      it "all routes should return 401 when no token is present" do
+        expect(get :index, params: { class_id => object.id, format: 'json' }).to have_http_status(401)
+        expect(get :show, params: { class_id => object.id, id: supplemental_file.id, format: 'json' }).to have_http_status(401)
+        expect(post :create, params: { class_id => object.id, format: 'json' }).to have_http_status(401)
+        expect(put :update, params: { class_id => object.id, id: supplemental_file.id, format: 'json' }).to have_http_status(401)
+        expect(delete :destroy, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
+      end
+      it "all routes should return 403 when a bad token in present" do
+        request.headers['Avalon-Api-Key'] = 'badtoken'
+        expect(get :index, params: { class_id => object.id, format: 'json' }).to have_http_status(403)
+        expect(get :show, params: { class_id => object.id, id: supplemental_file.id, format: 'json' }).to have_http_status(403)
+        expect(post :create, params: { class_id => object.id, format: 'json' }).to have_http_status(403)
+        expect(put :update, params: { class_id => object.id, id: supplemental_file.id, format: 'json' }).to have_http_status(403)
+        expect(delete :destroy, params: { class_id => object.id, id: supplemental_file.id, format: 'json' }).to have_http_status(403)
       end
     end
-    context 'with end-user without permission' do
-      before do
-        login_as :user
+    describe 'normal auth' do
+      context 'with unauthenticated user' do
+        it "all routes should return 401" do
+          expect(get :index, params: { class_id => object.id }).to have_http_status(401)
+          expect(get :show, params: { class_id =>  object.id, id: supplemental_file.id }).to have_http_status(401)
+          expect(post :create, params: { class_id => object.id }).to have_http_status(401)
+          expect(put :update, params: { class_id =>  object.id, id: supplemental_file.id }).to have_http_status(401)
+          expect(delete :destroy, params: { class_id =>  object.id, id: supplemental_file.id }).to have_http_status(401)
+        end
       end
-      it "all routes should return 401" do
-        expect(post :create, params: { class_id => object.id }).to have_http_status(401)
-        expect(put :update, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
-        expect(delete :destroy, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
-        expect(get :show, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
+      context 'with end-user without permission' do
+        before do
+          login_as :user
+        end
+        it "all routes should return 401" do
+          expect(get :index, params: { class_id => object.id }).to have_http_status(401)
+          expect(get :show, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
+          expect(post :create, params: { class_id => object.id }).to have_http_status(401)
+          expect(put :update, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
+          expect(delete :destroy, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
+        end
       end
     end
   end
 
+  describe "GET #index" do
+    subject { JSON.parse(response.body) }
+    let(:master_file) { FactoryBot.create(:master_file, media_object: media_object) }
+    let(:media_object) { FactoryBot.create(:fully_searchable_media_object) }
+    let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_attached_file, parent_id: object.id) }
+    let(:caption) { FactoryBot.create(:supplemental_file, :with_caption_file, tags: ['caption', 'transcript'], parent_id: object.id) }
+    let(:transcript) { FactoryBot.create(:supplemental_file, :with_transcript_file, tags: ['transcript', 'machine_generated'], parent_id: object.id) }
+    let(:object) { object_class == MasterFile ? master_file : media_object }
+
+    before :each do
+      login_user manager
+      object.supplemental_files = [supplemental_file, caption, transcript]
+      object.save
+    end
+
+    it "returns metadata for all associated supplemental files" do
+      get :index, params: { class_id => object.id }, session: valid_session
+      expect(subject.count).to eq 3
+      [supplemental_file, caption, transcript].each do |file|
+        expect(subject.any? { |s| s == JSON.parse(file.as_json.to_json) }).to eq true
+      end
+    end
+
+    it "paginates results when requested" do
+      get :index, params: { class_id => object.id, per_page: 1, page: 1 }, session: valid_session
+      expect(subject.count).to eq 1
+      expect(subject.first.symbolize_keys).to eq supplemental_file.as_json
+    end
+  end
+
   describe "GET #show" do
     let(:master_file) { FactoryBot.create(:master_file, media_object: public_media_object, supplemental_files: [supplemental_file]) }
     let(:public_media_object) { FactoryBot.create(:fully_searchable_media_object, supplemental_files: [supplemental_file]) }
@@ -95,6 +144,13 @@
       get :show, params: { class_id => object.id, id: supplemental_file.id }, session: valid_session
       expect(response).to redirect_to Rails.application.routes.url_helpers.rails_blob_path(supplemental_file.file, disposition: "attachment")
     end
+
+    context '.json' do
+      it 'returns the supplemental file metadata' do
+        get :show, params: { class_id => object.id, id: supplemental_file.id, format: 'json' }, session: valid_session
+        expect(JSON.parse(response.body).symbolize_keys).to eq supplemental_file.as_json
+      end
+    end
   end
 
   describe "POST #create" do
@@ -107,52 +163,71 @@
     end
 
     context 'json request' do
+      let(:metadata) { { label: 'label', type: 'caption', language: 'French', machine_generated: 1, treat_as_transcript: 1 } }
       context "with valid params" do
-        it "updates the SupplementalFile for #{object_class}" do
+        let(:uploaded_file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', 'captions.vtt'), 'text/vtt') }
+        let(:valid_create_attributes) { { file: uploaded_file, metadata: metadata.to_json } }
+        it "creates a SupplementalFile for #{object_class}" do
           expect {
-            post :create, params: { class_id => object.id, supplemental_file: valid_create_attributes, format: :json }, session: valid_session
+            post :create, params: { class_id => object.id, **valid_create_attributes, format: :json }, session: valid_session
           }.to change { object.reload.supplemental_files.size }.by(1)
           expect(response).to have_http_status(:created)
-          expect(response.location).to eq "/#{object_class.model_name.plural}/#{object.id}/supplemental_files/#{assigns(:supplemental_file).id}"
+          expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
 
           expect(object.supplemental_files.first.id).to eq 1
           expect(object.supplemental_files.first.label).to eq 'label'
           expect(object.supplemental_files.first.file).to be_attached
         end
 
-        let(:tags) { ["transcript"] }
-        let(:uploaded_file) { fixture_file_upload('/captions.vtt', 'text/vtt') }
-        let(:valid_create_attributes_with_tags) { valid_create_attributes.merge(tags: tags) }
+        context 'with just metadata' do
+          it "creates a SupplementalFile for #{object_class}" do
+            request.headers['Content-Type'] = 'application/json'
+            expect {
+              post :create, params: { class_id => object.id, metadata: metadata.to_json, format: :json }, session: valid_session
+            }.to change { object.reload.supplemental_files.size }.by(1)
+            expect(response).to have_http_status(:created)
+            expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
 
-        it "creates a SupplementalFile with tags for #{object_class}" do
-          expect {
-            post :create, params: { class_id => object.id, supplemental_file: valid_create_attributes_with_tags, format: :json }, session: valid_session
-          }.to change { object.reload.supplemental_files.size }.by(1)
-          expect(response).to have_http_status(:created)
-          expect(response.location).to eq "/#{object_class.model_name.plural}/#{object.id}/supplemental_files/#{assigns(:supplemental_file).id}"
+            expect(object.supplemental_files.first.id).to eq 1
+            expect(object.supplemental_files.first.label).to eq 'label'
+            expect(object.supplemental_files.first.language).to eq 'fre'
+            expect(object.supplemental_files.first.tags).to match_array ['caption', 'transcript', 'machine_generated']
+            expect(object.supplemental_files.first.file).to_not be_attached
+          end
 
-          expect(object.supplemental_files.first.id).to eq 1
-          expect(object.supplemental_files.first.label).to eq 'label'
-          expect(object.supplemental_files.first.tags).to eq tags
-          expect(object.supplemental_files.first.file).to be_attached
+          context 'with just inlined metadata' do
+            it "creates a SupplementalFile for #{object_class}" do
+              request.headers['Content-Type'] = 'application/json'
+              expect {
+                post :create, params: { class_id => object.id, **metadata, format: :json }, session: valid_session
+              }.to change { object.reload.supplemental_files.size }.by(1)
+              expect(response).to have_http_status(:created)
+              expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
+
+              expect(object.supplemental_files.first.id).to eq 1
+              expect(object.supplemental_files.first.label).to eq 'label'
+              expect(object.supplemental_files.first.language).to eq 'fre'
+              expect(object.supplemental_files.first.tags).to match_array ['caption', 'transcript', 'machine_generated']
+              expect(object.supplemental_files.first.file).to_not be_attached
+            end
+          end
         end
 
-        context 'with mime type that does not match extension' do
-          let(:tags) { ['caption'] }
-          let(:extension) { 'srt' }
-          let(:uploaded_file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', 'captions.srt'), 'text/plain') }
-          it "creates a SupplementalFile with correct content_type" do
-            expect{
-              post :create, params: { class_id => object.id, supplemental_file: valid_create_attributes_with_tags, format: :json}, session: valid_session
+        context 'with incomplete metadata' do
+          let(:metadata) { { label: 'label' } }
+          it "creates a SupplementalFile for #{object_class} with default values" do
+            request.headers['Content-Type'] = 'application/json'
+            expect {
+              post :create, params: { class_id => object.id, metadata: metadata.to_json, format: :json }, session: valid_session
             }.to change { object.reload.supplemental_files.size }.by(1)
             expect(response).to have_http_status(:created)
-            expect(response.location).to eq "/#{object_class.model_name.plural}/#{object.id}/supplemental_files/#{assigns(:supplemental_file).id}"
+            expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
 
             expect(object.supplemental_files.first.id).to eq 1
             expect(object.supplemental_files.first.label).to eq 'label'
-            expect(object.supplemental_files.first.tags).to eq tags
-            expect(object.supplemental_files.first.file).to be_attached
-            expect(object.supplemental_files.first.file.content_type).to eq Mime::Type.lookup_by_extension(extension)
+            expect(object.supplemental_files.first.language).to eq 'eng'
+            expect(object.supplemental_files.first.tags).to eq []
+            expect(object.supplemental_files.first.file).to_not be_attached
           end
         end
       end
@@ -163,11 +238,18 @@
           expect(response).to have_http_status(400)
         end
       end
+
+      context "with invalid file type" do
+        it "returns a 500" do
+          post :create, params: { class_id=> object.id, file: uploaded_file, type: 'caption', format: :json }, session: valid_session
+          expect(response).to have_http_status(500)
+        end
+      end
     end
 
     context 'html request' do
       context "with valid params" do
-        it "updates the SupplementalFile" do
+        it "creates a SupplementalFile for #{object_class}" do
           expect {
             post :create, params: { class_id => object.id, supplemental_file: valid_create_attributes, format: :html }, session: valid_session
           }.to change { object.reload.supplemental_files.size }.by(1)
@@ -180,6 +262,64 @@
         end
       end
 
+      context "including tags" do
+        let(:tags) { ["transcript"] }
+        let(:uploaded_file) { fixture_file_upload('/captions.vtt', 'text/vtt') }
+        let(:valid_create_attributes_with_tags) { valid_create_attributes.merge(tags: tags) }
+
+        it "creates a SupplementalFile with tags for #{object_class}" do
+          expect {
+            post :create, params: { class_id => object.id, supplemental_file: valid_create_attributes_with_tags, format: :html }, session: valid_session
+          }.to change { object.reload.supplemental_files.size }.by(1)
+          expect(response).to redirect_to("http://#{@request.host}/media_objects/#{media_object.id}/edit?step=file-upload")
+          expect(flash[:success]).to be_present
+
+          expect(object.supplemental_files.first.id).to eq 1
+          expect(object.supplemental_files.first.label).to eq 'label'
+          expect(object.supplemental_files.first.tags).to eq tags
+          expect(object.supplemental_files.first.file).to be_attached
+        end
+      end
+
+      context 'with mime type that does not match extension' do
+        let(:tags) { ['caption'] }
+        let(:extension) { 'srt' }
+        let(:uploaded_file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', 'captions.srt'), 'text/plain') }
+        let(:valid_create_attributes_with_tags) { valid_create_attributes.merge(tags: tags) }
+        it "creates a SupplementalFile with correct content_type" do
+          expect{
+            post :create, params: { class_id => object.id, supplemental_file: valid_create_attributes_with_tags, format: :html }, session: valid_session
+          }.to change { object.reload.supplemental_files.size }.by(1)
+          expect(response).to redirect_to("http://#{@request.host}/media_objects/#{media_object.id}/edit?step=file-upload")
+          expect(flash[:success]).to be_present
+
+          expect(object.supplemental_files.first.id).to eq 1
+          expect(object.supplemental_files.first.label).to eq 'label'
+          expect(object.supplemental_files.first.tags).to eq tags
+          expect(object.supplemental_files.first.file).to be_attached
+          expect(object.supplemental_files.first.file.content_type).to eq Mime::Type.lookup_by_extension(extension)
+        end
+      end
+
+      context 'API with just a file' do
+        let(:uploaded_file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', 'captions.srt'), 'text/plain') }
+        before { ApiToken.create token: 'secret_token', username: 'archivist1@example.com', email: 'archivist1@example.com' }
+        it "creates a SupplementalFile for #{object_class}" do
+          request.headers['Avalon-Api-Key'] = 'secret_token'
+          expect {
+            post :create, params: { class_id => object.id, file: uploaded_file, format: :html }, session: valid_session
+          }.to change { object.reload.supplemental_files.size }.by(1)
+          expect(response).to have_http_status(:created)
+          expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
+
+          expect(object.supplemental_files.first.id).to eq 1
+          expect(object.supplemental_files.first.label).to eq 'captions.srt'
+          expect(object.supplemental_files.first.language).to eq Settings.caption_default.language
+          expect(object.supplemental_files.first.tags).to be_empty
+          expect(object.supplemental_files.first.file).to be_attached
+        end
+      end
+
       context "with invalid params" do
         it "returns a 400" do
           post :create, params: { class_id => object.id, supplemental_file: invalid_create_attributes, format: :html }, session: valid_session
@@ -189,7 +329,7 @@
       end
     end
 
-    context 'does not attach' do
+    context 'fails to attach file' do
       let(:supplemental_file) { FactoryBot.build(:supplemental_file) }
       before do
         allow(SupplementalFile).to receive(:new).and_return(supplemental_file)
@@ -228,14 +368,27 @@
     end
 
     context 'json request' do
-      context "with valid params" do
-        it "creates a new SupplementalFile" do
+      before :each do
+        request.headers['Content-Type'] = 'application/json'
+      end
+      context "with valid metadata params" do
+        let(:valid_update_attributes) { { label: 'new label', type: 'transcript', machine_generated: 1 }.to_json }
+        it "updates the SupplementalFile metadata for #{object_class}" do
           expect {
-            put :update, params: { class_id => object.id, id: supplemental_file.id, supplemental_file: valid_update_attributes, format: :json }, session: valid_session
+            put :update, params: { class_id => object.id, id: supplemental_file.id, metadata: valid_update_attributes, format: :json }, session: valid_session
           }.to change { object.reload.supplemental_files.first.label }.from('label').to('new label')
+           .and change { object.reload.supplemental_files.first.tags }.from([]).to(['transcript', 'machine_generated'])
 
           expect(response).to have_http_status(:ok)
-          expect(response.location).to eq "/#{object_class.model_name.plural}/#{object.id}/supplemental_files/#{assigns(:supplemental_file).id}"
+          expect(response.body).to eq({ "supplemental_file": supplemental_file.id }.to_json)
+        end
+      end
+
+      context "with new file and valid metadata params" do
+        let(:file_update) { fixture_file_upload(Rails.root.join('spec', 'fixtures', 'captions.vtt'), 'text/vtt') }
+        it "returns a 400" do
+          put :update, params: { class_id => object.id, id: supplemental_file.id, file: file_update, supplemental_file: valid_update_attributes, format: :json }, session: valid_session
+          expect(response).to have_http_status(400)
         end
       end
 
@@ -256,7 +409,7 @@
 
     context 'html request' do
       context "with valid params" do
-        it "creates a new SupplementalFile" do
+        it "updates the SupplementalFile for #{object_class}" do
           expect {
             put :update, params: { class_id => object.id, id: supplemental_file.id, supplemental_file: valid_update_attributes, format: :html }, session: valid_session
           }.to change { master_file.reload.supplemental_files.first.label }.from('label').to('new label')
@@ -322,6 +475,29 @@
         end
       end
 
+      context "API with new file" do
+        let(:file_update) { fixture_file_upload(Rails.root.join('spec', 'fixtures', 'captions.vtt'), 'text/vtt') }
+        before { ApiToken.create token: 'secret_token', username: 'archivist1@example.com', email: 'archivist1@example.com' }
+        it "updates the SupplementalFile attached file for #{object_class}" do
+          request.headers['Avalon-Api-Key'] = 'secret_token'
+          expect {
+            put :update, params: { class_id => object.id, id: supplemental_file.id, file: file_update, format: :html }, session: valid_session
+          }.to change { object.reload.supplemental_files.first.file }
+
+          expect(response).to have_http_status(:ok)
+          expect(response.body).to eq({ "supplemental_file": supplemental_file.id }.to_json)
+        end
+      end
+
+      context "API without file" do
+        before { ApiToken.create token: 'secret_token', username: 'archivist1@example.com', email: 'archivist1@example.com' }
+        it "returns a 400" do
+          request.headers['Avalon-Api-Key'] = 'secret_token'
+          put :update, params: { class_id=> object.id, id: supplemental_file.id, metadata: valid_update_attributes, format: :html }, session: valid_session
+          expect(response).to have_http_status(400)
+        end
+      end
+
       context "with invalid params" do
         it "returns a 400" do
           put :update, params: { class_id => object.id, id: supplemental_file.id, supplemental_file: invalid_update_attributes, format: :html }, session: valid_session
@@ -337,6 +513,16 @@
           expect(flash[:error]).to be_present
         end
       end
+
+      context "API updating caption with invalid file type" do
+        let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_caption_tag, skip_file_type: true) }
+        before { ApiToken.create token: 'secret_token', username: 'archivist1@example.com', email: 'archivist1@example.com' }
+        it "returns a 500" do
+          request.headers['Avalon-Api-Key'] = 'secret_token'
+          put :update, params: { class_id=> object.id, id: supplemental_file.id, file: uploaded_file, format: :html }, session: valid_session
+          expect(response).to have_http_status(500)
+        end
+      end
     end
   end
 

From 429ea7eb3da95740aa7660f79c05c7f0cccb99bb Mon Sep 17 00:00:00 2001
From: Mason Ballengee <masaball@iu.edu>
Date: Thu, 6 Jun 2024 15:53:56 -0400
Subject: [PATCH 2/4] Fix update_index callbacks

---
 app/models/supplemental_file.rb       |  6 +++-
 spec/models/supplemental_file_spec.rb | 48 +++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/app/models/supplemental_file.rb b/app/models/supplemental_file.rb
index 2659f18707..2124c8ba59 100644
--- a/app/models/supplemental_file.rb
+++ b/app/models/supplemental_file.rb
@@ -32,7 +32,7 @@ class SupplementalFile < ApplicationRecord
 
   # Need to prepend so this runs before the callback added by `has_one_attached` above
   # See https://github.com/rails/rails/issues/37304
-  after_create_commit :update_index, prepend: true, unless: :skip_index
+  after_create_commit :index_file, prepend: true, unless: :skip_index
   after_update_commit :update_index, prepend: true
 
   def attach_file(new_file)
@@ -98,9 +98,13 @@ def self.convert_from_srt(srt)
     "WEBVTT\n\n#{conversion}".strip
   end
   
+  # We need to use both after_create_commit and after_update_commit to update the index properly in both cases. 
+  # However, they cannot call the same method name or only the last defined callback will take effect.
+  # https://guides.rubyonrails.org/active_record_callbacks.html#aliases-for-after-commit
   def update_index
     ActiveFedora::SolrService.add(to_solr, softCommit: true)
   end
+  alias index_file update_index
 
   # Creates a solr document hash for the {#object}
   # @return [Hash] the solr document
diff --git a/spec/models/supplemental_file_spec.rb b/spec/models/supplemental_file_spec.rb
index 9ad8838fd9..5169ecc2ef 100644
--- a/spec/models/supplemental_file_spec.rb
+++ b/spec/models/supplemental_file_spec.rb
@@ -44,6 +44,12 @@
           expect(subject.errors[:file_type]).not_to be_empty
         end
       end
+      context "caption metadata only with skip_file_type" do
+        let(:subject) { FactoryBot.build(:supplemental_file, :with_caption_tag, skip_file_type: true) }
+        it 'should skip validation' do
+          expect(subject.valid?).to be_truthy
+        end
+      end
     end
 
     describe 'scopes' do
@@ -64,6 +70,48 @@
       end
     end
 
+    describe '#update_index' do
+      let(:transcript) { FactoryBot.build(:supplemental_file, :with_transcript_file, :with_transcript_tag) }
+      context 'on create' do
+        it 'triggers callback' do
+          expect(transcript).to receive(:index_file)
+          transcript.save
+        end
+
+        it 'indexes the transcript' do
+          transcript.save
+          solr_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first
+          expect(solr_doc["transcript_tsim"]).to eq ["00:00:03.500 --> 00:00:05.000 Example captions"]
+        end
+
+        context 'skip index' do
+          let(:transcript) { FactoryBot.build(:supplemental_file, :with_transcript_file, :with_transcript_tag, skip_index: true) }
+          it 'does not trigger callback' do
+            expect(transcript).to_not receive(:index_file)
+            transcript.save
+          end
+        end
+      end
+
+      context 'on update' do
+        let(:transcript) { FactoryBot.create(:supplemental_file, :with_transcript_file, :with_transcript_tag) }
+        it 'triggers callback' do
+          transcript.file.attach(fixture_file_upload(Rails.root.join('spec', 'fixtures', 'chunk_test.vtt'), 'text/vtt'))
+          expect(transcript).to receive(:update_index)
+          transcript.save
+        end
+
+        it 'updates the indexed transcript' do
+          before_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first
+          expect(before_doc["transcript_tsim"].first).to eq "00:00:03.500 --> 00:00:05.000 Example captions"
+          transcript.file.attach(fixture_file_upload(Rails.root.join('spec', 'fixtures', 'chunk_test.vtt'), 'text/vtt'))
+          transcript.save
+          after_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first
+          expect(after_doc["transcript_tsim"].first).to eq("00:00:01.200 --> 00:00:21.000 [music]")
+        end
+      end
+    end
+
     describe '#to_solr' do
       let(:caption) { FactoryBot.create(:supplemental_file, :with_caption_file, :with_caption_tag) }
       let(:transcript) { FactoryBot.create(:supplemental_file, :with_transcript_file, :with_transcript_tag) }

From bb38720a3bded926ffc7d29fdfa49e6da3e68b98 Mon Sep 17 00:00:00 2001
From: Mason Ballengee <masaball@iu.edu>
Date: Tue, 11 Jun 2024 15:31:48 -0400
Subject: [PATCH 3/4] Fixes from review

---
 .../supplemental_files_controller.rb          | 47 ++++++++-----------
 app/models/supplemental_file.rb               | 20 +++-----
 config/routes.rb                              |  7 ++-
 .../supplemental_files_controller_examples.rb | 32 +++++++------
 4 files changed, 50 insertions(+), 56 deletions(-)

diff --git a/app/controllers/supplemental_files_controller.rb b/app/controllers/supplemental_files_controller.rb
index 0fb3770732..c9b8f3c8ea 100644
--- a/app/controllers/supplemental_files_controller.rb
+++ b/app/controllers/supplemental_files_controller.rb
@@ -16,7 +16,6 @@
 class SupplementalFilesController < ApplicationController
   include Rails::Pagination
 
-  before_action :authenticate_user!, except: [:show, :captions]
   before_action :set_object
   before_action :authorize_object
 
@@ -55,32 +54,27 @@ def create
 
       # Raise errror if file wasn't attached
       raise Avalon::SaveError, "File could not be attached." unless @supplemental_file.file.attached?
-
-      raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save
-    else
-      # For multi-step API upload, we need to skip file type validation on the metadata portion of the save.
-      # Captions are validated as VTT or SRT which cannot be validated if there is no file attached.
-      @supplemental_file.skip_file_type = true
-      # Transcripts are automatically indexed on creation, so we need to skip indexing to avoid an error
-      # when saving without an uploaded file.
-      @supplemental_file.skip_index = true
-      raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save
     end
 
+    raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save
+
     @object.supplemental_files += [@supplemental_file]
     raise Avalon::SaveError, @object.errors[:supplemental_files_json].full_messages unless @object.save
 
     flash[:success] = "Supplemental file successfully added."
 
     respond_to do |format|
-      format.html { 
-        if request.headers['Avalon-Api-Key'].present?
-          render json: { supplemental_file: @supplemental_file.id }, status: :created
+      format.html {
+        # This path is for uploading the binary file. We need to provide a JSON response
+        # for the case of someone uploading through a CLI.
+        if request.headers['Accept'] == 'application/json'
+          render json: { id: @supplemental_file.id }, status: :created
         else
           redirect_to edit_structure_path
         end
       }
-      format.json { render json: { supplemental_file: @supplemental_file.id }, status: :created }
+      # This path is for uploading the metadata payload.
+      format.json { render json: { id: @supplemental_file.id }, status: :created }
     end
   end
 
@@ -113,20 +107,23 @@ def update
 
     edit_file_information if !attachment
 
-    update_attached_file if attachment
+    @supplemental_file.attach_file(attachment) if attachment
 
     raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save
 
     flash[:success] = "Supplemental file successfully updated."
     respond_to do |format|
-      format.html { 
-        if request.headers['Avalon-Api-Key'].present?
-          render json: { supplemental_file: @supplemental_file.id }
+      format.html {
+        # This path is for uploading the binary file. We need to provide a JSON response
+        # for the case of someone uploading through a CLI.
+        if request.headers['Accept'] == 'application/json'
+          render json: { id: @supplemental_file.id }
         else
           redirect_to edit_structure_path
         end
       }
-      format.json { render json: { supplemental_file: @supplemental_file.id }, status: :ok  }
+      # This path is for uploading the metadata payload.
+      format.json { render json: { id: @supplemental_file.id }, status: :ok  }
     end
   end
 
@@ -177,10 +174,10 @@ def supplemental_file_params
              when 'transcript'
                'transcript'
              else
-              nil
+               nil
              end
-      treat_as_transcript = 'transcript' if meta_params[:treat_as_transcript] == 1
-      machine_generated = 'machine_generated' if meta_params[:machine_generated] == 1
+      treat_as_transcript = 'transcript' if meta_params[:treat_as_transcript] == true
+      machine_generated = 'machine_generated' if meta_params[:machine_generated] == true
 
       sup_file_params[:label] ||= meta_params[:label].presence
       sup_file_params[:language] ||= meta_params[:language].presence
@@ -269,10 +266,6 @@ def attachment
       params[:file] || supplemental_file_params[:file]
     end
 
-    def update_attached_file
-      @supplemental_file.attach_file(attachment)
-    end
-
     def object_supplemental_file_path
       if @object.is_a? MasterFile
         master_file_supplemental_file_path(id: @supplemental_file.id, master_file_id: @object.id)
diff --git a/app/models/supplemental_file.rb b/app/models/supplemental_file.rb
index 2124c8ba59..4d091b8b23 100644
--- a/app/models/supplemental_file.rb
+++ b/app/models/supplemental_file.rb
@@ -19,20 +19,17 @@ class SupplementalFile < ApplicationRecord
 
   scope :with_tag, ->(tag_filter) { where("tags LIKE ?", "%\n- #{tag_filter}\n%") }
 
-  attr_accessor :skip_file_type
-  attr_accessor :skip_index
-
   # TODO: the empty tag should represent a generic supplemental file
   validates :tags, array_inclusion: ['transcript', 'caption', 'machine_generated', '', nil]
   validates :language, inclusion: { in: LanguageTerm.map.keys }
   validates :parent_id, presence: true
-  validate  :validate_file_type, if: :validate_caption?
+  validate  :validate_file_type, if: :caption?
 
   serialize :tags, Array
 
   # Need to prepend so this runs before the callback added by `has_one_attached` above
   # See https://github.com/rails/rails/issues/37304
-  after_create_commit :index_file, prepend: true, unless: :skip_index
+  after_create_commit :index_file, prepend: true
   after_update_commit :update_index, prepend: true
 
   def attach_file(new_file)
@@ -77,8 +74,8 @@ def as_json(options={})
       type: type,
       label: label,
       language: LanguageTerm.find(language).text,
-      treat_as_transcript: caption_transcript? ? '1' : nil,
-      machine_generated: machine_generated? ? '1' : nil
+      treat_as_transcript: caption_transcript? ? true : false,
+      machine_generated: machine_generated? ? true : false
     }.compact
   end
 
@@ -102,7 +99,7 @@ def self.convert_from_srt(srt)
   # However, they cannot call the same method name or only the last defined callback will take effect.
   # https://guides.rubyonrails.org/active_record_callbacks.html#aliases-for-after-commit
   def update_index
-    ActiveFedora::SolrService.add(to_solr, softCommit: true)
+    ActiveFedora::SolrService.add(to_solr, softCommit: true) if file.present?
   end
   alias index_file update_index
 
@@ -134,11 +131,8 @@ def segment_transcript transcript
   private
 
   def validate_file_type
-    errors.add(:file_type, "Uploaded file is not a recognized captions file") unless ['text/vtt', 'text/srt'].include? file.content_type
-  end
-
-  def validate_caption?
-    caption? && !skip_file_type
+    return unless file.present?
+    errors.add(:file_type, "Uploaded file is not a recognized captions file") unless ['text/vtt', 'text/srt'].include?(file.content_type)
   end
 
   def c_time
diff --git a/config/routes.rb b/config/routes.rb
index 463beacd3b..d9d6e7c836 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -139,7 +139,9 @@
     end
 
     # Supplemental Files
-    resources :supplemental_files, except: [:new, :edit]
+    resources :supplemental_files, except: [:new, :index, :edit] do
+      get :index, constraints: { format: 'json' }, on: :collection
+    end
   end
 
   resources :master_files, except: [:new, :index] do
@@ -168,11 +170,12 @@
     end
 
     # Supplemental Files
-    resources :supplemental_files, except: [:new, :edit] do
+    resources :supplemental_files, except: [:new, :index, :edit] do
       member do
         get 'captions'
         get 'transcripts', :to => redirect('/master_files/%{master_file_id}/supplemental_files/%{id}')
       end
+      get :index, constraints: { format: 'json' }, on: :collection
     end
   end
 
diff --git a/spec/support/supplemental_files_controller_examples.rb b/spec/support/supplemental_files_controller_examples.rb
index 365ff25bd5..eadf174aa5 100644
--- a/spec/support/supplemental_files_controller_examples.rb
+++ b/spec/support/supplemental_files_controller_examples.rb
@@ -82,7 +82,7 @@
     describe 'normal auth' do
       context 'with unauthenticated user' do
         it "all routes should return 401" do
-          expect(get :index, params: { class_id => object.id }).to have_http_status(401)
+          expect(get :index, params: { class_id => object.id, format: 'json' }).to have_http_status(401)
           expect(get :show, params: { class_id =>  object.id, id: supplemental_file.id }).to have_http_status(401)
           expect(post :create, params: { class_id => object.id }).to have_http_status(401)
           expect(put :update, params: { class_id =>  object.id, id: supplemental_file.id }).to have_http_status(401)
@@ -94,7 +94,7 @@
           login_as :user
         end
         it "all routes should return 401" do
-          expect(get :index, params: { class_id => object.id }).to have_http_status(401)
+          expect(get :index, params: { class_id => object.id, format: 'json' }).to have_http_status(401)
           expect(get :show, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
           expect(post :create, params: { class_id => object.id }).to have_http_status(401)
           expect(put :update, params: { class_id => object.id, id: supplemental_file.id }).to have_http_status(401)
@@ -120,7 +120,7 @@
     end
 
     it "returns metadata for all associated supplemental files" do
-      get :index, params: { class_id => object.id }, session: valid_session
+      get :index, params: { class_id => object.id, format: 'json' }, session: valid_session
       expect(subject.count).to eq 3
       [supplemental_file, caption, transcript].each do |file|
         expect(subject.any? { |s| s == JSON.parse(file.as_json.to_json) }).to eq true
@@ -128,7 +128,7 @@
     end
 
     it "paginates results when requested" do
-      get :index, params: { class_id => object.id, per_page: 1, page: 1 }, session: valid_session
+      get :index, params: { class_id => object.id, format: 'json', per_page: 1, page: 1 }, session: valid_session
       expect(subject.count).to eq 1
       expect(subject.first.symbolize_keys).to eq supplemental_file.as_json
     end
@@ -163,7 +163,7 @@
     end
 
     context 'json request' do
-      let(:metadata) { { label: 'label', type: 'caption', language: 'French', machine_generated: 1, treat_as_transcript: 1 } }
+      let(:metadata) { { label: 'label', type: 'caption', language: 'French', machine_generated: true, treat_as_transcript: true } }
       context "with valid params" do
         let(:uploaded_file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', 'captions.vtt'), 'text/vtt') }
         let(:valid_create_attributes) { { file: uploaded_file, metadata: metadata.to_json } }
@@ -172,7 +172,7 @@
             post :create, params: { class_id => object.id, **valid_create_attributes, format: :json }, session: valid_session
           }.to change { object.reload.supplemental_files.size }.by(1)
           expect(response).to have_http_status(:created)
-          expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
+          expect(JSON.parse(response.body)).to eq ({ "id" => assigns(:supplemental_file).id })
 
           expect(object.supplemental_files.first.id).to eq 1
           expect(object.supplemental_files.first.label).to eq 'label'
@@ -186,7 +186,7 @@
               post :create, params: { class_id => object.id, metadata: metadata.to_json, format: :json }, session: valid_session
             }.to change { object.reload.supplemental_files.size }.by(1)
             expect(response).to have_http_status(:created)
-            expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
+            expect(JSON.parse(response.body)).to eq ({ "id" => assigns(:supplemental_file).id })
 
             expect(object.supplemental_files.first.id).to eq 1
             expect(object.supplemental_files.first.label).to eq 'label'
@@ -202,7 +202,7 @@
                 post :create, params: { class_id => object.id, **metadata, format: :json }, session: valid_session
               }.to change { object.reload.supplemental_files.size }.by(1)
               expect(response).to have_http_status(:created)
-              expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
+              expect(JSON.parse(response.body)).to eq ({ "id" => assigns(:supplemental_file).id })
 
               expect(object.supplemental_files.first.id).to eq 1
               expect(object.supplemental_files.first.label).to eq 'label'
@@ -221,7 +221,7 @@
               post :create, params: { class_id => object.id, metadata: metadata.to_json, format: :json }, session: valid_session
             }.to change { object.reload.supplemental_files.size }.by(1)
             expect(response).to have_http_status(:created)
-            expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
+            expect(JSON.parse(response.body)).to eq ({ "id" => assigns(:supplemental_file).id })
 
             expect(object.supplemental_files.first.id).to eq 1
             expect(object.supplemental_files.first.label).to eq 'label'
@@ -306,11 +306,12 @@
         before { ApiToken.create token: 'secret_token', username: 'archivist1@example.com', email: 'archivist1@example.com' }
         it "creates a SupplementalFile for #{object_class}" do
           request.headers['Avalon-Api-Key'] = 'secret_token'
+          request.headers['Accept'] = 'application/json'
           expect {
             post :create, params: { class_id => object.id, file: uploaded_file, format: :html }, session: valid_session
           }.to change { object.reload.supplemental_files.size }.by(1)
           expect(response).to have_http_status(:created)
-          expect(JSON.parse(response.body)).to eq ({ "supplemental_file" => assigns(:supplemental_file).id })
+          expect(JSON.parse(response.body)).to eq ({ "id" => assigns(:supplemental_file).id })
 
           expect(object.supplemental_files.first.id).to eq 1
           expect(object.supplemental_files.first.label).to eq 'captions.srt'
@@ -372,7 +373,7 @@
         request.headers['Content-Type'] = 'application/json'
       end
       context "with valid metadata params" do
-        let(:valid_update_attributes) { { label: 'new label', type: 'transcript', machine_generated: 1 }.to_json }
+        let(:valid_update_attributes) { { label: 'new label', type: 'transcript', machine_generated: true }.to_json }
         it "updates the SupplementalFile metadata for #{object_class}" do
           expect {
             put :update, params: { class_id => object.id, id: supplemental_file.id, metadata: valid_update_attributes, format: :json }, session: valid_session
@@ -380,7 +381,7 @@
            .and change { object.reload.supplemental_files.first.tags }.from([]).to(['transcript', 'machine_generated'])
 
           expect(response).to have_http_status(:ok)
-          expect(response.body).to eq({ "supplemental_file": supplemental_file.id }.to_json)
+          expect(response.body).to eq({ "id": supplemental_file.id }.to_json)
         end
       end
 
@@ -480,12 +481,13 @@
         before { ApiToken.create token: 'secret_token', username: 'archivist1@example.com', email: 'archivist1@example.com' }
         it "updates the SupplementalFile attached file for #{object_class}" do
           request.headers['Avalon-Api-Key'] = 'secret_token'
+          request.headers['Accept'] = 'application/json'
           expect {
             put :update, params: { class_id => object.id, id: supplemental_file.id, file: file_update, format: :html }, session: valid_session
           }.to change { object.reload.supplemental_files.first.file }
 
           expect(response).to have_http_status(:ok)
-          expect(response.body).to eq({ "supplemental_file": supplemental_file.id }.to_json)
+          expect(response.body).to eq({ "id": supplemental_file.id }.to_json)
         end
       end
 
@@ -493,6 +495,7 @@
         before { ApiToken.create token: 'secret_token', username: 'archivist1@example.com', email: 'archivist1@example.com' }
         it "returns a 400" do
           request.headers['Avalon-Api-Key'] = 'secret_token'
+          request.headers['Accept'] = 'application/json'
           put :update, params: { class_id=> object.id, id: supplemental_file.id, metadata: valid_update_attributes, format: :html }, session: valid_session
           expect(response).to have_http_status(400)
         end
@@ -515,10 +518,11 @@
       end
 
       context "API updating caption with invalid file type" do
-        let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_caption_tag, skip_file_type: true) }
+        let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_caption_tag) }
         before { ApiToken.create token: 'secret_token', username: 'archivist1@example.com', email: 'archivist1@example.com' }
         it "returns a 500" do
           request.headers['Avalon-Api-Key'] = 'secret_token'
+          request.headers['Accept'] = 'application/json'
           put :update, params: { class_id=> object.id, id: supplemental_file.id, file: uploaded_file, format: :html }, session: valid_session
           expect(response).to have_http_status(500)
         end

From 27c6db0e9c151f9ccb6ab019cc5ab17e0876e9f7 Mon Sep 17 00:00:00 2001
From: Mason Ballengee <masaball@iu.edu>
Date: Tue, 11 Jun 2024 16:07:16 -0400
Subject: [PATCH 4/4] Fix tests

---
 spec/models/supplemental_file_spec.rb | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/spec/models/supplemental_file_spec.rb b/spec/models/supplemental_file_spec.rb
index 5169ecc2ef..fd50cc22d2 100644
--- a/spec/models/supplemental_file_spec.rb
+++ b/spec/models/supplemental_file_spec.rb
@@ -44,12 +44,6 @@
           expect(subject.errors[:file_type]).not_to be_empty
         end
       end
-      context "caption metadata only with skip_file_type" do
-        let(:subject) { FactoryBot.build(:supplemental_file, :with_caption_tag, skip_file_type: true) }
-        it 'should skip validation' do
-          expect(subject.valid?).to be_truthy
-        end
-      end
     end
 
     describe 'scopes' do
@@ -83,14 +77,6 @@
           solr_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first
           expect(solr_doc["transcript_tsim"]).to eq ["00:00:03.500 --> 00:00:05.000 Example captions"]
         end
-
-        context 'skip index' do
-          let(:transcript) { FactoryBot.build(:supplemental_file, :with_transcript_file, :with_transcript_tag, skip_index: true) }
-          it 'does not trigger callback' do
-            expect(transcript).to_not receive(:index_file)
-            transcript.save
-          end
-        end
       end
 
       context 'on update' do
@@ -192,7 +178,7 @@
     context 'machine generated file' do
       let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_caption_file, tags: ['machine_generated'], label: 'Test') }
       it 'includes machine_generated in json' do
-        expect(subject[:machine_generated]).to eq '1'
+        expect(subject[:machine_generated]).to eq true
       end
     end
 
@@ -205,7 +191,7 @@
       context 'as transcript' do
         let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_caption_file, tags: ['caption', 'transcript'], label: 'Test') }
         it 'includes treat_as_transcript in JSON' do
-          expect(subject[:treat_as_transcript]).to eq '1'
+          expect(subject[:treat_as_transcript]).to eq true
         end
       end
     end