Merge branch '1012-assign-code-owner-as-approver' into 'master'

Assign approvers based on code owners

Closes #1012

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

app/models/compare.rb

 # frozen_string_literal: true
 
+require 'set'
+
 class Compare
   include Gitlab::Utils::StrongMemoize
 
@@ -77,4 +79,13 @@ class Compare
       head_sha: head_commit_sha
     )
   end
+
+  def modified_paths
+    paths = Set.new
+    diffs.diff_files.each do |diff|
+      paths.add diff.old_path
+      paths.add diff.new_path
+    end
+    paths.to_a
+  end
 end

app/models/merge_request.rb

@@ -411,6 +411,18 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff&.real_size || diffs.real_size
   end
 
+  def modified_paths(past_merge_request_diff: nil)
+    diffs = if past_merge_request_diff
+              past_merge_request_diff
+            elsif compare
+              compare
+            else
+              self.merge_request_diff
+            end
+
+    diffs.modified_paths
+  end
+
   def diff_base_commit
     if persisted?
       merge_request_diff.base_commit

app/models/merge_request_diff.rb

@@ -6,6 +6,7 @@ class MergeRequestDiff < ActiveRecord::Base
   include ManualInverseAssociation
   include IgnorableColumn
   include EachBatch
+  include Gitlab::Utils::StrongMemoize
 
   # Don't display more than 100 commits at once
   COMMITS_SAFE_SIZE = 100
@@ -234,6 +235,12 @@ class MergeRequestDiff < ActiveRecord::Base
   end
   # rubocop: enable CodeReuse/ServiceClass
 
+  def modified_paths
+    strong_memoize(:modified_paths) do
+      merge_request_diff_files.pluck(:new_path, :old_path).flatten.uniq
+    end
+  end
+
   private
 
   def create_merge_request_diff_files(diffs)

app/services/merge_requests/refresh_service.rb

@@ -2,18 +2,18 @@
 
 module MergeRequests
   class RefreshService < MergeRequests::BaseService
+    attr_reader :push
+
     def execute(oldrev, newrev, ref)
-      push = Gitlab::Git::Push.new(@project, oldrev, newrev, ref)
-      return true unless push.branch_push?
+      @push = Gitlab::Git::Push.new(@project, oldrev, newrev, ref)
+      return true unless @push.branch_push?
 
-      refresh_merge_requests!(push)
+      refresh_merge_requests!
     end
 
     private
 
-    def refresh_merge_requests!(push)
-      @push = push
-
+    def refresh_merge_requests!
       Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits))
       # Be sure to close outstanding MRs before reloading them to avoid generating an
       # empty diff during a manual merge

config/routes/project.rb

@@ -156,6 +156,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
 
         ## EE-specific
         resources :approvers, only: :destroy
+        delete 'approvers', to: 'approvers#destroy_via_user_id', as: :approver_via_user_id
         resources :approver_groups, only: :destroy
         ## EE-specific
 

doc/user/project/merge_requests/merge_request_approvals.md

@@ -19,7 +19,6 @@ To edit the merge request approvals:
 
 1. Navigate to your project's **Settings > General** and expand the
    **Merge requests settings**
-1. Tick the "Merge requests approvals" checkbox
 1. Search for users or groups that will be [eligible to approve](#eligible-approvers)
    merge requests and click the **Add** button to add them as approvers
 1. Set the minimum number of required approvals under the "Approvals required"
@@ -36,36 +35,31 @@ suitable to your workflow:
   [overridden per merge request](#overriding-the-merge-request-approvals-default-settings)
 - Choose whether [approvals will be reset with new pushed commits](#resetting-approvals-on-push)
 
-NOTE: **Note:**
-If the approvers are changed via the project's settings after a merge request
-is created, the merge request retains the previous approvers, but you can always
-change them by [editing the merge request](#overriding-the-merge-request-approvals-default-settings).
-
 ## Eligible approvers
 
-An individual user is an eligible approver if they are a member of the given project,
-a member of the project's immediate parent group, or a member of a group that has share access
-to the project via a [share](../members/share_project_with_groups.md).
+The following can approve merge requests:
+
+- Users being added as approvers at project or merge request level.
+- [Code owners](../code_owners.md) related to the merge request ([introduced](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7933) in [GitLab Starter](https://about.gitlab.com/pricing/) 11.5).
 
-A group is also an eligible approver. [In the future](https://gitlab.com/gitlab-org/gitlab-ee/issues/2048),
+An individual user can be added as an approver for a project if they are a member of:
+
+- The project.
+- The project's immediate parent group.
+- A group that has access to the project via a [share](../members/share_project_with_groups.md).
+
+A group can also be added as an approver. [In the future](https://gitlab.com/gitlab-org/gitlab-ee/issues/2048),
 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, 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:
-
-- If `m <= count(Ω)`, then only those explicit approvers can approve the merge request.
-- If `m > count(Ω)` , then all the explicit approvers _and_ the members of the given
-  project with Developer role or higher are eligible approvers of the merge
-  request.
+### Implicit approvers
 
-NOTE: **Note:**
-If the approvals settings are [overridden](#overriding-the-merge-request-approvals-default-settings)
-for the particular merge request, then the set of explicit approvers is the
-union of the default approvers and the extra approvers set in the merge request.
+If the number of required approvals is greater than the number of approvers,
+other users will become implicit approvers to fill the gap.
+Those implicit approvers include members of the given project with Developer role or higher.
 
 ## Adding or removing an approval
 
@@ -124,6 +118,12 @@ First, you have to enable this option in the project's settings:
 
 1. Click **Save changes**
 
+NOTE: **Note:**
+If approver overriding is enabled
+and the project level approvers are changed after a merge request is created,
+the merge request retains the previous approvers.
+However, the approvers can be changed by [editing the merge request](#overriding-the-merge-request-approvals-default-settings).
+
 ---
 
 The default approval settings can now be overridden when creating a
@@ -200,9 +200,3 @@ request itself. It belongs to the target branch's project.
 ## Approver suggestions
 
 Approvers are suggested for merge requests based on the previous authors of the files affected by the merge request.
-
-### CODEOWNERS file
-
-> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7437>) in [GitLab Premium](https://about.gitlab.com/pricing/) 11.4.
-
-If the [CODEOWNERS](../code_owners.md) file is present in the target branch, more precise suggestions are provided based on its rules.

ee/app/controllers/projects/approvers_controller.rb

 class Projects::ApproversController < Projects::ApplicationController
   before_action :authorize_for_subject!
 
+  # @deprecated
   def destroy
     subject.approvers.find(params[:id]).destroy
 
     redirect_back_or_default(default: { action: 'index' })
   end
 
+  def destroy_via_user_id
+    subject.approvers.find_by_user_id(params[:user_id]).destroy
+
+    redirect_back_or_default(default: { action: 'index' })
+  end
+
   private
 
   def authorize_for_subject!

ee/app/models/approver.rb

@@ -3,4 +3,8 @@ class Approver < ActiveRecord::Base
   belongs_to :user
 
   validates :user, presence: true
+
+  def find_by_user_id(user_id)
+    find_by(user_id: user_id)
+  end
 end

ee/app/models/concerns/visible_approvable.rb

@@ -23,14 +23,23 @@ module VisibleApprovable
   # Before a merge request has been created, author will be nil, so pass the current user
   # on the MR create page.
   #
+  # @return [Array<User>]
   def overall_approvers
-    approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
+    if approvers_overwritten?
+      code_owners = [] # already persisted into database, no need to recompute
+      approvers_relation = approvers
+    else
+      code_owners = self.code_owners.dup
+      approvers_relation = target_project.approvers
+    end
 
     if author && !authors_can_approve?
       approvers_relation = approvers_relation.where.not(user_id: author.id)
     end
 
-    approvers_relation.includes(:user)
+    results = code_owners.concat(approvers_relation.includes(:user).map(&:user))
+    results.uniq!
+    results
   end
 
   def overall_approver_groups
@@ -41,8 +50,8 @@ module VisibleApprovable
     strong_memoize(:all_approvers_including_groups) do
       approvers = []
 
-      # Approvers from direct assignment
-      approvers << approvers_from_users
+      # Approvers not sourced from group level
+      approvers << overall_approvers
 
       approvers << approvers_from_groups
 
@@ -50,10 +59,6 @@ module VisibleApprovable
     end
   end
 
-  def approvers_from_users
-    overall_approvers.map(&:user)
-  end
-
   def approvers_from_groups
     group_approvers = overall_approver_groups.flat_map(&:users)
     group_approvers.delete(author) unless authors_can_approve?

ee/app/models/ee/merge_request.rb

@@ -50,5 +50,11 @@ module EE
     def participant_approvers
       approval_needed? ? approvers_left : []
     end
+
+    def code_owners
+      strong_memoize(:code_owners) do
+        ::Gitlab::CodeOwners.for_merge_request(self).freeze
+      end
+    end
   end
 end

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

@@ -6,8 +6,10 @@ module EE
       private
 
       override :refresh_merge_requests!
-      def refresh_merge_requests!(push)
-        super && reset_approvals_for_merge_requests(push.ref, push.newrev)
+      def refresh_merge_requests!
+        update_approvers do
+          super && reset_approvals_for_merge_requests(push.ref, push.newrev)
+        end
       end
 
       # Note: Closed merge requests also need approvals reset.
@@ -25,6 +27,53 @@ module EE
           end
         end
       end
+
+      # @return [Hash<Integer, MergeRequestDiff>] Diffs prior to code push, mapped from merge request id
+      def fetch_latest_merge_request_diffs
+        merge_requests = merge_requests_for_source_branch
+        ActiveRecord::Associations::Preloader.new.preload(merge_requests, :latest_merge_request_diff) # rubocop: disable CodeReuse/ActiveRecord
+        merge_requests.map(&:latest_merge_request_diff)
+      end
+
+      def update_approvers
+        return yield unless project.feature_available?(:code_owners)
+
+        previous_diffs = fetch_latest_merge_request_diffs
+
+        results = yield
+
+        merge_requests = merge_requests_for_source_branch
+        ActiveRecord::Associations::Preloader.new.preload(merge_requests, :latest_merge_request_diff) # rubocop: disable CodeReuse/ActiveRecord)
+
+        merge_requests.each do |merge_request|
+          previous_diff = previous_diffs.find { |diff| diff.merge_request == merge_request }
+          previous_code_owners = ::Gitlab::CodeOwners.for_merge_request(merge_request, merge_request_diff: previous_diff)
+          new_code_owners = merge_request.code_owners - previous_code_owners
+
+          create_approvers(merge_request, new_code_owners)
+        end
+
+        results
+      end
+
+      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)
+        end
+
+        todo_service.add_merge_request_approvers(merge_request, users)
+        notification_service.add_merge_request_approvers(merge_request, users, current_user)
+      end
     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.to_a
+        old_approvers = merge_request.overall_approvers
 
         merge_request = super(merge_request)
 
-        new_approvers = merge_request.overall_approvers.to_a - old_approvers
+        new_approvers = merge_request.overall_approvers - old_approvers
 
         if new_approvers.any?
           todo_service.add_merge_request_approvers(merge_request, new_approvers)

ee/app/services/ee/notification_service.rb

@@ -70,9 +70,7 @@ module EE
 
     def add_mr_approvers_email(merge_request, approvers, current_user)
       approvers.each do |approver|
-        recipient = approver.user
-
-        mailer.add_merge_request_approver_email(recipient.id, merge_request.id, current_user.id).deliver_later
+        mailer.add_merge_request_approver_email(approver.id, merge_request.id, current_user.id).deliver_later
       end
     end
 

ee/app/services/ee/todo_service.rb

@@ -42,7 +42,7 @@ module EE
 
     def create_approval_required_todos(merge_request, approvers, author)
       attributes = attributes_for_todo(merge_request.project, merge_request, author, ::Todo::APPROVAL_REQUIRED)
-      create_todos(approvers.map(&:user), attributes)
+      create_todos(approvers, attributes)
     end
   end
 end

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

@@ -42,16 +42,16 @@
           - unsaved_approvers = !presenter.approvers_overwritten?
           - item_classes = unsaved_approvers ? ['unsaved-approvers'] : []
           - presenter.overall_approvers.each do |approver|
-            %li{ id: dom_id(approver.user), class: item_classes + ['approver'] }
-              = link_to approver.user.name, approver.user
+            %li{ id: dom_id(approver), class: item_classes + ['approver'] }
+              = link_to approver.name, approver
               - if can_update_approvers
                 .float-right
                   - if unsaved_approvers
-                    = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-sm btn btn-remove", title: 'Remove approver' do
+                    = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.name}"}, class: "btn-sm btn btn-remove", title: 'Remove approver' do
                       = icon("sign-out")
                       Remove
                   - else
-                    = link_to project_merge_request_approver_path(@project, issuable, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-sm btn btn-remove", title: 'Remove approver' do
+                    = link_to project_merge_request_approver_via_user_id_path(@project, issuable, user_id: approver.id), data: { confirm: "Are you sure you want to remove approver #{approver.name}"}, method: :delete, class: "btn-sm btn btn-remove", title: 'Remove approver' do
                       = icon("sign-out")
                       Remove
           - presenter.overall_approver_groups.each do |approver_group|

ee/changelogs/unreleased/1012-assign-code-owner-as-approver.yml

+---
+title: Assign code owner as approver
+merge_request: 7933
+author:
+type: added

ee/lib/gitlab/code_owners.rb

@@ -12,5 +12,20 @@ module Gitlab
         User.none
       end
     end
+
+    # @param merge_request [MergeRequest]
+    # @param merge_request_diff [MergeRequestDiff]
+    #   Find code owners at a particular MergeRequestDiff.
+    #   Assumed to be the most recent one if not provided.
+    def self.for_merge_request(merge_request, merge_request_diff: nil)
+      return [] if merge_request.source_project.nil? || merge_request.source_branch.nil?
+      return [] unless merge_request.target_project.feature_available?(:code_owners)
+
+      Loader.new(
+        merge_request.target_project,
+        merge_request.target_branch,
+        merge_request.modified_paths(past_merge_request_diff: merge_request_diff)
+      ).members.where_not_in(merge_request.author).to_a
+    end
   end
 end

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -339,7 +339,7 @@ describe Projects::MergeRequestsController do
     end
 
     context 'with a forked project' do
-      let(:forked_project) { fork_project(project, fork_owner) }
+      let(:forked_project) { fork_project(project, fork_owner, repository: true) }
       let(:fork_owner) { create(:user) }
 
       before do

ee/spec/lib/gitlab/code_owners_spec.rb

@@ -7,18 +7,22 @@ describe Gitlab::CodeOwners do
 
   let!(:code_owner) { create(:user, username: 'owner-1') }
   let(:project) { create(:project, :repository) }
-  let(:blob) do
-    project.repository.blob_at(TestEnv::BRANCH_SHA['with-codeowners'], 'docs/CODEOWNERS')
-  end
   let(:codeowner_content) { "docs/CODEOWNERS @owner-1" }
   let(:codeowner_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) }
+  let(:codeowner_blob_ref) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) }
 
   before do
     project.add_developer(code_owner)
-    allow(project.repository).to receive(:code_owners_blob).and_return(codeowner_blob)
+    allow(project.repository).to receive(:code_owners_blob)
+      .with(ref: codeowner_lookup_ref)
+      .and_return(codeowner_blob)
   end
 
   describe '.for_blob' do
+    let(:branch) { TestEnv::BRANCH_SHA['with-codeowners'] }
+    let(:blob) { project.repository.blob_at(branch, 'docs/CODEOWNERS') }
+    let(:codeowner_lookup_ref) { branch }
+
     context 'when the feature is available' do
       before do
         stub_licensed_features(code_owners: true)
@@ -39,4 +43,59 @@ describe Gitlab::CodeOwners do
       end
     end
   end
+
+  describe '.for_merge_request' do
+    let(:codeowner_lookup_ref) { merge_request.target_branch }
+    let(:merge_request) do
+      build(
+        :merge_request,
+        source_project: project,
+        source_branch: 'feature',
+        target_project: project,
+        target_branch: 'with-codeowners'
+      )
+    end
+
+    context 'when the feature is available' do
+      before do
+        stub_licensed_features(code_owners: true)
+      end
+
+      it 'returns owners for merge request' do
+        expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: nil).and_return(['docs/CODEOWNERS'])
+
+        expect(described_class.for_merge_request(merge_request)).to eq([code_owner])
+      end
+
+      context 'when owner is merge request author' do
+        let(:merge_request) { build(:merge_request, target_project: project, author: code_owner) }
+
+        it 'excludes author' do
+          expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: nil).and_return(['docs/CODEOWNERS'])
+
+          expect(described_class.for_merge_request(merge_request)).to eq([])
+        end
+      end
+
+      context 'when merge_request_diff is specified' do
+        let(:merge_request_diff) { double(:merge_request_diff) }
+
+        it 'returns owners at the specified ref' do
+          expect(merge_request).to receive(:modified_paths).with(past_merge_request_diff: merge_request_diff).and_return(['docs/CODEOWNERS'])
+
+          expect(described_class.for_merge_request(merge_request, merge_request_diff: merge_request_diff)).to eq([code_owner])
+        end
+      end
+    end
+
+    context 'when the feature is not available' do
+      before do
+        stub_licensed_features(code_owners: false)
+      end
+
+      it 'returns no users' do
+        expect(described_class.for_merge_request(merge_request)).to eq([])
+      end
+    end
+  end
 end

ee/spec/models/merge_request_spec.rb

@@ -14,6 +14,18 @@ describe MergeRequest do
     it { is_expected.to have_many(:approved_by_users) }
   end
 
+  describe '#code_owners' do
+    subject(:merge_request) { build(:merge_request) }
+    let(:owners) { [double(:owner)] }
+
+    it 'returns code owners, frozen' do
+      allow(::Gitlab::CodeOwners).to receive(:for_merge_request).with(subject).and_return(owners)
+
+      expect(subject.code_owners).to eq(owners)
+      expect(subject.code_owners).to be_frozen
+    end
+  end
+
   describe '#approvals_before_merge' do
     where(:license_value, :db_value, :expected) do
       true  | 5   | 5

ee/spec/models/visible_approvable_spec.rb

@@ -39,20 +39,20 @@ describe VisibleApprovable do
     subject { resource.overall_approvers }
 
     it 'returns a list of all the project approvers' do
-      is_expected.to match_array(project_approver)
+      is_expected.to match_array(project_approver.user)
     end
 
     context 'when author is approver' do
       let!(:author_approver) { create(:approver, target: project, user: resource.author) }
 
       it 'excludes author if authors cannot approve' do
-        is_expected.not_to include(author_approver)
+        is_expected.not_to include(author_approver.user)
       end
 
       it 'includes author if authors are able to approve' do
         project.update(merge_requests_author_approval: true)
 
-        is_expected.to include(author_approver)
+        is_expected.to include(author_approver.user)
       end
     end
 
@@ -60,7 +60,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)
+        is_expected.to match_array(approver.user)
       end
     end
   end
@@ -95,7 +95,7 @@ describe VisibleApprovable do
     subject { resource.all_approvers_including_groups }
 
     it 'only queries once' do
-      expect(resource).to receive(:approvers_from_users).and_call_original.once
+      expect(resource).to receive(:overall_approvers).and_call_original.once
 
       3.times { subject }
     end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::RefreshService do
+  include ProjectForksHelper
+
+  let(:group) { create(:group) }
+  let(:project) { create(:project, :repository, namespace: group, approvals_before_merge: 1, reset_approvals_on_push: true) }
+  let(:forked_project) { fork_project(project, fork_user, repository: true) }
+
+  let(:fork_user) { create(:user) }
+
+  let(:source_branch) { 'between-create-delete-modify-move' }
+  let(:merge_request) do
+    create(:merge_request,
+      source_project: project,
+      source_branch: source_branch,
+      target_branch: 'master',
+      target_project: project)
+  end
+  let(:another_merge_request) do
+    create(:merge_request,
+      source_project: project,
+      source_branch: source_branch,
+      target_branch: 'test',
+      target_project: project)
+  end
+  let(:forked_merge_request) do
+    create(:merge_request,
+      source_project: forked_project,
+      source_branch: source_branch,
+      target_branch: 'master',
+      target_project: project)
+  end
+  let(:oldrev) { TestEnv::BRANCH_SHA[source_branch] }
+  let(:newrev) { TestEnv::BRANCH_SHA['after-create-delete-modify-move'] } # Pretend source_branch is now updated
+
+  subject { service.execute(oldrev, newrev, "refs/heads/#{source_branch}") }
+
+  describe '#execute' do
+    context '#update_approvers' do
+      let(:owner) { create(:user) }
+      let(:current_user) { merge_request.author }
+      let(:service) { described_class.new(project, current_user) }
+      let(:enable_code_owner) { true }
+      let(:todo_service) { double(:todo_service) }
+      let(:notification_service) { double(:notification_service) }
+
+      before do
+        stub_licensed_features(code_owners: enable_code_owner)
+
+        allow(service).to receive(:mark_pending_todos_done)
+        allow(service).to receive(:notify_about_push)
+        allow(service).to receive(:execute_hooks)
+        allow(service).to receive(:todo_service).and_return(todo_service)
+        allow(service).to receive(:notification_service).and_return(notification_service)
+
+        group.add_master(fork_user)
+
+        merge_request
+        another_merge_request
+        forked_merge_request
+      end
+
+      context 'when code owners disabled' do
+        let(:enable_code_owner) { false }
+
+        it 'does nothing' do
+          expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request)
+
+          subject
+        end
+      end
+
+      context 'when code owners enabled' do
+        let(:old_owners) { [] }
+        let(:new_owners) { [] }
+        let(:relevant_merge_requests) { [merge_request, another_merge_request] }
+
+        before do
+          relevant_merge_requests.each do |merge_request|
+            expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
+            expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request, merge_request_diff: anything).and_wrap_original do |m, *args|
+              expect(args.last[:merge_request_diff]).to eq(merge_request.merge_request_diffs.order(id: :desc).offset(1).first)
+
+              old_owners
+            end
+          end
+
+          [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
+
+          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
+            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) }
+
+          before 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(merge_request.approvers.first.user).to eq(existing_approver)
+              expect(merge_request.approvers.last.user).to eq(owner)
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -6,7 +6,7 @@ describe QuickActions::InterpretService do
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
   let(:group) { create(:group) }
-  let(:project) { create(:project, :public, group: group) }
+  let(:project) { create(:project, :repository, :public, group: group) }
   let(:issue) { create(:issue, project: project) }
   let(:service) { described_class.new(project, current_user) }
 

ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe 'shared/issuable/_approvals.html.haml' do
   let(:user) { create(:user) }
-  let(:project) { build(:project) }
+  let(:project) { create(:project, :repository) }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
   let(:presenter) { merge_request.present(current_user: user) }
   let(:approver_presenter) { double(any?: false, show_code_owner_tips?: true) }

spec/controllers/projects/milestones_controller_spec.rb

 require 'spec_helper'
 
 describe Projects::MilestonesController do
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :repository) }
   let(:user)    { create(:user) }
   let(:milestone) { create(:milestone, project: project) }
   let(:issue) { create(:issue, project: project, milestone: milestone) }

spec/factories/merge_request_diff_files.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :merge_request_diff_file do
+    association :merge_request_diff
+
+    relative_order 0
+    new_file true
+    renamed_file false
+    deleted_file false
+    too_large false
+    a_mode 0
+    b_mode 100644
+    new_path 'foo'
+    old_path 'foo'
+    diff ''
+    binary false
+
+    trait :new_file do
+      relative_order 0
+      new_file true
+      renamed_file false
+      deleted_file false
+      too_large false
+      a_mode 0
+      b_mode 100644
+      new_path 'foo'
+      old_path 'foo'
+      diff ''
+      binary false
+    end
+
+    trait :renamed_file do
+      relative_order 662
+      new_file false
+      renamed_file true
+      deleted_file false
+      too_large false
+      a_mode 100644
+      b_mode 100644
+      new_path 'bar'
+      old_path 'baz'
+      diff ''
+      binary false
+    end
+  end
+end

spec/factories/merge_request_diffs.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :merge_request_diff do
+    association :merge_request
+    state :collected
+    commits_count 1
+
+    base_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
+    head_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
+    start_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
+  end
+end

spec/models/compare_spec.rb

@@ -92,4 +92,33 @@ describe Compare do
       expect(subject.diff_refs.head_sha).to eq(head_commit.id)
     end
   end
+
+  describe '#modified_paths' do
+    context 'changes are present' do
+      let(:raw_compare) do
+        Gitlab::Git::Compare.new(
+          project.repository.raw_repository, 'before-create-delete-modify-move', 'after-create-delete-modify-move'
+        )
+      end
+
+      it 'returns affected file paths, without duplication' do
+        expect(subject.modified_paths).to contain_exactly(*%w{
+          foo/for_move.txt
+          foo/bar/for_move.txt
+          foo/for_create.txt
+          foo/for_delete.txt
+          foo/for_edit.txt
+        })
+      end
+    end
+
+    context 'changes are absent' do
+      let(:start_commit) { sample_commit }
+      let(:head_commit) { sample_commit }
+
+      it 'returns empty array' do
+        expect(subject.modified_paths).to eq([])
+      end
+    end
+  end
 end

spec/models/merge_request_diff_spec.rb

@@ -232,4 +232,17 @@ describe MergeRequestDiff do
       expect(commits.map(&:sha)).to match_array(commit_shas)
     end
   end
+
+  describe '#modified_paths' do
+    subject do
+      diff = create(:merge_request_diff)
+      create(:merge_request_diff_file, :new_file, merge_request_diff: diff)
+      create(:merge_request_diff_file, :renamed_file, merge_request_diff: diff)
+      diff
+    end
+
+    it 'returns affected file paths' do
+      expect(subject.modified_paths).to eq(%w{foo bar baz})
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -632,6 +632,44 @@ describe MergeRequest do
     end
   end
 
+  describe '#modified_paths' do
+    let(:paths) { double(:paths) }
+    subject(:merge_request) { build(:merge_request) }
+
+    before do
+      expect(diff).to receive(:modified_paths).and_return(paths)
+    end
+
+    context 'when past_merge_request_diff is specified' do
+      let(:another_diff) { double(:merge_request_diff) }
+      let(:diff) { another_diff }
+
+      it 'returns affected file paths from specified past_merge_request_diff' do
+        expect(merge_request.modified_paths(past_merge_request_diff: another_diff)).to eq(paths)
+      end
+    end
+
+    context 'when compare is present' do
+      let(:compare) { double(:compare) }
+      let(:diff) { compare }
+
+      it 'returns affected file paths from compare' do
+        merge_request.compare = compare
+
+        expect(merge_request.modified_paths).to eq(paths)
+      end
+    end
+
+    context 'when no arguments provided' do
+      let(:diff) { merge_request.merge_request_diff }
+      subject(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') }
+
+      it 'returns affected file paths for merge_request_diff' do
+        expect(merge_request.modified_paths).to eq(paths)
+      end
+    end
+  end
+
   describe "#related_notes" do
     let!(:merge_request) { create(:merge_request) }
 
@@ -2142,7 +2180,7 @@ describe MergeRequest do
       end
     end
 
-    let(:project) { create(:project) }
+    let(:project) { create(:project, :repository) }
     let(:merge_request) { create(:merge_request, source_project: project, author: author) }
     let(:author) { create(:user) }
     let(:approver) { create(:user) }

spec/services/issuable/bulk_update_service_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Issuable::BulkUpdateService do
   let(:user)    { create(:user) }
-  let(:project) { create(:project, namespace: user.namespace) }
+  let(:project) { create(:project, :repository, namespace: user.namespace) }
 
   def bulk_update(issuables, extra_params = {})
     bulk_update_params = extra_params

spec/services/merge_requests/update_service_spec.rb

@@ -685,8 +685,8 @@ describe MergeRequests::UpdateService, :mailer do
     end
 
     context 'setting `allow_collaboration`' do
-      let(:target_project) { create(:project, :public) }
-      let(:source_project) { fork_project(target_project) }
+      let(:target_project) { create(:project, :repository, :public) }
+      let(:source_project) { fork_project(target_project, nil, repository: true) }
       let(:user) { create(:user) }
       let(:merge_request) do
         create(:merge_request,

spec/services/milestones/destroy_service_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Milestones::DestroyService do
   let(:user) { create(:user) }
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :repository) }
   let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) }
 
   before do

spec/services/notes/quick_actions_service_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Notes::QuickActionsService do
   shared_context 'note on noteable' do
-    let(:project) { create(:project) }
+    let(:project) { create(:project, :repository) }
     let(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } }
     let(:assignee) { create(:user) }
 

spec/services/todo_service_spec.rb

@@ -10,7 +10,7 @@ describe TodoService do
   let(:john_doe) { create(:user) }
   let(:skipped) { create(:user) }
   let(:skip_users) { [skipped] }
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :repository) }
   let(:mentions) { 'FYI: ' + [author, assignee, john_doe, member, guest, non_member, admin, skipped].map(&:to_reference).join(' ') }
   let(:directly_addressed) { [author, assignee, john_doe, member, guest, non_member, admin, skipped].map(&:to_reference).join(' ') }
   let(:directly_addressed_and_mentioned) { member.to_reference + ", what do you think? cc: " + [guest, admin, skipped].map(&:to_reference).join(' ') }

spec/support/helpers/test_env.rb

@@ -55,6 +55,9 @@ module TestEnv
     'update-gitlab-shell-v-6-0-1'        => '2f61d70',
     'update-gitlab-shell-v-6-0-3'        => 'de78448',
     '2-mb-file'                          => 'bf12d25',
+    'before-create-delete-modify-move'   => '845009f',
+    'between-create-delete-modify-move'  => '3f5f443',
+    'after-create-delete-modify-move'    => 'ba3faa7',
     'with-codeowners'                    => '219560e'
   }.freeze