From 87a3bd26fa1c6379801062fd65fea59e587baeee Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 26 Apr 2017 15:48:49 -0500 Subject: [PATCH] Address feedback --- app/helpers/blob_helper.rb | 8 +-- app/models/blob_viewer/base.rb | 21 ++++---- .../projects/blob/_render_error.html.haml | 4 +- app/views/projects/blob/_viewer.html.haml | 2 +- .../features/projects/blobs/blob_show_spec.rb | 4 +- spec/helpers/blob_helper_spec.rb | 54 ++++++++++--------- spec/models/blob_spec.rb | 4 +- 7 files changed, 53 insertions(+), 44 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 00cf0ac96c9..cc47654dc06 100644 --- a/app/helpers/blob_helper.rb +++ b/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 diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index ab89429d07d..3ca73565d81 100644 --- a/app/models/blob_viewer/base.rb +++ b/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! diff --git a/app/views/projects/blob/_render_error.html.haml b/app/views/projects/blob/_render_error.html.haml index 026b7c95163..9eef6cafd04 100644 --- a/app/views/projects/blob/_render_error.html.haml +++ b/app/views/projects/blob/_render_error.html.haml @@ -1,7 +1,7 @@ .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. diff --git a/app/views/projects/blob/_viewer.html.haml b/app/views/projects/blob/_viewer.html.haml index 1c45c4a68ce..5326bb3e0cf 100644 --- a/app/views/projects/blob/_viewer.html.haml +++ b/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 diff --git a/spec/features/projects/blobs/blob_show_spec.rb b/spec/features/projects/blobs/blob_show_spec.rb index aea9f66eec3..cc11cb7a55f 100644 --- a/spec/features/projects/blobs/blob_show_spec.rb +++ b/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"]') diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index 8a89cd71e98..379f62f73e1 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/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 diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb index 80dcfeeb3b5..7e8a1c8add7 100644 --- a/spec/models/blob_spec.rb +++ b/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