Resolves APPEALS-46700 (#1647) (#1648)

* add missing binwriter from record_fetcher

* update metric wording to include API

app/controllers/api/v2/records_controller.rb

@@ -2,9 +2,16 @@ class Api::V2::RecordsController < Api::V2::ApplicationController
   before_action :validate_access
 
   def show
-    #Check before fetch if the file is saved in S3
+    result = if FeatureToggle.enabled?(:vbms_to_reader_on_s3_miss, user: current_user)
+               # return s3 cache miss directly from VBMS
+               # instead of saving to s3 first
+               record.api_fetch!
+             else
+               record.fetch!
+             end
+
     document_source_to_headers
-    result = record.fetch!
+
     return document_failed if record.failed?
 
     # Only cache if we're not returning an error
@@ -47,7 +54,6 @@ def validate_access
   end
 
   def document_source_to_headers
-    from_s3 ||= S3Service.exists? record.s3_filename
-    headers["X-Document-Source"] = !!from_s3 ? "S3" : "VBMS"
+    headers["X-Document-Source"] = @record.sourced || ""
   end
 end

app/models/record.rb

@@ -5,6 +5,8 @@ class Record < ApplicationRecord
 
   belongs_to :manifest_source
 
+  attr_accessor :sourced
+
   validates :manifest_source, :version_id, :series_id, presence: true
 
   enum status: {
@@ -38,6 +40,10 @@ def fetch!
     fetcher.process
   end
 
+  def api_fetch!
+    api_fetcher.process
+  end
+
   # TODO: remove this method when implmenentation of VVA/VBMS service is changed towards v2 API
   def ssn
     file_number
@@ -90,6 +96,10 @@ def fetcher
     @fetcher ||= RecordFetcher.new(record: self)
   end
 
+  def api_fetcher
+    @api_fetcher ||= RecordApiFetcher.new(record: self)
+  end
+
   # Since Windows has the maximum length for a path, we crop type_name if the filename is longer than set maximum (issue #371)
   def cropped_type_name
     over_limit = (Zaru.sanitize! "#{type_description}-#{filename_date}-#{filename_doc_id}.#{preferred_extension}").size - MAXIMUM_FILENAME_LENGTH

app/services/record_api_fetcher.rb

@@ -0,0 +1,26 @@
+class RecordApiFetcher < RecordFetcherBase
+
+  EXCEPTIONS = [VBMS::ClientError, VVA::ClientError].freeze
+  SECONDS_TO_AUTO_UNLOCK = 90
+
+  def process
+    content_from_s3 || content_from_va_service
+  rescue *EXCEPTIONS => error
+    Rails.logger.error("Caught #{error}")
+    nil
+  end
+
+  private
+
+  def content_from_va_service
+    record.update(sourced: "VBMS")
+    content = MetricsService.record("RecordFetcher fetch content from VA manifest  - API source name: #{record.manifest_source.name} for file_number #{record.file_number}",
+                                    service: record.manifest_source.name.downcase.to_sym,
+                                    name: "v2_fetch_document_file") do
+      record.service.v2_fetch_document_file(record)
+    end
+
+    image_converter(content)
+  end
+
+end

app/services/record_fetcher.rb

@@ -1,7 +1,4 @@
-class RecordFetcher
-  include ActiveModel::Model
-
-  attr_accessor :record
+class RecordFetcher < RecordFetcherBase
 
   EXCEPTIONS = [VBMS::ClientError, VVA::ClientError].freeze
   SECONDS_TO_AUTO_UNLOCK = 90
@@ -27,38 +24,22 @@ def process
   private
 
   def content_from_va_service
+    record.update(sourced: "VBMS")
     content = MetricsService.record("RecordFetcher fetch content from VA manifest source name: #{record.manifest_source.name} for file_number #{record.file_number}",
                                     service: record.manifest_source.name.downcase.to_sym,
                                     name: "v2_fetch_document_file") do
       record.service.v2_fetch_document_file(record)
     end
 
-    content = MetricsService.record("ImageConverterService for #{record.s3_filename}",
-                                    service: :image_converter,
-                                    name: "image_converter_service") do
-      ImageConverterService.new(image: content, record: record).process
-    end
+    content = image_converter(content)
 
     if BaseController.dependencies_faked_for_CEAPI?
       save_path = Rails.root.join('tmp', 'temp_pdf.pdf')
       IO.binwrite(save_path, content)
     else
-      MetricsService.record("RecordFetcher S3 store content for #{record.s3_filename}",
-                            service: :s3,
-                            name: "content_from_va_service") do
-        S3Service.store_file(record.s3_filename, content)
-      end
+      content_store_to_s3(content)
     end
     content
   end
 
-  def content_from_s3
-    return false if BaseController.dependencies_faked_for_CEAPI?
-
-    @content_from_s3 ||= MetricsService.record("RecordFetcher fetch content from S3 filename: #{record.s3_filename} for file_number #{record.file_number}",
-                                               service: :s3,
-                                               name: "content_from_s3") do
-      S3Service.fetch_content(record.s3_filename)
-    end
-  end
 end

app/services/record_fetcher_base.rb

@@ -0,0 +1,40 @@
+class RecordFetcherBase
+  include ActiveModel::Model
+
+  attr_accessor :record
+
+  protected
+
+  def content_from_s3
+    record.update(sourced: "S3")
+    return false if BaseController.dependencies_faked_for_CEAPI?
+
+    @content_from_s3 ||= MetricsService.record("RecordFetcher fetch content from S3 filename: #{record.s3_filename} for file_number #{record.file_number}",
+                                               service: :s3,
+                                               name: "content_from_s3") do
+      S3Service.fetch_content(record.s3_filename)
+    end
+  end
+
+  def image_converter(content)
+    MetricsService.record("ImageConverterService for #{record.s3_filename}",
+                          service: :image_converter,
+                          name: "image_converter_service") do
+      ImageConverterService.new(image: content, record: record).process
+    end
+  end
+
+  def content_store_to_s3(content)
+    if BaseController.dependencies_faked_for_CEAPI?
+      save_path = Rails.root.join('tmp', 'temp_pdf.pdf')
+      IO.binwrite(save_path, content)
+    else
+      MetricsService.record("RecordFetcher S3 store content for #{record.s3_filename}",
+                            service: :s3,
+                            name: "content_from_va_service") do
+        S3Service.store_file(record.s3_filename, content)
+      end
+    end
+  end
+
+end

spec/requests/api/v2/records_spec.rb

@@ -2,6 +2,7 @@
   include ActiveJob::TestHelper
 
   context "Record by document ID" do
+    FeatureToggle.disable!(:vbms_to_reader_on_s3_miss)
     let!(:current_user) do
       User.authenticate!(roles: ["Reader"])
     end
@@ -122,23 +123,20 @@
 
     context "document_source_to_headers" do
       let!(:files_download) { FilesDownload.create(user: current_user, manifest: manifest2) }
-
 
       context "when document is not in S3" do
-        it "hedaers include X-Document-Source is VBMS" do
+        it "headers include X-Document-Source is VBMS" do
           allow(S3Service).to receive(:exists?).and_return(false)
           expect(S3Service).to receive(:fetch_content).and_return nil
-          expect(VBMSService).to receive(:v2_fetch_document_file)
+          allow(Fakes::DocumentService).to receive(:v2_fetch_document_file)
           expect(S3Service).to receive(:store_file)
           get "/api/v2/records/#{version_id}"
           expect(response.code).to eq("200")
           expect(response.headers["X-Document-Source"]).to eq("VBMS")
         end
-
       end
 
       context "when document is in S3" do
-        #Caseflow::Fake::S3Service.exists? returns true by default
         it "headers include X-Document-Source is S3" do
           allow(S3Service).to receive(:fetch_content).and_return("much document")
           expect(VBMSService).not_to receive(:v2_fetch_document_file)
@@ -149,6 +147,36 @@
           expect(response.headers["X-Document-Source"]).to eq("S3")
         end
       end
+
+      context "when document is not in S3 and enabled vbms to reader on s3 miss" do
+        before { FeatureToggle.enable!(:vbms_to_reader_on_s3_miss, users: [current_user.css_id]) }
+        after { FeatureToggle.disable!(:vbms_to_reader_on_s3_miss, users: [current_user.css_id]) }
+
+        it "headers include X-Document-Source is VBMS for api" do
+          allow(S3Service).to receive(:exists?).and_return(false)
+          expect(S3Service).to receive(:fetch_content).and_return nil
+          allow(Fakes::DocumentService).to receive(:v2_fetch_document_file)
+          expect(S3Service).not_to receive(:store_file)
+          get "/api/v2/records/#{version_id}"
+          expect(response.code).to eq("200")
+          expect(response.headers["X-Document-Source"]).to eq("VBMS")
+        end
+      end
+
+      context "when document is in S3 and enabled vbms to reader on s3 miss" do
+        before { FeatureToggle.enable!(:vbms_to_reader_on_s3_miss, users: [current_user.css_id]) }
+        after { FeatureToggle.disable!(:vbms_to_reader_on_s3_miss, users: [current_user.css_id]) }
+
+        it "headers include X-Document-Source is S3 for api" do
+          allow(S3Service).to receive(:fetch_content).and_return("much document")
+          expect(VBMSService).not_to receive(:v2_fetch_document_file)
+          expect(S3Service).not_to receive(:store_file)
+          expect(S3Service).to receive(:fetch_content)
+          get "/api/v2/records/#{version_id}"
+          expect(response.code).to eq("200")
+          expect(response.headers["X-Document-Source"]).to eq("S3")
+        end
+      end
     end
   end
 end