From 08b9532e376eae369cd04d9f86ea560acfd19ed0 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 19 Jul 2016 20:59:38 +0800
Subject: [PATCH 01/45] 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           |   3 +
 app/models/ci/pipeline.rb        |  10 +-
 app/models/project.rb            |   7 ++
 lib/api/builds.rb                |  58 +++++----
 spec/models/build_spec.rb        |  63 ++++++++--
 spec/models/project_spec.rb      |   2 +-
 spec/requests/api/builds_spec.rb | 196 ++++++++++++++++++++++++++++---
 7 files changed, 290 insertions(+), 49 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index ffac3a22efc..65dfe4f0190 100644
--- a/app/models/ci/build.rb
+++ b/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
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index a65a826536d..f5b4124d1ee 100644
--- a/app/models/ci/pipeline.rb
+++ b/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
@@ -222,7 +230,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/project.rb b/app/models/project.rb
index a805f5d97bc..29aaaf5117f 100644
--- a/app/models/project.rb
+++ b/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
diff --git a/lib/api/builds.rb b/lib/api/builds.rb
index d36047acd1f..bb9e8f1ae6e 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,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
+        present_artifact!(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
+        builds = user_project.latest_successful_builds_for(params[:ref_name])
+        latest_build = builds.find_by!(name: params[:job])
 
-        return not_found! unless artifacts_file.exists?
-
-        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?
 
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 481416319dd..355cb8fdfff 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: '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 'should return true' do
-        expect(build.retryable?).to be true
+      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
+
+        expect(builds).to be_empty
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 9dc34276f18..53b420d808f 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
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index f5b39c3d698..47bcc0eebdd 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -1,21 +1,26 @@
 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!(: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
 

From 6054c855a082fd822611d358914ac20695608e5b Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 19 Jul 2016 22:08:16 +0800
Subject: [PATCH 02/45] Only allow branches/tags, disallow SHA:

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13170854
---
 app/models/ci/pipeline.rb        | 7 ++-----
 spec/requests/api/builds_spec.rb | 8 --------
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index fab91bdae5a..7efa67466c1 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -20,12 +20,9 @@ module Ci
     after_touch :update_state
     after_save :keep_around_commits
 
-    # ref can't be HEAD, can only be branch/tag name or SHA
+    # ref can't be HEAD or SHA, can only be branch/tag name
     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)
+      where(ref: ref).success.order(id: :desc)
     end
 
     def self.truncate_sha(sha)
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 47bcc0eebdd..20d3ed61123 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -255,14 +255,6 @@ describe API::API, api: true do
         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',

From 0882355db354e295fe541d823433d0f43969380e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 19 Jul 2016 22:12:28 +0800
Subject: [PATCH 03/45] CHANGELOG for downloading latest successful build API

---
 CHANGELOG | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG b/CHANGELOG
index 93a47b87d63..490ff151b76 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -15,6 +15,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
   - Wrap code blocks on Activies and Todos page. !4783 (winniehell)
   - Align flash messages with left side of page content !4959 (winniehell)
   - Display tooltip for "Copy to Clipboard" button !5164 (winniehell)

From f2969b1bb08cbe1d2111e975a2a75d66c16acbef Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:23:45 +0800
Subject: [PATCH 04/45] Artifacts are plural, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173284
---
 lib/api/builds.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/api/builds.rb b/lib/api/builds.rb
index bb9e8f1ae6e..7e5114052c4 100644
--- a/lib/api/builds.rb
+++ b/lib/api/builds.rb
@@ -70,7 +70,7 @@ module API
 
         build = get_build!(params[:build_id])
 
-        present_artifact!(build.artifacts_file)
+        present_artifacts!(build.artifacts_file)
       end
 
       # Download the artifacts file from ref_name and job
@@ -86,7 +86,7 @@ module API
         builds = user_project.latest_successful_builds_for(params[:ref_name])
         latest_build = builds.find_by!(name: params[:job])
 
-        present_artifact!(latest_build.artifacts_file)
+        present_artifacts!(latest_build.artifacts_file)
       end
 
       # Get a trace of a specific build of a project
@@ -198,7 +198,7 @@ module API
         get_build(id) || not_found!
       end
 
-      def present_artifact!(artifacts_file)
+      def present_artifacts!(artifacts_file)
         if !artifacts_file.file_storage?
           redirect_to(build.artifacts_file.url)
         elsif artifacts_file.exists?

From cd449a736b375094d26d5fc62f973e00e0fb207a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:26:40 +0800
Subject: [PATCH 05/45] Remove descriptions for simple case, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173304
---
 spec/models/build_spec.rb | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index bd120e84c8f..ac148cec6b0 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -662,9 +662,7 @@ describe Ci::Build, models: true do
         build.run!
       end
 
-      it 'returns false' do
-        expect(build).not_to be_retryable
-      end
+      it { expect(build).not_to be_retryable }
     end
 
     context 'when build is finished' do
@@ -672,9 +670,7 @@ describe Ci::Build, models: true do
         build.success!
       end
 
-      it 'returns true' do
-        expect(build).to be_retryable
-      end
+      it { expect(build).to be_retryable }
     end
   end
 

From c3c0406a7e4ead20aae1b06a590ee346cb2f4b75 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:29:36 +0800
Subject: [PATCH 06/45] Use let! feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173336

let! is not very flexible but I guess it's fine since this is not in
top-level anyway.
---
 spec/models/build_spec.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index ac148cec6b0..7333a8151cc 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -675,14 +675,10 @@ describe Ci::Build, models: true do
   end
 
   describe 'Project#latest_successful_builds_for' do
-    let(:build) 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')

From 42656104864ce2078f587ad35cb89942d7b48c6d Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:30:33 +0800
Subject: [PATCH 07/45] Use for, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173345
---
 spec/models/build_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 7333a8151cc..157b11d9130 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -680,7 +680,7 @@ describe Ci::Build, models: true do
     end
 
     context 'with succeed pipeline' do
-      it 'returns builds from ref' do
+      it 'returns builds for ref' do
         builds = project.latest_successful_builds_for('fix')
 
         expect(builds).to contain_exactly(build)

From 43b6a3eac14a3fb96ed248120c31b41ba8cd6a06 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:33:31 +0800
Subject: [PATCH 08/45] Check against type explicit, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173368
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173370
---
 spec/models/build_spec.rb | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 157b11d9130..e4fdfdcf03e 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -687,8 +687,9 @@ describe Ci::Build, models: true do
       end
 
       it 'returns empty relation if the build cannot be found' do
-        builds = project.latest_successful_builds_for('TAIL').all
+        builds = project.latest_successful_builds_for('TAIL')
 
+        expect(builds).to be_kind_of(ActiveRecord::Relation)
         expect(builds).to be_empty
       end
     end
@@ -699,8 +700,9 @@ describe Ci::Build, models: true do
       end
 
       it 'returns empty relation' do
-        builds = project.latest_successful_builds_for('fix').all
+        builds = project.latest_successful_builds_for('fix')
 
+        expect(builds).to be_kind_of(ActiveRecord::Relation)
         expect(builds).to be_empty
       end
     end

From 361f3d067cd69df9ab4f9f5af0b72bc213edc283 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:37:42 +0800
Subject: [PATCH 09/45] when unauthorized, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173429
---
 spec/requests/api/builds_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 20d3ed61123..6505a75e9c8 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -205,7 +205,7 @@ describe API::API, api: true do
       api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", user)
     end
 
-    context 'not logging in' do
+    context 'when unauthorized' do
       let(:user) { nil }
 
       before do

From 6da27aa6a583ddb4d73b9dfaaaa3e9fdf98e9a6d Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:38:50 +0800
Subject: [PATCH 10/45] Drop description for simple case, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173445
---
 spec/requests/api/builds_spec.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 6505a75e9c8..ef489a1ee90 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -212,9 +212,7 @@ describe API::API, api: true do
         get path_from_ref
       end
 
-      it 'gives 401 for unauthorized user' do
-        expect(response).to have_http_status(401)
-      end
+      it { expect(response).to have_http_status(401) }
     end
 
     context 'non-existing build' do

From ae83ac9969973fe58bc0ec3b65bd6071fad8623f Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:39:41 +0800
Subject: [PATCH 11/45] path_from_ref -> path_for_ref, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347/diffs#note_13173404
---
 spec/requests/api/builds_spec.rb | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index ef489a1ee90..f28c027287f 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -201,7 +201,7 @@ describe API::API, api: true do
       project.team << [user, :developer]
     end
 
-    def path_from_ref(ref = pipeline.ref, job = build.name)
+    def path_for_ref(ref = pipeline.ref, job = build.name)
       api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", user)
     end
 
@@ -209,7 +209,7 @@ describe API::API, api: true do
       let(:user) { nil }
 
       before do
-        get path_from_ref
+        get path_for_ref
       end
 
       it { expect(response).to have_http_status(401) }
@@ -222,7 +222,7 @@ describe API::API, api: true do
 
       context 'has no such ref' do
         before do
-          get path_from_ref('TAIL', build.name)
+          get path_for_ref('TAIL', build.name)
         end
 
         it('gives 404') { verify }
@@ -230,7 +230,7 @@ describe API::API, api: true do
 
       context 'has no such build' do
         before do
-          get path_from_ref(pipeline.ref, 'NOBUILD')
+          get path_for_ref(pipeline.ref, 'NOBUILD')
         end
 
         it('gives 404') { verify }
@@ -260,7 +260,7 @@ describe API::API, api: true do
         end
 
         before do
-          get path_from_ref('master')
+          get path_for_ref('master')
         end
 
         it('gives the file') { verify }
@@ -273,7 +273,7 @@ describe API::API, api: true do
         end
 
         before do
-          get path_from_ref('improve/awesome')
+          get path_for_ref('improve/awesome')
         end
 
         it('gives the file') { verify }
@@ -287,7 +287,7 @@ describe API::API, api: true do
         end
 
         before do
-          get path_from_ref
+          get path_for_ref
         end
 
         it('gives the file') { verify }
@@ -300,7 +300,7 @@ describe API::API, api: true do
         end
 
         before do
-          get path_from_ref
+          get path_for_ref
         end
 
         it('gives the file') { verify }

From 0465876197b58bee0223b5ca5cfd2da971ea60b9 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 00:52:16 +0800
Subject: [PATCH 12/45] Use default_branch rather than master, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13173871
---
 app/models/ci/pipeline.rb | 2 +-
 app/models/project.rb     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 7efa67466c1..3646baea88e 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -21,7 +21,7 @@ module Ci
     after_save :keep_around_commits
 
     # ref can't be HEAD or SHA, can only be branch/tag name
-    scope :latest_successful_for, ->(ref) do
+    scope :latest_successful_for, ->(ref = default_branch) do
       where(ref: ref).success.order(id: :desc)
     end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 29aaaf5117f..4ba3881d30e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -430,7 +430,7 @@ class Project < ActiveRecord::Base
   end
 
   # ref can't be HEAD, can only be branch/tag name or SHA
-  def latest_successful_builds_for(ref = 'master')
+  def latest_successful_builds_for(ref = default_branch)
     Ci::Build.joins(:pipeline).
       merge(pipelines.latest_successful_for(ref)).
       latest_successful_with_artifacts

From 091142118ee9555544acf6d05f61fd109e400ff2 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 01:08:16 +0800
Subject: [PATCH 13/45] Now we could use normal relation, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13173842
---
 app/models/project.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index 4ba3881d30e..026fff0da0c 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -431,8 +431,7 @@ class Project < ActiveRecord::Base
 
   # ref can't be HEAD, can only be branch/tag name or SHA
   def latest_successful_builds_for(ref = default_branch)
-    Ci::Build.joins(:pipeline).
-      merge(pipelines.latest_successful_for(ref)).
+    builds.where(pipeline: pipelines.latest_successful_for(ref)).
       latest_successful_with_artifacts
   end
 

From dc6afd575fdcceb51e44565e68f4db9bf3e19124 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 01:29:44 +0800
Subject: [PATCH 14/45] Just use default_branch, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13176885
---
 spec/models/build_spec.rb        | 8 ++++----
 spec/requests/api/builds_spec.rb | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index e4fdfdcf03e..d01a066f4f6 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -6,7 +6,7 @@ describe Ci::Build, models: true do
   let(:pipeline) do
     create(:ci_pipeline, project: project,
                          sha: project.commit.id,
-                         ref: 'fix',
+                         ref: project.default_branch,
                          status: 'success')
   end
 
@@ -680,8 +680,8 @@ describe Ci::Build, models: true do
     end
 
     context 'with succeed pipeline' do
-      it 'returns builds for ref' do
-        builds = project.latest_successful_builds_for('fix')
+      it 'returns builds for ref for default_branch' do
+        builds = project.latest_successful_builds_for
 
         expect(builds).to contain_exactly(build)
       end
@@ -700,7 +700,7 @@ describe Ci::Build, models: true do
       end
 
       it 'returns empty relation' do
-        builds = project.latest_successful_builds_for('fix')
+        builds = project.latest_successful_builds_for
 
         expect(builds).to be_kind_of(ActiveRecord::Relation)
         expect(builds).to be_empty
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index f28c027287f..2782d7e0490 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -192,8 +192,8 @@ describe API::API, api: true do
     let(:pipeline) do
       create(:ci_pipeline,
               project: project,
-              sha: project.commit('fix').sha,
-              ref: 'fix')
+              sha: project.commit.sha,
+              ref: project.default_branch)
     end
     let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
 

From cfd7c44513c1c04f170c6956eba9f76d2fd5c811 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 01:34:54 +0800
Subject: [PATCH 15/45] Move Project#latest_successful_builds_for to
 project_spec.rb

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13173313
---
 spec/models/build_spec.rb   | 34 -----------------------------
 spec/models/project_spec.rb | 43 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index d01a066f4f6..95573f0a419 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -674,40 +674,6 @@ describe Ci::Build, models: true do
     end
   end
 
-  describe 'Project#latest_successful_builds_for' do
-    let!(:build) do
-      create(:ci_build, :artifacts, :success, pipeline: pipeline)
-    end
-
-    context 'with succeed 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 pending pipeline' do
-      before do
-        pipeline.update(status: 'pending')
-      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 '#manual?' do
     before do
       build.update(when: value)
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 53b420d808f..b03fdebf8e7 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1114,6 +1114,49 @@ describe Project, models: true do
     end
   end
 
+  describe '#latest_successful_builds_for' do
+    let(:project) { create(:project) }
+
+    let(:pipeline) do
+      create(:ci_pipeline, project: project,
+                           sha: project.commit.id,
+                           ref: project.default_branch,
+                           status: 'success')
+    end
+
+    let!(:build) do
+      create(:ci_build, :artifacts, :success, pipeline: pipeline)
+    end
+
+    context 'with succeed 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 pending pipeline' do
+      before do
+        pipeline.update(status: 'pending')
+      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

From 719b0f11aaca424d4a5e4614008108af530ec1da Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Tue, 19 Jul 2016 19:24:59 +0200
Subject: [PATCH 16/45] Make minimal changes to specs

---
 spec/requests/api/builds_spec.rb | 73 ++++++++------------------------
 1 file changed, 17 insertions(+), 56 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 2782d7e0490..669fb1663e5 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -6,21 +6,16 @@ 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, ref: project.default_branch) }
+  let!(:build) { create(:ci_build, pipeline: pipeline) }
 
   describe 'GET /projects/:id/builds ' do
     let(:query) { '' }
 
-    before do
-      developer
-      build
-
-      get api("/projects/#{project.id}/builds?#{query}", api_user)
-    end
+    before { get api("/projects/#{project.id}/builds?#{query}", api_user) }
 
     context 'authorized user' do
       it 'should return project builds' do
@@ -82,9 +77,9 @@ describe API::API, api: true do
       context 'when user is authorized' do
         context 'when pipeline has builds' do
           before do
-            developer
-            build
+            create(:ci_pipeline, project: project, sha: project.commit.id)
             create(:ci_build, pipeline: pipeline)
+            create(:ci_build)
 
             get api("/projects/#{project.id}/repository/commits/#{project.commit.id}/builds", api_user)
           end
@@ -98,8 +93,6 @@ 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
@@ -114,7 +107,8 @@ describe API::API, api: true do
 
       context 'when user is not authorized' do
         before do
-          build
+          create(:ci_pipeline, project: project, sha: project.commit.id)
+          create(:ci_build, pipeline: pipeline)
 
           get api("/projects/#{project.id}/repository/commits/#{project.commit.id}/builds", nil)
         end
@@ -128,11 +122,7 @@ describe API::API, api: true do
   end
 
   describe 'GET /projects/:id/builds/:build_id' do
-    before do
-      developer
-
-      get api("/projects/#{project.id}/builds/#{build.id}", api_user)
-    end
+    before { get api("/projects/#{project.id}/builds/#{build.id}", api_user) }
 
     context 'authorized user' do
       it 'should return specific build data' do
@@ -151,11 +141,7 @@ describe API::API, api: true do
   end
 
   describe 'GET /projects/:id/builds/:build_id/artifacts' do
-    before do
-      developer
-
-      get api("/projects/#{project.id}/builds/#{build.id}/artifacts", api_user)
-    end
+    before { get api("/projects/#{project.id}/builds/#{build.id}/artifacts", api_user) }
 
     context 'build with artifacts' do
       let(:build) { create(:ci_build, :artifacts, pipeline: pipeline) }
@@ -187,26 +173,15 @@ describe API::API, api: true do
   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.sha,
-              ref: project.default_branch)
-    end
+    let(:api_user) { user2 } # is a reporter of the project
     let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
 
-    before do
-      project.team << [user, :developer]
-    end
-
     def path_for_ref(ref = pipeline.ref, job = build.name)
-      api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", user)
+      api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", api_user)
     end
 
     context 'when unauthorized' do
-      let(:user) { nil }
+      let(:api_user) { nil }
 
       before do
         get path_for_ref
@@ -334,12 +309,7 @@ describe API::API, api: true do
   end
 
   describe 'POST /projects/:id/builds/:build_id/cancel' do
-    before do
-      developer
-      reporter
-
-      post api("/projects/#{project.id}/builds/#{build.id}/cancel", api_user)
-    end
+    before { post api("/projects/#{project.id}/builds/#{build.id}/cancel", api_user) }
 
     context 'authorized user' do
       context 'user with :update_build persmission' do
@@ -370,12 +340,7 @@ describe API::API, api: true do
   describe 'POST /projects/:id/builds/:build_id/retry' do
     let(:build) { create(:ci_build, :canceled, pipeline: pipeline) }
 
-    before do
-      developer
-      reporter
-
-      post api("/projects/#{project.id}/builds/#{build.id}/retry", api_user)
-    end
+    before { post api("/projects/#{project.id}/builds/#{build.id}/retry", api_user) }
 
     context 'authorized user' do
       context 'user with :update_build permission' do
@@ -406,8 +371,6 @@ 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
 
@@ -438,8 +401,6 @@ 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
 

From 02f58dbd6f8d82fdbb728c2cac34a6fba466a14f Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 02:15:26 +0800
Subject: [PATCH 17/45] It's no longer needed

---
 spec/requests/api/builds_spec.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 669fb1663e5..351eb6caf98 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -287,8 +287,6 @@ describe API::API, api: true do
     let(:build) { create(:ci_build, :trace, pipeline: pipeline) }
 
     before do
-      developer
-
       get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user)
     end
 

From b0f7384f95e94961b6ed91ef0fdddbbe28d77e6e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 16:25:28 +0800
Subject: [PATCH 18/45] Create the pipelines/builds in mixed order, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13176921

Also we give undesired builds another artifacts so that we could tell
if returned artifacts was we actual expected.
---
 spec/requests/api/builds_spec.rb | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 351eb6caf98..e19c7a98799 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -213,6 +213,10 @@ describe API::API, api: true do
     end
 
     context 'find proper build' do
+      let(:another_artifacts) do
+        fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/gif')
+      end
+
       def verify
         download_headers =
           { 'Content-Transfer-Encoding' => 'binary',
@@ -223,11 +227,6 @@ describe API::API, api: true do
         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 regular branch' do
         before do
           pipeline.update(ref: 'master',
@@ -256,8 +255,13 @@ describe API::API, api: true do
 
       context 'with latest pipeline' do
         before do
-          3.times do # creating some old pipelines
-            create_new_pipeline(:success)
+          pipelines = 3.times.map do # creating some old pipelines
+            create(:ci_pipeline, status: 'success')
+          end
+
+          pipelines.reverse_each do |pipe|
+            new_build = create(:ci_build, :success, pipeline: pipe)
+            new_build.update(artifacts_file: another_artifacts)
           end
         end
 
@@ -271,7 +275,10 @@ describe API::API, api: true do
       context 'with success pipeline' do
         before do
           build # make sure pipeline was old, but still the latest success one
-          create_new_pipeline(:pending)
+          new_pipeline = create(:ci_pipeline, status: 'success')
+          new_build = create(:ci_build, :pending,
+                             pipeline: new_pipeline)
+          new_build.update(artifacts_file: another_artifacts)
         end
 
         before do

From 709e805d65ef15081341e12dce6acf356893459b Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 16:33:04 +0800
Subject: [PATCH 19/45] That means different things but it's ok here.

rubocop was complaining
---
 spec/requests/api/builds_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index e19c7a98799..871309827c0 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -255,7 +255,7 @@ describe API::API, api: true do
 
       context 'with latest pipeline' do
         before do
-          pipelines = 3.times.map do # creating some old pipelines
+          pipelines = Array.new(3).map do # creating some old pipelines
             create(:ci_pipeline, status: 'success')
           end
 

From cf78c35701dc282a99894566a67b42b235dfc151 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 17:30:34 +0800
Subject: [PATCH 20/45] Use shared_example rather than methods, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13173653
---
 spec/requests/api/builds_spec.rb | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 871309827c0..06ebe4a8dd3 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -217,14 +217,15 @@ describe API::API, api: true do
         fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/gif')
       end
 
-      def verify
-        download_headers =
-          { 'Content-Transfer-Encoding' => 'binary',
-            'Content-Disposition' =>
-              "attachment; filename=#{build.artifacts_file.filename}" }
+      let(:download_headers) do
+        { 'Content-Transfer-Encoding' => 'binary',
+          'Content-Disposition' =>
+            "attachment; filename=#{build.artifacts_file.filename}" }
+      end
 
-        expect(response).to have_http_status(200)
-        expect(response.headers).to include(download_headers)
+      shared_examples 'a valid file' do
+        it { expect(response).to have_http_status(200) }
+        it { expect(response.headers).to include(download_headers) }
       end
 
       context 'with regular branch' do
@@ -237,7 +238,7 @@ describe API::API, api: true do
           get path_for_ref('master')
         end
 
-        it('gives the file') { verify }
+        it_behaves_like 'a valid file'
       end
 
       context 'with branch name containing slash' do
@@ -250,7 +251,7 @@ describe API::API, api: true do
           get path_for_ref('improve/awesome')
         end
 
-        it('gives the file') { verify }
+        it_behaves_like 'a valid file'
       end
 
       context 'with latest pipeline' do
@@ -269,7 +270,7 @@ describe API::API, api: true do
           get path_for_ref
         end
 
-        it('gives the file') { verify }
+        it_behaves_like 'a valid file'
       end
 
       context 'with success pipeline' do
@@ -285,7 +286,7 @@ describe API::API, api: true do
           get path_for_ref
         end
 
-        it('gives the file') { verify }
+        it_behaves_like 'a valid file'
       end
     end
   end

From 3925436a9582ec35c0eb4fddaeeaf71c23076754 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 17:38:49 +0800
Subject: [PATCH 21/45] Use shared_examples, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13173599
---
 spec/requests/api/builds_spec.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 06ebe4a8dd3..c2541346a51 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -191,8 +191,8 @@ describe API::API, api: true do
     end
 
     context 'non-existing build' do
-      def verify
-        expect(response).to have_http_status(404)
+      shared_examples 'not found' do
+        it { expect(response).to have_http_status(:not_found) }
       end
 
       context 'has no such ref' do
@@ -200,7 +200,7 @@ describe API::API, api: true do
           get path_for_ref('TAIL', build.name)
         end
 
-        it('gives 404') { verify }
+        it_behaves_like 'not found'
       end
 
       context 'has no such build' do
@@ -208,7 +208,7 @@ describe API::API, api: true do
           get path_for_ref(pipeline.ref, 'NOBUILD')
         end
 
-        it('gives 404') { verify }
+        it_behaves_like 'not found'
       end
     end
 

From 2fe8ebc143737390ea8ba952ad1b8ba4c82dae84 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 18:06:35 +0800
Subject: [PATCH 22/45] We need INNER JOIN to get the right pipeline,

also added a test for checking this.
---
 app/models/project.rb            |  2 +-
 spec/requests/api/builds_spec.rb | 11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index 026fff0da0c..f2e9d607967 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -431,7 +431,7 @@ class Project < ActiveRecord::Base
 
   # ref can't be HEAD, can only be branch/tag name or SHA
   def latest_successful_builds_for(ref = default_branch)
-    builds.where(pipeline: pipelines.latest_successful_for(ref)).
+    Ci::Build.joins(:pipeline).merge(pipelines.latest_successful_for(ref)).
       latest_successful_with_artifacts
   end
 
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index c2541346a51..553b432c7c7 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -256,14 +256,17 @@ describe API::API, api: true do
 
       context 'with latest pipeline' do
         before do
-          pipelines = Array.new(3).map do # creating some old pipelines
+          old_pipelines = Array.new(3).map do # creating some old pipelines
             create(:ci_pipeline, status: 'success')
           end
 
-          pipelines.reverse_each do |pipe|
-            new_build = create(:ci_build, :success, pipeline: pipe)
-            new_build.update(artifacts_file: another_artifacts)
+          old_pipelines.reverse_each do |pipe|
+            old_build = create(:ci_build, :success, pipeline: pipe)
+            old_build.update(artifacts_file: another_artifacts)
           end
+
+          wrong_build = create(:ci_build, :success, pipeline: pipeline)
+          wrong_build.update(artifacts_file: another_artifacts)
         end
 
         before do

From 66c8c3d4c390f886306c5bc074f9c0b3298f6911 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 19:13:32 +0800
Subject: [PATCH 23/45] More complex data manipulating tests to model, and

this should properly test that it's really getting the
builds from the latest successful pipelines and latest
successful builds.
---
 spec/models/project_spec.rb      | 68 ++++++++++++++++++++++++++++----
 spec/requests/api/builds_spec.rb | 38 ------------------
 2 files changed, 60 insertions(+), 46 deletions(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index b03fdebf8e7..373170f9769 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1124,22 +1124,74 @@ describe Project, models: true do
                            status: 'success')
     end
 
-    let!(:build) do
+    let(:build) do
       create(:ci_build, :artifacts, :success, pipeline: pipeline)
     end
 
     context 'with succeed pipeline' do
-      it 'returns builds for ref for default_branch' do
-        builds = project.latest_successful_builds_for
+      context 'standalone pipeline' do
+        before do
+          build
+        end
 
-        expect(builds).to contain_exactly(build)
+        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
 
-      it 'returns empty relation if the build cannot be found' do
-        builds = project.latest_successful_builds_for('TAIL')
+      context 'with multiple pipelines and builds' do
+        shared_examples 'latest successful one' do
+          it 'gives the latest build from latest pipeline' do
+            latest_build = project.latest_successful_builds_for.first
 
-        expect(builds).to be_kind_of(ActiveRecord::Relation)
-        expect(builds).to be_empty
+            expect(latest_build).to eq(build)
+          end
+        end
+
+        context 'with all success pipeline' do
+          before do
+            old_pipelines = Array.new(3).map do
+              create(:ci_pipeline, project: project,
+                                   sha: project.commit.sha,
+                                   ref: project.default_branch,
+                                   status: 'success')
+            end
+
+            # should not give this old build for the latest pipeline
+            create(:ci_build, :success, :artifacts, pipeline: pipeline)
+            build
+
+            old_pipelines.reverse_each do |pipe|
+              create(:ci_build, :success, :artifacts, pipeline: pipe)
+            end
+          end
+
+          it_behaves_like 'latest successful one'
+        end
+
+        context 'with some pending pipeline' do
+          before do
+            # make sure pipeline was old, but still the latest success one
+            build
+
+            new_pipeline = create(:ci_pipeline, project: project,
+                                                sha: project.commit.sha,
+                                                ref: project.default_branch,
+                                                status: 'pending')
+            create(:ci_build, :pending, :artifacts, pipeline: new_pipeline)
+          end
+
+          it_behaves_like 'latest successful one'
+        end
       end
     end
 
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 553b432c7c7..6e84604c949 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -253,44 +253,6 @@ describe API::API, api: true do
 
         it_behaves_like 'a valid file'
       end
-
-      context 'with latest pipeline' do
-        before do
-          old_pipelines = Array.new(3).map do # creating some old pipelines
-            create(:ci_pipeline, status: 'success')
-          end
-
-          old_pipelines.reverse_each do |pipe|
-            old_build = create(:ci_build, :success, pipeline: pipe)
-            old_build.update(artifacts_file: another_artifacts)
-          end
-
-          wrong_build = create(:ci_build, :success, pipeline: pipeline)
-          wrong_build.update(artifacts_file: another_artifacts)
-        end
-
-        before do
-          get path_for_ref
-        end
-
-        it_behaves_like 'a valid file'
-      end
-
-      context 'with success pipeline' do
-        before do
-          build # make sure pipeline was old, but still the latest success one
-          new_pipeline = create(:ci_pipeline, status: 'success')
-          new_build = create(:ci_build, :pending,
-                             pipeline: new_pipeline)
-          new_build.update(artifacts_file: another_artifacts)
-        end
-
-        before do
-          get path_for_ref
-        end
-
-        it_behaves_like 'a valid file'
-      end
     end
   end
 

From a563f3311399d526bdfbef067c5a4c571acaaed5 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 19:27:53 +0800
Subject: [PATCH 24/45] Join on association

---
 app/models/project.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index f2e9d607967..5cfc1d407e4 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -431,7 +431,7 @@ class Project < ActiveRecord::Base
 
   # ref can't be HEAD, can only be branch/tag name or SHA
   def latest_successful_builds_for(ref = default_branch)
-    Ci::Build.joins(:pipeline).merge(pipelines.latest_successful_for(ref)).
+    builds.joins(:pipeline).merge(pipelines.latest_successful_for(ref)).
       latest_successful_with_artifacts
   end
 

From bdb900cdbd40a0b4a18da4eb016ead94e7212784 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 20:04:56 +0800
Subject: [PATCH 25/45] Past tense, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13195632
---
 spec/models/project_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 373170f9769..531b6afc580 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1157,7 +1157,7 @@ describe Project, models: true do
           end
         end
 
-        context 'with all success pipeline' do
+        context 'with all succeeded pipeline' do
           before do
             old_pipelines = Array.new(3).map do
               create(:ci_pipeline, project: project,

From 0c202f3751e1aba94743a9e1b985a6c528775c33 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 20:05:51 +0800
Subject: [PATCH 26/45] Past tense:

Feedback:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13195632
---
 spec/models/project_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 531b6afc580..b4112d65a34 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1128,7 +1128,7 @@ describe Project, models: true do
       create(:ci_build, :artifacts, :success, pipeline: pipeline)
     end
 
-    context 'with succeed pipeline' do
+    context 'with succeeded pipeline' do
       context 'standalone pipeline' do
         before do
           build

From 145da6b1c1794d8e7191e99edc71269130cbfbb9 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 20:08:09 +0800
Subject: [PATCH 27/45] Use only one before, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13195672
---
 spec/requests/api/builds_spec.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 6e84604c949..e623eed7e9e 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -232,9 +232,7 @@ describe API::API, api: true do
         before do
           pipeline.update(ref: 'master',
                           sha: project.commit('master').sha)
-        end
 
-        before do
           get path_for_ref('master')
         end
 

From a9f3f3c8c9a1656efae35b9837ab4076cc236bf1 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 20:09:29 +0800
Subject: [PATCH 28/45] Cleanup let a bit, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13195683
and
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13195692

another_artifacts was no longer used and then there's no point to
put let in outer scope anymore.
---
 spec/requests/api/builds_spec.rb | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index e623eed7e9e..d14fd53afb9 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -213,17 +213,13 @@ describe API::API, api: true do
     end
 
     context 'find proper build' do
-      let(:another_artifacts) do
-        fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/gif')
-      end
-
-      let(:download_headers) do
-        { 'Content-Transfer-Encoding' => 'binary',
-          'Content-Disposition' =>
-            "attachment; filename=#{build.artifacts_file.filename}" }
-      end
-
       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

From 9f70abf185c5d55afc392f2ed39246594c62886d Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 21:46:46 +0800
Subject: [PATCH 29/45] Avoid mixing builds from different pipelines:

So we no longer join anything, just find the latest pipeline
and load builds from there to avoid mixing builds.

Thanks Kamil for the help and tests.
---
 app/models/ci/build.rb      |  3 ---
 app/models/ci/pipeline.rb   |  2 +-
 app/models/project.rb       |  9 +++++--
 spec/models/project_spec.rb | 51 ++++++++++++++++++++++---------------
 4 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 20492c54729..49a123d488b 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -15,9 +15,6 @@ 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
     scope :manual_actions, ->() { where(when: :manual) }
 
     mount_uploader :artifacts_file, ArtifactUploader
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 3646baea88e..63246de4692 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -22,7 +22,7 @@ module Ci
 
     # 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)
+      where(ref: ref).success.order(id: :desc).limit(1)
     end
 
     def self.truncate_sha(sha)
diff --git a/app/models/project.rb b/app/models/project.rb
index 5cfc1d407e4..4fd635abbb3 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -431,8 +431,13 @@ class Project < ActiveRecord::Base
 
   # ref can't be HEAD, can only be branch/tag name or SHA
   def latest_successful_builds_for(ref = default_branch)
-    builds.joins(:pipeline).merge(pipelines.latest_successful_for(ref)).
-      latest_successful_with_artifacts
+    latest_successful_pipeline = pipelines.latest_successful_for(ref).first
+
+    if latest_successful_pipeline
+      latest_successful_pipeline.builds.with_artifacts.latest
+    else
+      builds.none
+    end
   end
 
   def merge_base_commit(first_commit_id, second_commit_id)
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index b4112d65a34..163b7caf55a 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1148,6 +1148,36 @@ describe Project, models: true do
         end
       end
 
+      context 'with many builds' do
+        before do
+          @pipeline1 = create_pipeline
+          @pipeline2 = create_pipeline
+          @build1_p2 = create_build(@pipeline2, 'test')
+          @build1_p1 = create_build(@pipeline1, 'test')
+          @build2_p1 = create_build(@pipeline1, 'test2')
+          @build2_p2 = create_build(@pipeline2, 'test2')
+        end
+
+        it 'gives the latest build from latest pipeline' do
+          latest_builds = project.latest_successful_builds_for
+
+          expect(latest_builds).to contain_exactly(@build2_p2, @build1_p2)
+        end
+
+        def create_pipeline
+          create(:ci_pipeline, project: project,
+                 sha: project.commit.sha,
+                 ref: project.default_branch,
+                 status: 'success')
+        end
+
+        def create_build(pipe, name = 'test')
+          create(:ci_build, :success, :artifacts,
+                 pipeline: pipe,
+                 name: name)
+        end
+      end
+
       context 'with multiple pipelines and builds' do
         shared_examples 'latest successful one' do
           it 'gives the latest build from latest pipeline' do
@@ -1157,27 +1187,6 @@ describe Project, models: true do
           end
         end
 
-        context 'with all succeeded pipeline' do
-          before do
-            old_pipelines = Array.new(3).map do
-              create(:ci_pipeline, project: project,
-                                   sha: project.commit.sha,
-                                   ref: project.default_branch,
-                                   status: 'success')
-            end
-
-            # should not give this old build for the latest pipeline
-            create(:ci_build, :success, :artifacts, pipeline: pipeline)
-            build
-
-            old_pipelines.reverse_each do |pipe|
-              create(:ci_build, :success, :artifacts, pipeline: pipe)
-            end
-          end
-
-          it_behaves_like 'latest successful one'
-        end
-
         context 'with some pending pipeline' do
           before do
             # make sure pipeline was old, but still the latest success one

From fcdfeba922a8480cf9cafdb98bb96c558165e51e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 21:51:47 +0800
Subject: [PATCH 30/45] It should be plural

---
 spec/models/project_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 163b7caf55a..cbd1f5f9e32 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1158,7 +1158,7 @@ describe Project, models: true do
           @build2_p2 = create_build(@pipeline2, 'test2')
         end
 
-        it 'gives the latest build from latest pipeline' do
+        it 'gives the latest builds from latest pipeline' do
           latest_builds = project.latest_successful_builds_for
 
           expect(latest_builds).to contain_exactly(@build2_p2, @build1_p2)

From 4a1edae3ac321241e2168b98df695e11048f7724 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 22:00:34 +0800
Subject: [PATCH 31/45] Use one query. Feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13199055
---
 app/models/project.rb | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index 4fd635abbb3..80860f142d4 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -431,13 +431,8 @@ class Project < ActiveRecord::Base
 
   # ref can't be HEAD, can only be branch/tag name or SHA
   def latest_successful_builds_for(ref = default_branch)
-    latest_successful_pipeline = pipelines.latest_successful_for(ref).first
-
-    if latest_successful_pipeline
-      latest_successful_pipeline.builds.with_artifacts.latest
-    else
-      builds.none
-    end
+    pipeline = pipelines.latest_successful_for(ref)
+    builds.where(pipeline: pipeline).latest.with_artifacts
   end
 
   def merge_base_commit(first_commit_id, second_commit_id)

From fea934b596323190c966c5edf1c8631c725f3820 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 22:02:42 +0800
Subject: [PATCH 32/45] Still give descriptions, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13198953
---
 spec/requests/api/builds_spec.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index d14fd53afb9..d274466edec 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -187,7 +187,9 @@ describe API::API, api: true do
         get path_for_ref
       end
 
-      it { expect(response).to have_http_status(401) }
+      it 'gives 401' do
+        expect(response).to have_http_status(401)
+      end
     end
 
     context 'non-existing build' do

From a7713232ea723a7215ab0aff4c6fed87343df1fd Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 22:12:17 +0800
Subject: [PATCH 33/45] Also exclude artifacts_file with empty string,
 feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13198105
---
 app/models/ci/build.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 49a123d488b..f87c1206e91 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -12,7 +12,9 @@ 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, ->() do
+      where.not(artifacts_file: [nil, ''])
+    end
     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) }

From 88aacaa7e5ec57b85749028f4463a498fc1e35f1 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 22:31:40 +0800
Subject: [PATCH 34/45] Reuse those two methods

---
 spec/models/project_spec.rb | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index cbd1f5f9e32..9d122fd0083 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1115,19 +1115,23 @@ describe Project, models: true do
   end
 
   describe '#latest_successful_builds_for' do
-    let(:project) { create(:project) }
-
-    let(:pipeline) do
+    def create_pipeline
       create(:ci_pipeline, project: project,
-                           sha: project.commit.id,
+                           sha: project.commit.sha,
                            ref: project.default_branch,
                            status: 'success')
     end
 
-    let(:build) do
-      create(:ci_build, :artifacts, :success, pipeline: pipeline)
+    def create_build(new_pipeline = pipeline, name = 'test')
+      create(:ci_build, :success, :artifacts,
+             pipeline: new_pipeline,
+             name: name)
     end
 
+    let(:project) { create(:project) }
+    let(:pipeline) { create_pipeline }
+    let(:build) { create_build }
+
     context 'with succeeded pipeline' do
       context 'standalone pipeline' do
         before do
@@ -1163,19 +1167,6 @@ describe Project, models: true do
 
           expect(latest_builds).to contain_exactly(@build2_p2, @build1_p2)
         end
-
-        def create_pipeline
-          create(:ci_pipeline, project: project,
-                 sha: project.commit.sha,
-                 ref: project.default_branch,
-                 status: 'success')
-        end
-
-        def create_build(pipe, name = 'test')
-          create(:ci_build, :success, :artifacts,
-                 pipeline: pipe,
-                 name: name)
-        end
       end
 
       context 'with multiple pipelines and builds' do

From ff3776c8d5a84f7ea6b1b50ad8c7add02d3f0434 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 22:40:49 +0800
Subject: [PATCH 35/45] Should check against `authorize_read_builds!`

---
 lib/api/builds.rb                |  2 ++
 spec/requests/api/builds_spec.rb | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/lib/api/builds.rb b/lib/api/builds.rb
index 7e5114052c4..657d421fe97 100644
--- a/lib/api/builds.rb
+++ b/lib/api/builds.rb
@@ -83,6 +83,8 @@ module API
       #   GET /projects/:id/artifacts/:ref_name/download?job=name
       get ':id/builds/artifacts/:ref_name/download',
         requirements: { ref_name: /.+/ } do
+        authorize_read_builds!
+
         builds = user_project.latest_successful_builds_for(params[:ref_name])
         latest_build = builds.find_by!(name: params[:job])
 
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index d274466edec..43fb2edb730 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -6,9 +6,11 @@ describe API::API, api: true do
   let(:user) { create(:user) }
   let(:api_user) { user }
   let(:user2) { create(:user) }
+  let(:guest_user) { 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!(:guest) { create(:project_member, :guest, user: guest_user, project: project) }
   let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id, ref: project.default_branch) }
   let!(:build) { create(:ci_build, pipeline: pipeline) }
 
@@ -192,6 +194,18 @@ describe API::API, api: true do
       end
     end
 
+    context 'when forbidden' 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) }

From 782b384f8a738ecffef27b0651d986a9823045e9 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 22:42:14 +0800
Subject: [PATCH 36/45] Rename user2 to reporter_user

---
 spec/requests/api/builds_spec.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 43fb2edb730..e110699596c 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -5,11 +5,11 @@ describe API::API, api: true do
 
   let(:user) { create(:user) }
   let(:api_user) { user }
-  let(:user2) { create(:user) }
+  let(:reporter_user) { create(:user) }
   let(:guest_user) { 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!(:reporter) { create(:project_member, :reporter, user: reporter_user, project: project) }
   let!(:guest) { create(:project_member, :guest, user: guest_user, project: project) }
   let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id, ref: project.default_branch) }
   let!(:build) { create(:ci_build, pipeline: pipeline) }
@@ -175,7 +175,7 @@ describe API::API, api: true do
   end
 
   describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do
-    let(:api_user) { user2 } # is a reporter of the project
+    let(:api_user) { reporter_user }
     let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
 
     def path_for_ref(ref = pipeline.ref, job = build.name)
@@ -301,7 +301,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)
@@ -333,7 +333,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)

From e8bab842cb519cd8af61368d7307c9c84b6f8d38 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 23:17:28 +0800
Subject: [PATCH 37/45] Cleanup that a bit

---
 spec/models/project_spec.rb | 69 +++++++++++++++----------------------
 1 file changed, 27 insertions(+), 42 deletions(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 9d122fd0083..3a5e922bae7 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1115,29 +1115,44 @@ describe Project, models: true do
   end
 
   describe '#latest_successful_builds_for' do
-    def create_pipeline
+    def create_pipeline(status = 'success')
       create(:ci_pipeline, project: project,
                            sha: project.commit.sha,
                            ref: project.default_branch,
-                           status: 'success')
+                           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 }
-    let(:build) { create_build }
+
+    context 'with many builds' do
+      before do
+        @pipeline1 = create_pipeline
+        @pipeline2 = create_pipeline
+        @build1_p2 = create_build(@pipeline2, 'test')
+        @build1_p1 = create_build(@pipeline1, 'test')
+        @build2_p1 = create_build(@pipeline1, 'test2')
+        @build2_p2 = create_build(@pipeline2, 'test2')
+      end
+
+      it 'gives the latest builds from latest pipeline' do
+        latest_builds = project.latest_successful_builds_for
+
+        expect(latest_builds).to contain_exactly(@build2_p2, @build1_p2)
+      end
+    end
 
     context 'with succeeded pipeline' do
-      context 'standalone pipeline' do
-        before do
-          build
-        end
+      let!(:build) { create_build }
 
+      context 'standalone pipeline' do
         it 'returns builds for ref for default_branch' do
           builds = project.latest_successful_builds_for
 
@@ -1152,45 +1167,15 @@ describe Project, models: true do
         end
       end
 
-      context 'with many builds' do
+      context 'with some pending pipeline' do
         before do
-          @pipeline1 = create_pipeline
-          @pipeline2 = create_pipeline
-          @build1_p2 = create_build(@pipeline2, 'test')
-          @build1_p1 = create_build(@pipeline1, 'test')
-          @build2_p1 = create_build(@pipeline1, 'test2')
-          @build2_p2 = create_build(@pipeline2, 'test2')
+          create_build(create_pipeline('pending'))
         end
 
-        it 'gives the latest builds from latest pipeline' do
-          latest_builds = project.latest_successful_builds_for
+        it 'gives the latest build from latest pipeline' do
+          latest_build = project.latest_successful_builds_for
 
-          expect(latest_builds).to contain_exactly(@build2_p2, @build1_p2)
-        end
-      end
-
-      context 'with multiple pipelines and builds' do
-        shared_examples 'latest successful one' do
-          it 'gives the latest build from latest pipeline' do
-            latest_build = project.latest_successful_builds_for.first
-
-            expect(latest_build).to eq(build)
-          end
-        end
-
-        context 'with some pending pipeline' do
-          before do
-            # make sure pipeline was old, but still the latest success one
-            build
-
-            new_pipeline = create(:ci_pipeline, project: project,
-                                                sha: project.commit.sha,
-                                                ref: project.default_branch,
-                                                status: 'pending')
-            create(:ci_build, :pending, :artifacts, pipeline: new_pipeline)
-          end
-
-          it_behaves_like 'latest successful one'
+          expect(latest_build).to contain_exactly(build)
         end
       end
     end

From a77a58dffb20838e4e3a51796b4b3d95d7b8c400 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 23:17:49 +0800
Subject: [PATCH 38/45] Use 'when logging as guest' for context, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13200997
---
 spec/requests/api/builds_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index e110699596c..9b205d66ff6 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -194,7 +194,7 @@ describe API::API, api: true do
       end
     end
 
-    context 'when forbidden' do
+    context 'when logging as guest' do
       let(:api_user) { guest_user }
 
       before do

From eea2fd80fc3fbcdfd9beeff2bec5f667bf6135de Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 23:19:35 +0800
Subject: [PATCH 39/45] Use the same logic, it should specify that it's not
 logged in

---
 spec/requests/api/builds_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 9b205d66ff6..e408ea06e7e 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -182,7 +182,7 @@ describe API::API, api: true do
       api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", api_user)
     end
 
-    context 'when unauthorized' do
+    context 'when not logged in' do
       let(:api_user) { nil }
 
       before do

From 08f01dbbc5dc7eb608e4a331a536cc08d297d31a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 23:38:47 +0800
Subject: [PATCH 40/45] Make sure there's a build

---
 spec/models/project_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 3a5e922bae7..91b24e03793 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1183,6 +1183,7 @@ describe Project, models: true do
     context 'with pending pipeline' do
       before do
         pipeline.update(status: 'pending')
+        create_build(pipeline)
       end
 
       it 'returns empty relation' do

From caf6438a2495c09a10f047d18a67b7071d05f7a2 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 23:42:07 +0800
Subject: [PATCH 41/45] It's not longer than 80 chars

---
 app/models/ci/build.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index f87c1206e91..a24665eed72 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -12,9 +12,7 @@ module Ci
 
     scope :unstarted, ->() { where(runner_id: nil) }
     scope :ignore_failures, ->() { where(allow_failure: false) }
-    scope :with_artifacts, ->() do
-      where.not(artifacts_file: [nil, ''])
-    end
+    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) }

From 01dcb5c2623f2123bdf6d7d6164aa90c35796f8a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 20 Jul 2016 23:56:54 +0800
Subject: [PATCH 42/45] Lose unneeded instance variables and variables,
 feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13202326
---
 spec/models/project_spec.rb | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 91b24e03793..c25c971c606 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1134,12 +1134,12 @@ describe Project, models: true do
 
     context 'with many builds' do
       before do
-        @pipeline1 = create_pipeline
-        @pipeline2 = create_pipeline
-        @build1_p2 = create_build(@pipeline2, 'test')
-        @build1_p1 = create_build(@pipeline1, 'test')
-        @build2_p1 = create_build(@pipeline1, 'test2')
-        @build2_p2 = create_build(@pipeline2, 'test2')
+        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')
       end
 
       it 'gives the latest builds from latest pipeline' do

From ba962aa3f05bf325e0c435dfba5e9d939ab62085 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 21 Jul 2016 00:09:35 +0800
Subject: [PATCH 43/45] Cleanup the use of let, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13202424
---
 spec/requests/api/builds_spec.rb | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index e408ea06e7e..86a7b242fbe 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -5,12 +5,10 @@ describe API::API, api: true do
 
   let(:user) { create(:user) }
   let(:api_user) { user }
-  let(:reporter_user) { create(:user) }
-  let(:guest_user) { 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: reporter_user, project: project) }
-  let!(:guest) { create(:project_member, :guest, user: guest_user, project: project) }
+  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) }
 
@@ -175,7 +173,7 @@ describe API::API, api: true do
   end
 
   describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do
-    let(:api_user) { reporter_user }
+    let(:api_user) { reporter.user }
     let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
 
     def path_for_ref(ref = pipeline.ref, job = build.name)
@@ -195,7 +193,7 @@ describe API::API, api: true do
     end
 
     context 'when logging as guest' do
-      let(:api_user) { guest_user }
+      let(:api_user) { guest.user }
 
       before do
         get path_for_ref
@@ -301,7 +299,7 @@ describe API::API, api: true do
       end
 
       context 'user without :update_build permission' do
-        let(:api_user) { reporter_user }
+        let(:api_user) { reporter.user }
 
         it 'should not cancel build' do
           expect(response).to have_http_status(403)
@@ -333,7 +331,7 @@ describe API::API, api: true do
       end
 
       context 'user without :update_build permission' do
-        let(:api_user) { reporter_user }
+        let(:api_user) { reporter.user }
 
         it 'should not retry build' do
           expect(response).to have_http_status(403)

From 8ad92b95c92c6263d051c9f3045c9c2ad7e28f07 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 21 Jul 2016 00:13:02 +0800
Subject: [PATCH 44/45] Just put setup directly in the test, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13202462
---
 spec/models/project_spec.rb | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index c25c971c606..76dc70ae893 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1133,19 +1133,17 @@ describe Project, models: true do
     let(:pipeline) { create_pipeline }
 
     context 'with many builds' do
-      before do
+      it 'gives the latest builds from latest pipeline' do
         pipeline1 = create_pipeline
         pipeline2 = create_pipeline
-        @build1_p2 = create_build(pipeline2, 'test')
+        build1_p2 = create_build(pipeline2, 'test')
         create_build(pipeline1, 'test')
         create_build(pipeline1, 'test2')
-        @build2_p2 = create_build(pipeline2, 'test2')
-      end
+        build2_p2 = create_build(pipeline2, 'test2')
 
-      it 'gives the latest builds from latest pipeline' do
         latest_builds = project.latest_successful_builds_for
 
-        expect(latest_builds).to contain_exactly(@build2_p2, @build1_p2)
+        expect(latest_builds).to contain_exactly(build2_p2, build1_p2)
       end
     end
 

From 122b046b92573f3e1db579b5ff73fa3bdfffc124 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 21 Jul 2016 01:10:08 +0800
Subject: [PATCH 45/45] Workaround MySQL with INNER JOIN:

This version of MySQL doesn't yet support 'LIMIT & IN/ALL/ANY/SOME
subquery'

Oh well.
---
 app/models/commit_status.rb | 6 +++++-
 app/models/project.rb       | 9 +++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

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 80860f142d4..9f0dcb9a212 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -431,8 +431,13 @@ class Project < ActiveRecord::Base
 
   # 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)
-    builds.where(pipeline: pipeline).latest.with_artifacts
+    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)