Fix Merge When Succeeded for multiple stages

Use around_transition to trigger build creation for next stages

4 files changed, 46 insertions, 10 deletions

diff --git a/CHANGELOG b/CHANGELOG
index b4578592600..7911e8e48c6 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -52,6 +52,7 @@ v 8.5.0 (unreleased)
   - Improve UI consistency between projects and groups lists
   - Add sort dropdown to dashboard projects page
   - Fixed logo animation on Safari (Roman Rott)
+  - Fix Merge When Succeeded when multiple stages
   - Hide remove source branch button when the MR is merged but new commits are pushed (Zeger-Jan van de Weg)
   - In seach autocomplete show only groups and projects you are member of
   - Don't process cross-reference notes from forks
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 5d800cd1f85..1227458e525 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -107,15 +107,18 @@ module Ci
     end
 
     state_machine :status, initial: :pending do
-      after_transition pending: :running do |build, transition|
+      after_transition pending: :running do |build|
         build.execute_hooks
       end
 
-      after_transition any => [:success, :failed, :canceled] do |build, transition|
-        return unless build.project
+      # We use around_transition to create builds for next stage as soon as possible, before the `after_*` is executed
+      around_transition any => [:success, :failed, :canceled] do |build, block|
+        block.call
+        build.commit.create_next_builds(build) if build.commit
+      end
 
+      after_transition any => [:success, :failed, :canceled] do |build|
         build.update_coverage
-        build.commit.create_next_builds(build)
         build.execute_hooks
       end
     end
@@ -179,6 +182,7 @@ module Ci
     end
 
     def update_coverage
+      return unless project
       coverage_regex = project.build_coverage_regex
       return unless coverage_regex
       coverage = extract_coverage(trace, coverage_regex)
@@ -334,6 +338,7 @@ module Ci
     end
 
     def execute_hooks
+      return unless project
       build_data = Gitlab::BuildDataBuilder.build(self)
       project.execute_hooks(build_data.dup, :build_hooks)
       project.execute_services(build_data.dup, :build_hooks)
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 434b3560d09..7ef50836322 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -75,16 +75,16 @@ class CommitStatus < ActiveRecord::Base
       transition [:pending, :running] => :canceled
     end
 
-    after_transition pending: :running do |build, transition|
-      build.update_attributes started_at: Time.now
+    after_transition pending: :running do |commit_status|
+      commit_status.update_attributes started_at: Time.now
     end
 
-    after_transition any => [:success, :failed, :canceled] do |build, transition|
-      build.update_attributes finished_at: Time.now
+    after_transition any => [:success, :failed, :canceled] do |commit_status|
+      commit_status.update_attributes finished_at: Time.now
     end
 
-    after_transition [:pending, :running] => :success do |build, transition|
-      MergeRequests::MergeWhenBuildSucceedsService.new(build.commit.project, nil).trigger(build)
+    after_transition [:pending, :running] => :success do |commit_status|
+      MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.commit.project, nil).trigger(commit_status)
     end
 
     state :pending, value: 'pending'
diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
index de9fed2b7dd..160c7786bf2 100644
--- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
+++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
@@ -63,6 +63,36 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
       expect(MergeWorker).to receive(:perform_async)
       service.trigger(build)
     end
+
+    context 'properly handles multiple stages' do
+      let(:ref) { mr_merge_if_green_enabled.source_branch }
+      let(:build) { create(:ci_build, commit: ci_commit, ref: ref, name: 'build', stage: 'build') }
+      let(:test) { create(:ci_build, commit: ci_commit, ref: ref, name: 'test', stage: 'test') }
+
+      before do
+        # This behavior of MergeRequest: we instantiate a new object
+        allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_wrap_original do
+          Ci::Commit.find(ci_commit.id)
+        end
+
+        # We create test after the build
+        allow(ci_commit).to receive(:create_next_builds).and_wrap_original do
+          test
+        end
+      end
+
+      it "doesn't merge if some stages failed" do
+        expect(MergeWorker).to_not receive(:perform_async)
+        build.success
+        test.drop
+      end
+
+      it 'merge when all stages succeeded' do
+        expect(MergeWorker).to receive(:perform_async)
+        build.success
+        test.success
+      end
+    end
   end
 
   describe "#cancel" do