Merge branch 'rs-uploaders' into 'master'

Minor refactoring of Uploaders

See merge request !9490

15 files changed, 191 insertions, 94 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/features/issues_spec.rb b/spec/features/issues_spec.rb
index 1e0db4a0499..1c8267b1593 100644
--- a/spec/features/issues_spec.rb
+++ b/spec/features/issues_spec.rb
@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe 'Issues', feature: true do
+  include DropzoneHelper
   include IssueHelpers
   include SortingHelper
   include WaitForAjax
@@ -570,19 +571,13 @@ describe 'Issues', feature: true do
       end
 
       it 'uploads file when dragging into textarea' do
-        drop_in_dropzone test_image_file
-
-        # Wait for the file to upload
-        sleep 1
+        dropzone_file Rails.root.join('spec', 'fixtures', 'banana_sample.gif')
 
         expect(page.find_field("issue_description").value).to have_content 'banana_sample'
       end
 
       it 'adds double newline to end of attachment markdown' do
-        drop_in_dropzone test_image_file
-
-        # Wait for the file to upload
-        sleep 1
+        dropzone_file Rails.root.join('spec', 'fixtures', 'banana_sample.gif')
 
         expect(page.find_field("issue_description").value).to match /\n\n$/
       end
@@ -665,25 +660,4 @@ describe 'Issues', feature: true do
       end
     end
   end
-
-  def drop_in_dropzone(file_path)
-    # Generate a fake input selector
-    page.execute_script <<-JS
-      var fakeFileInput = window.$('<input/>').attr(
-        {id: 'fakeFileInput', type: 'file'}
-      ).appendTo('body');
-    JS
-    # Attach the file to the fake input selector with Capybara
-    attach_file("fakeFileInput", file_path)
-    # Add the file to a fileList array and trigger the fake drop event
-    page.execute_script <<-JS
-      var fileList = [$('#fakeFileInput')[0].files[0]];
-      var e = jQuery.Event('drop', { dataTransfer : { files : fileList } });
-      $('.div-dropzone')[0].dropzone.listeners[0].events.drop(e);
-    JS
-  end
-
-  def test_image_file
-    File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif')
-  end
 end
diff --git a/spec/features/uploads/user_uploads_avatar_to_group_spec.rb b/spec/features/uploads/user_uploads_avatar_to_group_spec.rb
new file mode 100644
index 00000000000..f88a515f7fc
--- /dev/null
+++ b/spec/features/uploads/user_uploads_avatar_to_group_spec.rb
@@ -0,0 +1,26 @@
+require 'rails_helper'
+
+feature 'User uploads avatar to group', feature: true do
+  scenario 'they see the new avatar' do
+    user = create(:user)
+    group = create(:group)
+    group.add_owner(user)
+    login_as(user)
+
+    visit edit_group_path(group)
+    attach_file(
+      'group_avatar',
+      Rails.root.join('spec', 'fixtures', 'dk.png'),
+      visible: false
+    )
+
+    click_button 'Save group'
+
+    visit group_path(group)
+
+    expect(page).to have_selector(%Q(img[src$="/uploads/group/avatar/#{group.id}/dk.png"]))
+
+    # Cheating here to verify something that isn't user-facing, but is important
+    expect(group.reload.avatar.file).to exist
+  end
+end
diff --git a/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb
new file mode 100644
index 00000000000..0dfd29045e5
--- /dev/null
+++ b/spec/features/uploads/user_uploads_avatar_to_profile_spec.rb
@@ -0,0 +1,24 @@
+require 'rails_helper'
+
+feature 'User uploads avatar to profile', feature: true do
+  scenario 'they see their new avatar' do
+    user = create(:user)
+    login_as(user)
+
+    visit profile_path
+    attach_file(
+      'user_avatar',
+      Rails.root.join('spec', 'fixtures', 'dk.png'),
+      visible: false
+    )
+
+    click_button 'Update profile settings'
+
+    visit user_path(user)
+
+    expect(page).to have_selector(%Q(img[src$="/uploads/user/avatar/#{user.id}/dk.png"]))
+
+    # Cheating here to verify something that isn't user-facing, but is important
+    expect(user.reload.avatar.file).to exist
+  end
+end
diff --git a/spec/features/uploads/user_uploads_file_to_note_spec.rb b/spec/features/uploads/user_uploads_file_to_note_spec.rb
new file mode 100644
index 00000000000..0c160dd74b4
--- /dev/null
+++ b/spec/features/uploads/user_uploads_file_to_note_spec.rb
@@ -0,0 +1,22 @@
+require 'rails_helper'
+
+feature 'User uploads file to note', feature: true do
+  include DropzoneHelper
+
+  let(:user) { create(:user) }
+  let(:project) { create(:empty_project, creator: user, namespace: user.namespace) }
+
+  scenario 'they see the attached file', js: true do
+    issue = create(:issue, project: project, author: user)
+
+    login_as(user)
+    visit namespace_project_issue_path(project.namespace, project, issue)
+
+    dropzone_file(Rails.root.join('spec', 'fixtures', 'dk.png'))
+    click_button 'Comment'
+    wait_for_ajax
+
+    expect(find('a.no-attachment-icon img[alt="dk"]')['src'])
+      .to match(%r{/#{project.full_path}/uploads/\h{32}/dk\.png$})
+  end
+end
diff --git a/spec/support/dropzone_helper.rb b/spec/support/dropzone_helper.rb
new file mode 100644
index 00000000000..984ec7d2741
--- /dev/null
+++ b/spec/support/dropzone_helper.rb
@@ -0,0 +1,37 @@
+module DropzoneHelper
+  # Provides a way to perform `attach_file` for a Dropzone-based file input
+  #
+  # This is accomplished by creating a standard HTML file input on the page,
+  # performing `attach_file` on that field, and then triggering the appropriate
+  # Dropzone events to perform the actual upload.
+  #
+  # This method waits for the upload to complete before returning.
+  def dropzone_file(file_path)
+    # Generate a fake file input that Capybara can attach to
+    page.execute_script <<-JS.strip_heredoc
+      var fakeFileInput = window.$('<input/>').attr(
+        {id: 'fakeFileInput', type: 'file'}
+      ).appendTo('body');
+
+      window._dropzoneComplete = false;
+    JS
+
+    # Attach the file to the fake input selector with Capybara
+    attach_file('fakeFileInput', file_path)
+
+    # Manually trigger a Dropzone "drop" event with the fake input's file list
+    page.execute_script <<-JS.strip_heredoc
+      var fileList = [$('#fakeFileInput')[0].files[0]];
+      var e = jQuery.Event('drop', { dataTransfer : { files : fileList } });
+
+      var dropzone = $('.div-dropzone')[0].dropzone;
+      dropzone.on('queuecomplete', function() {
+        window._dropzoneComplete = true;
+      });
+      dropzone.listeners[0].events.drop(e);
+    JS
+
+    # Wait until Dropzone's fired `queuecomplete`
+    loop until page.evaluate_script('window._dropzoneComplete === true')
+  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