Merge branch '8396-exclude-code-owner-from-mr-participants' into 'master'

Exclude code owner from merge request participants to avoid spam

Closes #8396

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

ee/app/models/concerns/visible_approvable.rb

@@ -24,12 +24,14 @@ module VisibleApprovable
   # on the MR create page.
   #
   # @return [Array<User>]
-  def overall_approvers
+  def overall_approvers(exclude_code_owners: false)
+    code_owners = [] if exclude_code_owners
+
     if approvers_overwritten?
-      code_owners = [] # already persisted into database, no need to recompute
+      code_owners ||= [] # already persisted into database, no need to recompute
       approvers_relation = approvers
     else
-      code_owners = self.code_owners.dup
+      code_owners ||= self.code_owners.dup
       approvers_relation = target_project.approvers
     end
 

ee/app/models/ee/merge_request.rb

@@ -4,6 +4,7 @@ module EE
     extend ::Gitlab::Utils::Override
 
     include ::Approvable
+    include ::Gitlab::Utils::StrongMemoize
 
     prepended do
       include Elastic::MergeRequestsSearch
@@ -48,7 +49,15 @@ module EE
     end
 
     def participant_approvers
-      approval_needed? ? approvers_left : []
+      strong_memoize(:participant_approvers) do
+        next [] unless approval_needed?
+
+        approvers = []
+        approvers.concat(overall_approvers(exclude_code_owners: true))
+        approvers.concat(approvers_from_groups)
+
+        ::User.where(id: approvers.map(&:id)).where.not(id: approved_by_users.select(:id))
+      end
     end
 
     def code_owners

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -58,21 +58,17 @@ module EE
 
       def create_approvers(merge_request, users)
         return if users.empty?
-
-        if merge_request.approvers_overwritten?
-          rows = users.map do |user|
-            {
-              target_id: merge_request.id,
-              target_type: merge_request.class.name,
-              user_id: user.id
-            }
-          end
-
-          ::Gitlab::Database.bulk_insert(Approver.table_name, rows)
+        return unless merge_request.approvers_overwritten?
+
+        rows = users.map do |user|
+          {
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            user_id: user.id
+          }
         end
 
-        todo_service.add_merge_request_approvers(merge_request, users)
-        notification_service.add_merge_request_approvers(merge_request, users, current_user)
+        ::Gitlab::Database.bulk_insert(Approver.table_name, rows)
       end
     end
   end

ee/app/services/ee/merge_requests/update_service.rb

@@ -8,11 +8,11 @@ module EE
       override :execute
       def execute(merge_request)
         should_remove_old_approvers = params.delete(:remove_old_approvers)
-        old_approvers = merge_request.overall_approvers
+        old_approvers = merge_request.overall_approvers(exclude_code_owners: true)
 
         merge_request = super(merge_request)
 
-        new_approvers = merge_request.overall_approvers - old_approvers
+        new_approvers = merge_request.overall_approvers(exclude_code_owners: true) - old_approvers
 
         if new_approvers.any?
           todo_service.add_merge_request_approvers(merge_request, new_approvers)

ee/app/services/ee/todo_service.rb

@@ -13,7 +13,7 @@ module EE
     override :new_issuable
     def new_issuable(issuable, author)
       if issuable.is_a?(MergeRequest)
-        create_approval_required_todos(issuable, issuable.overall_approvers, author)
+        create_approval_required_todos(issuable, issuable.overall_approvers(exclude_code_owners: true), author)
       end
 
       super

ee/spec/models/merge_request_spec.rb

@@ -14,6 +14,27 @@ describe MergeRequest do
     it { is_expected.to have_many(:approved_by_users) }
   end
 
+  describe '#participant_approvers' do
+    let!(:approver) { create(:approver, target: project) }
+    let(:code_owners) { [double(:code_owner)] }
+
+    before do
+      allow(subject).to receive(:code_owners).and_return(code_owners)
+    end
+
+    it 'returns empty array if approval not needed' do
+      allow(subject).to receive(:approval_needed?).and_return(false)
+
+      expect(subject.participant_approvers).to eq([])
+    end
+
+    it 'returns approvers if approval is needed, excluding code owners' do
+      allow(subject).to receive(:approval_needed?).and_return(true)
+
+      expect(subject.participant_approvers).to eq([approver.user])
+    end
+  end
+
   describe '#code_owners' do
     subject(:merge_request) { build(:merge_request) }
     let(:owners) { [double(:owner)] }

ee/spec/models/visible_approvable_spec.rb

@@ -35,11 +35,24 @@ describe VisibleApprovable do
 
   describe '#overall_approvers' do
     let!(:project_approver) { create(:approver, target: project) }
+    let(:code_owner) { build(:user) }
+
+    before do
+      allow(resource).to receive(:code_owners).and_return([code_owner])
+    end
 
     subject { resource.overall_approvers }
 
     it 'returns a list of all the project approvers' do
-      is_expected.to match_array(project_approver.user)
+      is_expected.to contain_exactly(project_approver.user, code_owner)
+    end
+
+    context 'when exclude_code_owners is true' do
+      subject { resource.overall_approvers(exclude_code_owners: true) }
+
+      it 'excludes code owners' do
+        is_expected.to contain_exactly(project_approver.user)
+      end
     end
 
     context 'when author is approver' do
@@ -60,7 +73,7 @@ describe VisibleApprovable do
       let!(:approver) { create(:approver, target: resource) }
 
       it 'returns the list of all the merge request user approvers' do
-        is_expected.to match_array(approver.user)
+        is_expected.to contain_exactly(approver.user)
       end
     end
   end

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -91,45 +91,21 @@ describe MergeRequests::RefreshService do
           [forked_merge_request].each do |merge_request|
             expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request).with(merge_request, anything)
           end
-        end
-
-        shared_examples 'notification and todo' do
-          it 'does nothing if owners do not change' do
-            expect(service.todo_service).not_to receive(:add_merge_request_approvers)
-            expect(service.notification_service).not_to receive(:add_merge_request_approvers)
 
-            subject
-          end
+          expect(service.todo_service).not_to receive(:add_merge_request_approvers)
+          expect(service.notification_service).not_to receive(:add_merge_request_approvers)
+        end
 
+        context 'merge request has overwritten approvers' do
           context 'when new owners are being added' do
             let(:new_owners) { [owner] }
 
-            it 'notifies new owner' do
-              relevant_merge_requests.each do |merge_request|
-                expect(todo_service).to receive(:add_merge_request_approvers).with(merge_request, [owner])
-                expect(notification_service).to receive(:add_merge_request_approvers).with(merge_request, [owner], current_user)
-              end
-
-              subject
-            end
-          end
-
-          context 'when old owners are being removed' do
-            let(:old_owners) { [owner] }
-
-            it 'does nothing' do
-              expect(service.todo_service).not_to receive(:add_merge_request_approvers)
-              expect(service.notification_service).not_to receive(:add_merge_request_approvers)
-
-              subject
+            it 'does not create Approver' do
+              expect { subject }.not_to change { Approver.count }
             end
           end
         end
 
-        context 'merge request has overwritten approvers' do
-          include_examples 'notification and todo'
-        end
-
         context 'merge request has default approvers' do
           let(:existing_approver) { create(:user) }
 
@@ -137,16 +113,11 @@ describe MergeRequests::RefreshService do
             create(:approver, target: merge_request, user: existing_approver)
           end
 
-          include_examples 'notification and todo'
-
           context 'when new owners are being added' do
             let(:new_owners) { [owner] }
 
             it 'creates Approver' do
-              allow(service.todo_service).to receive(:add_merge_request_approvers)
-              allow(service.notification_service).to receive(:add_merge_request_approvers)
-
-              subject
+              expect { subject }.to change { Approver.count }.by(1)
 
               expect(merge_request.approvers.first.user).to eq(existing_approver)
               expect(merge_request.approvers.last.user).to eq(owner)

ee/spec/services/merge_requests/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::UpdateService, :mailer do
+  include ProjectForksHelper
+
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:user3) { create(:user) }
+  let(:label) { create(:label, project: project) }
+  let(:label2) { create(:label) }
+
+  let(:merge_request) do
+    create(
+      :merge_request,
+      :simple,
+      title: 'Old title',
+      description: "FYI #{user2.to_reference}",
+      assignee_id: user3.id,
+      source_project: project,
+      author: create(:user)
+    )
+  end
+
+  before do
+    project.add_maintainer(user)
+    project.add_developer(user2)
+    project.add_developer(user3)
+  end
+
+  describe '#execute' do
+    def update_merge_request(opts)
+      described_class.new(project, user, opts).execute(merge_request)
+    end
+
+    context 'when code owners changes' do
+      let(:code_owner) { create(:user) }
+
+      before do
+        project.add_maintainer(code_owner)
+
+        allow(merge_request).to receive(:code_owners).and_return([], [code_owner])
+      end
+
+      it 'does not create any todos' do
+        expect do
+          update_merge_request(title: 'New title')
+        end.not_to change { Todo.count }
+      end
+
+      it 'does not send any emails' do
+        expect do
+          update_merge_request(title: 'New title')
+        end.not_to change { ActionMailer::Base.deliveries.count }
+      end
+    end
+  end
+end

ee/spec/services/todo_service_spec.rb

 require 'spec_helper'
 
 describe TodoService do
+  let(:author) { create(:user, username: 'author') }
+  let(:non_member) { create(:user, username: 'non_member') }
+  let(:member) { create(:user, username: 'member') }
+  let(:guest) { create(:user, username: 'guest') }
+  let(:admin) { create(:admin, username: 'administrator') }
+  let(:john_doe) { create(:user, username: 'john_doe') }
+  let(:skipped) { create(:user, username: 'skipped') }
+  let(:skip_users) { [skipped] }
+  let(:service) { described_class.new }
+
   describe 'Epics' do
-    let(:author) { create(:user, username: 'author') }
-    let(:non_member) { create(:user, username: 'non_member') }
-    let(:member) { create(:user, username: 'member') }
-    let(:guest) { create(:user, username: 'guest') }
-    let(:admin) { create(:admin, username: 'administrator') }
-    let(:john_doe) { create(:user, username: 'john_doe') }
-    let(:skipped) { create(:user, username: 'skipped') }
-    let(:skip_users) { [skipped] }
     let(:users) { [author, non_member, member, guest, admin, john_doe, skipped] }
     let(:mentions) { users.map(&:to_reference).join(' ') }
     let(:combined_mentions) { member.to_reference + ", what do you think? cc: " + [guest, admin, skipped].map(&:to_reference).join(' ') }
@@ -20,8 +22,6 @@ describe TodoService do
     let(:group) { create(:group) }
     let(:epic) { create(:epic, group: group,  author: author, description: description_mentions) }
 
-    let(:service) { described_class.new }
-
     let(:todos_for) { [] }
     let(:todos_not_for) { [] }
     let(:target) { epic }
@@ -245,4 +245,81 @@ describe TodoService do
       end
     end
   end
+
+  context 'Merge Requests' do
+    let(:project) { create(:project, :repository) }
+    let(:merge_request) { create(:merge_request, source_project: project, author: author, description: description) }
+
+    let(:assignee) { create(:user) }
+    let(:approver_1) { create(:user) }
+    let(:approver_2) { create(:user) }
+    let(:approver_3) { create(:user) }
+    let(:code_owner) { create(:user, username: 'codeowner') }
+    let(:description) { 'FYI: ' + [john_doe, approver_1].map(&:to_reference).join(' ') }
+
+    before do
+      project.add_guest(guest)
+      project.add_developer(author)
+      project.add_developer(member)
+      project.add_developer(john_doe)
+      project.add_developer(skipped)
+      project.add_developer(approver_1)
+      project.add_developer(approver_2)
+      project.add_developer(approver_3)
+      project.add_developer(code_owner)
+
+      create(:approver, user: approver_1, target: project)
+      create(:approver, user: approver_2, target: project)
+
+      allow(merge_request).to receive(:code_owners).and_return([code_owner])
+
+      service.new_merge_request(merge_request, author)
+    end
+
+    describe '#new_merge_request' do
+      context 'when the merge request has approvers' do
+        it 'creates a todo' do
+          # for each approver
+          should_create_todo(user: approver_1, target: merge_request, action: Todo::APPROVAL_REQUIRED)
+          should_create_todo(user: approver_2, target: merge_request, action: Todo::APPROVAL_REQUIRED)
+          should_not_create_todo(user: approver_3, target: merge_request, action: Todo::APPROVAL_REQUIRED)
+
+          # for each valid mentioned user
+          should_create_todo(user: john_doe, target: merge_request, action: Todo::MENTIONED)
+          should_not_create_todo(user: approver_1, target: merge_request, action: Todo::MENTIONED)
+
+          # skip for code owner
+          should_not_create_todo(user: code_owner, target: merge_request, action: Todo::APPROVAL_REQUIRED)
+        end
+
+        context 'when code owner is mentioned' do
+          let(:description) { 'FYI: ' + [code_owner].map(&:to_reference).join(' ') }
+
+          it 'creates a todo' do
+            should_create_todo(user: code_owner, target: merge_request, action: Todo::MENTIONED)
+          end
+        end
+      end
+    end
+  end
+
+  def should_create_todo(attributes = {})
+    attributes.reverse_merge!(
+      project: project,
+      author: author,
+      state: :pending
+    )
+
+    expect(Todo.where(attributes).count).to eq 1
+  end
+
+  def should_not_create_todo(attributes = {})
+    attributes.reverse_merge!(
+      project: project,
+      author: author,
+      state: :pending
+    )
+
+    expect(Todo.where(attributes).count).to eq 0
+  end
 end

spec/services/todo_service_spec.rb

@@ -551,36 +551,6 @@ describe TodoService do
         should_not_create_todo(user: john_doe, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
         should_not_create_todo(user: non_member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
       end
-
-      context 'when the merge request has approvers' do
-        let(:approver_1) { create(:user) }
-        let(:approver_2) { create(:user) }
-        let(:approver_3) { create(:user) }
-        let(:approver_mentions) { 'FYI: ' + [john_doe, approver_1].map(&:to_reference).join(' ') }
-        let(:mr_approvers) { create(:merge_request, source_project: project, author: author, description: approver_mentions) }
-
-        before do
-          project.add_developer(approver_1)
-          project.add_developer(approver_2)
-          project.add_developer(approver_3)
-
-          create(:approver, user: approver_1, target: mr_approvers)
-          create(:approver, user: approver_2, target: mr_approvers)
-
-          service.new_merge_request(mr_approvers, author)
-        end
-
-        it 'creates a todo for each approver' do
-          should_create_todo(user: approver_1, target: mr_approvers, action: Todo::APPROVAL_REQUIRED)
-          should_create_todo(user: approver_2, target: mr_approvers, action: Todo::APPROVAL_REQUIRED)
-          should_not_create_todo(user: approver_3, target: mr_approvers, action: Todo::APPROVAL_REQUIRED)
-        end
-
-        it 'creates a todo for each valid mentioned user' do
-          should_create_todo(user: john_doe, target: mr_approvers, action: Todo::MENTIONED)
-          should_not_create_todo(user: approver_1, target: mr_approvers, action: Todo::MENTIONED)
-        end
-      end
     end
 
     describe '#update_merge_request' do