Address feedback

app/helpers/blob_helper.rb

@@ -216,8 +216,8 @@ module BlobHelper
     link_to icon('file-code-o'), path, class: 'btn btn-sm has-tooltip', target: '_blank', rel: 'noopener noreferrer', title: 'Open raw', data: { container: 'body' }
   end
 
-  def blob_render_error_reason(viewer, error)
-    case error
+  def blob_render_error_reason(viewer)
+    case viewer.render_error
     when :too_large
       max_size =
         if viewer.absolutely_too_large?
@@ -231,10 +231,10 @@ module BlobHelper
     end
   end
 
-  def blob_render_error_options(viewer, error)
+  def blob_render_error_options(viewer)
     options = []
 
-    if error == :too_large && viewer.can_override_max_size?
+    if viewer.render_error == :too_large && viewer.can_override_max_size?
       options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil)))
     end
 

app/models/blob_viewer/base.rb

@@ -67,15 +67,18 @@ module BlobViewer
     # binary from `blob_raw_url` and does its own format validation and error
     # rendering, especially for potentially large binary formats.
     def render_error
-      if server_side_but_stored_in_lfs?
-        # Files stored in LFS can only be rendered using a client-side viewer,
-        # since we do not want to read large amounts of data into memory on the
-        # server side. Client-side viewers use JS and can fetch the file from
-        # `blob_raw_url` using AJAX.
-        :server_side_but_stored_in_lfs
-      elsif override_max_size ? absolutely_too_large? : too_large?
-        :too_large
-      end
+      return @render_error if defined?(@render_error)
+
+      @render_error =
+        if server_side_but_stored_in_lfs?
+          # Files stored in LFS can only be rendered using a client-side viewer,
+          # since we do not want to read large amounts of data into memory on the
+          # server side. Client-side viewers use JS and can fetch the file from
+          # `blob_raw_url` using AJAX.
+          :server_side_but_stored_in_lfs
+        elsif override_max_size ? absolutely_too_large? : too_large?
+          :too_large
+        end
     end
 
     def prepare!

app/views/projects/blob/_render_error.html.haml

 .file-content.code
   .nothing-here-block
-    The #{viewer.switcher_title} could not be displayed because #{blob_render_error_reason(viewer, error)}.
+    The #{viewer.switcher_title} could not be displayed because #{blob_render_error_reason(viewer)}.
 
     You can
-    = blob_render_error_options(viewer, error).to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe
+    = blob_render_error_options(viewer).to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe
     instead.

app/views/projects/blob/_viewer.html.haml

@@ -8,7 +8,7 @@
     .text-center.prepend-top-default.append-bottom-default
       = icon('spinner spin 2x', 'aria-hidden' => 'true', 'aria-label' => 'Loading content')
   - elsif render_error
-    = render 'projects/blob/render_error', viewer: viewer, error: render_error
+    = render 'projects/blob/render_error', viewer: viewer
   - else
     - viewer.prepare!
     = render viewer.partial_path, viewer: viewer

spec/features/projects/blobs/blob_show_spec.rb

@@ -149,7 +149,7 @@ feature 'File blob', :js, feature: true do
         wait_for_ajax
       end
 
-      it 'displays the blob' do
+      it 'displays an error' do
         aggregate_failures do
           # hides the simple viewer
           expect(page).to have_selector('.blob-viewer[data-type="simple"]', visible: false)
@@ -173,7 +173,7 @@ feature 'File blob', :js, feature: true do
           wait_for_ajax
         end
 
-        it 'displays the blob' do
+        it 'displays an error' do
           aggregate_failures do
             # hides the rich viewer
             expect(page).to have_selector('.blob-viewer[data-type="simple"]')

spec/helpers/blob_helper_spec.rb

@@ -108,7 +108,11 @@ describe BlobHelper do
   context 'viewer related' do
     include FakeBlobHelpers
 
-    let(:project) { build(:empty_project) }
+    let(:project) { build(:empty_project, lfs_enabled: true) }
+
+    before do
+      allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+    end
 
     let(:viewer_class) do
       Class.new(BlobViewer::Base) do
@@ -122,24 +126,13 @@ describe BlobHelper do
     let(:viewer) { viewer_class.new(blob) }
     let(:blob) { fake_blob }
 
-    before do
-      assign(:project, project)
-      assign(:id, File.join('master', blob.path))
-
-      controller.params[:controller] = 'projects/blob'
-      controller.params[:action] = 'show'
-      controller.params[:namespace_id] = project.namespace.to_param
-      controller.params[:project_id] = project.to_param
-      controller.params[:id] = File.join('master', blob.path)
-    end
-
     describe '#blob_render_error_reason' do
       context 'for error :too_large' do
         context 'when the blob size is larger than the absolute max size' do
           let(:blob) { fake_blob(size: 10.megabytes) }
 
           it 'returns an error message' do
-            expect(helper.blob_render_error_reason(viewer, :too_large)).to eq('it is larger than 5 MB')
+            expect(helper.blob_render_error_reason(viewer)).to eq('it is larger than 5 MB')
           end
         end
 
@@ -147,25 +140,38 @@ describe BlobHelper do
           let(:blob) { fake_blob(size: 2.megabytes) }
 
           it 'returns an error message' do
-            expect(helper.blob_render_error_reason(viewer, :too_large)).to eq('it is larger than 1 MB')
+            expect(helper.blob_render_error_reason(viewer)).to eq('it is larger than 1 MB')
           end
         end
       end
 
       context 'for error :server_side_but_stored_in_lfs' do
+        let(:blob) { fake_blob(lfs: true) }
+
         it 'returns an error message' do
-          expect(helper.blob_render_error_reason(viewer, :server_side_but_stored_in_lfs)).to eq('it is stored in LFS')
+          expect(helper.blob_render_error_reason(viewer)).to eq('it is stored in LFS')
         end
       end
     end
 
     describe '#blob_render_error_options' do
+      before do
+        assign(:project, project)
+        assign(:id, File.join('master', blob.path))
+
+        controller.params[:controller] = 'projects/blob'
+        controller.params[:action] = 'show'
+        controller.params[:namespace_id] = project.namespace.to_param
+        controller.params[:project_id] = project.to_param
+        controller.params[:id] = File.join('master', blob.path)
+      end
+
       context 'for error :too_large' do
         context 'when the max size can be overridden' do
           let(:blob) { fake_blob(size: 2.megabytes) }
 
           it 'includes a "load it anyway" link' do
-            expect(helper.blob_render_error_options(viewer, :too_large)).to include(/load it anyway/)
+            expect(helper.blob_render_error_options(viewer)).to include(/load it anyway/)
           end
         end
 
@@ -173,25 +179,25 @@ describe BlobHelper do
           let(:blob) { fake_blob(size: 10.megabytes) }
 
           it 'does not include a "load it anyway" link' do
-            expect(helper.blob_render_error_options(viewer, :too_large)).not_to include(/load it anyway/)
+            expect(helper.blob_render_error_options(viewer)).not_to include(/load it anyway/)
           end
         end
       end
 
       context 'when the viewer is rich' do
         context 'the blob is rendered as text' do
-          let(:blob) { fake_blob(path: 'file.md') }
+          let(:blob) { fake_blob(path: 'file.md', lfs: true) }
 
           it 'includes a "view the source" link' do
-            expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).to include(/view the source/)
+            expect(helper.blob_render_error_options(viewer)).to include(/view the source/)
           end
         end
 
         context 'the blob is not rendered as text' do
-          let(:blob) { fake_blob(path: 'file.pdf', binary: true) }
+          let(:blob) { fake_blob(path: 'file.pdf', binary: true, lfs: true) }
 
           it 'does not include a "view the source" link' do
-            expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).not_to include(/view the source/)
+            expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/)
           end
         end
       end
@@ -201,15 +207,15 @@ describe BlobHelper do
           viewer_class.type = :simple
         end
 
-        let(:blob) { fake_blob(path: 'file.md') }
+        let(:blob) { fake_blob(path: 'file.md', lfs: true) }
 
         it 'does not include a "view the source" link' do
-          expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).not_to include(/view the source/)
+          expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/)
         end
       end
 
       it 'includes a "download it" link' do
-        expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).to include(/download it/)
+        expect(helper.blob_render_error_options(viewer)).to include(/download it/)
       end
     end
   end

spec/models/blob_spec.rb

@@ -39,7 +39,7 @@ describe Blob do
     context 'if the blob is a valid LFS pointer' do
       context 'if the extension has a rich viewer' do
         context 'if the viewer is binary' do
-          it 'return true' do
+          it 'returns true' do
             blob = fake_blob(path: 'file.pdf', lfs: true)
 
             expect(blob.raw_binary?).to be_truthy
@@ -66,7 +66,7 @@ describe Blob do
 
     context 'if the blob is not an LFS pointer' do
       context 'if the blob is binary' do
-        it 'return true' do
+        it 'returns true' do
           blob = fake_blob(path: 'file.pdf', binary: true)
 
           expect(blob.raw_binary?).to be_truthy