Add setting to only allow merge requests to be merged when all discussions are resolved

Signed-off-by: Rémy Coutable <remy@rymai.me>

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
 

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),

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

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'
 

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.

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

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

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

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
 }
 ```

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

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?

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
 

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

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

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) }
 

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