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.

4 files changed, 53 insertions, 42 deletions

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