Revert "Only exclude commit authors in merge request approvers"

This reverts commit 4425026d.

app/models/commit_collection.rb

@@ -20,8 +20,8 @@ class CommitCollection
     commits.each(&block)
   end
 
-  def authors
-    emails = without_merge_commits.map(&:author_email).uniq
+  def committers
+    emails = without_merge_commits.map(&:committer_email).uniq
 
     User.by_any_email(emails)
   end

app/models/merge_request.rb

@@ -325,12 +325,8 @@ class MergeRequest < ApplicationRecord
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
-  def commit_authors
-    @commit_authors ||= commits.authors
-  end
-
-  def authors
-    User.from_union([commit_authors, User.where(id: self.author_id)])
+  def committers
+    @committers ||= commits.committers
   end
 
   # Verifies if title has changed not taking into account WIP prefix

db/schema.rb

@@ -2500,6 +2500,7 @@ ActiveRecord::Schema.define(version: 20190325165127) do
     t.string "runners_token_encrypted"
     t.string "bfg_object_map"
     t.boolean "merge_requests_require_code_owner_approval"
+    t.boolean "merge_requests_disable_committers_approval"
     t.index ["archived", "pending_delete", "merge_requests_require_code_owner_approval"], name: "projects_requiring_code_owner_approval", where: "((pending_delete = false) AND (archived = false) AND (merge_requests_require_code_owner_approval = true))", using: :btree
     t.index ["ci_id"], name: "index_projects_on_ci_id", using: :btree
     t.index ["created_at"], name: "index_projects_on_created_at", using: :btree

doc/user/project/merge_requests/merge_request_approvals.md

@@ -111,9 +111,11 @@ A group can also be added as an approver. [In the future](https://gitlab.com/git
 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 and users who have authored
-commits in the merge request do not count as eligible approvers,
-unless [self-approval] is explicitly enabled on the project settings.
+then that user is just counted once. The merge request author, as well as users who have committed
+to the merge request, do not count as eligible approvers,
+if [**Prevent author approval**](#allowing-merge-request-authors-to-approve-their-own-merge-requests) (enabled by default)
+and [**Prevent committers approval**](#prevent-approval-of-merge-requests-by-their-committers) (disabled by default)
+are enabled on the project settings.
 
 ### Implicit approvers
 
@@ -145,7 +147,7 @@ the following is possible:
 
 NOTE: **Note:**
 The merge request author is only allowed to approve their own merge request
-if [self-approval] is enabled on the project settings.
+if [**Prevent author approval**](#allowing-merge-request-authors-to-approve-their-own-merge-requests) is disabled on the project settings.
 
 For a given merge request, if the approval restrictions have been satisfied,
 the merge request is unblocked and can be merged.
@@ -264,15 +266,27 @@ turn this setting to off by unchecking the box and saving the changes.
 
 ## Allowing merge request authors to approve their own merge requests
 
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/3349) in [GitLab Starter](https://about.gitlab.com/pricing/) 11.3.
+
 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
-   **Merge request approvals**
-1. Tick the "Enable self approval of merge requests" checkbox
-1. Click **Save changes**
+1. Navigate to your project's **Settings > General** and expand **Merge request approvals**.
+1. Uncheck the **Prevent approval of merge requests by merge request author** checkbox, which is enabled by default.
+1. Click **Save changes**.
+
+## Prevent approval of merge requests by their committers
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/10441) in [GitLab Starter](https://about.gitlab.com/pricing/) 11.10.
+
+You can prevent users that have committed to a merge request from approving it by
+enabling [**Prevent approval of merge requests by their committers**](#prevent-approval-of-merge-requests-by-their-committers).
+
+1. Navigate to your project's **Settings > General** and expand **Merge request approvals**.
+1. Tick the checkbox **Prevent approval of merge requests by their committers**.
+1. Click **Save changes**.
 
 ## Merge requests with different source branch and target branch projects
 
@@ -285,8 +299,6 @@ 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
-
 ## Approver suggestions
 
 Approvers are suggested for merge requests based on the previous authors of the files affected by the merge request.

ee/app/controllers/ee/projects_controller.rb

@@ -42,6 +42,7 @@ module EE
         use_custom_template
         packages_enabled
         merge_requests_author_approval
+        merge_requests_disable_committers_approval
         merge_requests_require_code_owner_approval
         group_with_project_templates_id
       ]

ee/app/models/approval_state.rb

@@ -18,14 +18,25 @@ class ApprovalState
     @project = merge_request.target_project
   end
 
-  # Excludes the author if 'self-approval' isn't explicitly enabled on project settings.
+  # Excludes the author if 'author-approval' is explicitly disabled on project settings.
   def self.filter_author(users, merge_request)
     return users if merge_request.target_project.merge_requests_author_approval?
 
     if users.is_a?(ActiveRecord::Relation) && !users.loaded?
-      users.where.not(id: merge_request.authors)
+      users.where.not(id: merge_request.author_id)
     else
-      users - merge_request.authors
+      users - [merge_request.author]
+    end
+  end
+
+  # Excludes the author if 'committers-approval' is explicitly disabled on project settings.
+  def self.filter_committers(users, merge_request)
+    return users unless merge_request.target_project.merge_requests_disable_committers_approval?
+
+    if users.is_a?(ActiveRecord::Relation) && !users.loaded?
+      users.where.not(id: merge_request.committers.select(:id))
+    else
+      users - merge_request.committers
     end
   end
 
@@ -111,7 +122,8 @@ class ApprovalState
 
     users -= approved_approvers if unactioned
 
-    self.class.filter_author(users, merge_request)
+    users = self.class.filter_author(users, merge_request)
+    self.class.filter_committers(users, merge_request)
   end
 
   # approvers_left
@@ -124,8 +136,9 @@ class ApprovalState
     # The check below considers authors being able to approve the MR.
     # That is, they're included/excluded from that list accordingly.
     return true if unactioned_approvers.include?(user)
-    # We can safely unauthorize authors if it reaches this guard clause.
-    return false if merge_request.authors.include?(user)
+    # We can safely unauthorize author and committers if it reaches this guard clause.
+    return false if merge_request.author == user
+    return false if merge_request.committers.include?(user)
     return false unless user.can?(:update_merge_request, merge_request)
 
     any_approver_allowed? && merge_request.approvals.where(user: user).empty?

ee/app/models/approval_wrapped_rule.rb

@@ -24,7 +24,10 @@ class ApprovalWrappedRule
   end
 
   def approvers
+    filtered_approvers =
       ApprovalState.filter_author(@approval_rule.approvers, merge_request)
+
+    ApprovalState.filter_committers(filtered_approvers, merge_request)
   end
 
   # @return [Array<User>] all approvers related to this rule

ee/app/models/concerns/approvable.rb

@@ -72,11 +72,12 @@ 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.
+    # The check below considers authors and committers 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 authors.include?(user)
+    # We can safely unauthorize authors and committers if it reaches this guard clause.
+    return false if author == user
+    return false if committers.include?(user)
     return false unless user.can?(:update_merge_request, self)
 
     any_approver_allowed? && approvals.where(user: user).empty?
@@ -102,6 +103,10 @@ module Approvable
     target_project.merge_requests_author_approval?
   end
 
+  def committers_can_approve?
+    !target_project.merge_requests_disable_committers_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/concerns/approvable_for_rule.rb

@@ -11,6 +11,7 @@ module ApprovableForRule
     has_approved?
     any_approver_allowed?
     authors_can_approve?
+    committers_can_approve?
     approvers_overwritten?
   }.freeze
 

ee/app/models/concerns/visible_approvable.rb

@@ -34,8 +34,12 @@ module VisibleApprovable
 
     approvers_relation = project.members_among(approvers_relation)
 
-    if authors.any? && !authors_can_approve?
-      approvers_relation = approvers_relation.where.not(id: authors.select(:id))
+    if author && !authors_can_approve?
+      approvers_relation = approvers_relation.where.not(id: author.id)
+    end
+
+    if committers.any? && !committers_can_approve?
+      approvers_relation = approvers_relation.where.not(id: committers.select(:id))
     end
 
     results = code_owners.concat(approvers_relation)
@@ -62,10 +66,11 @@ module VisibleApprovable
 
   def approvers_from_groups
     group_approvers = overall_approver_groups.flat_map(&:users)
+    group_approvers.delete(author) unless authors_can_approve?
 
-    unless authors_can_approve?
-      author_approver_ids = authors.where(id: group_approvers.map(&:id)).pluck(:id)
-      group_approvers.reject! { |user| author_approver_ids.include?(user.id) }
+    unless committers_can_approve?
+      committer_approver_ids = committers.where(id: group_approvers.map(&:id)).pluck(:id)
+      group_approvers.reject! { |user| committer_approver_ids.include?(user.id) }
     end
 
     group_approvers

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

@@ -28,8 +28,16 @@
 
 .form-group.self-approval
   .form-check
-    = form.check_box :merge_requests_author_approval, class: 'form-check-input'
+    = form.check_box :merge_requests_author_approval, { class: 'form-check-input', checked: !project.merge_requests_author_approval? }, false, true
     = form.label :merge_requests_author_approval, class: 'form-check-label' do
-      %strong= _('Enable self approval of merge requests')
+      %strong= _('Prevent approval of merge requests by merge request author')
+      = 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'
+
+.form-group.committers-approval
+  .form-check
+    = form.check_box :merge_requests_disable_committers_approval, class: 'form-check-input'
+    = form.label :merge_requests_disable_committers_approval, class: 'form-check-label' do
+      %strong= _('Prevent approval of merge requests by merge request committers')
       = 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_single_rule.html.haml

@@ -5,7 +5,7 @@
 - can_update_approvers = can?(current_user, :update_approvers, issuable)
 
 - if can_update_approvers
-  - skip_users = [*presenter.all_approvers_including_groups, *(issuable.authors unless presenter.authors_can_approve?)].compact
+  - skip_users = [*presenter.all_approvers_including_groups, (issuable.author unless presenter.authors_can_approve?), *(issuable.committers unless presenter.committers_can_approve?)].compact
 
   = users_select_tag("merge_request[approver_ids]",
     multiple: true,

ee/db/migrate/20190318120957_add_merge_requests_disable_committers_approval_to_projects.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddMergeRequestsDisableCommittersApprovalToProjects < ActiveRecord::Migration[5.0]
+  DOWNTIME = false
+
+  def change
+    add_column :projects, :merge_requests_disable_committers_approval, :boolean
+  end
+end

ee/spec/lib/gitlab/import_export/safe_model_attributes.yml

 ---
 Project:
+- merge_requests_disable_committers_approval
 - merge_requests_require_code_owner_approval
 ProjectTracingSetting:
 - external_url

ee/spec/models/approvable_for_rule_spec.rb

@@ -42,58 +42,56 @@ describe ApprovableForRule do
       expect(merge_request.can_approve?(user)).to be true
     end
 
-    context 'when authors can approve' do
+    context 'when the user is the author' do
+      context 'and user is an approver' do
         before do
-        project.update(merge_requests_author_approval: true)
+          create(:approver, target: merge_request, user: author)
         end
 
-      context 'when the user is the author' do
-        it 'returns true when user is approver' do
-          create(:approver, target: merge_request, user: author)
+        it 'return true when authors can approve' do
+          project.update(merge_requests_author_approval: true)
 
           expect(merge_request.can_approve?(author)).to be true
         end
 
-        it 'returns false when user is not approver' do
+        it 'return false when authors cannot approve' do
+          project.update(merge_requests_author_approval: false)
+
           expect(merge_request.can_approve?(author)).to be false
         end
       end
 
-      context 'when user is committer' do
+      it 'returns false when user is not an approver' do
+        expect(merge_request.can_approve?(author)).to be false
+      end
+    end
+
+    context 'when user is a committer' do
       let(:user) { create(:user, email: merge_request.commits.without_merge_commits.first.committer_email) }
 
       before do
         project.add_developer(user)
       end
 
-        it 'returns true when user is approver' do
+      context 'and user is an approver' do
+        before do
           create(:approver, target: merge_request, user: user)
-
-          expect(merge_request.can_approve?(user)).to be true
         end
 
-        it 'returns false when user is not approver' do
-          expect(merge_request.can_approve?(user)).to be false
-        end
-      end
-    end
+        it 'return true when committers can approve' do
+          project.update(merge_requests_disable_committers_approval: false)
 
-    context 'when authors cannot approve' do
-      before do
-        project.update(merge_requests_author_approval: false)
+          expect(merge_request.can_approve?(user)).to be true
         end
 
-      it 'returns false when user is the author' do
-        create(:approver, target: merge_request, user: author)
+        it 'return false when committers cannot approve' do
+          project.update(merge_requests_disable_committers_approval: true)
 
-        expect(merge_request.can_approve?(author)).to be false
+          expect(merge_request.can_approve?(user)).to be false
+        end
       end
 
-      it 'returns false when user is a committer' do
-        user = create(:user, email: merge_request.commits.without_merge_commits.first.committer_email)
-        project.add_developer(user)
-        create(:approver, target: merge_request, user: user)
-
+      it 'returns false when user is not an approver' do
         expect(merge_request.can_approve?(user)).to be false
       end
     end

ee/spec/models/approvable_spec.rb

@@ -85,6 +85,23 @@ describe Approvable do
           project.add_developer(user)
         end
 
+        context 'and committers can not approve' do
+          before do
+            project.update(merge_requests_disable_committers_approval: true)
+          end
+
+          it 'returns true when user is approver' do
+            create(:approver, target: merge_request, user: user)
+
+            expect(merge_request.can_approve?(user)).to be false
+          end
+
+          it 'returns false when user is not approver' do
+            expect(merge_request.can_approve?(user)).to be false
+          end
+        end
+
+        context 'and committers can approve' do
           it 'returns true when user is approver' do
             create(:approver, target: merge_request, user: user)
 
@@ -96,6 +113,7 @@ describe Approvable do
           end
         end
       end
+    end
 
     context 'when authors cannot approve' do
       before do
@@ -108,12 +126,12 @@ describe Approvable do
         expect(merge_request.can_approve?(author)).to be false
       end
 
-      it 'returns false when user is a committer' do
+      it 'returns true when user is a committer' do
         user = create(:user, email: merge_request.commits.without_merge_commits.first.committer_email)
         project.add_developer(user)
         create(:approver, target: merge_request, user: user)
 
-        expect(merge_request.can_approve?(user)).to be false
+        expect(merge_request.can_approve?(user)).to be true
       end
     end
 

ee/spec/models/approval_state_spec.rb

@@ -36,19 +36,23 @@ describe ApprovalState do
 
   subject { merge_request.approval_state }
 
-  shared_examples 'filtering author' do
+  shared_examples 'filtering author and committers' do
+    let(:committers) { [merge_request.author, create(:user, username: 'commiter')] }
+    let(:merge_requests_disable_committers_approval) { nil }
+
     before do
-      allow(merge_request).to receive(:authors).and_return([merge_request.author, create(:user, username: 'commiter')])
+      allow(merge_request).to receive(:committers).and_return(User.where(id: committers))
 
       project.update(merge_requests_author_approval: merge_requests_author_approval)
-      create_rule(users: merge_request.authors)
+      project.update(merge_requests_disable_committers_approval: merge_requests_disable_committers_approval)
+      create_rule(users: committers)
     end
 
     context 'when self approval is disabled' do
       let(:merge_requests_author_approval) { false }
 
       it 'excludes authors' do
-        expect(results).not_to include(*merge_request.authors)
+        expect(results).not_to include(merge_request.author)
       end
     end
 
@@ -56,7 +60,25 @@ describe ApprovalState do
       let(:merge_requests_author_approval) { true }
 
       it 'includes author' do
-        expect(results).to include(*merge_request.authors)
+        expect(results).to include(merge_request.author)
+      end
+    end
+
+    context 'when committers approval is enabled' do
+      let(:merge_requests_author_approval) { true }
+      let(:merge_requests_disable_committers_approval) { false }
+
+      it 'excludes committers' do
+        expect(results).to include(*committers)
+      end
+    end
+
+    context 'when committers approval is disabled' do
+      let(:merge_requests_author_approval) { true }
+      let(:merge_requests_disable_committers_approval) { true }
+
+      it 'includes committers' do
+        expect(results).not_to include(*committers)
       end
     end
   end
@@ -330,7 +352,7 @@ describe ApprovalState do
         expect(subject.approvers).to contain_exactly(approver1, group_approver1)
       end
 
-      it_behaves_like 'filtering author' do
+      it_behaves_like 'filtering author and committers' do
         let(:results) { subject.approvers }
       end
     end
@@ -368,7 +390,7 @@ describe ApprovalState do
         end
       end
 
-      it_behaves_like 'filtering author' do
+      it_behaves_like 'filtering author and committers' do
         let(:results) { subject.filtered_approvers }
       end
     end
@@ -383,7 +405,7 @@ describe ApprovalState do
         expect(subject.unactioned_approvers).to contain_exactly(approver1)
       end
 
-      it_behaves_like 'filtering author' do
+      it_behaves_like 'filtering author and committers' do
         let(:results) { subject.unactioned_approvers }
       end
     end
@@ -900,7 +922,7 @@ describe ApprovalState do
         expect(subject.approvers).to contain_exactly(*approvers)
       end
 
-      it_behaves_like 'filtering author' do
+      it_behaves_like 'filtering author and committers' do
         let(:results) { subject.approvers }
       end
     end
@@ -938,7 +960,7 @@ describe ApprovalState do
         end
       end
 
-      it_behaves_like 'filtering author' do
+      it_behaves_like 'filtering author and committers' do
         let(:results) { subject.filtered_approvers }
       end
     end
@@ -953,7 +975,7 @@ describe ApprovalState do
         expect(subject.unactioned_approvers).to contain_exactly(approver1)
       end
 
-      it_behaves_like 'filtering author' do
+      it_behaves_like 'filtering author and committers' do
         let(:results) { subject.unactioned_approvers }
       end
     end

ee/spec/models/visible_approvable_for_rule_spec.rb

@@ -64,30 +64,36 @@ describe VisibleApprovableForRule do
       end
     end
 
-    shared_examples_for 'able to exclude authors' do
-      it 'excludes author if authors cannot approve' do
+    context 'when author is an approver' do
+      let!(:approver) { resource.author }
+
+      it 'excludes author if author cannot approve' do
         project.update(merge_requests_author_approval: false)
 
         is_expected.not_to include(approver)
       end
 
-      it 'includes author if authors are able to approve' do
+      it 'includes author if author is able to approve' do
         project.update(merge_requests_author_approval: true)
 
         is_expected.to include(approver)
       end
     end
 
-    context 'when author is approver' do
-      let!(:approver) { resource.author }
+    context 'when a committer is an approver' do
+      let!(:approver) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) }
+
+      it 'excludes the committer if committers cannot approve' do
+        project.update(merge_requests_disable_committers_approval: true)
 
-      it_behaves_like 'able to exclude authors'
+        is_expected.not_to include(approver)
       end
 
-    context 'when committer is approver' do
-      let(:approver) { create(:user, email: resource.commits.without_merge_commits.first.committer_email) }
+      it 'includes the committer if committers are able to approve' do
+        project.update(merge_requests_disable_committers_approval: false)
 
-      it_behaves_like 'able to exclude authors'
+        is_expected.to include(approver)
+      end
     end
   end
 

ee/spec/models/visible_approvable_spec.rb

@@ -77,12 +77,12 @@ describe VisibleApprovable do
       end
 
       it 'excludes committer if committers cannot approve' do
+        project.update(merge_requests_disable_committers_approval: true)
+
         is_expected.not_to include(committer_approver.user)
       end
 
       it 'includes committer if committers are able to approve' do
-        project.update(merge_requests_author_approval: true)
-
         is_expected.to include(committer_approver.user)
       end
     end

locale/gitlab.pot

@@ -3823,9 +3823,6 @@ msgstr ""
 msgid "Enable reCAPTCHA or Akismet and set IP limits."
 msgstr ""
 
-msgid "Enable self approval of merge requests"
-msgstr ""
-
 msgid "Enable shared Runners"
 msgstr ""
 
@@ -7863,6 +7860,12 @@ msgstr ""
 msgid "Prevent adding new members to project membership within this group"
 msgstr ""
 
+msgid "Prevent approval of merge requests by merge request author"
+msgstr ""
+
+msgid "Prevent approval of merge requests by merge request committers"
+msgstr ""
+
 msgid "Preview"
 msgstr ""
 

spec/models/commit_collection_spec.rb

@@ -14,26 +14,26 @@ describe CommitCollection do
     end
   end
 
-  describe '.authors' do
+  describe '.committers' do
     it 'returns a relation of users when users are found' do
-      user = create(:user, email: commit.author_email.upcase)
+      user = create(:user, email: commit.committer_email.upcase)
       collection = described_class.new(project, [commit])
 
-      expect(collection.authors).to contain_exactly(user)
+      expect(collection.committers).to contain_exactly(user)
     end
 
-    it 'returns empty array when authors cannot be found' do
+    it 'returns empty array when committers cannot be found' do
       collection = described_class.new(project, [commit])
 
-      expect(collection.authors).to be_empty
+      expect(collection.committers).to be_empty
     end
 
     it 'excludes authors of merge commits' do
       commit = project.commit("60ecb67744cb56576c30214ff52294f8ce2def98")
-      create(:user, email: commit.author_email.upcase)
+      create(:user, email: commit.committer_email.upcase)
       collection = described_class.new(project, [commit])
 
-      expect(collection.authors).to be_empty
+      expect(collection.committers).to be_empty
     end
   end
 

spec/models/merge_request_spec.rb

@@ -1064,31 +1064,17 @@ describe MergeRequest do
     end
   end
 
-  describe '#commit_authors' do
-    it 'returns all the authors of every commit in the merge request' do
-      users = subject.commits.without_merge_commits.map(&:author_email).uniq.map do |email|
+  describe '#committers' do
+    it 'returns all the committers of every commit in the merge request' do
+      users = subject.commits.without_merge_commits.map(&:committer_email).uniq.map do |email|
         create(:user, email: email)
       end
 
-      expect(subject.commit_authors).to match_array(users)
+      expect(subject.committers).to match_array(users)
     end
 
-    it 'returns an empty array if no author is associated with a user' do
-      expect(subject.commit_authors).to be_empty
-    end
-  end
-
-  describe '#authors' do
-    it 'returns a list with all the commit authors in the merge request and author' do
-      users = subject.commits.without_merge_commits.map(&:author_email).uniq.map do |email|
-        create(:user, email: email)
-      end
-
-      expect(subject.authors).to match_array([subject.author, *users])
-    end
-
-    it 'returns only the author if no committer is associated with a user' do
-      expect(subject.authors).to contain_exactly(subject.author)
+    it 'returns an empty array if no committer is associated with a user' do
+      expect(subject.committers).to be_empty
     end
   end