diff options
author | Robert Speicher <rspeicher@gmail.com> | 2017-02-24 00:54:25 +0300 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2017-02-25 00:41:27 +0300 |
commit | a8c62dfe5c01ed08f170c1d41a39d5167b09631b (patch) | |
tree | 9c25e207356ebc7b2b88f3c0a7cd0e9374f571c8 | |
parent | 1f8d6c790a8493a341f1786710aac36621e93bf6 (diff) |
Minor refactoring of Uploaders
- Moves a duplicate `file_storage?` definition into the common
`GitlabUploader` ancestor.
- Get the `uploads` base directory from a class method rather than
hard-coding it where it's needed. This will be used in a subsequent MR
to store Uploads in the database.
- Improves the specs for uploaders.
-rw-r--r-- | app/uploaders/artifact_uploader.rb | 4 | ||||
-rw-r--r-- | app/uploaders/attachment_uploader.rb | 2 | ||||
-rw-r--r-- | app/uploaders/avatar_uploader.rb | 2 | ||||
-rw-r--r-- | app/uploaders/file_uploader.rb | 25 | ||||
-rw-r--r-- | app/uploaders/gitlab_uploader.rb | 10 | ||||
-rw-r--r-- | app/uploaders/uploader_helper.rb | 6 | ||||
-rw-r--r-- | spec/uploaders/attachment_uploader_spec.rb | 7 | ||||
-rw-r--r-- | spec/uploaders/avatar_uploader_spec.rb | 7 | ||||
-rw-r--r-- | spec/uploaders/file_uploader_spec.rb | 46 | ||||
-rw-r--r-- | spec/uploaders/uploader_helper_spec.rb | 35 |
10 files changed, 79 insertions, 65 deletions
diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index 86f317dcd18..e84944ed411 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -27,10 +27,6 @@ class ArtifactUploader < GitlabUploader File.join(self.class.artifacts_cache_path, @build.artifacts_path) end - def file_storage? - self.class.storage == CarrierWave::Storage::File - end - def filename file.try(:filename) end diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb index cfcb877cc3e..6aa1f5a8c50 100644 --- a/app/uploaders/attachment_uploader.rb +++ b/app/uploaders/attachment_uploader.rb @@ -4,6 +4,6 @@ class AttachmentUploader < GitlabUploader storage :file def store_dir - "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" + "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" end end diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb index 265cea2d2c6..b4c393c6f2c 100644 --- a/app/uploaders/avatar_uploader.rb +++ b/app/uploaders/avatar_uploader.rb @@ -4,7 +4,7 @@ class AvatarUploader < GitlabUploader storage :file def store_dir - "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" + "#{base_dir}/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" end def exists? diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 23b7318827c..0d2edaeff3b 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -4,15 +4,12 @@ class FileUploader < GitlabUploader storage :file - attr_accessor :project, :secret + attr_accessor :project + attr_reader :secret def initialize(project, secret = nil) @project = project - @secret = secret || self.class.generate_secret - end - - def base_dir - "uploads" + @secret = secret || generate_secret end def store_dir @@ -23,10 +20,6 @@ class FileUploader < GitlabUploader File.join(base_dir, 'tmp', @project.path_with_namespace, @secret) end - def secure_url - File.join("/uploads", @secret, file.filename) - end - def to_markdown to_h[:markdown] end @@ -35,17 +28,23 @@ class FileUploader < GitlabUploader filename = image_or_video? ? self.file.basename : self.file.filename escaped_filename = filename.gsub("]", "\\]") - markdown = "[#{escaped_filename}](#{self.secure_url})" + markdown = "[#{escaped_filename}](#{secure_url})" markdown.prepend("!") if image_or_video? || dangerous? { alt: filename, - url: self.secure_url, + url: secure_url, markdown: markdown } end - def self.generate_secret + private + + def generate_secret SecureRandom.hex end + + def secure_url + File.join('/uploads', @secret, file.filename) + end end diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 02d7c601d6c..bd7de4ed562 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -1,4 +1,14 @@ class GitlabUploader < CarrierWave::Uploader::Base + def self.base_dir + 'uploads' + end + + delegate :base_dir, to: :class + + def file_storage? + self.class.storage == CarrierWave::Storage::File + end + # Reduce disk IO def move_to_cache true diff --git a/app/uploaders/uploader_helper.rb b/app/uploaders/uploader_helper.rb index bee311583ea..7635c20ab3a 100644 --- a/app/uploaders/uploader_helper.rb +++ b/app/uploaders/uploader_helper.rb @@ -27,6 +27,8 @@ module UploaderHelper extension_match?(DANGEROUS_EXT) end + private + def extension_match?(extensions) return false unless file @@ -40,8 +42,4 @@ module UploaderHelper extensions.include?(extension.downcase) end - - def file_storage? - self.class.storage == CarrierWave::Storage::File - end end diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb index 6098be5cd45..ea714fb08f0 100644 --- a/spec/uploaders/attachment_uploader_spec.rb +++ b/spec/uploaders/attachment_uploader_spec.rb @@ -1,18 +1,17 @@ require 'spec_helper' describe AttachmentUploader do - let(:issue) { build(:issue) } - subject { described_class.new(issue) } + let(:uploader) { described_class.new(build_stubbed(:user)) } describe '#move_to_cache' do it 'is true' do - expect(subject.move_to_cache).to eq(true) + expect(uploader.move_to_cache).to eq(true) end end describe '#move_to_store' do it 'is true' do - expect(subject.move_to_store).to eq(true) + expect(uploader.move_to_store).to eq(true) end end end diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb index 76f5a4b42ed..c4d558805ab 100644 --- a/spec/uploaders/avatar_uploader_spec.rb +++ b/spec/uploaders/avatar_uploader_spec.rb @@ -1,18 +1,17 @@ require 'spec_helper' describe AvatarUploader do - let(:user) { build(:user) } - subject { described_class.new(user) } + let(:uploader) { described_class.new(build_stubbed(:user)) } describe '#move_to_cache' do it 'is false' do - expect(subject.move_to_cache).to eq(false) + expect(uploader.move_to_cache).to eq(false) end end describe '#move_to_store' do it 'is false' do - expect(subject.move_to_store).to eq(false) + expect(uploader.move_to_store).to eq(false) end end end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index 6a712e33c96..b0f5be55c33 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -1,57 +1,35 @@ require 'spec_helper' describe FileUploader do - let(:project) { create(:project) } + let(:uploader) { described_class.new(build_stubbed(:project)) } - before do - @previous_enable_processing = FileUploader.enable_processing - FileUploader.enable_processing = false - @uploader = FileUploader.new(project) - end - - after do - FileUploader.enable_processing = @previous_enable_processing - @uploader.remove! - end + describe 'initialize' do + it 'generates a secret if none is provided' do + expect(SecureRandom).to receive(:hex).and_return('secret') - describe '#image_or_video?' do - context 'given an image file' do - before do - @uploader.store!(fixture_file_upload(Rails.root.join('spec', 'fixtures', 'rails_sample.jpg'))) - end + uploader = described_class.new(double) - it 'detects an image based on file extension' do - expect(@uploader.image_or_video?).to be true - end + expect(uploader.secret).to eq 'secret' end - context 'given an video file' do - before do - video_file = fixture_file_upload(Rails.root.join('spec', 'fixtures', 'video_sample.mp4')) - @uploader.store!(video_file) - end - - it 'detects a video based on file extension' do - expect(@uploader.image_or_video?).to be true - end - end + it 'accepts a secret parameter' do + expect(SecureRandom).not_to receive(:hex) - it 'does not return image_or_video? for other types' do - @uploader.store!(fixture_file_upload(Rails.root.join('spec', 'fixtures', 'doc_sample.txt'))) + uploader = described_class.new(double, 'secret') - expect(@uploader.image_or_video?).to be false + expect(uploader.secret).to eq 'secret' end end describe '#move_to_cache' do it 'is true' do - expect(@uploader.move_to_cache).to eq(true) + expect(uploader.move_to_cache).to eq(true) end end describe '#move_to_store' do it 'is true' do - expect(@uploader.move_to_store).to eq(true) + expect(uploader.move_to_store).to eq(true) end end end diff --git a/spec/uploaders/uploader_helper_spec.rb b/spec/uploaders/uploader_helper_spec.rb new file mode 100644 index 00000000000..e9efd13b9aa --- /dev/null +++ b/spec/uploaders/uploader_helper_spec.rb @@ -0,0 +1,35 @@ +require 'rails_helper' + +describe UploaderHelper do + class ExampleUploader < CarrierWave::Uploader::Base + include UploaderHelper + + storage :file + end + + def upload_fixture(filename) + fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) + end + + describe '#image_or_video?' do + let(:uploader) { ExampleUploader.new } + + it 'returns true for an image file' do + uploader.store!(upload_fixture('dk.png')) + + expect(uploader).to be_image_or_video + end + + it 'it returns true for a video file' do + uploader.store!(upload_fixture('video_sample.mp4')) + + expect(uploader).to be_image_or_video + end + + it 'returns false for other extensions' do + uploader.store!(upload_fixture('doc_sample.txt')) + + expect(uploader).not_to be_image_or_video + end + end +end |