Merge branch 'ruianderson/gitlab-ce-option-to-allow-or-not-merge-failed-builds' into 'master'

Add option to restrict merge MR with failed build

_Originally opened at !3828 by @ruianderson._

-----

## What does this MR do?

This MR adds an option to prevent MR from being merged if their build status is not a success. Please note that if the MR has no `ci_commit`, the MR can be merged (i.e. we don't enforce builds to be configured).

## Are there points in the code the reviewer needs to double check?

Probably the copy in the edit project's page and in the documentation.

## What are the relevant issue numbers?

Closes #5940.

## Screenshots

![only_allow_merge_if_build_succeeds](/uploads/bb43cf131f680c9af0eb2ea5155189e0/only_allow_merge_if_build_succeeds.png)

See merge request !4503

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,13 +260,22 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def mergeable?
-    return false unless open? && !work_in_progress? && !broken?
+    return false unless mergeable_state?
 
     check_if_can_be_merged
 
     can_be_merged?
   end
 
+  def mergeable_state?
+    return false unless open?
+    return false if work_in_progress?
+    return false if broken?
+    return false unless mergeable_ci_state?
+
+    true
+  end
+
   def gitlab_merge_status
     if work_in_progress?
       "work_in_progress"
@@ -481,6 +490,12 @@ class MergeRequest < ActiveRecord::Base
     ::Gitlab::GitAccess.new(user, project).can_push_to_branch?(target_branch)
   end
 
+  def mergeable_ci_state?
+    return true unless project.only_allow_merge_if_build_succeeds?
+
+    !pipeline || pipeline.success?
+  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.mergeable_ci_state? && @pipeline && @pipeline.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,19 +10,20 @@
           %span.btn-group
             = button_tag class: "btn btn-create js-merge-button merge_when_build_succeeds" do
               Merge When Build Succeeds
-            = button_tag class: "btn btn-success dropdown-toggle", 'data-toggle' => 'dropdown' do
-              %span.caret
-              %span.sr-only
-                Select Merge Moment
-            %ul.js-merge-dropdown.dropdown-menu.dropdown-menu-right{ role: 'menu' }
-              %li
-                = link_to "#", class: "merge_when_build_succeeds" do
-                  = icon('check fw')
-                  Merge When Build Succeeds
-              %li
-                = link_to "#", class: "accept_merge_request" do
-                  = icon('warning fw')
-                  Merge Immediately
+            - 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
+                  Select Merge Moment
+              %ul.js-merge-dropdown.dropdown-menu.dropdown-menu-right{ role: 'menu' }
+                %li
+                  = link_to "#", class: "merge_when_build_succeeds" do
+                    = icon('check fw')
+                    Merge When Build Succeeds
+                %li
+                  = link_to "#", class: "accept_merge_request" do
+                    = icon('warning fw')
+                    Merge Immediately
         - else
           = f.button class: "btn btn-create btn-grouped js-merge-button accept_merge_request #{status_class}" do
             Accept Merge Request

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

@@ -747,38 +747,39 @@ ActiveRecord::Schema.define(version: 20160608155312) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer  "creator_id"
-    t.boolean  "issues_enabled",               default: true,     null: false
-    t.boolean  "merge_requests_enabled",       default: true,     null: false
-    t.boolean  "wiki_enabled",                 default: true,     null: false
+    t.boolean  "issues_enabled",                     default: true,     null: false
+    t.boolean  "merge_requests_enabled",             default: true,     null: false
+    t.boolean  "wiki_enabled",                       default: true,     null: false
     t.integer  "namespace_id"
-    t.string   "issues_tracker",               default: "gitlab", null: false
+    t.string   "issues_tracker",                     default: "gitlab", null: false
     t.string   "issues_tracker_id"
-    t.boolean  "snippets_enabled",             default: true,     null: false
+    t.boolean  "snippets_enabled",                   default: true,     null: false
     t.datetime "last_activity_at"
     t.string   "import_url"
-    t.integer  "visibility_level",             default: 0,        null: false
-    t.boolean  "archived",                     default: false,    null: false
+    t.integer  "visibility_level",                   default: 0,        null: false
+    t.boolean  "archived",                           default: false,    null: false
     t.string   "avatar"
     t.string   "import_status"
-    t.float    "repository_size",              default: 0.0
-    t.integer  "star_count",                   default: 0,        null: false
+    t.float    "repository_size",                    default: 0.0
+    t.integer  "star_count",                         default: 0,        null: false
     t.string   "import_type"
     t.string   "import_source"
-    t.integer  "commit_count",                 default: 0
+    t.integer  "commit_count",                       default: 0
     t.text     "import_error"
     t.integer  "ci_id"
-    t.boolean  "builds_enabled",               default: true,     null: false
-    t.boolean  "shared_runners_enabled",       default: true,     null: false
+    t.boolean  "builds_enabled",                     default: true,     null: false
+    t.boolean  "shared_runners_enabled",             default: true,     null: false
     t.string   "runners_token"
     t.string   "build_coverage_regex"
-    t.boolean  "build_allow_git_fetch",        default: true,     null: false
-    t.integer  "build_timeout",                default: 3600,     null: false
-    t.boolean  "pending_delete",               default: false
-    t.boolean  "public_builds",                default: true,     null: false
-    t.integer  "pushes_since_gc",              default: 0
+    t.boolean  "build_allow_git_fetch",              default: true,     null: false
+    t.integer  "build_timeout",                      default: 3600,     null: false
+    t.boolean  "pending_delete",                     default: false
+    t.boolean  "public_builds",                      default: true,     null: false
+    t.integer  "pushes_since_gc",                    default: 0
     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,    null: false
   end
 
   add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree

doc/workflow/merge_requests.md

@@ -2,6 +2,17 @@
 
 Merge requests allow you to exchange changes you made to source code
 
+## Only allow merge requests to be merged if the build succeeds
+
+You can prevent merge requests from being merged if their build did not succeed
+in the project settings page.
+
+![only_allow_merge_if_build_succeeds](merge_requests/only_allow_merge_if_build_succeeds.png)
+
+Navigate to project settings page and select the `Only allow merge requests to be merged if the build succeeds` check box.
+
+Please note that you need to have builds configured to enable this feature.
+
 ## Checkout merge requests locally
 
 Locate the section for your GitLab remote in the `.git/config` file. It looks like this:

lib/api/merge_requests.rb

@@ -228,11 +228,10 @@ 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?
 
-          merge_request.check_if_can_be_merged
+          not_allowed! unless merge_request.mergeable_state?
 
-          render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged?
+          render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?
 
           if params[:sha] && merge_request.source_sha != params[:sha]
             render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409)

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 do
+  let(:project)       { create(:project, :public) }
+  let(:merge_request) { create(:merge_request_with_diffs, source_project: project) }
+
+  before do
+    login_as merge_request.author
+
+    project.team << [merge_request.author, :master]
+  end
+
+  context 'project does not have 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(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
+
+    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
+        before { pipeline.update_column(:status, :running) }
+
+        it 'does not allow to merge immediately' do
+          visit_merge_request(merge_request)
+
+          expect(page).to have_button 'Merge When Build Succeeds'
+          expect(page).not_to have_button 'Select Merge Moment'
+        end
+      end
+
+      context 'when CI failed' do
+        before { pipeline.update_column(:status, :failed) }
+
+        it 'does not allow MR to be merged' do
+          visit_merge_request(merge_request)
+
+          expect(page).not_to have_button 'Accept Merge Request'
+          expect(page).to have_content('Please retry the build or push a new commit to fix the failure.')
+        end
+      end
+
+      context 'when CI succeeded' do
+        before { pipeline.update_column(:status, :success) }
+
+        it 'allows MR to be merged' do
+          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
+        before { pipeline.update_column(:status, :running) }
+
+        it 'allows MR to be merged immediately', js: true do
+          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
+        before { pipeline.update_column(:status, :failed) }
+
+        it 'allows MR to be merged' do
+          visit_merge_request(merge_request)
+
+          expect(page).to have_button 'Accept Merge Request'
+        end
+      end
+
+      context 'when CI succeeded' do
+        before { pipeline.update_column(:status, :success) }
+
+        it 'allows MR to be merged' do
+          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,157 @@ 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) }
+
+    subject { create(:merge_request, source_project: project) }
+
+    it 'returns false if #mergeable_state? is false' do
+      expect(subject).to receive(:mergeable_state?) { false }
+
+      expect(subject.mergeable?).to be_falsey
+    end
+
+    it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do
+      allow(subject).to receive(:mergeable_state?) { true }
+      expect(subject).to receive(:check_if_can_be_merged)
+      expect(subject).to receive(:can_be_merged?) { true }
+
+      expect(subject.mergeable?).to be_truthy
+    end
+  end
+
+  describe '#mergeable_state?' do
+    let(:project) { create(:project) }
+
+    subject { create(:merge_request, source_project: project) }
+
+    it 'checks if merge request can be merged' do
+      allow(subject).to receive(:mergeable_ci_state?) { true }
+      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_state?).to be_falsey
+      end
+    end
+
+    context 'when working in progress' do
+      before { subject.title = 'WIP MR' }
+
+      it 'returns false' do
+        expect(subject.mergeable_state?).to be_falsey
+      end
+    end
+
+    context 'when broken' do
+      before { allow(subject).to receive(:broken?) { true } }
+
+      it 'returns false' do
+        expect(subject.mergeable_state?).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 and build failed' do
+        before do
+          project.only_allow_merge_if_build_succeeds = true
+          allow(subject).to receive(:mergeable_ci_state?) { false }
+        end
+
+        it 'returns false' do
+          expect(subject.mergeable_state?).to be_falsey
+        end
+      end
+    end
+  end
+
+  describe '#mergeable_ci_state?' do
+    let(:project) { create(:empty_project, only_allow_merge_if_build_succeeds: true) }
+    let(:pipeline) { create(:ci_empty_pipeline) }
+
+    subject { build(:merge_request, target_project: project) }
+
+    context 'when it is only allowed to merge when build is green' do
+      context 'and a failed pipeline is associated' do
+        before do
+          pipeline.statuses << create(:commit_status, status: 'failed', project: project)
+          allow(subject).to receive(:pipeline) { pipeline }
+        end
+
+        it { expect(subject.mergeable_ci_state?).to be_falsey }
+      end
+
+      context 'when no pipeline is associated' do
+        before do
+          allow(subject).to receive(:pipeline) { nil }
+        end
+
+        it { expect(subject.mergeable_ci_state?).to be_truthy }
+      end
+    end
+
+    context 'when merges are not restricted to green builds' do
+      subject { build(:merge_request, target_project: build(:empty_project, only_allow_merge_if_build_succeeds: false)) }
+
+      context 'and a failed pipeline is associated' do
+        before do
+          pipeline.statuses << create(:commit_status, status: 'failed', project: project)
+          allow(subject).to receive(:pipeline) { pipeline }
+        end
+
+        it { expect(subject.mergeable_ci_state?).to be_truthy }
+      end
+
+      context 'when no pipeline is associated' do
+        before do
+          allow(subject).to receive(:pipeline) { nil }
+        end
+
+        it { expect(subject.mergeable_ci_state?).to be_truthy }
+      end
+    end
+  end
 end

spec/requests/api/merge_requests_spec.rb

@@ -419,6 +419,15 @@ describe API::API, api: true  do
       expect(json_response['message']).to eq('405 Method Not Allowed')
     end
 
+    it 'returns 405 if the build failed for a merge request that requires success' do
+      allow_any_instance_of(MergeRequest).to receive(:mergeable_ci_state?).and_return(false)
+
+      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]