diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/assets/javascripts/project_new.js.coffee | 19 | ||||
| -rw-r--r-- | app/controllers/projects_controller.rb | 2 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 6 | ||||
| -rw-r--r-- | app/views/projects/_merge_request_settings.html.haml | 11 | ||||
| -rw-r--r-- | app/views/projects/edit.html.haml | 2 | ||||
| -rw-r--r-- | app/views/projects/merge_requests/widget/_open.html.haml | 2 | ||||
| -rw-r--r-- | app/views/projects/merge_requests/widget/open/_accept.html.haml | 27 | ||||
| -rw-r--r-- | app/views/projects/merge_requests/widget/open/_build_failed.html.haml | 6 | ||||
| -rw-r--r-- | db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb | 15 | ||||
| -rw-r--r-- | db/schema.rb | 1 | ||||
| -rw-r--r-- | lib/api/merge_requests.rb | 2 | ||||
| -rw-r--r-- | spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb | 105 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 117 | ||||
| -rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 8 |
15 files changed, 301 insertions, 23 deletions
diff --git a/CHANGELOG b/CHANGELOG index b00c149a753..176fc16943f 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/assets/javascripts/project_new.js.coffee b/app/assets/javascripts/project_new.js.coffee index 63dee4ed5d7..e48343a19b5 100644 --- a/app/assets/javascripts/project_new.js.coffee +++ b/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() diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 3af62c7696c..a6479c42d94 100644 --- a/app/controllers/projects_controller.rb +++ b/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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b0ed8182855..43c6bcb8715 100644 --- a/app/models/merge_request.rb +++ b/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" diff --git a/app/views/projects/_merge_request_settings.html.haml b/app/views/projects/_merge_request_settings.html.haml new file mode 100644 index 00000000000..da522b53417 --- /dev/null +++ b/app/views/projects/_merge_request_settings.html.haml @@ -0,0 +1,11 @@ +%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') diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 18b125ff9d4..8449fe1e4e0 100644 --- a/app/views/projects/edit.html.haml +++ b/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 diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 13359abede7..8de587009e1 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/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' diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml index 60d7d6ff1f5..941513febbd 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/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 diff --git a/app/views/projects/merge_requests/widget/open/_build_failed.html.haml b/app/views/projects/merge_requests/widget/open/_build_failed.html.haml new file mode 100644 index 00000000000..14f51af5360 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_build_failed.html.haml @@ -0,0 +1,6 @@ +%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. diff --git a/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb b/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb new file mode 100644 index 00000000000..69d64ccd006 --- /dev/null +++ b/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb @@ -0,0 +1,15 @@ +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 diff --git a/db/schema.rb b/db/schema.rb index b7adf48fdb4..03070f0d593 100644 --- a/db/schema.rb +++ b/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 diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 43221d5622a..5822e19cd44 100644 --- a/lib/api/merge_requests.rb +++ b/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 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] |