Add missing specs

app/controllers/projects/artifacts_controller.rb

@@ -10,7 +10,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
   before_action :set_path_and_entry, only: [:file, :raw]
 
   def download
-    if artifacts_file.local_file?
+    if artifacts_file.file_storage?
       send_file artifacts_file.path, disposition: 'attachment'
     else
       redirect_to artifacts_file.url

app/controllers/uploads_controller.rb

@@ -80,7 +80,7 @@ class UploadsController < ApplicationController
     else
       @uploader = @model.send(upload_mount)
 
-      redirect_to @uploader.url unless @uploader.local_storage?
+      redirect_to @uploader.url unless @uploader.file_storage?
     end
 
     @uploader

app/uploaders/artifact_uploader.rb

@@ -7,6 +7,10 @@ class ArtifactUploader < GitlabUploader
     Gitlab.config.artifacts.path
   end
 
+  def self.artifacts_upload_path
+    File.join(self.local_artifacts_store, 'tmp/uploads/')
+  end
+
   def initialize(job, field)
     @job, @field = job, field
   end

app/uploaders/gitlab_uploader.rb

@@ -9,15 +9,11 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   delegate :base_dir, to: :class
 
-  def local_file?
-    local_storage? && file&.is_a?(CarrierWave::SanitizedFile)
-  end
-
-  def local_storage?
+  def file_storage?
     storage.is_a?(CarrierWave::Storage::File)
   end
 
-  def local_cache_storage?
+  def file_cache_storage?
     cache_storage.is_a?(CarrierWave::Storage::File)
   end
 

app/uploaders/records_uploads.rb

@@ -16,7 +16,7 @@ module RecordsUploads
   #
   # Called `after :store`
   def record_upload(_tempfile)
-    return unless local_file?
+    return unless file_storage?
     return unless file.exists?
 
     Upload.record(self)
@@ -26,7 +26,7 @@ module RecordsUploads
   #
   # Called `before :remove`
   def destroy_upload(*args)
-    return unless local_file?
+    return unless file_storage?
     return unless file
 
     Upload.remove_path(relative_path)

db/post_migrate/20170523083112_migrate_old_artifacts.rb

@@ -22,7 +22,7 @@ class MigrateOldArtifacts < ActiveRecord::Migration
   def builds_with_artifacts
     Build.with_artifacts
       .joins('JOIN projects ON projects.id = ci_builds.project_id')
-      .where('ci_builds.id < ?', min_id || 0)
+      .where('ci_builds.id < ?', min_id)
       .where('projects.ci_id IS NOT NULL')
       .select('id', 'created_at', 'project_id', 'projects.ci_id AS ci_id')
   end
@@ -30,18 +30,18 @@ class MigrateOldArtifacts < ActiveRecord::Migration
   def min_id
     Build.joins('JOIN projects ON projects.id = ci_builds.project_id')
       .where('projects.ci_id IS NULL')
-      .pluck('min(ci_builds.id)')
+      .pluck('coalesce(min(ci_builds.id), 0)')
       .first
   end
 
   class Build < ActiveRecord::Base
     self.table_name = 'ci_builds'
 
-    scope :with_artifacts, ->() { where.not(artifacts_file: [nil, '']) }
+    scope :with_artifacts, -> { where.not(artifacts_file: [nil, '']) }
 
     def migrate_artifacts!
-      return unless File.exists?(source_artifacts_path)
-      return if File.exists?(target_artifacts_path)
+      return unless File.exist?(source_artifacts_path)
+      return if File.exist?(target_artifacts_path)
 
       ensure_target_path
 

lib/api/helpers.rb

@@ -311,6 +311,16 @@ module API
       end
     end
 
+    def present_artifacts!(artifacts_file)
+      return not_found! unless artifacts_file.exists?
+  
+      if artifacts_file.file_storage?
+        present_file!(artifacts_file.path, artifacts_file.filename)
+      else
+        redirect_to(artifacts_file.url)
+      end
+    end
+
     private
 
     def private_token

lib/api/jobs.rb

@@ -224,16 +224,6 @@ module API
         find_build(id) || not_found!
       end
 
-      def present_artifacts!(artifacts_file)
-        if !artifacts_file.local_file?
-          redirect_to(build.artifacts_file.url)
-        elsif artifacts_file.exists?
-          present_file!(artifacts_file.path, artifacts_file.filename)
-        else
-          not_found!
-        end
-      end
-
       def filter_builds(builds, scope)
         return builds if scope.nil? || scope.empty?
 

lib/api/runner.rb

@@ -241,16 +241,7 @@ module API
       get '/:id/artifacts' do
         job = authenticate_job!
 
-        artifacts_file = job.artifacts_file
-        unless artifacts_file.local_file?
-          return redirect_to job.artifacts_file.url
-        end
-
-        unless artifacts_file.exists?
-          not_found!
-        end
-
-        present_file!(artifacts_file.path, artifacts_file.filename)
+        present_artifacts!(job.artifacts_file)
       end
     end
   end

lib/api/v3/builds.rb

@@ -225,16 +225,6 @@ module API
           find_build(id) || not_found!
         end
 
-        def present_artifacts!(artifacts_file)
-          if !artifacts_file.local_file?
-            redirect_to(build.artifacts_file.url)
-          elsif artifacts_file.exists?
-            present_file!(artifacts_file.path, artifacts_file.filename)
-          else
-            not_found!
-          end
-        end
-
         def filter_builds(builds, scope)
           return builds if scope.nil? || scope.empty?
 

lib/ci/api/builds.rb

@@ -187,14 +187,14 @@ module Ci
           build = authenticate_build!
           artifacts_file = build.artifacts_file
 
-          unless artifacts_file.local_file?
-            return redirect_to build.artifacts_file.url
-          end
-
           unless artifacts_file.exists?
             not_found!
           end
 
+          unless artifacts_file.file_storage?
+            return redirect_to build.artifacts_file.url
+          end
+
           present_file!(artifacts_file.path, artifacts_file.filename)
         end
 

spec/migrations/migrate_old_artifacts_spec.rb

@@ -94,16 +94,16 @@ describe MigrateOldArtifacts do
 
       FileUtils.copy(
         Rails.root.join('spec/fixtures/ci_build_artifacts.zip'),
-          File.join(legacy_path(build), "ci_build_artifacts.zip")
-      )
+        File.join(legacy_path(build), "ci_build_artifacts.zip"))
+  
       FileUtils.copy(
         Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'),
-          File.join(legacy_path(build), "ci_build_artifacts_metadata.gz")
-      )
+        File.join(legacy_path(build), "ci_build_artifacts_metadata.gz"))
+
       build.update_columns(
         artifacts_file: 'ci_build_artifacts.zip',
-        artifacts_metadata: 'ci_build_artifacts_metadata.gz',
-      )
+        artifacts_metadata: 'ci_build_artifacts_metadata.gz')
+
       build.reload
     end
 

spec/uploaders/artifact_uploader_spec.rb

+require 'rails_helper'
+
+describe ArtifactUploader do
+  let(:job) { create(:ci_build) }
+  let(:uploader) { described_class.new(job, :artifacts_file) }
+  let(:path) { Gitlab.config.artifacts.path }
+
+  describe '.local_artifacts_store' do
+    subject { described_class.local_artifacts_store }
+
+    it "delegate to artifacts path" do
+      expect(Gitlab.config.artifacts).to receive(:path)
+
+      subject
+    end
+  end
+
+  describe '.artifacts_upload_path' do
+    subject { described_class.artifacts_upload_path }
+    
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('tmp/uploads/') }
+  end
+
+  describe '#store_dir' do
+    subject { uploader.store_dir }
+    
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with("#{job.project_id}/#{job.id}") }
+  end
+
+  describe '#cache_dir' do
+    subject { uploader.cache_dir }
+    
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('tmp/cache') }
+  end
+end

spec/uploaders/gitlab_uploader_spec.rb

+require 'rails_helper'
+require 'carrierwave/storage/fog'
+
+describe GitlabUploader do
+  let(:uploader_class) { Class.new(described_class) }
+
+  subject { uploader_class.new }
+
+  describe '#file_storage?' do
+    context 'when file storage is used' do
+      before do
+        uploader_class.storage(:file)
+      end
+
+      it { is_expected.to be_file_storage }
+    end
+
+    context 'when is remote storage' do
+      before do
+        uploader_class.storage(:fog)
+      end
+
+      it { is_expected.not_to be_file_storage }
+    end
+  end
+
+  describe '#file_cache_storage?' do
+    context 'when file storage is used' do
+      before do
+        uploader_class.cache_storage(:file)
+      end
+
+      it { is_expected.to be_file_cache_storage }
+    end
+
+    context 'when is remote storage' do
+      before do
+        uploader_class.cache_storage(:fog)
+      end
+
+      it { is_expected.not_to be_file_cache_storage }
+    end
+  end
+
+  describe '#move_to_cache' do
+    it 'is true' do
+      expect(subject.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)
+    end
+  end
+end