Include code owner in `overall_approvers`

Its return type is change from `Approver` to `User`,
as code owner is type `User`, and it does not make sense
to wrap them in `Approver`.

When approvers are overriden, code owner is not included,
because the persisted approvers will include code owners already.

Add endpoint to delete approver by user_id,
since overall_approvers now returns User,
and this avoids one query for fetching user's approver association.

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