Merge branch '20968-add-setting-to-check-unresolved-discussion' into 'master'

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

_Originally opened at !6385 by @rodolfoasantos._

- - -

## What does this MR do?
Based on #20968 this merge request adds setting only to allow merge requests to be merged when all discussions are resolved. 

## Are there points in the code the reviewer needs to double check?
Check if there are other points to add the resolved discussion setting

## Why was this MR needed?
Add the possibility to configure the project to only accept merge request when all discussions are resolved

## Screenshots

![only_allow_merge_if_all_discussions_are_resolved](/uploads/9388db9421da0214590ffab6fb29f985/only_allow_merge_if_all_discussions_are_resolved.png)

![only_allow_merge_if_all_discussions_are_resolved_msg](/uploads/b1bba0c72ad67d3a1b34718baa08526e/only_allow_merge_if_all_discussions_are_resolved_msg.png)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes #20968

See merge request !7125

app/controllers/projects_controller.rb

@@ -335,6 +335,7 @@ class ProjectsController < Projects::ApplicationController
       :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,
+      :only_allow_merge_if_all_discussions_are_resolved,
       :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

+%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

changelogs/unreleased/20968-add-setting-to-check-unresolved-discussion.yml

+---
+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

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

doc/user/project/merge_requests/merge_request_discussion_resolution.md

@@ -33,7 +33,25 @@ resolved discussions tracker.
 
 !["3/4 discussions resolved"][discussions-resolved]
 
+## Only allow merge requests to be merged if all discussions are resolved
+
+> [Introduced][ce-7125] in GitLab 8.14.
+
+You can prevent merge requests from being merged until all discussions are resolved.
+
+Navigate to your project's settings page, select the
+**Only allow merge requests to be merged if all discussions are resolved** check
+box and hit **Save** for the changes to take effect.
+
+![Only allow merge if all the discussions are resolved settings](img/only_allow_merge_if_all_discussions_are_resolved.png)
+
+From now on, you will not be able to merge from the UI until all discussions
+are resolved.
+
+![Only allow merge if all the discussions are resolved message](img/only_allow_merge_if_all_discussions_are_resolved_msg.png)
+
 [ce-5022]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022
+[ce-7125]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7125
 [resolve-discussion-button]: img/resolve_discussion_button.png
 [resolve-comment-button]: img/resolve_comment_button.png
 [discussion-view]: img/discussion_view.png

doc/user/project/merge_requests/merge_when_build_succeeds.md

@@ -40,7 +40,7 @@ hit **Save** for the changes to take effect.
 
 ![Only allow merge if build succeeds settings](img/merge_when_build_succeeds_only_if_succeeds_settings.png)
 
-From now on, every time the pipelinefails you will not be able to merge the
+From now on, every time the pipeline fails you will not be able to merge the
 merge request from the UI, until you make all relevant builds pass.
 
-![Only allow merge if build succeeds msg](img/merge_when_build_succeeds_only_if_succeeds_msg.png)
+![Only allow merge if build succeeds message](img/merge_when_build_succeeds_only_if_succeeds_msg.png)

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,72 @@ describe Projects::MergeRequestsController do
           end
         end
       end
+
+      describe 'only_allow_merge_if_all_discussions_are_resolved? setting' do
+        let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project, author: user) }
+
+        context 'when enabled' do
+          before do
+            project.update_column(:only_allow_merge_if_all_discussions_are_resolved, true)
+          end
+
+          context 'with unresolved discussion' do
+            before do
+              expect(merge_request).not_to be_discussions_resolved
+            end
+
+            it 'returns :failed' do
+              merge_with_sha
+
+              expect(assigns(:status)).to eq(:failed)
+            end
+          end
+
+          context 'with all discussions resolved' do
+            before do
+              merge_request.discussions.each { |d| d.resolve!(user) }
+              expect(merge_request).to be_discussions_resolved
+            end
+
+            it 'returns :success' do
+              merge_with_sha
+
+              expect(assigns(:status)).to eq(:success)
+            end
+          end
+        end
+
+        context 'when disabled' do
+          before do
+            project.update_column(:only_allow_merge_if_all_discussions_are_resolved, false)
+          end
+
+          context 'with unresolved discussion' do
+            before do
+              expect(merge_request).not_to be_discussions_resolved
+            end
+
+            it 'returns :success' do
+              merge_with_sha
+
+              expect(assigns(:status)).to eq(:success)
+            end
+          end
+
+          context 'with all discussions resolved' do
+            before do
+              merge_request.discussions.each { |d| d.resolve!(user) }
+              expect(merge_request).to be_discussions_resolved
+            end
+
+            it 'returns :success' do
+              merge_with_sha
+
+              expect(assigns(:status)).to eq(:success)
+            end
+          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

+require 'spec_helper'
+
+feature 'Check if mergeable with unresolved discussions', js: true, feature: true do
+  let(:user)           { create(:user) }
+  let(:project)        { create(:project) }
+  let!(:merge_request) { create(:merge_request_with_diff_notes, source_project: project, author: user) }
+
+  before do
+    login_as user
+    project.team << [user, :master]
+  end
+
+  context 'when project.only_allow_merge_if_all_discussions_are_resolved == true' do
+    before do
+      project.update_column(:only_allow_merge_if_all_discussions_are_resolved, true)
+    end
+
+    context 'with unresolved discussions' 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
+
+    context 'with all discussions 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
+  end
+
+  context 'when project.only_allow_merge_if_all_discussions_are_resolved == false' do
+    before do
+      project.update_column(:only_allow_merge_if_all_discussions_are_resolved, false)
+    end
+
+    context 'with unresolved discussions' do
+      it 'does not allow to merge' do
+        visit_merge_request(merge_request)
+
+        expect(page).to have_button 'Accept Merge Request'
+      end
+    end
+
+    context 'with all discussions 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
+  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

@@ -825,11 +825,8 @@ describe MergeRequest, models: true do
     end
 
     context 'when failed' do
-      before { allow(subject).to receive(:broken?) { false } }
-
-      context 'when project settings restrict to merge only if build succeeds and build failed' do
+      context 'when #mergeable_ci_state? is false' do
         before do
-          project.only_allow_merge_if_build_succeeds = true
           allow(subject).to receive(:mergeable_ci_state?) { false }
         end
 
@@ -837,6 +834,16 @@ describe MergeRequest, models: true do
           expect(subject.mergeable_state?).to be_falsey
         end
       end
+
+      context 'when #mergeable_discussions_state? is false' do
+        before do
+          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 +894,49 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe '#environments' do
+  describe '#mergeable_discussions_state?' do
+    let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) }
+
+    context 'when project.only_allow_merge_if_all_discussions_are_resolved == true' do
+      let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: true) }
+
+      context 'with all discussions resolved' do
+        before do
+          merge_request.discussions.each { |d| d.resolve!(merge_request.author) }
+        end
+
+        it 'returns true' do
+          expect(merge_request.mergeable_discussions_state?).to be_truthy
+        end
+      end
+
+      context 'with unresolved discussions' do
+        before do
+          merge_request.discussions.each(&:unresolve!)
+        end
+
+        it 'returns false' do
+          expect(merge_request.mergeable_discussions_state?).to be_falsey
+        end
+      end
+    end
+
+    context 'when project.only_allow_merge_if_all_discussions_are_resolved == false' do
+      let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: false) }
+
+      context 'with unresolved discussions' do
+        before do
+          merge_request.discussions.each(&:unresolve!)
+        end
+
+        it 'returns true' do
+          expect(merge_request.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