From 2e22b626ab61697cc6e31623e939752c7305defa 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 --- CHANGELOG | 2 + 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 ++ ...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 | 28 ++++++++- 16 files changed, 228 insertions(+), 8 deletions(-) create mode 100644 app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml 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/CHANGELOG b/CHANGELOG index df93f89a41a7..7c929a281940 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -27,6 +27,8 @@ v 8.12.1 - Fix bug where 'Search results' repeated many times when a search in the emoji search form is cleared (Xavier Bick) (@zeiv) v 8.12.0 +v 8.12.0 (unreleased) + - Add setting to only allow merge requests to be merged when all discussions are resolved - Update the rouge gem to 2.0.6, which adds highlighting support for JSX, Prometheus, and others. !6251 - Only check :can_resolve permission if the note is resolvable - Bump fog-aws to v0.11.0 to support ap-south-1 region diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index eaa38fa6c983..d7a100d354ba 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -318,8 +318,9 @@ def project_params :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 aec555dcec06..4c3de590c28f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -412,6 +412,7 @@ def mergeable_state?(skip_ci_check: false) 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 @@ -480,6 +481,12 @@ def discussions_resolved? 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 80053dd501bc..6e143c4b570e 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 6f5ee5f16c5a..f5541806f93b 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 000000000000..35d5677ee377 --- /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/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 000000000000..fad62d716b30 --- /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 ad62c249b3f6..6069ce0a9187 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -872,6 +872,7 @@ t.boolean "request_access_enabled", default: true, null: false t.boolean "has_external_wiki" t.boolean "lfs_enabled" + 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 750ce1508df3..c5ccea744f9d 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -86,6 +86,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 }, { @@ -148,6 +149,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 } ] @@ -286,6 +288,7 @@ Parameters: } ], "only_allow_merge_if_build_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, "request_access_enabled": false } ``` @@ -457,6 +460,7 @@ Parameters: - `only_allow_merge_if_build_succeeds` (optional) - `lfs_enabled` (optional) - `request_access_enabled` (optional) - Allow users to request member access. +- `only_allow_merge_if_all_discussions_are_resolved` (optional) ### Create project for user @@ -485,6 +489,7 @@ Parameters: - `only_allow_merge_if_build_succeeds` (optional) - `lfs_enabled` (optional) - `request_access_enabled` (optional) - Allow users to request member access. +- `only_allow_merge_if_all_discussions_are_resolved` (optional) ### Edit project @@ -514,6 +519,7 @@ Parameters: - `only_allow_merge_if_build_succeeds` (optional) - `lfs_enabled` (optional) - `request_access_enabled` (optional) - Allow users to request member access. +- `only_allow_merge_if_all_discussions_are_resolved` (optional) On success, method returns 200 with the updated project. If parameters are invalid, 400 is returned. @@ -595,6 +601,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 } ``` @@ -663,6 +670,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 } ``` @@ -751,6 +759,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 } ``` @@ -839,6 +848,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 0adc118ba277..7ebcc744de78 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -101,6 +101,7 @@ class Project < Grape::Entity 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 6d99617b56ff..eb288e83c0aa 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -128,7 +128,8 @@ def map_public_to_visibility_level(attrs) :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? @@ -182,7 +183,8 @@ def map_public_to_visibility_level(attrs) :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? @@ -264,7 +266,8 @@ def map_public_to_visibility_level(attrs) :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 94c9edc91fe2..175efd29b549 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -297,6 +297,32 @@ def merge_when_build_succeeds 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 c6a08d78b78f..d2fc79c40226 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -68,5 +68,10 @@ 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 end end 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 000000000000..100ddda01677 --- /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 580a3235127b..359261c8f8d2 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -749,6 +749,17 @@ 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 @@ -799,7 +810,52 @@ 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 192c7d14c13a..abf23f14895f 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -226,7 +226,8 @@ 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 @@ -297,6 +298,18 @@ 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 }) @@ -418,6 +431,18 @@ 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 @@ -479,6 +504,7 @@ 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 -- GitLab