Merge branch 'artifacts-from-ref-and-build-name-api' into 'master'

API for downloading latest successful build

## What does this MR do?

Implement parts of #4255, particularly the API.

## Are there points in the code the reviewer needs to double check?

I still made it that `ref` could be either branch, tag, or even SHA with:

``` ruby
# 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
```

Because the reasons I put in:

* https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13165543
* https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13165921

But if you still think that it's not good to do it this way, I'll drop it and let's think about the other way to satisfy the requirement specified in https://gitlab.com/gitlab-org/gitlab-ce/issues/4255#note_13101233 It could be `status=any` or `sha=DEADBEAF`

## What are the relevant issue numbers?

Part of #4255

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5347

9 files changed, 258 insertions, 43 deletions

diff --git a/CHANGELOG b/CHANGELOG
index edbf1d8f06e..3ae4e0ff629 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -19,6 +19,7 @@ v 8.10.0 (unreleased)
   - Add Application Setting to configure default Repository Path for new projects
   - Delete award emoji when deleting a user
   - Remove pinTo from Flash and make inline flash messages look nicer !4854 (winniehell)
+  - Add an API for downloading latest successful build from a particular branch or tag !5347
   - Add link to profile to commit avatar !5163 (winniehell)
   - Wrap code blocks on Activies and Todos page. !4783 (winniehell)
   - Align flash messages with left side of page content !4959 (winniehell)
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 58f25b20acf..cbfa14e81f1 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -12,7 +12,7 @@ module Ci
 
     scope :unstarted, ->() { where(runner_id: nil) }
     scope :ignore_failures, ->() { where(allow_failure: false) }
-    scope :with_artifacts, ->() { where.not(artifacts_file: nil) }
+    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 :manual_actions, ->() { where(when: :manual) }
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 6d3bbb03484..bce6a992af6 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -20,6 +20,11 @@ module Ci
     after_touch :update_state
     after_save :keep_around_commits
 
+    # ref can't be HEAD or SHA, can only be branch/tag name
+    scope :latest_successful_for, ->(ref = default_branch) do
+      where(ref: ref).success.order(id: :desc).limit(1)
+    end
+
     def self.truncate_sha(sha)
       sha[0...8]
     end
@@ -237,7 +242,7 @@ module Ci
 
     def keep_around_commits
       return unless project
-      
+
       project.repository.keep_around(self.sha)
       project.repository.keep_around(self.before_sha)
     end
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 535db26240a..2d185c28809 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -16,7 +16,11 @@ class CommitStatus < ActiveRecord::Base
 
   alias_attribute :author, :user
 
-  scope :latest, -> { where(id: unscope(:select).select('max(id)').group(:name, :commit_id)) }
+  scope :latest, -> do
+    max_id = unscope(:select).select("max(#{quoted_table_name}.id)")
+
+    where(id: max_id.group(:name, :commit_id))
+  end
   scope :retried, -> { where.not(id: latest) }
   scope :ordered, -> { order(:name) }
   scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) }
diff --git a/app/models/project.rb b/app/models/project.rb
index d6191e80e62..f09d915f20e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -429,6 +429,17 @@ 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 = default_branch)
+    pipeline = pipelines.latest_successful_for(ref).to_sql
+    join_sql = "INNER JOIN (#{pipeline}) pipelines" +
+               " ON pipelines.id = #{Ci::Build.quoted_table_name}.commit_id"
+    builds.joins(join_sql).latest.with_artifacts
+    # TODO: Whenever we dropped support for MySQL, we could change to:
+    # pipeline = pipelines.latest_successful_for(ref)
+    # builds.where(pipeline: pipeline).latest.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
diff --git a/lib/api/builds.rb b/lib/api/builds.rb
index d36047acd1f..657d421fe97 100644
--- a/lib/api/builds.rb
+++ b/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,27 @@ 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
+        present_artifacts!(build.artifacts_file)
+      end
 
-        unless artifacts_file.file_storage?
-          return redirect_to build.artifacts_file.url
-        end
+      # 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
+        authorize_read_builds!
 
-        return not_found! unless artifacts_file.exists?
+        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_artifacts!(latest_build.artifacts_file)
       end
 
       # Get a trace of a specific build of a project
@@ -97,8 +105,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 +125,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 +143,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 +162,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 +180,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 +196,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_artifacts!(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?
 
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 2de305738cc..978ad9c52d5 100644
--- a/spec/models/build_spec.rb
+++ b/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: project.default_branch,
+                         status: 'success')
   end
 
   let(:build) { create(:ci_build, pipeline: pipeline) }
@@ -720,7 +722,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
@@ -728,7 +730,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
@@ -736,12 +738,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
@@ -763,19 +766,19 @@ describe Ci::Build, models: true do
 
   describe '#retryable?' do
     context 'when build is running' do
-      before { build.run! }
-
-      it 'should return false' do
-        expect(build.retryable?).to be false
+      before do
+        build.run!
       end
+
+      it { expect(build).not_to be_retryable }
     end
 
     context 'when build is finished' do
-      before { build.success! }
-
-      it 'should return true' do
-        expect(build.retryable?).to be true
+      before do
+        build.success!
       end
+
+      it { expect(build).to be_retryable }
     end
   end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index a2b089c51e2..e937beddc46 100644
--- a/spec/models/project_spec.rb
+++ b/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
@@ -1155,6 +1155,85 @@ describe Project, models: true do
     end
   end
 
+  describe '#latest_successful_builds_for' do
+    def create_pipeline(status = 'success')
+      create(:ci_pipeline, project: project,
+                           sha: project.commit.sha,
+                           ref: project.default_branch,
+                           status: status)
+    end
+
+    def create_build(new_pipeline = pipeline, name = 'test')
+      create(:ci_build, :success, :artifacts,
+             pipeline: new_pipeline,
+             status: new_pipeline.status,
+             name: name)
+    end
+
+    let(:project) { create(:project) }
+    let(:pipeline) { create_pipeline }
+
+    context 'with many builds' do
+      it 'gives the latest builds from latest pipeline' do
+        pipeline1 = create_pipeline
+        pipeline2 = create_pipeline
+        build1_p2 = create_build(pipeline2, 'test')
+        create_build(pipeline1, 'test')
+        create_build(pipeline1, 'test2')
+        build2_p2 = create_build(pipeline2, 'test2')
+
+        latest_builds = project.latest_successful_builds_for
+
+        expect(latest_builds).to contain_exactly(build2_p2, build1_p2)
+      end
+    end
+
+    context 'with succeeded pipeline' do
+      let!(:build) { create_build }
+
+      context 'standalone pipeline' do
+        it 'returns builds for ref for default_branch' do
+          builds = project.latest_successful_builds_for
+
+          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')
+
+          expect(builds).to be_kind_of(ActiveRecord::Relation)
+          expect(builds).to be_empty
+        end
+      end
+
+      context 'with some pending pipeline' do
+        before do
+          create_build(create_pipeline('pending'))
+        end
+
+        it 'gives the latest build from latest pipeline' do
+          latest_build = project.latest_successful_builds_for
+
+          expect(latest_build).to contain_exactly(build)
+        end
+      end
+    end
+
+    context 'with pending pipeline' do
+      before do
+        pipeline.update(status: 'pending')
+        create_build(pipeline)
+      end
+
+      it 'returns empty relation' do
+        builds = project.latest_successful_builds_for
+
+        expect(builds).to be_kind_of(ActiveRecord::Relation)
+        expect(builds).to be_empty
+      end
+    end
+  end
+
   describe '.where_paths_in' do
     context 'without any paths' do
       it 'returns an empty relation' do
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index f5b39c3d698..86a7b242fbe 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -1,15 +1,15 @@
 require 'spec_helper'
 
-describe API::API, api: true  do
+describe API::API, api: true do
   include ApiHelpers
 
   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(:reporter) { create(:project_member, :reporter, project: project) }
+  let(:guest) { create(:project_member, :guest, project: project) }
+  let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id, ref: project.default_branch) }
   let!(:build) { create(:ci_build, pipeline: pipeline) }
 
   describe 'GET /projects/:id/builds ' do
@@ -172,10 +172,104 @@ describe API::API, api: true  do
     end
   end
 
+  describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do
+    let(:api_user) { reporter.user }
+    let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
+
+    def path_for_ref(ref = pipeline.ref, job = build.name)
+      api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", api_user)
+    end
+
+    context 'when not logged in' do
+      let(:api_user) { nil }
+
+      before do
+        get path_for_ref
+      end
+
+      it 'gives 401' do
+        expect(response).to have_http_status(401)
+      end
+    end
+
+    context 'when logging as guest' do
+      let(:api_user) { guest.user }
+
+      before do
+        get path_for_ref
+      end
+
+      it 'gives 403' do
+        expect(response).to have_http_status(403)
+      end
+    end
+
+    context 'non-existing build' do
+      shared_examples 'not found' do
+        it { expect(response).to have_http_status(:not_found) }
+      end
+
+      context 'has no such ref' do
+        before do
+          get path_for_ref('TAIL', build.name)
+        end
+
+        it_behaves_like 'not found'
+      end
+
+      context 'has no such build' do
+        before do
+          get path_for_ref(pipeline.ref, 'NOBUILD')
+        end
+
+        it_behaves_like 'not found'
+      end
+    end
+
+    context 'find proper build' do
+      shared_examples 'a valid file' do
+        let(:download_headers) do
+          { 'Content-Transfer-Encoding' => 'binary',
+            'Content-Disposition' =>
+              "attachment; filename=#{build.artifacts_file.filename}" }
+        end
+
+        it { expect(response).to have_http_status(200) }
+        it { expect(response.headers).to include(download_headers) }
+      end
+
+      context 'with regular branch' do
+        before do
+          pipeline.update(ref: 'master',
+                          sha: project.commit('master').sha)
+
+          get path_for_ref('master')
+        end
+
+        it_behaves_like 'a valid file'
+      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_for_ref('improve/awesome')
+        end
+
+        it_behaves_like 'a valid file'
+      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
+      get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user)
+    end
 
     context 'authorized user' do
       it 'should return specific build trace' do
@@ -205,7 +299,7 @@ describe API::API, api: true  do
       end
 
       context 'user without :update_build permission' do
-        let(:api_user) { user2 }
+        let(:api_user) { reporter.user }
 
         it 'should not cancel build' do
           expect(response).to have_http_status(403)
@@ -237,7 +331,7 @@ describe API::API, api: true  do
       end
 
       context 'user without :update_build permission' do
-        let(:api_user) { user2 }
+        let(:api_user) { reporter.user }
 
         it 'should not retry build' do
           expect(response).to have_http_status(403)