From 792466ec6e67ec18abb42a3e648becec4da25e1f Mon Sep 17 00:00:00 2001 From: Rui Anderson Date: Wed, 27 Apr 2016 15:34:42 -0300 Subject: [PATCH] Allow or not merge MR with failed build --- CHANGELOG | 1 + app/assets/javascripts/project_new.js.coffee | 19 +-- app/controllers/projects_controller.rb | 2 +- app/models/merge_request.rb | 6 +- .../_merge_request_settings.html.haml | 11 ++ app/views/projects/edit.html.haml | 1 + .../merge_requests/widget/_open.html.haml | 2 + .../widget/open/_accept.html.haml | 27 ++-- .../widget/open/_build_failed.html.haml | 7 ++ ...low_merge_if_build_succeeds_to_projects.rb | 5 + db/schema.rb | 1 + lib/api/merge_requests.rb | 2 +- .../only_allow_merge_if_build_succeeds.rb | 105 ++++++++++++++++ spec/models/merge_request_spec.rb | 117 ++++++++++++++++++ spec/requests/api/merge_requests_spec.rb | 8 ++ 15 files changed, 291 insertions(+), 23 deletions(-) create mode 100644 app/views/projects/_merge_request_settings.html.haml create mode 100644 app/views/projects/merge_requests/widget/open/_build_failed.html.haml create mode 100644 db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb create mode 100644 spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb diff --git a/CHANGELOG b/CHANGELOG index 2d1c561fb825..53a63f0dfc8c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -23,6 +23,7 @@ v 8.7.0 - All service classes (those residing in app/services) are now instrumented - Developers can now add custom tags to transactions - Loading of an issue's referenced merge requests and related branches is now done asynchronously + - Add option to project to only allow merge requests to be merged if the build succeeds (Rui Santos) - Enable gzip for assets, makes the page size significantly smaller. !3544 / !3632 (Connor Shea) - Add support to cherry-pick any commit into any branch in the web interface (Minqi Pan) - Project switcher uses new dropdown styling diff --git a/app/assets/javascripts/project_new.js.coffee b/app/assets/javascripts/project_new.js.coffee index 63dee4ed5d79..e48343a19b54 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 3768efe142aa..0ac6391213b5 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -238,7 +238,7 @@ def project_params :issues_enabled, :merge_requests_enabled, :snippets_enabled, :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 d00919c3b0c1..93da9c82c32c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -285,7 +285,7 @@ def wipless_title 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 @@ -498,6 +498,10 @@ def can_be_merged_by?(user) ::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 000000000000..36d7a5ecb8e3 --- /dev/null +++ b/app/views/projects/_merge_request_settings.html.haml @@ -0,0 +1,11 @@ +%fieldset.merge-requests-feature + %legend + Merge Requests: + + .form-group + .col-sm-offset-2.col-sm-10 + .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 + diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 76a4f41193cc..e87ded1561dc 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -84,6 +84,7 @@ %br %span.descr Share code pastes with others out of git repository + = render 'merge_request_settings', f: f = render 'builds_settings', f: f %fieldset.features diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 55dbae598d3c..b2eea8f47bd1 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 807833741afc..fd218feb6bc9 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -9,19 +9,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 000000000000..57fcf05ed806 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_build_failed.html.haml @@ -0,0 +1,7 @@ +%h4 + = icon("exclamation-triangle") + The build for this merge request failed + +%p + Please retry the build or push code 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 000000000000..abcf7424bead --- /dev/null +++ b/db/migrate/20160419122101_add_only_allow_merge_if_build_succeeds_to_projects.rb @@ -0,0 +1,5 @@ +class AddOnlyAllowMergeIfBuildSucceedsToProjects < ActiveRecord::Migration + def change + add_column :projects, :only_allow_merge_if_build_succeeds, :boolean, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 42457d923534..c375aa4501e8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -760,6 +760,7 @@ t.integer "pushes_since_gc", default: 0 t.boolean "last_repository_check_failed" t.datetime "last_repository_check_at" + 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 7e78609ecb98..3eea7592c8df 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -227,7 +227,7 @@ def handle_merge_request_errors!(errors) # 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 000000000000..1627aa7287a1 --- /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 d7884cea336f..9fa2b9ac46db 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -424,4 +424,121 @@ end 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 isn't 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_commit) } + + 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 1fa7e76894fb..c75b374e4d45 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -392,6 +392,14 @@ 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] -- GitLab