Merge branch 'osw-self-approval-without-approvers-reduction' into 'master'

Allow MR authors to approve their MRs

Closes #3349 and #4738

See merge request gitlab-org/gitlab-ee!7144

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180826111825) do
+ActiveRecord::Schema.define(version: 20180831152625) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -2233,6 +2233,7 @@ ActiveRecord::Schema.define(version: 20180826111825) do
     t.boolean "pages_https_only", default: true
     t.string "external_webhook_token"
     t.boolean "packages_enabled"
+    t.boolean "merge_requests_author_approval"
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree

doc/user/project/merge_requests/merge_request_approvals.md

@@ -52,7 +52,7 @@ group approvers will be restricted.
 
 If a user is added as an individual approver and is also part of a group approver,
 then that user is just counted once. The merge request author does not count as
-an eligible approver.
+an eligible approver, unless [self-approval] is explicitly enabled on the project settings.
 
 Let's say that `m` is the number of required approvals, and `Ω` is the set of
 explicit approvers. Depending on their number, there are different cases:
@@ -91,7 +91,8 @@ the following is possible:
     ![Remove approval](img/remove_approval.png)
 
 NOTE: **Note:**
-The merge request author is not allowed to approve their own merge request.
+The merge request author is only allowed to approve their own merge request 
+if [self-approval] is enabled on the project settings.
 
 For the given merge request, if the required number of approvals has been met
 (i.e., the number of approvals given to the merge request is greater or equal
@@ -172,6 +173,18 @@ However, approvals will be reset if the target branch is changed.
 If you want approvals to persist, independent of changes to the merge request,
 turn this setting to off by unchecking the box and saving the changes.
 
+## Allowing merge request authors to approve their own merge requests
+
+You can allow merge request authors to self-approve merge requests by
+enabling it [at the project level](#editing-approvals). Authors
+also need to be included in the approvers list in order to be able to
+approve their merge request.
+
+1. Navigate to your project's **Settings > General** and expand the
+   **Merge requests settings**
+1. Tick the "Enable self approval of merge requests" checkbox
+1. Click **Save changes**
+
 ## Merge requests with different source branch and target branch projects
 
 If the merge request source branch and target branch belong to different
@@ -182,3 +195,5 @@ branch's project, the relevant settings are the target project's. The source
 branch's project settings are not applicable. Even if you start the merge
 request from the source branch's project UI, pay attention to the created merge
 request itself. It belongs to the target branch's project.
+
+[self-approval]: #allowing-merge-request-authors-to-approve-their-own-merge-requests

ee/app/controllers/ee/projects_controller.rb

@@ -23,6 +23,7 @@ module EE
         ci_cd_only
         use_custom_template
         packages_enabled
+        merge_requests_author_approval
       ]
 
       if allow_mirror_params?

ee/app/models/concerns/approvable.rb

@@ -16,13 +16,14 @@ module Approvable
 
   # Number of approvals remaining (excluding existing approvals) before the MR is
   # considered approved. If there are fewer potential approvers than approvals left,
-  # choose the lower so the MR doesn't get 'stuck' in a state where it can't be approved.
+  # users should either reduce the number of approvers on projects and/or merge
+  # requests settings and/or allow MR authors to approve their own merge
+  # requests (in case only one approval is needed).
   #
   def approvals_left
-    [
-      [approvals_required - approvals.size, number_of_potential_approvers].min,
-      0
-    ].max
+    approvals_left_count = approvals_required - approvals.size
+
+    [approvals_left_count, 0].max
   end
 
   def approvals_required
@@ -35,41 +36,6 @@ module Approvable
     super
   end
 
-  # An MR can potentially be approved by:
-  # - anyone in the approvers list
-  # - any other project member with developer access or higher (if there are no approvers
-  #   left)
-  #
-  # It cannot be approved by:
-  # - a user who has already approved the MR
-  # - the MR author
-  #
-  def number_of_potential_approvers
-    has_access = ['access_level > ?', Member::REPORTER]
-    users_with_access = { id: project.project_authorizations.where(has_access).select(:user_id) }
-    all_approvers = all_approvers_including_groups
-
-    users_relation = User.active.where.not(id: approvals.select(:user_id))
-    users_relation = users_relation.where.not(id: author.id) if author
-
-    # This is an optimisation for large instances. Instead of getting the
-    # count of all users who meet the conditions in a single query, which
-    # produces a slow query plan, we get the union of all users with access
-    # and all users in the approvers list, and count them.
-    if all_approvers.any?
-      specific_approvers = { id: all_approvers.map(&:id) }
-
-      union = Gitlab::SQL::Union.new([
-        users_relation.where(users_with_access).select(:id),
-        users_relation.where(specific_approvers).select(:id)
-      ])
-
-      User.from("(#{union.to_sql}) subquery").count
-    else
-      users_relation.where(users_with_access).count
-    end
-  end
-
   # Users in the list of approvers who have not already approved this MR.
   #
   def approvers_left
@@ -79,14 +45,18 @@ module Approvable
   end
 
   # The list of approvers from either this MR (if they've been set on the MR) or the
-  # target project. Excludes the author by default.
+  # target project. Excludes the author if 'self-approval' isn't explicitly
+  # enabled on project settings.
   #
   # Before a merge request has been created, author will be nil, so pass the current user
   # on the MR create page.
   #
   def overall_approvers
     approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
-    approvers_relation = approvers_relation.where.not(user_id: author.id) if author
+
+    if author && !authors_can_approve?
+      approvers_relation = approvers_relation.where.not(user_id: author.id)
+    end
 
     approvers_relation.includes(:user)
   end
@@ -121,7 +91,7 @@ module Approvable
 
     group_approvers.flatten!
 
-    group_approvers.delete(author)
+    group_approvers.delete(author) unless authors_can_approve?
 
     group_approvers
   end
@@ -132,7 +102,10 @@ module Approvable
 
   def can_approve?(user)
     return false unless user
+    # The check below considers authors being able to approve the MR. That is,
+    # they're included/excluded from that list accordingly.
     return true if approvers_left.include?(user)
+    # We can safely unauthorize authors if it reaches this guard clause.
     return false if user == author
     return false unless user.can?(:update_merge_request, self)
 
@@ -155,6 +128,10 @@ module Approvable
     remaining_approvals.zero? || remaining_approvals > approvers_left.count
   end
 
+  def authors_can_approve?
+    target_project.merge_requests_author_approval?
+  end
+
   def approver_ids=(value)
     ::Gitlab::Utils.ensure_array_from_string(value).each do |user_id|
       next if author && user_id == author.id

ee/app/models/ee/merge_request.rb

@@ -46,6 +46,7 @@ module EE
       delegate :expose_sast_container_data?, to: :head_pipeline, allow_nil: true
       delegate :expose_container_scanning_data?, to: :head_pipeline, allow_nil: true
       delegate :expose_dast_data?, to: :head_pipeline, allow_nil: true
+      delegate :merge_requests_author_approval?, to: :target_project, allow_nil: true
 
       participant :participant_approvers
     end

ee/app/views/projects/_merge_request_approvals_settings.html.haml

@@ -73,3 +73,11 @@
       = form.label :reset_approvals_on_push do
         %strong Remove all approvals in a merge request when new commits are pushed to its source branch
 
+  .form-group.self-approval
+    .form-check
+      = form.check_box :merge_requests_author_approval, class: 'form-check-input'
+      = form.label :merge_requests_author_approval do
+        %strong Enable self approval of merge requests
+        = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals",
+          anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank'
+

ee/app/views/shared/issuable/_approvals.html.haml

@@ -4,7 +4,8 @@
 - return unless issuable.is_a?(MergeRequest)
 - return unless issuable.requires_approve?
 
-- ineligible_approver = issuable.author || current_user
+- author = issuable.author || current_user
+- authors_can_approve = issuable.merge_requests_author_approval?
 - can_update_approvers = can?(current_user, :update_approvers, issuable)
 
 .form-group.row
@@ -12,7 +13,15 @@
     Approvers
   .col-sm-10
     - if can_update_approvers
-      = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups + [ineligible_approver], project: issuable.target_project)
+      - skip_users = [*issuable.all_approvers_including_groups, (author unless authors_can_approve)].compact
+
+      = users_select_tag("merge_request[approver_ids]",
+        multiple: true,
+        class: 'input-large',
+        email_user: true,
+        skip_users: skip_users,
+        project: issuable.target_project)
+
       .form-text.text-muted
         This merge request must be approved by these users.
         You can override the project settings by setting your own list of approvers.

ee/changelogs/unreleased/osw-self-approval-without-approvers-reduction.yml

+---
+title: Allow MR authors to approve their MRs
+merge_request:
+author:
+type: other

ee/db/migrate/20180831152625_add_merge_requests_author_approval_to_projects.rb

+# frozen_string_literal: true
+
+class AddMergeRequestsAuthorApprovalToProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :projects, :merge_requests_author_approval, :boolean
+  end
+end

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -479,6 +479,7 @@ Project:
 - merge_requests_template
 - merge_requests_rebase_enabled
 - approvals_before_merge
+- merge_requests_author_approval
 - reset_approvals_on_push
 - disable_overriding_approvers_per_merge_request
 - merge_requests_ff_only_enabled

spec/models/merge_request_spec.rb

@@ -825,91 +825,6 @@ describe MergeRequest do
     end
   end
 
-  describe "#number_of_potential_approvers" do
-    let(:project) { create(:project) }
-    let(:author) { create(:user) }
-    let(:merge_request) { create(:merge_request, source_project: project, author: author) }
-
-    def reloaded_merge_request
-      merge_request.reload
-      merge_request.reset_approval_cache!
-
-      merge_request
-    end
-
-    it "includes approvers set on the MR" do
-      expect do
-        create(:approver, user: create(:user), target: merge_request)
-      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
-    end
-
-    it "includes approvers from group" do
-      group = create(:group_with_members)
-
-      expect do
-        create(:approver_group, group: group, target: merge_request)
-      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
-    end
-
-    it "includes project members with developer access and up" do
-      expect do
-        developer = create(:user)
-
-        project.add_guest(create(:user))
-        project.add_reporter(create(:user))
-        project.add_developer(developer)
-        project.add_maintainer(create(:user))
-
-        # Add this user as both someone with access, and an explicit approver,
-        # to ensure they aren't double-counted.
-        create(:approver, user: developer, target: merge_request)
-      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(2)
-    end
-
-    it "excludes users who have already approved the MR" do
-      expect do
-        approver = create(:user)
-        create(:approver, user: approver, target: merge_request)
-        create(:approval, user: approver, merge_request: merge_request)
-      end.not_to change { reloaded_merge_request.number_of_potential_approvers }
-    end
-
-    it "excludes the MR author" do
-      expect do
-        create(:approver, user: create(:user), target: merge_request)
-        create(:approver, user: author, target: merge_request)
-      end.to change { reloaded_merge_request.number_of_potential_approvers }.by(1)
-    end
-
-    it "excludes blocked users" do
-      developer = create(:user)
-      blocked_developer = create(:user).tap { |u| u.block! }
-      project.add_developer(developer)
-      project.add_developer(blocked_developer)
-
-      expect(reloaded_merge_request.number_of_potential_approvers).to eq(2)
-    end
-
-    context "when the project is part of a group" do
-      let(:group) { create(:group) }
-
-      before do
-        project.update(group: group)
-      end
-
-      it "includes group members with developer access and up" do
-        expect do
-          group.add_guest(create(:user))
-          group.add_reporter(create(:user))
-          group.add_developer(create(:user))
-          group.add_maintainer(create(:user))
-          blocked_developer = create(:user).tap { |u| u.block! }
-          group.add_developer(blocked_developer)
-        end.to change { reloaded_merge_request.number_of_potential_approvers }.by(2)
-      end
-    end
-  end
-
   describe "#overall_approver_groups" do
     it 'returns a merge request group approver' do
       project = create :project
@@ -2169,40 +2084,34 @@ describe MergeRequest do
   end
 
   describe 'approvals' do
-    let(:project) { create(:project) }
-    let(:merge_request) { create(:merge_request, source_project: project, author: author) }
-    let(:author) { create(:user) }
-    let(:approver) { create(:user) }
-
-    context 'on a project with only one member' do
-      let(:author) { project.owner }
-
-      context 'when there is one approver' do
+    shared_examples_for 'authors self-approval authorization' do
+      context 'when authors are authorized to approve their own MRs' do
         before do
-          project.update(approvals_before_merge: 1)
+          project.update!(merge_requests_author_approval: true)
         end
 
-        context 'when that approver is the MR author' do
-          before do
-            create(:approver, user: author, target: merge_request)
+        it 'allows the author to approve the MR if within the approvers list' do
+          expect(merge_request.can_approve?(author)).to be_truthy
         end
 
-          it 'does not require approval for the merge request' do
-            expect(merge_request.approvals_left).to eq(0)
-          end
+        it 'does not allow the author to approve the MR if not within the approvers list' do
+          merge_request.approvers.delete_all
 
-          it 'does not allow the approver to approve the MR' do
           expect(merge_request.can_approve?(author)).to be_falsey
         end
-
-          it 'does not allow a logged-out user to approve the MR' do
-            expect(merge_request.can_approve?(nil)).to be_falsey
       end
+
+      context 'when authors are not authorized to approve their own MRs' do
+        it 'does not allow the author to approve the MR' do
+          expect(merge_request.can_approve?(author)).to be_falsey
         end
       end
     end
 
-    context 'on a project with several members' do
+    let(:project) { create(:project) }
+    let(:merge_request) { create(:merge_request, source_project: project, author: author) }
+    let(:author) { create(:user) }
+    let(:approver) { create(:user) }
     let(:approver_2) { create(:user) }
     let(:developer) { create(:user) }
     let(:other_developer) { create(:user) }
@@ -2228,14 +2137,12 @@ describe MergeRequest do
           create(:approver, user: author, target: merge_request)
         end
 
+        it_behaves_like 'authors self-approval authorization'
+
         it 'requires one approval' do
           expect(merge_request.approvals_left).to eq(1)
         end
 
-          it 'does not allow the author to approve the MR' do
-            expect(merge_request.can_approve?(author)).to be_falsey
-          end
-
         it 'allows any other project member with write access to approve the MR' do
           expect(merge_request.can_approve?(developer)).to be_truthy
 
@@ -2281,14 +2188,12 @@ describe MergeRequest do
           create(:approver, user: approver_2, target: merge_request)
         end
 
+        it_behaves_like 'authors self-approval authorization'
+
         it 'requires the original number of approvals' do
           expect(merge_request.approvals_left).to eq(3)
         end
 
-          it 'does not allow the author to approve the MR' do
-            expect(merge_request.can_approve?(author)).to be_falsey
-          end
-
         it 'allows any other other approver to approve the MR' do
           expect(merge_request.can_approve?(approver)).to be_truthy
         end
@@ -2297,7 +2202,7 @@ describe MergeRequest do
           expect(merge_request.can_approve?(nil)).to be_falsey
         end
 
-          context 'when all of the valid approvers have approved the MR' do
+        context 'when self-approval is disabled and all of the valid approvers have approved the MR' do
           before do
             create(:approval, user: approver, merge_request: merge_request)
             create(:approval, user: approver_2, merge_request: merge_request)
@@ -2325,6 +2230,29 @@ describe MergeRequest do
           end
         end
 
+        context 'when self-approval is enabled and all of the valid approvers have approved the MR' do
+          before do
+            project.update!(merge_requests_author_approval: true)
+            create(:approval, user: author, merge_request: merge_request)
+            create(:approval, user: approver_2, merge_request: merge_request)
+          end
+
+          it 'requires the original number of approvals' do
+            expect(merge_request.approvals_left).to eq(1)
+          end
+
+          it 'does not allow the approvers to approve the MR again' do
+            expect(merge_request.can_approve?(author)).to be_falsey
+            expect(merge_request.can_approve?(approver_2)).to be_falsey
+          end
+
+          it 'allows any other project member with write access to approve the MR' do
+            expect(merge_request.can_approve?(reporter)).to be_falsey
+            expect(merge_request.can_approve?(stranger)).to be_falsey
+            expect(merge_request.can_approve?(nil)).to be_falsey
+          end
+        end
+
         context 'when more than the number of approvers have approved the MR' do
           before do
             create(:approval, user: approver, merge_request: merge_request)
@@ -2396,7 +2324,6 @@ describe MergeRequest do
       end
     end
   end
-  end
 
   describe '#branch_merge_base_commit' do
     context 'source and target branch exist' do