Pdffiller temp file fix, add unit test, fix attachment_ids (#19143)

* fix temp file issue * add unit test generate and create_tempfile methods * fix uuid naming and attachment id * fix unit test * 7959f1 unit test fix * remove comments * remove gsub as it wasn't performing anything * refactor attachments code to remove reduna * Moved this fix behind a feature flag * Fixes for linter * Fixes for linter * Workarounds for linter * Fixes for linter * Fixes for linter * Fixes for linter * Fixes for linter * Fixes for linter * Fix for 'undefined local variable' error * Fixes for linter * Fix for 'undefined local variable or method' * Fix for temp file missing uuid in name * No more def statements inside Flipper checks * In tests, replaced toggling flippers with mocks --------- Co-authored-by: Steve Long <[email protected]>
department-of-veterans-affairs · Nov 12, 2024 · afa2fd3 · afa2fd3
1 parent b10c7f8
commit afa2fd3
Show file tree

Hide file tree

Showing 8 changed files with 405 additions and 120 deletions.
diff --git a/modules/ivc_champva/app/controllers/ivc_champva/v1/uploads_controller.rb b/modules/ivc_champva/app/controllers/ivc_champva/v1/uploads_controller.rb
@@ -120,19 +120,37 @@ def supporting_document_ids(parsed_form_data)
           parsed_form_data['supporting_docs']&.pluck('claim_id')&.compact.presence || []
       end
 
+      # rubocop:disable Metrics/MethodLength
+      # rubocop:disable Style/IdenticalConditionalBranches
       def get_file_paths_and_metadata(parsed_form_data)
-        attachment_ids, form = get_attachment_ids_and_form(parsed_form_data)
-        filler = IvcChampva::PdfFiller.new(form_number: form.form_id, form:)
-        file_path = if @current_user
-                      filler.generate(@current_user.loa[:current])
-                    else
-                      filler.generate
-                    end
-        metadata = IvcChampva::MetadataValidator.validate(form.metadata)
-        file_paths = form.handle_attachments(file_path)
-
-        [file_paths, metadata.merge({ 'attachment_ids' => attachment_ids })]
+        if Flipper.enabled?(:champva_unique_temp_file_names, @user)
+          attachment_ids, form = get_attachment_ids_and_form(parsed_form_data)
+          filler = IvcChampva::PdfFiller.new(form_number: form.form_id, form:, uuid: form.uuid)
+          file_path = if @current_user
+                        filler.generate(@current_user.loa[:current])
+                      else
+                        filler.generate
+                      end
+          metadata = IvcChampva::MetadataValidator.validate(form.metadata)
+          file_paths = form.handle_attachments(file_path)
+
+          [file_paths, metadata.merge({ 'attachment_ids' => attachment_ids })]
+        else
+          attachment_ids, form = get_attachment_ids_and_form(parsed_form_data)
+          filler = IvcChampva::PdfFiller.new(form_number: form.form_id, form:)
+          file_path = if @current_user
+                        filler.generate(@current_user.loa[:current])
+                      else
+                        filler.generate
+                      end
+          metadata = IvcChampva::MetadataValidator.validate(form.metadata)
+          file_paths = form.handle_attachments(file_path)
+
+          [file_paths, metadata.merge({ 'attachment_ids' => attachment_ids })]
+        end
       end
+      # rubocop:enable Metrics/MethodLength
+      # rubocop:enable Style/IdenticalConditionalBranches
 
       def get_form_id
         form_number = params[:form_number]

diff --git a/modules/ivc_champva/app/services/ivc_champva/attachments.rb b/modules/ivc_champva/app/services/ivc_champva/attachments.rb
@@ -4,30 +4,49 @@ module IvcChampva
   module Attachments
     attr_accessor :form_id, :uuid, :data
 
+    # rubocop:disable Metrics/MethodLength
     def handle_attachments(file_path)
-      file_path_uuid = file_path.gsub("#{form_id}-tmp", "#{uuid}_#{form_id}-tmp")
-      File.rename(file_path, file_path_uuid)
-      attachments = get_attachments
-      file_paths = [file_path_uuid]
-
-      if attachments.count.positive?
-        supporting_doc_index = 0
-        attachments.each do |attachment|
-          new_file_name =
-            if attachment.include?('_additional_')
-              "#{uuid}_#{File.basename(attachment, '.*')}.pdf"
-            else
-              "#{uuid}_#{form_id}_supporting_doc-#{supporting_doc_index}.pdf".tap { supporting_doc_index += 1 }
-            end
+      if Flipper.enabled?(:champva_unique_temp_file_names, @user)
+        file_paths = [file_path]
+        attachments = get_attachments
 
+        attachments.each_with_index do |attachment, index|
+          new_file_name = if attachment.include?('_additional_')
+                            "#{uuid}_#{File.basename(attachment,
+                                                     '.*')}.pdf"
+                          else
+                            "#{uuid}_#{form_id}_supporting_doc-#{index}.pdf"
+                          end
           new_file_path = File.join(File.dirname(attachment), new_file_name)
           File.rename(attachment, new_file_path)
           file_paths << new_file_path
         end
+      else
+        file_path_uuid = file_path.gsub("#{form_id}-tmp", "#{uuid}_#{form_id}-tmp")
+        File.rename(file_path, file_path_uuid)
+        attachments = get_attachments
+        file_paths = [file_path_uuid]
+
+        if attachments.count.positive?
+          supporting_doc_index = 0
+          attachments.each do |attachment|
+            new_file_name =
+              if attachment.include?('_additional_')
+                "#{uuid}_#{File.basename(attachment, '.*')}.pdf"
+              else
+                "#{uuid}_#{form_id}_supporting_doc-#{supporting_doc_index}.pdf".tap { supporting_doc_index += 1 }
+              end
+
+            new_file_path = File.join(File.dirname(attachment), new_file_name)
+            File.rename(attachment, new_file_path)
+            file_paths << new_file_path
+          end
+        end
       end
 
       file_paths
     end
+    # rubocop:enable Metrics/MethodLength
 
     private
 

diff --git a/modules/ivc_champva/app/services/ivc_champva/pdf_filler.rb b/modules/ivc_champva/app/services/ivc_champva/pdf_filler.rb
@@ -6,22 +6,35 @@ module IvcChampva
   class PdfFiller
     attr_accessor :form, :form_number, :name
 
+    attr_accessor :uuid if Flipper.enabled?(:champva_unique_temp_file_names, @user)
+
     TEMPLATE_BASE = Rails.root.join('modules', 'ivc_champva', 'templates')
 
-    def initialize(form_number:, form:, name: nil)
+    def initialize(form_number:, form:, name: nil, uuid: nil)
       raise 'form_number is required' if form_number.blank?
       raise 'form needs a data attribute' unless form&.data
 
       @form = form
       @form_number = form_number
       @name = name || form_number
+      @uuid = uuid if Flipper.enabled?(:champva_unique_temp_file_names, @user)
     end
 
+    # rubocop:disable Metrics/MethodLength
     def generate(current_loa = nil)
-      template_form_path = "#{TEMPLATE_BASE}/#{form_number}.pdf"
-      generated_form_path = "tmp/#{name}-tmp.pdf"
-      stamped_template_path = "tmp/#{name}-stamped.pdf"
-      FileUtils.copy(template_form_path, stamped_template_path)
+      if Flipper.enabled?(:champva_unique_temp_file_names, @user)
+        generated_form_path = Rails.root.join("tmp/#{@uuid}_#{name}-tmp.pdf").to_s
+        stamped_template_path = Rails.root.join("tmp/#{@uuid}_#{name}-stamped.pdf").to_s
+
+        tempfile = create_tempfile
+        FileUtils.touch(tempfile)
+        FileUtils.copy_file(tempfile.path, stamped_template_path)
+      else
+        template_form_path = "#{TEMPLATE_BASE}/#{form_number}.pdf"
+        generated_form_path = "tmp/#{name}-tmp.pdf"
+        stamped_template_path = "tmp/#{name}-stamped.pdf"
+        FileUtils.copy(template_form_path, stamped_template_path)
+      end
 
       if File.exist? stamped_template_path
         begin
@@ -36,6 +49,19 @@ def generate(current_loa = nil)
         raise "stamped template file does not exist: #{stamped_template_path}"
       end
     end
+    # rubocop:enable Metrics/MethodLength
+
+    def create_tempfile
+      if Flipper.enabled?(:champva_unique_temp_file_names, @user)
+        # Tempfile workaround inspired by this:
+        #   https://github.com/actions/runner-images/issues/4443#issuecomment-965391736
+        template_form_path = "#{TEMPLATE_BASE}/#{form_number}.pdf"
+        Tempfile.new(['', '.pdf'], Rails.root.join('tmp')).tap do |tmpfile|
+          IO.copy_stream(template_form_path, tmpfile)
+          tmpfile.close
+        end
+      end
+    end
 
     private
 

diff --git a/modules/ivc_champva/spec/models/vha_10_7959c_spec.rb b/modules/ivc_champva/spec/models/vha_10_7959c_spec.rb
@@ -30,7 +30,6 @@
     }
   end
   let(:vha107959c) { described_class.new(data) }
-  let(:file_path) { 'vha_10_7959c-tmp.pdf' }
   let(:uuid) { SecureRandom.uuid }
   let(:instance) { IvcChampva::VHA107959c.new(data) }
 
@@ -72,10 +71,32 @@
   end
 
   describe '#handle_attachments' do
-    it 'renames the file and returns the new file path' do
-      allow(File).to receive(:rename)
-      result = instance.handle_attachments(file_path)
-      expect(result).to eq(["#{uuid}_vha_10_7959c-tmp.pdf"])
+    context 'Feature champva_unique_temp_file_names=true' do
+      before do
+        allow(Flipper).to receive(:enabled?).with(:champva_unique_temp_file_names, nil).and_return(true)
+      end
+
+      let(:file_path) { "#{uuid}_vha_10_7959c-tmp.pdf" }
+
+      it 'renames the file and returns the new file path' do
+        allow(File).to receive(:rename)
+        result = instance.handle_attachments(file_path)
+        expect(result).to eq(["#{uuid}_vha_10_7959c-tmp.pdf"])
+      end
+    end
+
+    context 'Feature champva_unique_temp_file_names=false' do
+      before do
+        allow(Flipper).to receive(:enabled?).with(:champva_unique_temp_file_names, nil).and_return(false)
+      end
+
+      let(:file_path) { 'vha_10_7959c-tmp.pdf' }
+
+      it 'renames the file and returns the new file path' do
+        allow(File).to receive(:rename)
+        result = instance.handle_attachments(file_path)
+        expect(result).to eq(["#{uuid}_vha_10_7959c-tmp.pdf"])
+      end
     end
   end
 

diff --git a/modules/ivc_champva/spec/models/vha_10_7959f_1_spec.rb b/modules/ivc_champva/spec/models/vha_10_7959f_1_spec.rb
@@ -26,7 +26,6 @@
     }
   end
   let(:vha107959f1) { described_class.new(data) }
-  let(:file_path) { 'vha_10_7959f_1-tmp.pdf' }
   let(:uuid) { SecureRandom.uuid }
   let(:instance) { IvcChampva::VHA107959f1.new(data) }
 
@@ -61,10 +60,32 @@
   end
 
   describe '#handle_attachments' do
-    it 'renames the file and returns the new file path' do
-      allow(File).to receive(:rename)
-      result = instance.handle_attachments(file_path)
-      expect(result).to eq(["#{uuid}_vha_10_7959f_1-tmp.pdf"])
+    context 'Feature champva_unique_temp_file_names=true' do
+      before do
+        allow(Flipper).to receive(:enabled?).with(:champva_unique_temp_file_names, nil).and_return(true)
+      end
+
+      let(:file_path) { "#{uuid}_vha_10_7959f_1-tmp.pdf" }
+
+      it 'renames the file and returns the new file path' do
+        allow(File).to receive(:rename)
+        result = instance.handle_attachments(file_path)
+        expect(result).to eq(["#{uuid}_vha_10_7959f_1-tmp.pdf"])
+      end
+    end
+
+    context 'Feature champva_unique_temp_file_names=false' do
+      before do
+        allow(Flipper).to receive(:enabled?).with(:champva_unique_temp_file_names, nil).and_return(false)
+      end
+
+      let(:file_path) { 'vha_10_7959f_1-tmp.pdf' }
+
+      it 'renames the file and returns the new file path' do
+        allow(File).to receive(:rename)
+        result = instance.handle_attachments(file_path)
+        expect(result).to eq(["#{uuid}_vha_10_7959f_1-tmp.pdf"])
+      end
     end
   end
 

diff --git a/modules/ivc_champva/spec/models/vha_10_7959f_2_spec.rb b/modules/ivc_champva/spec/models/vha_10_7959f_2_spec.rb
@@ -28,7 +28,6 @@
     }
   end
   let(:vha107959f2) { described_class.new(data) }
-  let(:file_path) { 'vha_10_7959f_2-tmp.pdf' }
   let(:uuid) { SecureRandom.uuid }
   let(:instance) { IvcChampva::VHA107959f2.new(data) }
 
@@ -63,10 +62,32 @@
   end
 
   describe '#handle_attachments' do
-    it 'renames the file and returns the new file path' do
-      allow(File).to receive(:rename)
-      result = instance.handle_attachments(file_path)
-      expect(result).to eq(["#{uuid}_vha_10_7959f_2-tmp.pdf"])
+    context 'Feature champva_unique_temp_file_names=true' do
+      before do
+        allow(Flipper).to receive(:enabled?).with(:champva_unique_temp_file_names, nil).and_return(true)
+      end
+
+      let(:file_path) { "#{uuid}_vha_10_7959f_2-tmp.pdf" }
+
+      it 'renames the file and returns the new file path' do
+        allow(File).to receive(:rename)
+        result = instance.handle_attachments(file_path)
+        expect(result).to eq(["#{uuid}_vha_10_7959f_2-tmp.pdf"])
+      end
+    end
+
+    context 'Feature champva_unique_temp_file_names=false' do
+      before do
+        allow(Flipper).to receive(:enabled?).with(:champva_unique_temp_file_names, nil).and_return(false)
+      end
+
+      let(:file_path) { 'vha_10_7959f_2-tmp.pdf' }
+
+      it 'renames the file and returns the new file path' do
+        allow(File).to receive(:rename)
+        result = instance.handle_attachments(file_path)
+        expect(result).to eq(["#{uuid}_vha_10_7959f_2-tmp.pdf"])
+      end
     end
   end