[SIMPLE-FORMS] fix: update s3 client pre-signed url generation logic …

…and add test coverage (#19176)

* submission archive specs in stable state

* further refinement of rspec file

* final refinement of submission archive spec

* s3 client logic and tests work

* rename thing

* add more test coverage and stub out more specs

* silence rubocop offense

modules/simple_forms_api/app/services/simple_forms_api/form_remediation/file_utilities.rb

@@ -70,7 +70,7 @@ def clean_s3_path!(*paths)
       def build_path(path_type, base_dir, *path_segments, ext: '.pdf')
         file_ext = path_type == :file ? ext : ''
         path = Pathname.new(base_dir.to_s).join(*path_segments)
-        path = path.to_s + file_ext unless file_ext.empty?
+        path = path.to_s + file_ext if file_ext.present?
         path.to_s
       end
 

modules/simple_forms_api/app/services/simple_forms_api/form_remediation/s3_client.rb

@@ -19,9 +19,9 @@ def initialize(config:, type: :remediation, **options)
         @upload_type = type
         @config = config
         @id = options[:id]
+        @parent_dir = config.parent_dir
 
         assign_defaults(options)
-        initialize_archive
         log_initialization
       rescue => e
         config.handle_error("#{self.class.name} initialization failed", e)
@@ -31,7 +31,7 @@ def upload
         config.log_info("Uploading #{upload_type}: #{id} to S3 bucket")
 
         upload_to_s3(archive_path)
-        update_manifest if config.include_manifest
+        update_manifest if manifest_required?
         cleanup!(archive_path)
 
         return generate_presigned_url if presign_required?
@@ -51,10 +51,6 @@ def assign_defaults(options)
         @temp_directory_path = File.dirname(archive_path)
       end
 
-      def initialize_archive
-        @parent_dir = config.parent_dir
-      end
-
       def log_initialization
         config.log_info("Initialized S3Client for #{upload_type} with ID: #{id}")
       end
@@ -117,15 +113,20 @@ def generate_presigned_url(type: upload_type)
       end
 
       def s3_upload_file_path(type)
+        extension = File.extname(archive_path)
         ext = type == :submission ? '.pdf' : '.zip'
-        build_path(:file, s3_directory_path, archive_path, ext:)
+        build_path(:file, s3_directory_path, archive_path, ext: extension ? nil : ext)
       end
 
       def presign_required?
         return true if upload_type == :submission
 
         config.presign_s3_url
       end
+
+      def manifest_required?
+        config.include_manifest && upload_type == :remediation
+      end
     end
   end
 end

modules/simple_forms_api/spec/services/form_remediation/s3_client_spec.rb

@@ -5,13 +5,21 @@
 require 'simple_forms_api/form_remediation/configuration/vff_config'
 
 RSpec.describe SimpleFormsApi::FormRemediation::S3Client do
+  include SimpleFormsApi::FormRemediation::FileUtilities
+
+  # Initial form data setup
   let(:form_type) { '20-10207' }
   let(:fixtures_path) { 'modules/simple_forms_api/spec/fixtures' }
-  let(:form_data) { Rails.root.join(fixtures_path, 'form_json', 'vba_20_10207_with_supporting_documents.json').read }
-  let(:file_path) { Rails.root.join(fixtures_path, 'pdfs', 'vba_20_10207-completed.pdf') }
-  let(:attachments) { Array.new(5) { fixture_file_upload('doctors-note.pdf', 'application/pdf').path } }
+  let(:form_data) do
+    Rails.root.join(fixtures_path, 'form_json', 'vba_20_10207_with_supporting_documents.json').read
+  end
+
+  # Params setup
+  let(:attachments) { Array.new(5) { Rails.root.join(fixtures_path, 'doctors-note.pdf') } }
   let(:submission) { create(:form_submission, :pending, form_type:, form_data:) }
   let(:benefits_intake_uuid) { submission.benefits_intake_uuid }
+  let(:config) { SimpleFormsApi::FormRemediation::Configuration::VffConfig.new }
+  let(:file_path) { Rails.root.join(fixtures_path, 'pdfs', 'vba_20_10207-completed.pdf') }
   let(:metadata) do
     {
       'veteranFirstName' => 'John',
@@ -23,14 +31,25 @@
       'businessLine' => 'CMP'
     }
   end
-  let(:submission_archive_instance) { instance_double(SimpleFormsApi::FormRemediation::SubmissionArchive) }
-  let(:temp_file_path) { Rails.root.join('tmp', 'random-letters-n-numbers-archive').to_s }
-  let(:submission_file_path) do
-    [Time.zone.today.strftime('%-m.%d.%y'), 'form', form_type, 'vagov', benefits_intake_uuid].join('_')
+  let(:default_args) { { id: benefits_intake_uuid, config:, type: } }
+  let(:hydrated_submission_args) { default_args.merge(submission:, file_path:, attachments:, metadata:) }
+
+  # Mock file paths
+  let(:submission_file_name) { unique_file_name(form_type, benefits_intake_uuid) }
+  let(:s3_archive_dir) { "#{type}/#{dated_directory_name(form_type)}" }
+  let(:s3_archive_path) { "#{s3_archive_dir}/#{submission_file_name}.#{type == :remediation ? 'zip' : 'pdf'}" }
+  let(:local_archive_dir) { Rails.root.join('tmp', 'random-letters-n-numbers-archive').to_s }
+  let(:local_archive_path) do
+    "#{local_archive_dir}/#{submission_file_name}.#{type == :remediation ? 'zip' : 'pdf'}"
   end
+
+  # Doubles and mocks
+  let(:submission_archive_double) { instance_double(SimpleFormsApi::FormRemediation::SubmissionArchive) }
   let(:uploader) { instance_double(SimpleFormsApi::FormRemediation::Uploader) }
-  let(:carrier_wave_file) { instance_double(CarrierWave::Storage::Fog::File) }
+  let(:carrier_wave_file) { instance_double(CarrierWave::SanitizedFile) }
+  let(:file_double) { instance_double(File, read: 'content') }
   let(:s3_file) { instance_double(Aws::S3::Object) }
+
   let(:manifest_entry) do
     [
       submission.created_at,
@@ -41,74 +60,140 @@
       metadata['veteranLastName']
     ]
   end
-  let(:config) { SimpleFormsApi::FormRemediation::Configuration::VffConfig.new }
 
   before do
     allow(FileUtils).to receive(:mkdir_p).and_return(true)
-    allow(File).to receive(:directory?).and_return(true)
+    allow(SecureRandom).to receive(:hex).and_return('random-letters-n-numbers')
+    allow(File).to receive(:directory?).with(local_archive_path).and_return(false)
+    allow(File).to receive(:directory?).with(a_string_including('-manifest')).and_return(false)
+    allow(File).to receive(:open).with(local_archive_path).and_yield(file_double)
+    allow(File).to receive(:open).with(a_string_including('-manifest')).and_yield(file_double)
+    allow(CarrierWave::SanitizedFile).to receive(:new).with(file_double).and_return(carrier_wave_file)
     allow(CSV).to receive(:open).and_return(true)
-    allow(SimpleFormsApi::FormRemediation::SubmissionArchive).to(receive(:new).and_return(submission_archive_instance))
-    allow(submission_archive_instance).to receive(:build!).and_return(
-      ["#{temp_file_path}/", manifest_entry]
-    )
+    allow(SimpleFormsApi::FormRemediation::SubmissionArchive).to(receive(:new).and_return(submission_archive_double))
+    allow(submission_archive_double).to receive(:build!).and_return([local_archive_path, manifest_entry])
     allow(SimpleFormsApi::FormRemediation::Uploader).to receive_messages(new: uploader)
-    allow(uploader).to receive_messages(get_s3_link: '/s3_url/stuff.pdf', get_s3_file: s3_file)
-    allow(uploader).to receive_messages(store!: carrier_wave_file)
+    allow(uploader).to receive(:get_s3_link).with(local_archive_path).and_return('/s3_url/stuff.pdf')
+    allow(uploader).to receive_messages(get_s3_file: s3_file, store!: carrier_wave_file)
     allow(Rails.logger).to receive(:info).and_call_original
     allow(Rails.logger).to receive(:error).and_call_original
   end
 
-  describe '#initialize' do
-    subject(:new) { described_class.new(id: benefits_intake_uuid, config:) }
+  %i[submission remediation].each do |archive_type|
+    describe "when archiving a #{archive_type}" do
+      let(:type) { archive_type }
 
-    context 'when initialized with a valid benefits_intake_uuid' do
-      it 'successfully completes initialization' do
-        expect { new }.not_to raise_exception
+      describe '.fetch_presigned_url' do
       end
-    end
-  end
 
-  describe '#upload' do
-    subject(:upload) { instance.upload }
-
-    let(:instance) { described_class.new(id: benefits_intake_uuid, config:) }
-
-    context 'when no errors occur' do
-      it 'logs notifications' do
-        upload
-        expect(Rails.logger).to have_received(:info).with(
-          { message: "Uploading remediation: #{benefits_intake_uuid} to S3 bucket" }
-        )
-        expect(Rails.logger).to have_received(:info).with(
-          { message: "Initialized S3Client for remediation with ID: #{benefits_intake_uuid}" }
-        )
-        expect(Rails.logger).to have_received(:info).with(
-          { message: "Cleaning up path: #{temp_file_path}/" }
-        )
-      end
+      describe '#initialize' do
+        subject(:new) { described_class.new(id: benefits_intake_uuid, type:, config:) }
 
-      it 'returns the s3 directory' do
-        expect(upload).to eq('/s3_url/stuff.pdf')
+        context 'when initialized with a valid benefits_intake_uuid' do
+          it 'successfully completes initialization' do
+            expect { new }.not_to raise_exception
+          end
+        end
       end
 
-      context 'when a different parent_dir is provided' do
-        let(:instance) { described_class.new(id: benefits_intake_uuid, config:) }
+      describe '#upload' do
+        shared_examples 's3 client acts as expected' do
+          context 'when no errors occur' do
+            before { upload }
+
+            it 'logs "uploading" notification' do
+              expect(Rails.logger).to have_received(:info).with(
+                { message: "Uploading #{type}: #{benefits_intake_uuid} to S3 bucket" }
+              )
+            end
+
+            describe '#log_initialization' do
+              it 'logs s3 client initialization notification' do
+                expect(Rails.logger).to have_received(:info).with(
+                  { message: "Initialized S3Client for #{type} with ID: #{benefits_intake_uuid}" }
+                )
+              end
+            end
+
+            describe '#build_archive!' do
+              it 'initializes the submission archive' do
+                expect(SimpleFormsApi::FormRemediation::SubmissionArchive).to have_received(:new)
+              end
+
+              it 'builds the submission archive' do
+                expect(submission_archive_double).to have_received(:build!)
+              end
+            end
 
-        it 'returns the s3 directory' do
-          expect(upload).to eq('/s3_url/stuff.pdf')
+            describe '#upload_to_s3' do
+              it 'opens the correct file path(s) and creates/stores sanitized file(s)' do
+                expect(File).to have_received(:open).with(local_archive_path).once
+                expect(File).to have_received(:open).with(a_string_including('-manifest')).once if type == :remediation
+
+                times = type == :remediation ? 2 : 1
+                expect(CarrierWave::SanitizedFile).to have_received(:new).with(file_double).exactly(times).times
+                expect(uploader).to have_received(:store!).with(carrier_wave_file).exactly(times).times
+              end
+            end
+
+            describe '#update_manifest'
+            describe '#build_s3_manifest_path'
+            describe '#download_manifest'
+            describe '#write_and_upload_manifest'
+
+            describe '#s3_uploader' do
+              it 'initializes uploader with correct directory' do
+                expect(SimpleFormsApi::FormRemediation::Uploader).to have_received(:new).with(
+                  config:, directory: s3_archive_dir
+                )
+              end
+            end
+
+            describe '#s3_directory_path'
+
+            describe '#generate_presigned_url' do
+              it 'requests the s3 link for the correct file' do
+                expect(uploader).to have_received(:get_s3_link).with(local_archive_path)
+              end
+            end
+
+            describe '#s3_upload_file_path'
+            describe '#presign_required?'
+            describe '#manifest_required?'
+
+            describe '#cleanup!' do
+              it 'logs clean up notification' do
+                expect(Rails.logger).to have_received(:info).with(
+                  { message: "Cleaning up path: #{local_archive_path}" }
+                )
+              end
+            end
+          end
+
+          context 'when an error occurs' do
+            before { allow(File).to receive(:directory?).and_raise('oops') }
+
+            it 'raises the error' do
+              expect { upload }.to raise_exception(RuntimeError, 'oops')
+            end
+          end
         end
-      end
-    end
 
-    context 'when an error occurs' do
-      before do
-        allow(File).to receive(:directory?).and_return(false)
-      end
+        context 'when initialized with a valid id' do
+          subject(:upload) { archive_instance.upload }
+
+          let(:archive_instance) { described_class.new(**default_args) }
 
-      let(:instance) { described_class.new(id: benefits_intake_uuid, config:) }
+          include_examples 's3 client acts as expected'
+        end
+
+        context 'when initialized with valid submission data' do
+          subject(:upload) { archive_instance.upload }
 
-      it 'raises the error' do
-        expect { upload }.to raise_exception(Errno::ENOENT)
+          let(:archive_instance) { described_class.new(**hydrated_submission_args) }
+
+          include_examples 's3 client acts as expected'
+        end
       end
     end
   end