Allow or not merge MR with failed build

Signed-off-by: Rémy Coutable <remy@rymai.me>

3 files changed, 230 insertions, 0 deletions

diff --git a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb b/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb
new file mode 100644
index 00000000000..1627aa7287a
--- /dev/null
+++ b/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb
@@ -0,0 +1,105 @@
+require 'spec_helper'
+
+feature 'Only allow merge requests to be merged if the build succeeds', feature: true, js: true do
+  let(:user) { create(:user) }
+
+  let(:project)       { create(:project, :public) }
+  let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user) }
+
+  before do
+    login_as user
+
+    project.team << [user, :master]
+  end
+
+  context "project hasn't ci enabled" do
+    it "allows MR to be merged" do
+      visit_merge_request(merge_request)
+      expect(page).to have_button "Accept Merge Request"
+    end
+  end
+
+  context "when project has ci enabled" do
+    let!(:ci_commit) { create(:ci_commit, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
+    let!(:ci_build) { create(:ci_build, commit: ci_commit) }
+
+    before do
+      project.enable_ci
+    end
+
+    context "when merge requests can only be merged if the build succeeds" do
+      before do
+        project.update_attribute(:only_allow_merge_if_build_succeeds, true)
+      end
+
+      context "when ci is running" do
+        it "doesn't allow to merge immediately" do
+          ci_commit.statuses.update_all(status: :pending)
+          visit_merge_request(merge_request)
+
+          expect(page).to have_button "Merge When Build Succeeds"
+          expect(page).to_not have_button "Select Merge Moment"
+        end
+      end
+
+      context "when ci failed" do
+        it "doesn't allow MR to be merged" do
+          ci_commit.statuses.update_all(status: :failed)
+          visit_merge_request(merge_request)
+
+          expect(page).to_not have_button "Accept Merge Request"
+          expect(page).to have_content("Please retry the build or push code to fix the failure.")
+        end
+      end
+
+      context "when ci succeed" do
+        it "allows MR to be merged" do
+          ci_commit.statuses.update_all(status: :success)
+          visit_merge_request(merge_request)
+
+          expect(page).to have_button "Accept Merge Request"
+        end
+      end
+    end
+
+    context "when merge requests can be merged when the build failed" do
+      before do
+        project.update_attribute(:only_allow_merge_if_build_succeeds, false)
+      end
+
+      context "when ci is running" do
+        it "allows MR to be merged immediately" do
+          ci_commit.statuses.update_all(status: :pending)
+          visit_merge_request(merge_request)
+
+          expect(page).to have_button "Merge When Build Succeeds"
+
+          click_button "Select Merge Moment"
+          expect(page).to have_content "Merge Immediately"
+        end
+      end
+
+      context "when ci failed" do
+        it "allows MR to be merged" do
+          ci_commit.statuses.update_all(status: :failed)
+          visit_merge_request(merge_request)
+
+          expect(page).to have_button "Accept Merge Request"
+        end
+      end
+
+      context "when ci succeed" do
+        it "allows MR to be merged" do
+          ci_commit.statuses.update_all(status: :success)
+          visit_merge_request(merge_request)
+
+          expect(page).to have_button "Accept Merge Request"
+        end
+      end
+    end
+  end
+
+  def visit_merge_request(merge_request)
+    visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request)
+  end
+end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 1b7cbc3efda..76912eed834 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -455,4 +455,121 @@ describe MergeRequest, models: true do
       expect(user2.assigned_open_merge_request_count).to eq(1)
     end
   end
+
+  describe '#check_if_can_be_merged' do
+    let(:project) { create(:project, only_allow_merge_if_build_succeeds: true) }
+
+    subject { create(:merge_request, source_project: project, merge_status: :unchecked) }
+
+    context 'when it is not broken and has no conflicts' do
+      it 'is marked as mergeable' do
+        allow(subject).to receive(:broken?) { false }
+        allow(project).to receive_message_chain(:repository, :can_be_merged?) { true }
+
+        expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
+      end
+    end
+
+    context 'when broken' do
+      before { allow(subject).to receive(:broken?) { true } }
+
+      it 'becomes unmergeable' do
+        expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
+      end
+    end
+
+    context 'when it has conflicts' do
+      before do
+        allow(subject).to receive(:broken?) { false }
+        allow(project).to receive_message_chain(:repository, :can_be_merged?) { false }
+      end
+
+      it 'becomes unmergeable' do
+        expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
+      end
+    end
+  end
+
+  describe '#mergeable?' do
+    let(:project) { create(:project, only_allow_merge_if_build_succeeds: true) }
+
+    subject { create(:merge_request, source_project: project) }
+
+    it "checks if merge request can be merged" do
+      allow(subject).to receive(:cannot_be_merged_because_build_failed?) { false }
+      expect(subject).to receive(:check_if_can_be_merged)
+
+      subject.mergeable?
+    end
+
+    context 'when not open' do
+      before { subject.close }
+
+      it 'returns false' do
+        expect(subject.mergeable?).to be_falsey
+      end
+    end
+
+    context 'when working in progress' do
+      before { subject.title = 'WIP MR' }
+
+      it 'returns false' do
+        expect(subject.mergeable?).to be_falsey
+      end
+    end
+
+    context 'when broken' do
+      before { allow(subject).to receive(:broken?) { true } }
+
+      it 'returns false' do
+        expect(subject.mergeable?).to be_falsey
+      end
+    end
+
+    context 'when failed' do
+      before { allow(subject).to receive(:broken?) { false } }
+
+      context "when project settings restrict to merge only if build succeeds" do
+        before { allow(subject).to receive(:cannot_be_merged_because_build_failed?) { true } }
+        it 'returns false if project settings restrict to merge only if build succeeds' do
+          expect(subject.mergeable?).to be_falsey
+        end
+      end
+    end
+  end
+
+  describe '#cannot_be_merged_because_build_failed?' do
+    let(:project) { create(:empty_project, only_allow_merge_if_build_succeeds: true) }
+    let(:commit_status) { create(:commit_status, status: 'failed', project: project) }
+    let(:ci_commit) { create(:ci_empty_pipeline) }
+
+    subject { build(:merge_request, target_project: project) }
+
+    before do
+      ci_commit.statuses << commit_status
+      allow(subject).to receive(:ci_commit) { ci_commit }
+    end
+
+    it "returns true if it's only allowed to merge green build and build has been failed" do
+      expect(subject.cannot_be_merged_because_build_failed?).to be_truthy
+    end
+
+    context 'when no ci_commit is associated' do
+      before do
+        allow(subject).to receive(:ci_commit) { nil }
+      end
+
+      it 'returns false' do
+        expect(subject.cannot_be_merged_because_build_failed?).to be_falsey
+      end
+    end
+
+    context "when isn't only allowed to merge green build at project settings" do
+      subject { build(:merge_request, target_project: build(:empty_project, only_allow_merge_if_build_succeeds: false)) }
+
+      it 'returns false' do
+        expect(subject.cannot_be_merged_because_build_failed?).to be_falsey
+      end
+    end
+  end
 end
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 9da69a913a8..91c25a0948f 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -419,6 +419,14 @@ describe API::API, api: true  do
       expect(json_response['message']).to eq('405 Method Not Allowed')
     end
 
+    it "should return 405 if merge_request build is failed it's restrict to merge only when susccess" do
+      allow_any_instance_of(MergeRequest).to receive(:cannot_be_merged_because_build_failed?).and_return(true)
+
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
+      expect(response.status).to eq(405)
+      expect(json_response['message']).to eq('405 Method Not Allowed')
+    end
+
     it "should return 401 if user has no permissions to merge" do
       user2 = create(:user)
       project.team << [user2, :reporter]