From 1db9f826c16053e32a1d234bf40b2ca399779cdf Mon Sep 17 00:00:00 2001 From: Rodolfo Santos Date: Fri, 16 Sep 2016 08:02:42 -0300 Subject: [PATCH] Add setting to only allow merge requests to be merged when all discussions are resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/controllers/projects_controller.rb | 5 +- app/models/merge_request.rb | 7 +++ .../_merge_request_settings.html.haml | 4 ++ .../merge_requests/widget/_open.html.haml | 4 +- .../open/_unresolved_discussions.html.haml | 6 ++ ...setting-to-check-unresolved-discussion.yml | 4 ++ ...w_merge_if_all_discussions_are_resolved.rb | 17 ++++++ db/schema.rb | 1 + doc/api/projects.md | 10 ++++ lib/api/entities.rb | 1 + lib/api/projects.rb | 9 ++- .../merge_requests_controller_spec.rb | 26 +++++++++ spec/factories/merge_requests.rb | 5 ++ ...f_mergeable_with_unresolved_discussions.rb | 53 +++++++++++++++++ spec/models/merge_request_spec.rb | 58 ++++++++++++++++++- spec/requests/api/projects_spec.rb | 36 +++++++++++- 16 files changed, 238 insertions(+), 8 deletions(-) create mode 100644 app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml create mode 100644 changelogs/unreleased/20968-add-setting-to-check-unresolved-discussion.yml create mode 100644 db/migrate/20160914131004_only_allow_merge_if_all_discussions_are_resolved.rb create mode 100644 spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index bce5e29d8d8..ee72c8ba72f 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -334,8 +334,9 @@ class ProjectsController < Projects::ApplicationController :issues_tracker_id, :default_branch, :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex, - :public_builds, :only_allow_merge_if_build_succeeds, :request_access_enabled, - :lfs_enabled, project_feature_attributes + :public_builds, :only_allow_merge_if_build_succeeds, + :only_allow_merge_if_all_discussions_are_resolved, + :request_access_enabled, :lfs_enabled, project_feature_attributes ) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 6b8ac3fb48b..d76feb9680e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -425,6 +425,7 @@ class MergeRequest < ActiveRecord::Base return false if work_in_progress? return false if broken? return false unless skip_ci_check || mergeable_ci_state? + return false unless mergeable_discussions_state? true end @@ -493,6 +494,12 @@ class MergeRequest < ActiveRecord::Base discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?) end + def mergeable_discussions_state? + return true unless project.only_allow_merge_if_all_discussions_are_resolved? + + discussions_resolved? + end + def hook_attrs attrs = { source: source_project.try(:hook_attrs), diff --git a/app/views/projects/_merge_request_settings.html.haml b/app/views/projects/_merge_request_settings.html.haml index 80053dd501b..6e143c4b570 100644 --- a/app/views/projects/_merge_request_settings.html.haml +++ b/app/views/projects/_merge_request_settings.html.haml @@ -12,3 +12,7 @@ %span.descr Builds need to be configured to enable this feature. = link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_when_build_succeeds', anchor: 'only-allow-merge-requests-to-be-merged-if-the-build-succeeds') + .checkbox + = f.label :only_allow_merge_if_all_discussions_are_resolved do + = f.check_box :only_allow_merge_if_all_discussions_are_resolved + %strong Only allow merge requests to be merged if all discussions are resolved diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 842b6df310d..01314eb37d0 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -23,8 +23,10 @@ = 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? + - elsif !@merge_request.mergeable_ci_state? = render 'projects/merge_requests/widget/open/build_failed' + - elsif !@merge_request.mergeable_discussions_state? + = render 'projects/merge_requests/widget/open/unresolved_discussions' - elsif @merge_request.can_be_merged? || resolved_conflicts = render 'projects/merge_requests/widget/open/accept' diff --git a/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml b/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml new file mode 100644 index 00000000000..35d5677ee37 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml @@ -0,0 +1,6 @@ +%h4 + = icon('exclamation-triangle') + This merge request has unresolved discussions + +%p + Please resolve these discussions to allow this merge request to be merged. \ No newline at end of file diff --git a/changelogs/unreleased/20968-add-setting-to-check-unresolved-discussion.yml b/changelogs/unreleased/20968-add-setting-to-check-unresolved-discussion.yml new file mode 100644 index 00000000000..8f03746ff80 --- /dev/null +++ b/changelogs/unreleased/20968-add-setting-to-check-unresolved-discussion.yml @@ -0,0 +1,4 @@ +--- +title: Add setting to only allow merge requests to be merged when all discussions are resolved +merge_request: 7125 +author: Rodolfo Arruda diff --git a/db/migrate/20160914131004_only_allow_merge_if_all_discussions_are_resolved.rb b/db/migrate/20160914131004_only_allow_merge_if_all_discussions_are_resolved.rb new file mode 100644 index 00000000000..fad62d716b3 --- /dev/null +++ b/db/migrate/20160914131004_only_allow_merge_if_all_discussions_are_resolved.rb @@ -0,0 +1,17 @@ +class OnlyAllowMergeIfAllDiscussionsAreResolved < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + def up + add_column_with_default(:projects, + :only_allow_merge_if_all_discussions_are_resolved, + :boolean, + default: false) + end + + def down + remove_column(:projects, :only_allow_merge_if_all_discussions_are_resolved) + end +end diff --git a/db/schema.rb b/db/schema.rb index dc088925d97..5476b0c93e5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -905,6 +905,7 @@ ActiveRecord::Schema.define(version: 20161103171205) do t.boolean "has_external_wiki" t.boolean "lfs_enabled" t.text "description_html" + t.boolean "only_allow_merge_if_all_discussions_are_resolved", default: false, null: false end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree diff --git a/doc/api/projects.md b/doc/api/projects.md index 4f4b20a1874..bbb3bfb4995 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -89,6 +89,7 @@ Parameters: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false }, { @@ -151,6 +152,7 @@ Parameters: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ] @@ -429,6 +431,7 @@ Parameters: } ], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -602,6 +605,7 @@ Parameters: | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, builds can be viewed by non-project-members | | `only_allow_merge_if_build_succeeds` | boolean | no | Set whether merge requests can only be merged with successful builds | +| `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `lfs_enabled` | boolean | no | Enable LFS | | `request_access_enabled` | boolean | no | Allow users to request member access | @@ -634,6 +638,7 @@ Parameters: | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, builds can be viewed by non-project-members | | `only_allow_merge_if_build_succeeds` | boolean | no | Set whether merge requests can only be merged with successful builds | +| `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `lfs_enabled` | boolean | no | Enable LFS | | `request_access_enabled` | boolean | no | Allow users to request member access | @@ -665,6 +670,7 @@ Parameters: | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, builds can be viewed by non-project-members | | `only_allow_merge_if_build_succeeds` | boolean | no | Set whether merge requests can only be merged with successful builds | +| `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `lfs_enabled` | boolean | no | Enable LFS | | `request_access_enabled` | boolean | no | Allow users to request member access | @@ -752,6 +758,7 @@ Example response: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -820,6 +827,7 @@ Example response: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -908,6 +916,7 @@ Example response: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -996,6 +1005,7 @@ Example response: "public_builds": true, "shared_with_groups": [], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 1f378ba1635..01e31f6f7d1 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -100,6 +100,7 @@ module API end expose :only_allow_merge_if_build_succeeds expose :request_access_enabled + expose :only_allow_merge_if_all_discussions_are_resolved end class Member < UserBasic diff --git a/lib/api/projects.rb b/lib/api/projects.rb index da16e24d7ea..6b856128c2e 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -139,7 +139,8 @@ module API :shared_runners_enabled, :snippets_enabled, :visibility_level, - :wiki_enabled] + :wiki_enabled, + :only_allow_merge_if_all_discussions_are_resolved] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(current_user, attrs).execute if @project.saved? @@ -193,7 +194,8 @@ module API :shared_runners_enabled, :snippets_enabled, :visibility_level, - :wiki_enabled] + :wiki_enabled, + :only_allow_merge_if_all_discussions_are_resolved] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(user, attrs).execute if @project.saved? @@ -275,7 +277,8 @@ module API :shared_runners_enabled, :snippets_enabled, :visibility_level, - :wiki_enabled] + :wiki_enabled, + :only_allow_merge_if_all_discussions_are_resolved] attrs = map_public_to_visibility_level(attrs) authorize_admin_project authorize! :rename_project, user_project if attrs[:name].present? diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 940d54f8686..79820e9a435 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -297,6 +297,32 @@ describe Projects::MergeRequestsController do end end end + + context 'when project project has unresolved discussion' do + before do + project.update_column(:only_allow_merge_if_all_discussions_are_resolved, allowed) + end + + context "when the only_allow_merge_if_all_discussions_are_resolved? is true" do + let(:allowed) { true } + + it 'returns :failed' do + merge_with_sha + + expect(assigns(:status)).to eq(:failed) + end + end + + context "when the only_allow_merge_if_all_discussions_are_resolved? is false" do + let(:allowed) { false } + + it 'returns :failed' do + merge_with_sha + + expect(assigns(:status)).to eq(:success) + end + end + end end end diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index f780e01253c..37eb49c94df 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -68,6 +68,11 @@ FactoryGirl.define do factory :closed_merge_request, traits: [:closed] factory :reopened_merge_request, traits: [:reopened] factory :merge_request_with_diffs, traits: [:with_diffs] + factory :merge_request_with_diff_notes do + after(:create) do |mr| + create(:diff_note_on_merge_request, noteable: mr, project: mr.source_project) + end + end factory :labeled_merge_request do transient do diff --git a/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb new file mode 100644 index 00000000000..100ddda0167 --- /dev/null +++ b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +feature 'Check if mergeable with unresolved discussions', js: true, feature: true do + let!(:user) { create(:user) } + let!(:project) { create(:project, :public, only_allow_merge_if_all_discussions_are_resolved: allowed) } + let!(:merge_request) { create(:merge_request_with_diff_notes, source_project: project, author: user, title: "Bug NS-04" ) } + + before do + login_as user + project.team << [user, :master] + end + + context 'when only_allow_merge_if_all_discussions_are_resolved is false' do + let(:allowed) { false } + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + + context 'when only_allow_merge_if_all_discussions_are_resolved is true' do + let(:allowed) { true } + + context "when discussions are resolved" do + + before do + merge_request.discussions.each { |d| d.resolve!(user) } + end + + it 'allows MR to be merged' do + visit_merge_request(merge_request) + + expect(page).to have_button 'Accept Merge Request' + end + end + + context "when discussions are unresolved" do + + it 'does not allow to merge' do + visit_merge_request(merge_request) + + expect(page).not_to have_button 'Accept Merge Request' + expect(page).to have_content('This merge request has unresolved discussions') + 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 1067ff7bb4d..f3d0373e6d7 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -837,6 +837,17 @@ describe MergeRequest, models: true do expect(subject.mergeable_state?).to be_falsey end end + + context "when project settings restrict to merge only when all the discussions are resolved" do + before do + project.only_allow_merge_if_all_discussions_are_resolved = true + allow(subject).to receive(:mergeable_discussions_state?) { false } + end + + it 'returns false' do + expect(subject.mergeable_state?).to be_falsey + end + end end end @@ -887,7 +898,52 @@ describe MergeRequest, models: true do end end - describe '#environments' do + describe '#mergeable_discussions_state?' do + let!(:user) { create(:user) } + let!(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: allowed) } + + subject { create(:merge_request_with_diff_notes, source_project: project) } + + context 'when is true' do + let(:allowed) { true } + + context 'when discussions are resolved' do + before do + subject.discussions.each { |d| d.resolve!(user) } + end + + it 'returns true' do + expect(subject.mergeable_discussions_state?).to be_truthy + end + end + + context 'when discussions are unresolved' do + before do + subject.discussions.map(&:unresolve!) + end + + it 'returns false' do + expect(subject.mergeable_discussions_state?).to be_falsey + end + end + end + + context 'when is false' do + let(:allowed) { false } + + context 'when discussions are unresolved' do + before do + subject.discussions.map(&:unresolve!) + end + + it 'returns true' do + expect(subject.mergeable_discussions_state?).to be_truthy + end + end + end + end + + describe "#environments" do let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project) } diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 973928d007a..3c8f0ac531a 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -256,7 +256,8 @@ describe API::API, api: true do merge_requests_enabled: false, wiki_enabled: false, only_allow_merge_if_build_succeeds: false, - request_access_enabled: true + request_access_enabled: true, + only_allow_merge_if_all_discussions_are_resolved: false }) post api('/projects', user), project @@ -327,6 +328,22 @@ describe API::API, api: true do expect(json_response['only_allow_merge_if_build_succeeds']).to be_truthy end + it 'sets a project as allowing merge even if discussions are unresolved' do + project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: false }) + + post api('/projects', user), project + + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to be_falsey + end + + it 'sets a project as allowing merge only if all discussions are resolved' do + project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: true }) + + post api('/projects', user), project + + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to be_truthy + end + context 'when a visibility level is restricted' do before do @project = attributes_for(:project, { public: true }) @@ -448,6 +465,22 @@ describe API::API, api: true do post api("/projects/user/#{user.id}", admin), project expect(json_response['only_allow_merge_if_build_succeeds']).to be_truthy end + + it 'sets a project as allowing merge even if discussions are unresolved' do + project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: false }) + + post api("/projects/user/#{user.id}", admin), project + + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to be_falsey + end + + it 'sets a project as allowing merge only if all discussions are resolved' do + project = attributes_for(:project, { only_allow_merge_if_all_discussions_are_resolved: true }) + + post api("/projects/user/#{user.id}", admin), project + + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to be_truthy + end end describe "POST /projects/:id/uploads" do @@ -509,6 +542,7 @@ describe API::API, api: true do expect(json_response['shared_with_groups'][0]['group_name']).to eq(group.name) expect(json_response['shared_with_groups'][0]['group_access_level']).to eq(link.group_access) expect(json_response['only_allow_merge_if_build_succeeds']).to eq(project.only_allow_merge_if_build_succeeds) + expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to eq(project.only_allow_merge_if_all_discussions_are_resolved) end it 'returns a project by path name' do