Merge branch 'feature/improve-mrwbs-and-todos-for-pipelines' into 'master'

Trigger Merge When Pipeline Succeeds on pipeline event

## What does this MR do?

This MR is meant to improve merge when build succeeds triggers, which has an impact on performance.

- [x] Move Merge When Build Succeeds trigger from commit status to pipeline event
- [x] Drop support for triggering event for branches that include commit status submitted without branch (no longer relevant)
- [x] Perform Merge When Pipeline Succeeds asynchronously to improve performance and avoid race conditions
- [x] Add missing feature test that verifies if MWBS feature actually works and merges merge requests
- [x] Update the documentation to reflect change in the behavior

Moved to separate merge request:

- [ ] Rename Merge When Build Succeeds to Merge When Pipeline Succeeds
- [ ] Update documentation to reflect name change for this feature

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing

See merge request !6675

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.13.0 (unreleased)
+  - Improve Merge When Build Succeeds triggers and execute on pipeline success. (!6675)
   - Respond with 404 Not Found for non-existent tags (Linus Thiel)
   - Truncate long labels with ellipsis in labels page
   - Adding members no longer silently fails when there is extra whitespace

app/models/ci/pipeline.rb

@@ -3,6 +3,7 @@ module Ci
     extend Ci::Model
     include HasStatus
     include Importable
+    include AfterCommitQueue
 
     self.table_name = 'ci_commits'
 
@@ -56,6 +57,10 @@ module Ci
         pipeline.finished_at = Time.now
       end
 
+      before_transition do |pipeline|
+        pipeline.update_duration
+      end
+
       after_transition [:created, :pending] => :running do |pipeline|
         MergeRequest::Metrics.where(merge_request_id: pipeline.merge_requests.map(&:id)).
           update_all(latest_build_started_at: pipeline.started_at, latest_build_finished_at: nil)
@@ -66,8 +71,8 @@ module Ci
           update_all(latest_build_finished_at: pipeline.finished_at)
       end
 
-      before_transition do |pipeline|
-        pipeline.update_duration
+      after_transition [:created, :pending, :running] => :success do |pipeline|
+        pipeline.run_after_commit { PipelineSuccessWorker.perform_async(id) }
       end
 
       after_transition do |pipeline, transition|
@@ -292,11 +297,9 @@ module Ci
     # Merge requests for which the current pipeline is running against
     # the merge request's latest commit.
     def merge_requests
-      @merge_requests ||=
-        begin
-          project.merge_requests.where(source_branch: self.ref).
-            select { |merge_request| merge_request.pipeline.try(:id) == self.id }
-        end
+      @merge_requests ||= project.merge_requests
+        .where(source_branch: self.ref)
+        .select { |merge_request| merge_request.pipeline.try(:id) == self.id }
     end
 
     private

app/models/commit_status.rb

@@ -86,29 +86,19 @@ class CommitStatus < ActiveRecord::Base
     end
 
     after_transition do |commit_status, transition|
-      return if transition.loopback?
+      next if transition.loopback?
 
       commit_status.run_after_commit do
         pipeline.try do |pipeline|
           if complete?
-            ProcessPipelineWorker.perform_async(pipeline.id)
+            PipelineProcessWorker.perform_async(pipeline.id)
           else
-            UpdatePipelineWorker.perform_async(pipeline.id)
+            PipelineUpdateWorker.perform_async(pipeline.id)
           end
         end
       end
     end
 
-    after_transition [:created, :pending, :running] => :success do |commit_status|
-      commit_status.run_after_commit do
-        # TODO, temporary fix for race condition
-        UpdatePipelineWorker.new.perform(pipeline.id)
-
-        MergeRequests::MergeWhenBuildSucceedsService
-          .new(pipeline.project, nil).trigger(self)
-      end
-    end
-
     after_transition any => :failed do |commit_status|
       commit_status.run_after_commit do
         MergeRequests::AddTodoWhenBuildFailsService

app/services/merge_requests/add_todo_when_build_fails_service.rb

@@ -2,14 +2,14 @@ module MergeRequests
   class AddTodoWhenBuildFailsService < MergeRequests::BaseService
     # Adds a todo to the parent merge_request when a CI build fails
     def execute(commit_status)
-      each_merge_request(commit_status) do |merge_request|
+      commit_status_merge_requests(commit_status) do |merge_request|
         todo_service.merge_request_build_failed(merge_request)
       end
     end
 
     # Closes any pending build failed todos for the parent MRs when a build is retried
     def close(commit_status)
-      each_merge_request(commit_status) do |merge_request|
+      commit_status_merge_requests(commit_status) do |merge_request|
         todo_service.merge_request_build_retried(merge_request)
       end
     end

app/services/merge_requests/base_service.rb

@@ -42,28 +42,33 @@ module MergeRequests
       super(:merge_request)
     end
 
-    def merge_request_from(commit_status)
-      branches = commit_status.ref
+    def merge_requests_for(branch)
+      origin_merge_requests = @project.origin_merge_requests
+        .opened.where(source_branch: branch).to_a
 
-      # This is for ref-less builds
-      branches ||= @project.repository.branch_names_contains(commit_status.sha)
+      fork_merge_requests = @project.fork_merge_requests
+        .opened.where(source_branch: branch).to_a
 
-      return [] if branches.blank?
+      (origin_merge_requests + fork_merge_requests)
+        .uniq.select(&:source_project)
+    end
 
-      merge_requests = @project.origin_merge_requests.opened.where(source_branch: branches).to_a
-      merge_requests += @project.fork_merge_requests.opened.where(source_branch: branches).to_a
+    def pipeline_merge_requests(pipeline)
+      merge_requests_for(pipeline.ref).each do |merge_request|
+        next unless pipeline == merge_request.pipeline
 
-      merge_requests.uniq.select(&:source_project)
+        yield merge_request
+      end
     end
 
-    def each_merge_request(commit_status)
-      merge_request_from(commit_status).each do |merge_request|
+    def commit_status_merge_requests(commit_status)
+      merge_requests_for(commit_status.ref).each do |merge_request|
         pipeline = merge_request.pipeline
 
         next unless pipeline
         next unless pipeline.sha == commit_status.sha
 
-        yield merge_request, pipeline
+        yield merge_request
       end
     end
   end

app/services/merge_requests/merge_when_build_succeeds_service.rb

@@ -18,12 +18,13 @@ module MergeRequests
       merge_request.save
     end
 
-    # Triggers the automatic merge of merge_request once the build succeeds
-    def trigger(commit_status)
-      each_merge_request(commit_status) do |merge_request, pipeline|
+    # Triggers the automatic merge of merge_request once the pipeline succeeds
+    def trigger(pipeline)
+      return unless pipeline.success?
+
+      pipeline_merge_requests(pipeline) do |merge_request|
         next unless merge_request.merge_when_build_succeeds?
         next unless merge_request.mergeable?
-        next unless pipeline.success?
 
         MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params)
       end

app/workers/process_pipeline_worker.rb → app/workers/pipeline_process_worker.rb

-class ProcessPipelineWorker
+class PipelineProcessWorker
   include Sidekiq::Worker
 
   sidekiq_options queue: :default

app/workers/pipeline_success_worker.rb

+class PipelineSuccessWorker
+  include Sidekiq::Worker
+  sidekiq_options queue: :default
+
+  def perform(pipeline_id)
+    Ci::Pipeline.find_by(id: pipeline_id).try do |pipeline|
+      MergeRequests::MergeWhenBuildSucceedsService
+        .new(pipeline.project, nil)
+        .trigger(pipeline)
+    end
+  end
+end

app/workers/update_pipeline_worker.rb → app/workers/pipeline_update_worker.rb

-class UpdatePipelineWorker
+class PipelineUpdateWorker
   include Sidekiq::Worker
 
   sidekiq_options queue: :default

doc/user/project/merge_requests/merge_when_build_succeeds.md

 # Merge When Build Succeeds
 
 When reviewing a merge request that looks ready to merge but still has one or
-more CI builds running, you can set it to be merged automatically when all
-builds succeed. This way, you don't have to wait for the builds to finish and
-remember to merge the request manually.
+more CI builds running, you can set it to be merged automatically when the
+builds pipeline succeed. This way, you don't have to wait for the builds to
+finish and remember to merge the request manually.
 
 ![Enable](img/merge_when_build_succeeds_enable.png)
 
 When you hit the "Merge When Build Succeeds" button, the status of the merge
 request will be updated to represent the impending merge. If you cannot wait
-for the build to succeed and want to merge immediately, this option is available
-in the dropdown menu on the right of the main button.
+for the pipeline to succeed and want to merge immediately, this option is
+available in the dropdown menu on the right of the main button.
 
 Both team developers and the author of the merge request have the option to
 cancel the automatic merge if they find a reason why it shouldn't be merged
@@ -18,9 +18,9 @@ after all.
 
 ![Status](img/merge_when_build_succeeds_status.png)
 
-When the build succeeds, the merge request will automatically be merged. When
-the build fails, the author gets a chance to retry any failed builds, or to
-push new commits to fix the failure.
+When the pipeline succeeds, the merge request will automatically be merged.
+When the pipeline fails, the author gets a chance to retry any failed builds,
+or to push new commits to fix the failure.
 
 When the builds are retried and succeed on the second try, the merge request
 will automatically be merged after all. When the merge request is updated with
@@ -40,7 +40,7 @@ hit **Save** for the changes to take effect.
 
 ![Only allow merge if build succeeds settings](img/merge_when_build_succeeds_only_if_succeeds_settings.png)
 
-From now on, every time the build fails you will not be able to merge the merge
-request from the UI, until you make the build pass.
+From now on, every time the pipelinefails you will not be able to merge the
+merge request from the UI, until you make all relevant builds pass.
 
 ![Only allow merge if build succeeds msg](img/merge_when_build_succeeds_only_if_succeeds_msg.png)

spec/features/merge_requests/merge_when_build_succeeds_spec.rb

@@ -2,18 +2,26 @@ require 'spec_helper'
 
 feature 'Merge When Build Succeeds', feature: true, js: true do
   let(:user) { create(:user) }
+  let(:project) { create(:project, :public) }
 
-  let(:project)       { create(:project, :public) }
-  let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: "Bug NS-04") }
+  let(:merge_request) do
+    create(:merge_request_with_diffs, source_project: project,
+                                      author: user,
+                                      title: 'Bug NS-04')
+  end
 
-  before do
-    project.team << [user, :master]
-    project.enable_ci
+  let(:pipeline) do
+    create(:ci_pipeline, project: project,
+                         sha: merge_request.diff_head_sha,
+                         ref: merge_request.source_branch)
   end
 
-  context "Active build for Merge Request" do
-    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
-    let!(:ci_build) { create(:ci_build, pipeline: pipeline) }
+  before { project.team << [user, :master] }
+
+  context 'when there is active build for merge request' do
+    background do
+      create(:ci_build, pipeline: pipeline)
+    end
 
     before do
       login_as user
@@ -41,26 +49,30 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
     end
   end
 
-  context 'When it is enabled' do
+  context 'when merge when build succeeds is enabled' do
     let(:merge_request) do
-      create(:merge_request_with_diffs, :simple,  source_project: project, author: user,
-                                                  merge_user: user, title: "MepMep", merge_when_build_succeeds: true)
+      create(:merge_request_with_diffs, :simple,  source_project: project,
+                                                  author: user,
+                                                  merge_user: user,
+                                                  title: 'MepMep',
+                                                  merge_when_build_succeeds: true)
     end
 
-    let!(:pipeline) { create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
-    let!(:ci_build) { create(:ci_build, pipeline: pipeline) }
+    let!(:build) do
+      create(:ci_build, pipeline: pipeline)
+    end
 
     before do
       login_as user
       visit_merge_request(merge_request)
     end
 
-    it 'cancels the automatic merge' do
+    it 'allows to cancel the automatic merge' do
       click_link "Cancel Automatic Merge"
 
       expect(page).to have_button "Merge When Build Succeeds"
 
-      visit_merge_request(merge_request) # Needed to refresh the page
+      visit_merge_request(merge_request) # refresh the page
       expect(page).to have_content "Canceled the automatic merge"
     end
 
@@ -70,10 +82,21 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
       click_link "Remove Source Branch When Merged"
       expect(page).to have_content "The source branch will be removed"
     end
+
+    context 'when build succeeds' do
+      background { build.success }
+
+      it 'merges merge request' do
+        visit_merge_request(merge_request) # refresh the page
+
+        expect(page).to have_content 'The changes were merged'
+        expect(merge_request.reload).to be_merged
+      end
+    end
   end
 
-  context 'Build is not active' do
-    it "does not allow for enabling" do
+  context 'when build is not active' do
+    it "does not allow to enable merge when build succeeds" do
       visit_merge_request(merge_request)
       expect(page).not_to have_link "Merge When Build Succeeds"
     end

spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb

@@ -58,57 +58,67 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
   end
 
   describe "#trigger" do
-    context 'build with ref' do
-      let(:build)     { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") }
+    let(:merge_request_ref) { mr_merge_if_green_enabled.source_branch }
+    let(:merge_request_head) do
+      project.commit(mr_merge_if_green_enabled.source_branch).id
+    end
 
-      it "merges all merge requests with merge when build succeeds enabled" do
-        allow_any_instance_of(MergeRequest).to receive(:pipeline).and_return(pipeline)
-        allow(pipeline).to receive(:success?).and_return(true)
+    context 'when triggered by pipeline with valid ref and sha' do
+      let(:triggering_pipeline) do
+        create(:ci_pipeline, project: project, ref: merge_request_ref,
+                             sha: merge_request_head, status: 'success')
+      end
 
+      it "merges all merge requests with merge when build succeeds enabled" do
         expect(MergeWorker).to receive(:perform_async)
-        service.trigger(build)
+        service.trigger(triggering_pipeline)
       end
     end
 
-    context 'triggered by an old build' do
-      let(:old_build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") }
-      let(:build)     { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") }
-
-      it "merges all merge requests with merge when build succeeds enabled" do
-        allow_any_instance_of(MergeRequest).to receive(:pipeline).and_return(pipeline)
-        allow(pipeline).to receive(:success?).and_return(true)
-        allow(old_build).to receive(:sha).and_return('1234abcdef')
+    context 'when triggered by an old pipeline' do
+      let(:old_pipeline) do
+        create(:ci_pipeline, project: project, ref: merge_request_ref,
+                             sha: '1234abcdef', status: 'success')
+      end
 
+      it 'it does not merge merge request' do
         expect(MergeWorker).not_to receive(:perform_async)
-        service.trigger(old_build)
+        service.trigger(old_pipeline)
       end
     end
 
-    context 'commit status without ref' do
-      let(:commit_status) { create(:generic_commit_status, status: 'success') }
-
-      before { mr_merge_if_green_enabled }
-
-      it "doesn't merge a requests for status on other branch" do
-        allow(project.repository).to receive(:branch_names_contains).with(commit_status.sha).and_return([])
+    context 'when triggered by pipeline from a different branch' do
+      let(:unrelated_pipeline) do
+        create(:ci_pipeline, project: project, ref: 'feature',
+                             sha: merge_request_head, status: 'success')
+      end
 
+      it 'does not merge request' do
         expect(MergeWorker).not_to receive(:perform_async)
-        service.trigger(commit_status)
+        service.trigger(unrelated_pipeline)
       end
+    end
+  end
 
-      it 'discovers branches and merges all merge requests when status is success' do
-        allow(project.repository).to receive(:branch_names_contains).
-          with(commit_status.sha).and_return([mr_merge_if_green_enabled.source_branch])
-        allow(pipeline).to receive(:success?).and_return(true)
-        allow_any_instance_of(MergeRequest).to receive(:pipeline).and_return(pipeline)
-        allow(pipeline).to receive(:success?).and_return(true)
+  describe "#cancel" do
+    before do
+      service.cancel(mr_merge_if_green_enabled)
+    end
 
-        expect(MergeWorker).to receive(:perform_async)
-        service.trigger(commit_status)
-      end
+    it "resets all the merge_when_build_succeeds params" do
+      expect(mr_merge_if_green_enabled.merge_when_build_succeeds).to be_falsey
+      expect(mr_merge_if_green_enabled.merge_params).to eq({})
+      expect(mr_merge_if_green_enabled.merge_user).to be nil
+    end
+
+    it 'Posts a system note' do
+      note = mr_merge_if_green_enabled.notes.last
+      expect(note.note).to include 'Canceled the automatic merge'
     end
+  end
 
-    context 'properly handles multiple stages' do
+  describe 'pipeline integration' do
+    context 'when there are multiple stages in the pipeline' do
       let(:ref) { mr_merge_if_green_enabled.source_branch }
       let(:sha) { project.commit(ref).id }
 
@@ -148,21 +158,4 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
       end
     end
   end
-
-  describe "#cancel" do
-    before do
-      service.cancel(mr_merge_if_green_enabled)
-    end
-
-    it "resets all the merge_when_build_succeeds params" do
-      expect(mr_merge_if_green_enabled.merge_when_build_succeeds).to be_falsey
-      expect(mr_merge_if_green_enabled.merge_params).to eq({})
-      expect(mr_merge_if_green_enabled.merge_user).to be nil
-    end
-
-    it 'Posts a system note' do
-      note = mr_merge_if_green_enabled.notes.last
-      expect(note.note).to include 'Canceled the automatic merge'
-    end
-  end
 end

spec/workers/process_pipeline_worker_spec.rb → spec/workers/pipeline_proccess_worker_spec.rb

 require 'spec_helper'
 
-describe ProcessPipelineWorker do
+describe PipelineProcessWorker do
   describe '#perform' do
     context 'when pipeline exists' do
       let(:pipeline) { create(:ci_pipeline) }

spec/workers/pipeline_success_worker_spec.rb

+require 'spec_helper'
+
+describe PipelineSuccessWorker do
+  describe '#perform' do
+    context 'when pipeline exists' do
+      let(:pipeline) { create(:ci_pipeline, status: 'success') }
+
+      it 'performs "merge when pipeline succeeds"' do
+        expect_any_instance_of(
+          MergeRequests::MergeWhenBuildSucceedsService
+        ).to receive(:trigger)
+
+        described_class.new.perform(pipeline.id)
+      end
+    end
+
+    context 'when pipeline does not exist' do
+      it 'does not raise exception' do
+        expect { described_class.new.perform(123) }
+          .not_to raise_error
+      end
+    end
+  end
+end

spec/workers/update_pipeline_worker_spec.rb → spec/workers/pipeline_update_worker_spec.rb

 require 'spec_helper'
 
-describe UpdatePipelineWorker do
+describe PipelineUpdateWorker do
   describe '#perform' do
     context 'when pipeline exists' do
       let(:pipeline) { create(:ci_pipeline) }