API for downloading latest successful build:

This was extracted from !5142 and implementing part of #4255.
We split it from !5142 because we want to ship it in 8.10
while !5142 was not ready yet.

app/models/ci/build.rb

@@ -15,6 +15,9 @@ module Ci
     scope :with_artifacts, ->() { where.not(artifacts_file: nil) }
     scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) }
     scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) }
+    scope :latest_successful_with_artifacts, ->() do
+      with_artifacts.success.order(id: :desc)
+    end
 
     mount_uploader :artifacts_file, ArtifactUploader
     mount_uploader :artifacts_metadata, ArtifactUploader

app/models/ci/pipeline.rb

@@ -20,6 +20,14 @@ module Ci
     after_touch :update_state
     after_save :keep_around_commits
 
+    # ref can't be HEAD, can only be branch/tag name or SHA
+    scope :latest_successful_for, ->(ref) do
+      table = quoted_table_name
+      # TODO: Use `where(ref: ref).or(sha: ref)` in Rails 5
+      where("#{table}.ref = ? OR #{table}.sha = ?", ref, ref).
+        success.order(id: :desc)
+    end
+
     def self.truncate_sha(sha)
       sha[0...8]
     end

app/models/project.rb

@@ -429,6 +429,13 @@ class Project < ActiveRecord::Base
     repository.commit(ref)
   end
 
+  # ref can't be HEAD, can only be branch/tag name or SHA
+  def latest_successful_builds_for(ref = 'master')
+    Ci::Build.joins(:pipeline).
+      merge(pipelines.latest_successful_for(ref)).
+      latest_successful_with_artifacts
+  end
+
   def merge_base_commit(first_commit_id, second_commit_id)
     sha = repository.merge_base(first_commit_id, second_commit_id)
     repository.commit(sha) if sha

lib/api/builds.rb

@@ -52,8 +52,7 @@ module API
       get ':id/builds/:build_id' do
         authorize_read_builds!
 
-        build = get_build(params[:build_id])
-        return not_found!(build) unless build
+        build = get_build!(params[:build_id])
 
         present build, with: Entities::Build,
                        user_can_download_artifacts: can?(current_user, :read_build, user_project)
@@ -69,18 +68,25 @@ module API
       get ':id/builds/:build_id/artifacts' do
         authorize_read_builds!
 
-        build = get_build(params[:build_id])
-        return not_found!(build) unless build
+        build = get_build!(params[:build_id])
 
-        artifacts_file = build.artifacts_file
-
-        unless artifacts_file.file_storage?
-          return redirect_to build.artifacts_file.url
+        present_artifact!(build.artifacts_file)
       end
 
-        return not_found! unless artifacts_file.exists?
+      # Download the artifacts file from ref_name and job
+      #
+      # Parameters:
+      #   id (required) - The ID of a project
+      #   ref_name (required) - The ref from repository
+      #   job (required) - The name for the build
+      # Example Request:
+      #   GET /projects/:id/artifacts/:ref_name/download?job=name
+      get ':id/builds/artifacts/:ref_name/download',
+        requirements: { ref_name: /.+/ } do
+        builds = user_project.latest_successful_builds_for(params[:ref_name])
+        latest_build = builds.find_by!(name: params[:job])
 
-        present_file!(artifacts_file.path, artifacts_file.filename)
+        present_artifact!(latest_build.artifacts_file)
       end
 
       # Get a trace of a specific build of a project
@@ -97,8 +103,7 @@ module API
       get ':id/builds/:build_id/trace' do
         authorize_read_builds!
 
-        build = get_build(params[:build_id])
-        return not_found!(build) unless build
+        build = get_build!(params[:build_id])
 
         header 'Content-Disposition', "infile; filename=\"#{build.id}.log\""
         content_type 'text/plain'
@@ -118,8 +123,7 @@ module API
       post ':id/builds/:build_id/cancel' do
         authorize_update_builds!
 
-        build = get_build(params[:build_id])
-        return not_found!(build) unless build
+        build = get_build!(params[:build_id])
 
         build.cancel
 
@@ -137,8 +141,7 @@ module API
       post ':id/builds/:build_id/retry' do
         authorize_update_builds!
 
-        build = get_build(params[:build_id])
-        return not_found!(build) unless build
+        build = get_build!(params[:build_id])
         return forbidden!('Build is not retryable') unless build.retryable?
 
         build = Ci::Build.retry(build, current_user)
@@ -157,8 +160,7 @@ module API
       post ':id/builds/:build_id/erase' do
         authorize_update_builds!
 
-        build = get_build(params[:build_id])
-        return not_found!(build) unless build
+        build = get_build!(params[:build_id])
         return forbidden!('Build is not erasable!') unless build.erasable?
 
         build.erase(erased_by: current_user)
@@ -176,8 +178,8 @@ module API
       post ':id/builds/:build_id/artifacts/keep' do
         authorize_update_builds!
 
-        build = get_build(params[:build_id])
-        return not_found!(build) unless build && build.artifacts?
+        build = get_build!(params[:build_id])
+        return not_found!(build) unless build.artifacts?
 
         build.keep_artifacts!
 
@@ -192,6 +194,20 @@ module API
         user_project.builds.find_by(id: id.to_i)
       end
 
+      def get_build!(id)
+        get_build(id) || not_found!
+      end
+
+      def present_artifact!(artifacts_file)
+        if !artifacts_file.file_storage?
+          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?
 

spec/models/build_spec.rb

@@ -5,7 +5,9 @@ describe Ci::Build, models: true do
 
   let(:pipeline) do
     create(:ci_pipeline, project: project,
-                         sha: project.commit.id)
+                         sha: project.commit.id,
+                         ref: 'fix',
+                         status: 'success')
   end
 
   let(:build) { create(:ci_build, pipeline: pipeline) }
@@ -612,7 +614,7 @@ describe Ci::Build, models: true do
 
       describe '#erasable?' do
         subject { build.erasable? }
-        it { is_expected.to eq true }
+        it { is_expected.to be_truthy }
       end
 
       describe '#erased?' do
@@ -620,7 +622,7 @@ describe Ci::Build, models: true do
         subject { build.erased? }
 
         context 'build has not been erased' do
-          it { is_expected.to be false }
+          it { is_expected.to be_falsey }
         end
 
         context 'build has been erased' do
@@ -628,12 +630,13 @@ describe Ci::Build, models: true do
             build.erase
           end
 
-          it { is_expected.to be true }
+          it { is_expected.to be_truthy }
         end
       end
 
       context 'metadata and build trace are not available' do
         let!(:build) { create(:ci_build, :success, :artifacts) }
+
         before do
           build.remove_artifacts_metadata!
         end
@@ -655,18 +658,58 @@ describe Ci::Build, models: true do
 
   describe '#retryable?' do
     context 'when build is running' do
-      before { build.run! }
+      before do
+        build.run!
+      end
 
-      it 'should return false' do
-        expect(build.retryable?).to be false
+      it 'returns false' do
+        expect(build).not_to be_retryable
       end
     end
 
     context 'when build is finished' do
-      before { build.success! }
+      before do
+        build.success!
+      end
+
+      it 'returns true' do
+        expect(build).to be_retryable
+      end
+    end
+  end
+
+  describe 'Project#latest_successful_builds_for' do
+    let(:build) do
+      create(:ci_build, :artifacts, :success, pipeline: pipeline)
+    end
+
+    before do
+      build
+    end
+
+    context 'with succeed pipeline' do
+      it 'returns builds from ref' do
+        builds = project.latest_successful_builds_for('fix')
+
+        expect(builds).to contain_exactly(build)
+      end
+
+      it 'returns empty relation if the build cannot be found' do
+        builds = project.latest_successful_builds_for('TAIL').all
+
+        expect(builds).to be_empty
+      end
+    end
+
+    context 'with pending pipeline' do
+      before do
+        pipeline.update(status: 'pending')
+      end
+
+      it 'returns empty relation' do
+        builds = project.latest_successful_builds_for('fix').all
 
-      it 'should return true' do
-        expect(build.retryable?).to be true
+        expect(builds).to be_empty
       end
     end
   end

spec/models/project_spec.rb

@@ -377,7 +377,7 @@ describe Project, models: true do
   describe '#repository' do
     let(:project) { create(:project) }
 
-    it 'should return valid repo' do
+    it 'returns valid repo' do
       expect(project.repository).to be_kind_of(Repository)
     end
   end

spec/requests/api/builds_spec.rb

@@ -6,16 +6,21 @@ describe API::API, api: true  do
   let(:user) { create(:user) }
   let(:api_user) { user }
   let(:user2) { create(:user) }
-  let!(:project) { create(:project, creator_id: user.id) }
-  let!(:developer) { create(:project_member, :developer, user: user, project: project) }
-  let!(:reporter) { create(:project_member, :reporter, user: user2, project: project) }
-  let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id) }
-  let!(:build) { create(:ci_build, pipeline: pipeline) }
+  let(:project) { create(:project, creator_id: user.id) }
+  let(:developer) { create(:project_member, :developer, user: user, project: project) }
+  let(:reporter) { create(:project_member, :reporter, user: user2, project: project) }
+  let(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id) }
+  let(:build) { create(:ci_build, pipeline: pipeline) }
 
   describe 'GET /projects/:id/builds ' do
     let(:query) { '' }
 
-    before { get api("/projects/#{project.id}/builds?#{query}", api_user) }
+    before do
+      developer
+      build
+
+      get api("/projects/#{project.id}/builds?#{query}", api_user)
+    end
 
     context 'authorized user' do
       it 'should return project builds' do
@@ -77,9 +82,9 @@ describe API::API, api: true  do
       context 'when user is authorized' do
         context 'when pipeline has builds' do
           before do
-            create(:ci_pipeline, project: project, sha: project.commit.id)
+            developer
+            build
             create(:ci_build, pipeline: pipeline)
-            create(:ci_build)
 
             get api("/projects/#{project.id}/repository/commits/#{project.commit.id}/builds", api_user)
           end
@@ -93,6 +98,8 @@ describe API::API, api: true  do
 
         context 'when pipeline has no builds' do
           before do
+            developer
+
             branch_head = project.commit('feature').id
             get api("/projects/#{project.id}/repository/commits/#{branch_head}/builds", api_user)
           end
@@ -107,8 +114,7 @@ describe API::API, api: true  do
 
       context 'when user is not authorized' do
         before do
-          create(:ci_pipeline, project: project, sha: project.commit.id)
-          create(:ci_build, pipeline: pipeline)
+          build
 
           get api("/projects/#{project.id}/repository/commits/#{project.commit.id}/builds", nil)
         end
@@ -122,7 +128,11 @@ describe API::API, api: true  do
   end
 
   describe 'GET /projects/:id/builds/:build_id' do
-    before { get api("/projects/#{project.id}/builds/#{build.id}", api_user) }
+    before do
+      developer
+
+      get api("/projects/#{project.id}/builds/#{build.id}", api_user)
+    end
 
     context 'authorized user' do
       it 'should return specific build data' do
@@ -141,7 +151,11 @@ describe API::API, api: true  do
   end
 
   describe 'GET /projects/:id/builds/:build_id/artifacts' do
-    before { get api("/projects/#{project.id}/builds/#{build.id}/artifacts", api_user) }
+    before do
+      developer
+
+      get api("/projects/#{project.id}/builds/#{build.id}/artifacts", api_user)
+    end
 
     context 'build with artifacts' do
       let(:build) { create(:ci_build, :artifacts, pipeline: pipeline) }
@@ -172,10 +186,146 @@ describe API::API, api: true  do
     end
   end
 
+  describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do
+    let(:user) { create(:user) }
+    let(:project) { create(:project) }
+    let(:pipeline) do
+      create(:ci_pipeline,
+              project: project,
+              sha: project.commit('fix').sha,
+              ref: 'fix')
+    end
+    let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
+
+    before do
+      project.team << [user, :developer]
+    end
+
+    def path_from_ref(ref = pipeline.ref, job = build.name)
+      api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", user)
+    end
+
+    context 'not logging in' do
+      let(:user) { nil }
+
+      before do
+        get path_from_ref
+      end
+
+      it 'gives 401 for unauthorized user' do
+        expect(response).to have_http_status(401)
+      end
+    end
+
+    context 'non-existing build' do
+      def verify
+        expect(response).to have_http_status(404)
+      end
+
+      context 'has no such ref' do
+        before do
+          get path_from_ref('TAIL', build.name)
+        end
+
+        it('gives 404') { verify }
+      end
+
+      context 'has no such build' do
+        before do
+          get path_from_ref(pipeline.ref, 'NOBUILD')
+        end
+
+        it('gives 404') { verify }
+      end
+    end
+
+    context 'find proper build' do
+      def verify
+        download_headers =
+          { 'Content-Transfer-Encoding' => 'binary',
+            'Content-Disposition' =>
+              "attachment; filename=#{build.artifacts_file.filename}" }
+
+        expect(response).to have_http_status(200)
+        expect(response.headers).to include(download_headers)
+      end
+
+      def create_new_pipeline(status)
+        new_pipeline = create(:ci_pipeline, status: 'success')
+        create(:ci_build, status, :artifacts, pipeline: new_pipeline)
+      end
+
+      context 'with sha' do
+        before do
+          get path_from_ref(pipeline.sha)
+        end
+
+        it('gives the file') { verify }
+      end
+
+      context 'with regular branch' do
+        before do
+          pipeline.update(ref: 'master',
+                          sha: project.commit('master').sha)
+        end
+
+        before do
+          get path_from_ref('master')
+        end
+
+        it('gives the file') { verify }
+      end
+
+      context 'with branch name containing slash' do
+        before do
+          pipeline.update(ref: 'improve/awesome',
+                          sha: project.commit('improve/awesome').sha)
+        end
+
+        before do
+          get path_from_ref('improve/awesome')
+        end
+
+        it('gives the file') { verify }
+      end
+
+      context 'with latest pipeline' do
+        before do
+          3.times do # creating some old pipelines
+            create_new_pipeline(:success)
+          end
+        end
+
+        before do
+          get path_from_ref
+        end
+
+        it('gives the file') { verify }
+      end
+
+      context 'with success pipeline' do
+        before do
+          build # make sure pipeline was old, but still the latest success one
+          create_new_pipeline(:pending)
+        end
+
+        before do
+          get path_from_ref
+        end
+
+        it('gives the file') { verify }
+      end
+    end
+  end
+
   describe 'GET /projects/:id/builds/:build_id/trace' do
     let(:build) { create(:ci_build, :trace, pipeline: pipeline) }
 
-    before { get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user) }
+    before do
+      developer
+
+      get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user)
+    end
 
     context 'authorized user' do
       it 'should return specific build trace' do
@@ -194,7 +344,12 @@ describe API::API, api: true  do
   end
 
   describe 'POST /projects/:id/builds/:build_id/cancel' do
-    before { post api("/projects/#{project.id}/builds/#{build.id}/cancel", api_user) }
+    before do
+      developer
+      reporter
+
+      post api("/projects/#{project.id}/builds/#{build.id}/cancel", api_user)
+    end
 
     context 'authorized user' do
       context 'user with :update_build persmission' do
@@ -225,7 +380,12 @@ describe API::API, api: true  do
   describe 'POST /projects/:id/builds/:build_id/retry' do
     let(:build) { create(:ci_build, :canceled, pipeline: pipeline) }
 
-    before { post api("/projects/#{project.id}/builds/#{build.id}/retry", api_user) }
+    before do
+      developer
+      reporter
+
+      post api("/projects/#{project.id}/builds/#{build.id}/retry", api_user)
+    end
 
     context 'authorized user' do
       context 'user with :update_build permission' do
@@ -256,6 +416,8 @@ describe API::API, api: true  do
 
   describe 'POST /projects/:id/builds/:build_id/erase' do
     before do
+      developer
+
       post api("/projects/#{project.id}/builds/#{build.id}/erase", user)
     end
 
@@ -286,6 +448,8 @@ describe API::API, api: true  do
 
   describe 'POST /projects/:id/builds/:build_id/artifacts/keep' do
     before do
+      developer
+
       post api("/projects/#{project.id}/builds/#{build.id}/artifacts/keep", user)
     end