Commit 87a3bd26 authored by Douwe Maan's avatar Douwe Maan

Address feedback

parent c6b2a22f
...@@ -216,8 +216,8 @@ module BlobHelper ...@@ -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' } 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 end
def blob_render_error_reason(viewer, error) def blob_render_error_reason(viewer)
case error case viewer.render_error
when :too_large when :too_large
max_size = max_size =
if viewer.absolutely_too_large? if viewer.absolutely_too_large?
...@@ -231,10 +231,10 @@ module BlobHelper ...@@ -231,10 +231,10 @@ module BlobHelper
end end
end end
def blob_render_error_options(viewer, error) def blob_render_error_options(viewer)
options = [] 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))) options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil)))
end end
......
...@@ -67,15 +67,18 @@ module BlobViewer ...@@ -67,15 +67,18 @@ module BlobViewer
# binary from `blob_raw_url` and does its own format validation and error # binary from `blob_raw_url` and does its own format validation and error
# rendering, especially for potentially large binary formats. # rendering, especially for potentially large binary formats.
def render_error def render_error
if server_side_but_stored_in_lfs? return @render_error if defined?(@render_error)
# 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 @render_error =
# server side. Client-side viewers use JS and can fetch the file from if server_side_but_stored_in_lfs?
# `blob_raw_url` using AJAX. # Files stored in LFS can only be rendered using a client-side viewer,
:server_side_but_stored_in_lfs # since we do not want to read large amounts of data into memory on the
elsif override_max_size ? absolutely_too_large? : too_large? # server side. Client-side viewers use JS and can fetch the file from
:too_large # `blob_raw_url` using AJAX.
end :server_side_but_stored_in_lfs
elsif override_max_size ? absolutely_too_large? : too_large?
:too_large
end
end end
def prepare! def prepare!
......
.file-content.code .file-content.code
.nothing-here-block .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 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. instead.
...@@ -8,7 +8,7 @@ ...@@ -8,7 +8,7 @@
.text-center.prepend-top-default.append-bottom-default .text-center.prepend-top-default.append-bottom-default
= icon('spinner spin 2x', 'aria-hidden' => 'true', 'aria-label' => 'Loading content') = icon('spinner spin 2x', 'aria-hidden' => 'true', 'aria-label' => 'Loading content')
- elsif render_error - elsif render_error
= render 'projects/blob/render_error', viewer: viewer, error: render_error = render 'projects/blob/render_error', viewer: viewer
- else - else
- viewer.prepare! - viewer.prepare!
= render viewer.partial_path, viewer: viewer = render viewer.partial_path, viewer: viewer
...@@ -149,7 +149,7 @@ feature 'File blob', :js, feature: true do ...@@ -149,7 +149,7 @@ feature 'File blob', :js, feature: true do
wait_for_ajax wait_for_ajax
end end
it 'displays the blob' do it 'displays an error' do
aggregate_failures do aggregate_failures do
# hides the simple viewer # hides the simple viewer
expect(page).to have_selector('.blob-viewer[data-type="simple"]', visible: false) expect(page).to have_selector('.blob-viewer[data-type="simple"]', visible: false)
...@@ -173,7 +173,7 @@ feature 'File blob', :js, feature: true do ...@@ -173,7 +173,7 @@ feature 'File blob', :js, feature: true do
wait_for_ajax wait_for_ajax
end end
it 'displays the blob' do it 'displays an error' do
aggregate_failures do aggregate_failures do
# hides the rich viewer # hides the rich viewer
expect(page).to have_selector('.blob-viewer[data-type="simple"]') expect(page).to have_selector('.blob-viewer[data-type="simple"]')
......
...@@ -108,7 +108,11 @@ describe BlobHelper do ...@@ -108,7 +108,11 @@ describe BlobHelper do
context 'viewer related' do context 'viewer related' do
include FakeBlobHelpers 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 let(:viewer_class) do
Class.new(BlobViewer::Base) do Class.new(BlobViewer::Base) do
...@@ -122,24 +126,13 @@ describe BlobHelper do ...@@ -122,24 +126,13 @@ describe BlobHelper do
let(:viewer) { viewer_class.new(blob) } let(:viewer) { viewer_class.new(blob) }
let(:blob) { fake_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 describe '#blob_render_error_reason' do
context 'for error :too_large' do context 'for error :too_large' do
context 'when the blob size is larger than the absolute max size' do context 'when the blob size is larger than the absolute max size' do
let(:blob) { fake_blob(size: 10.megabytes) } let(:blob) { fake_blob(size: 10.megabytes) }
it 'returns an error message' do 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
end end
...@@ -147,25 +140,38 @@ describe BlobHelper do ...@@ -147,25 +140,38 @@ describe BlobHelper do
let(:blob) { fake_blob(size: 2.megabytes) } let(:blob) { fake_blob(size: 2.megabytes) }
it 'returns an error message' do 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 end
end end
context 'for error :server_side_but_stored_in_lfs' do context 'for error :server_side_but_stored_in_lfs' do
let(:blob) { fake_blob(lfs: true) }
it 'returns an error message' do 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 end
end end
describe '#blob_render_error_options' do 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 'for error :too_large' do
context 'when the max size can be overridden' do context 'when the max size can be overridden' do
let(:blob) { fake_blob(size: 2.megabytes) } let(:blob) { fake_blob(size: 2.megabytes) }
it 'includes a "load it anyway" link' do 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
end end
...@@ -173,25 +179,25 @@ describe BlobHelper do ...@@ -173,25 +179,25 @@ describe BlobHelper do
let(:blob) { fake_blob(size: 10.megabytes) } let(:blob) { fake_blob(size: 10.megabytes) }
it 'does not include a "load it anyway" link' do 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 end
end end
context 'when the viewer is rich' do context 'when the viewer is rich' do
context 'the blob is rendered as text' 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 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
end end
context 'the blob is not rendered as text' do 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 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 end
end end
...@@ -201,15 +207,15 @@ describe BlobHelper do ...@@ -201,15 +207,15 @@ describe BlobHelper do
viewer_class.type = :simple viewer_class.type = :simple
end 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 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 end
it 'includes a "download it" link' do 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 end
end end
......
...@@ -39,7 +39,7 @@ describe Blob do ...@@ -39,7 +39,7 @@ describe Blob do
context 'if the blob is a valid LFS pointer' do context 'if the blob is a valid LFS pointer' do
context 'if the extension has a rich viewer' do context 'if the extension has a rich viewer' do
context 'if the viewer is binary' do context 'if the viewer is binary' do
it 'return true' do it 'returns true' do
blob = fake_blob(path: 'file.pdf', lfs: true) blob = fake_blob(path: 'file.pdf', lfs: true)
expect(blob.raw_binary?).to be_truthy expect(blob.raw_binary?).to be_truthy
...@@ -66,7 +66,7 @@ describe Blob do ...@@ -66,7 +66,7 @@ describe Blob do
context 'if the blob is not an LFS pointer' do context 'if the blob is not an LFS pointer' do
context 'if the blob is binary' do context 'if the blob is binary' do
it 'return true' do it 'returns true' do
blob = fake_blob(path: 'file.pdf', binary: true) blob = fake_blob(path: 'file.pdf', binary: true)
expect(blob.raw_binary?).to be_truthy expect(blob.raw_binary?).to be_truthy
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment