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.

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