Allow or not merge MR with failed build

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

CHANGELOG

@@ -31,6 +31,7 @@ v 8.9.0 (unreleased)
   - Add rake task 'gitlab:db:configure' for conditionally seeding or migrating the database
   - Changed the Slack build message to use the singular duration if necessary (Aran Koning)
   - Links from a wiki page to other wiki pages should be rewritten as expected
+  - Add option to project to only allow merge requests to be merged if the build succeeds (Rui Santos)
   - Fix issues filter when ordering by milestone
   - Todos will display target state if issuable target is 'Closed' or 'Merged'
   - Fix bug when sorting issues by milestone due date and filtering by two or more labels

app/assets/javascripts/project_new.js.coffee

@@ -7,12 +7,17 @@ class @ProjectNew
     @toggleSettingsOnclick()
 
 
-  toggleSettings: ->
-    checked = $("#project_builds_enabled").prop("checked")
-    if checked
-      $('.builds-feature').show()
-    else
-      $('.builds-feature').hide()
+  toggleSettings: =>
+    @_showOrHide('#project_builds_enabled', '.builds-feature')
+    @_showOrHide('#project_merge_requests_enabled', '.merge-requests-feature')
 
   toggleSettingsOnclick: ->
-    $("#project_builds_enabled").on 'click', @toggleSettings
+    $('#project_builds_enabled, #project_merge_requests_enabled').on 'click', @toggleSettings
+
+  _showOrHide: (checkElement, container) ->
+    $container = $(container)
+
+    if $(checkElement).prop('checked')
+      $container.show()
+    else
+      $container.hide()

app/controllers/projects_controller.rb

@@ -234,7 +234,7 @@ class ProjectsController < Projects::ApplicationController
       :issues_tracker_id, :default_branch,
       :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar,
       :builds_enabled, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex,
-      :public_builds,
+      :public_builds, :only_allow_merge_if_build_succeeds
     )
   end
 

app/models/merge_request.rb

@@ -260,7 +260,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def mergeable?
-    return false unless open? && !work_in_progress? && !broken?
+    return false if !open? || work_in_progress? || broken?  || cannot_be_merged_because_build_failed?
 
     check_if_can_be_merged
 
@@ -481,6 +481,10 @@ class MergeRequest < ActiveRecord::Base
     ::Gitlab::GitAccess.new(user, project).can_push_to_branch?(target_branch)
   end
 
+  def cannot_be_merged_because_build_failed?
+    project.only_allow_merge_if_build_succeeds? && ci_commit && ci_commit.failed?
+  end
+
   def state_human_name
     if merged?
       "Merged"

app/views/projects/_merge_request_settings.html.haml

+%fieldset.builds-feature
+  %h5.prepend-top-0
+    Merge Requests
+  .form-group
+    .checkbox
+      = f.label :only_allow_merge_if_build_succeeds do
+        = f.check_box :only_allow_merge_if_build_succeeds
+        %strong Only allow merge requests to be merged if the build succeeds
+      .help-block
+        Builds need to be configured to enable this feature.
+        = link_to icon('question-circle'), help_page_path('workflow', 'merge_requests#only-allow-merge-requests-to-be-merged-if-the-build-succeeds')

app/views/projects/edit.html.haml

@@ -84,6 +84,8 @@
                   %br
                   %span.descr Enable Container Registry for this repository
         %hr
+        = render 'merge_request_settings', f: f
+        %hr
         = render 'builds_settings', f: f
         %hr
         %fieldset.features.append-bottom-default

app/views/projects/merge_requests/widget/_open.html.haml

@@ -17,6 +17,8 @@
       = render 'projects/merge_requests/widget/open/merge_when_build_succeeds'
     - elsif !@merge_request.can_be_merged_by?(current_user)
       = render 'projects/merge_requests/widget/open/not_allowed'
+    - elsif @merge_request.cannot_be_merged_because_build_failed?
+      = render 'projects/merge_requests/widget/open/build_failed'
     - elsif @merge_request.can_be_merged?
       = render 'projects/merge_requests/widget/open/accept'
 

app/views/projects/merge_requests/widget/open/_accept.html.haml

@@ -10,6 +10,7 @@
           %span.btn-group
             = button_tag class: "btn btn-create js-merge-button merge_when_build_succeeds" do
               Merge When Build Succeeds
+            - unless @project.only_allow_merge_if_build_succeeds?
               = button_tag class: "btn btn-success dropdown-toggle", 'data-toggle' => 'dropdown' do
                 %span.caret
                 %span.sr-only

app/views/projects/merge_requests/widget/open/_build_failed.html.haml

+%h4
+  = icon('exclamation-triangle')
+  The build for this merge request failed
+
+%p
+  Please retry the build or push a new commit to fix the failure.

db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb

+class AddOnlyAllowMergeIfBuildSucceedsToProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:projects,
+                            :only_allow_merge_if_build_succeeds,
+                            :boolean,
+                            default: false)
+  end
+
+  def down
+    remove_column(:projects, :only_allow_merge_if_build_succeeds)
+  end
+end

db/schema.rb

@@ -779,6 +779,7 @@ ActiveRecord::Schema.define(version: 20160608155312) do
     t.boolean  "last_repository_check_failed"
     t.datetime "last_repository_check_at"
     t.boolean  "container_registry_enabled"
+    t.boolean  "only_allow_merge_if_build_succeeds", default: false
   end
 
   add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree

lib/api/merge_requests.rb

@@ -228,7 +228,7 @@ module API
           # Merge request can not be merged
           # because user dont have permissions to push into target branch
           unauthorized! unless merge_request.can_be_merged_by?(current_user)
-          not_allowed! if !merge_request.open? || merge_request.work_in_progress?
+          not_allowed! if !merge_request.open? || merge_request.work_in_progress? || merge_request.cannot_be_merged_because_build_failed?
 
           merge_request.check_if_can_be_merged
 

spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb

+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

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

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]