Merge branch '5382-approvers-from-code-owners' into 'master'

Suggest approvers based on code owners

Closes #5382

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

app/controllers/projects/merge_requests/creations_controller.rb

@@ -5,8 +5,6 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
   include DiffHelper
   include RendersCommits
 
-  prepend ::EE::Projects::MergeRequests::CreationsController
-
   skip_before_action :merge_request
   before_action :whitelist_query_limiting, only: [:create]
   before_action :authorize_create_merge_request_from!

doc/user/project/merge_requests/merge_request_approvals.md

@@ -196,3 +196,13 @@ 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.
+
+### 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/ee/projects/merge_requests/application_controller.rb

@@ -6,17 +6,6 @@ module EE
 
         private
 
-        # rubocop: disable CodeReuse/ActiveRecord
-        def set_suggested_approvers
-          if merge_request.requires_approve?
-            @suggested_approvers = ::Gitlab::AuthorityAnalyzer.new( # rubocop:disable Gitlab/ModuleWithInstanceVariables
-              merge_request,
-              merge_request.author || current_user
-            ).calculate(merge_request.approvals_required)
-          end
-        end
-        # rubocop: enable CodeReuse/ActiveRecord
-
         def merge_request_params
           clamp_approvals_before_merge(super)
         end

ee/app/controllers/ee/projects/merge_requests/creations_controller.rb

-module EE
-  module Projects
-    module MergeRequests
-      module CreationsController
-        extend ActiveSupport::Concern
-
-        private
-
-        def define_new_vars
-          super
-          set_suggested_approvers
-        end
-      end
-    end
-  end
-end

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -51,12 +51,6 @@ module EE
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      def define_edit_vars
-        super
-
-        set_suggested_approvers
-      end
-
       def render_approvals_json
         respond_to do |format|
           format.json do

ee/app/models/license.rb

@@ -66,6 +66,7 @@ class License < ActiveRecord::Base
     system_header_footer
     custom_project_templates
     packages
+    code_owner_as_approver_suggestion
   ].freeze
 
   EEU_FEATURES = EEP_FEATURES + %i[

ee/app/presenters/merge_request_approver_presenter.rb

+# frozen_string_literal: true
+
+# A view object to ONLY handle approver list display.
+# Keeps internal states for performance purpose.
+#
+# Initialize with following params:
+# - skip_user
+class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple
+  include ActionView::Helpers::TagHelper
+  include ActionView::Helpers::UrlHelper
+  include ActionView::Helpers::OutputSafetyHelper
+  include ActionView::RecordIdentifier
+  include Gitlab::Utils::StrongMemoize
+
+  presents :merge_request
+
+  attr_reader :skip_user
+
+  def initialize(subject, **attributes)
+    @skip_user = subject.author || attributes.delete(:skip_user)
+    super
+  end
+
+  def any?
+    users.any?
+  end
+
+  def render
+    safe_join(users.map { |user| render_user(user) }, ', ')
+  end
+
+  def render_user(user)
+    if eligible_approver?(user)
+      link_to user.name, '#', id: dom_id(user)
+    else
+      content_tag(:span, user.name, title: 'Not an eligible approver', class: 'has-tooltip')
+    end
+  end
+
+  def show_code_owner_tips?
+    code_owner_enabled? && code_owner_loader.empty_code_owners?
+  end
+
+  private
+
+  def users
+    return @users if defined?(@users)
+
+    load_users
+    @users
+  end
+
+  def authorized_users
+    return @authorized_users if defined?(@authorized_users)
+
+    load_users
+    @authorized_users
+  end
+
+  def load_users
+    set_users_from_code_owners if code_owner_enabled?
+    set_users_from_git_log_authors if @users.blank?
+  end
+
+  def code_owner_enabled?
+    strong_memoize(:code_owner_enabled) do
+      merge_request.project.feature_available?(:code_owner_as_approver_suggestion)
+    end
+  end
+
+  def eligible_approver?(user)
+    authorized_users.include?(user)
+  end
+
+  def set_users_from_code_owners
+    @authorized_users = code_owner_loader.members.to_a
+    @users = @authorized_users + code_owner_loader.non_members
+    @users.delete(skip_user)
+  end
+
+  def set_users_from_git_log_authors
+    @users = ::Gitlab::AuthorityAnalyzer.new(merge_request, skip_user).calculate.first(merge_request.approvals_required)
+
+    @authorized_users = @users
+  end
+
+  def related_paths_for_code_owners
+    diffs = merge_request.diffs
+
+    return unless diffs
+
+    paths = []
+
+    diffs.diff_files.each do |diff|
+      paths << diff.old_path
+      paths << diff.new_path
+    end
+
+    paths.compact!
+    paths.uniq!
+    paths
+  end
+
+  def code_owner_loader
+    @code_owner_loader ||= Gitlab::CodeOwners::Loader.new(
+      merge_request.target_project,
+      merge_request.target_branch,
+      related_paths_for_code_owners
+    )
+  end
+end

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

@@ -76,7 +76,13 @@
         = form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required, readonly: !can_update_approvers
 
     - if can_update_approvers
+      - approver_presenter = MergeRequestApproverPresenter.new(issuable, skip_user: current_user)
       .form-text.text-muted.suggested-approvers
-        - if @suggested_approvers&.any?
+        - if approver_presenter.any?
           Suggested approvers:
-          = raw @suggested_approvers.map { |approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ")
+          = approver_presenter.render
+      - if approver_presenter.show_code_owner_tips?
+        .form-text.text-muted
+          Tip: add a
+          = link_to 'CODEOWNERS', help_page_path('user/project/code_owners'), target: '_blank', tabindex: -1
+          to suggest approvers based on file paths and file types.

ee/changelogs/unreleased/5382-approvers-from-code-owners.yml

+---
+title: Suggest approvers based on code owners
+merge_request: 7437
+author:
+type: added

ee/lib/gitlab/authority_analyzer.rb

@@ -8,14 +8,12 @@ module Gitlab
       @users = Hash.new(0)
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def calculate(number_of_approvers)
+    def calculate
       involved_users
 
-      # Picks most active users from hash like: {user1: 2, user2: 6}
-      @users.sort_by { |user, count| -count }.map(&:first).take(number_of_approvers)
+      # Sort most active users from hash like: {user1: 2, user2: 6}
+      @users.sort_by { |user, count| -count }.map(&:first)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     private
 

ee/lib/gitlab/code_owners.rb

@@ -7,7 +7,7 @@ module Gitlab
 
     def self.for_blob(blob)
       if blob.project.feature_available?(:code_owners)
-        Loader.new(blob.project, blob.commit_id, blob.path).users
+        Loader.new(blob.project, blob.commit_id, blob.path).members
       else
         User.none # rubocop: disable CodeReuse/ActiveRecord
       end

ee/lib/gitlab/code_owners/loader.rb

@@ -3,17 +3,30 @@
 module Gitlab
   module CodeOwners
     class Loader
-      def initialize(project, ref, path)
-        @project, @ref, @path = project, ref, path
+      def initialize(project, ref, paths)
+        @project, @ref, @paths = project, ref, Array(paths)
       end
 
-      def users
-        return User.none if code_owners_file.empty? # rubocop: disable CodeReuse/ActiveRecord
+      def members
+        @_members ||= @project.members_among(raw_users)
+      end
+
+      def non_members
+        @_non_members ||= raw_users.where_not_in(@project.authorized_users)
+      end
+
+      def raw_users
+        return User.none if empty_code_owners? # rubocop: disable CodeReuse/ActiveRecord
 
-        owners = code_owners_file.owners_for_path(@path)
-        extracted_users = Gitlab::UserExtractor.new(owners).users
+        @_raw_users ||= begin
+          owner_lines = @paths.map { |path| code_owners_file.owners_for_path(path) }
+
+          Gitlab::UserExtractor.new(owner_lines).users
+        end
+      end
 
-        @project.authorized_users.merge(extracted_users)
+      def empty_code_owners?
+        code_owners_file.empty?
       end
 
       private

ee/spec/lib/gitlab/authority_analyzer_spec.rb

@@ -19,27 +19,15 @@ describe Gitlab::AuthorityAnalyzer do
       ]
     end
 
-    let(:approvers) { described_class.new(merge_request, author).calculate(number_of_approvers) }
+    let(:approvers) { described_class.new(merge_request, author).calculate }
 
     before do
       merge_request.compare = double(:compare, raw_diffs: files)
       allow(merge_request.target_project.repository).to receive(:commits).and_return(commits)
     end
 
-    context 'when there are fewer contributors than requested' do
-      let(:number_of_approvers) { 5 }
-
-      it 'returns the full number of users' do
-        expect(approvers.length).to eq(2)
-      end
-    end
-
-    context 'when there are more contributors than requested' do
-      let(:number_of_approvers) { 1 }
-
-      it 'returns only the top n contributors' do
-        expect(approvers).to contain_exactly(user_a)
-      end
+    it 'returns contributors in order, without skip_user' do
+      expect(approvers).to contain_exactly(user_a, user_b)
     end
   end
 end

ee/spec/lib/gitlab/code_owners/loader_spec.rb

@@ -12,10 +12,12 @@ describe Gitlab::CodeOwners::Loader do
   let!(:email_owner) { create(:user, username: 'owner-2') }
   let!(:owner_3) { create(:user, username: 'owner-3') }
   let!(:documentation_owner) { create(:user, username: 'documentation-owner') }
+  let!(:test_owner) { create(:user, username: 'test-owner') }
   let(:codeowner_content) do
     <<~CODEOWNERS
     docs/* @documentation-owner
     docs/CODEOWNERS @owner-1 owner2@gitlab.org @owner-3 @documentation-owner
+    spec/* @test-owner
     CODEOWNERS
   end
   let(:codeowner_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) }
@@ -23,28 +25,68 @@ describe Gitlab::CodeOwners::Loader do
 
   before do
     create(:email, user: email_owner, email: 'owner2@gitlab.org')
-    project.add_developer(owner_1)
-    project.add_developer(email_owner)
-    project.add_developer(documentation_owner)
 
     allow(project.repository).to receive(:code_owners_blob).and_return(codeowner_blob)
   end
 
-  describe '#users' do
+  describe '#non_members' do
+    before do
+      project.add_developer(owner_1)
+      project.add_developer(email_owner)
+      project.add_developer(test_owner)
+    end
+
+    it 'returns all existing users that are not members of the project' do
+      expect(loader.non_members).to contain_exactly(owner_3, documentation_owner)
+    end
+
+    it 'does not return users that are members of the project' do
+      expect(loader.non_members).not_to include(owner_1, email_owner)
+    end
+
+    it 'excludes group members of the project' do
+      group.add_developer(documentation_owner)
+
+      expect(loader.non_members).to include(owner_3)
+    end
+  end
+
+  describe '#members' do
+    before do
+      project.add_developer(owner_1)
+      project.add_developer(email_owner)
+      project.add_developer(documentation_owner)
+      project.add_developer(test_owner)
+    end
+
+    it 'returns all existing users that are members of the project' do
+      expect(loader.members).to contain_exactly(owner_1, email_owner, documentation_owner)
+    end
+
+    it 'does not return users that are not members of the project' do
+      expect(loader.members).not_to include(owner_3)
+    end
+
+    it 'includes group members of the project' do
+      group.add_developer(owner_3)
+
+      expect(loader.members).to include(owner_3)
+    end
+  end
+
+  describe '#raw_users' do
     context 'with a CODEOWNERS file' do
       context 'for a path with code owners' do
-        it 'returns all existing users that are members of the project' do
-          expect(loader.users).to contain_exactly(owner_1, email_owner, documentation_owner)
-        end
-
-        it 'does not return users that are not members of the project' do
-          expect(loader.users).not_to include(owner_3)
+        it 'returns all owners' do
+          expect(loader.raw_users).to contain_exactly(owner_1, owner_3, email_owner, documentation_owner)
         end
+      end
 
-        it 'includes group members of the project' do
-          group.add_developer(owner_3)
+      context 'for multiple paths with code owners' do
+        let(:path) { ['docs/test.rb', 'spec/spec_helper.rb', 'docs/foo.rb'] }
 
-          expect(loader.users).to include(owner_3)
+        it 'returns all owners for all paths' do
+          expect(loader.raw_users).to contain_exactly(documentation_owner, test_owner)
         end
       end
 
@@ -52,7 +94,7 @@ describe Gitlab::CodeOwners::Loader do
         let(:path) { 'no-codeowner' }
 
         it 'returns no users' do
-          expect(loader.users).to be_empty
+          expect(loader.raw_users).to be_empty
         end
       end
     end
@@ -61,7 +103,7 @@ describe Gitlab::CodeOwners::Loader do
       let(:codeowner_blob) { nil }
 
       it 'returns no users without failing' do
-        expect(loader.users).to be_empty
+        expect(loader.raw_users).to be_empty
       end
     end
 
@@ -69,7 +111,7 @@ describe Gitlab::CodeOwners::Loader do
       it 'only calls out to the repository once' do
         expect(project.repository).to receive(:code_owners_blob).once
 
-        2.times { loader.users }
+        2.times { loader.raw_users }
       end
 
       it 'only processes the file once' do
@@ -77,7 +119,31 @@ describe Gitlab::CodeOwners::Loader do
 
         expect(code_owners_file).to receive(:get_parsed_data).once.and_call_original
 
-        2.times { loader.users }
+        2.times { loader.raw_users }
+      end
+    end
+  end
+
+  describe '#empty_code_owners?' do
+    context 'when file does not exist' do
+      let(:codeowner_blob) { nil }
+
+      it 'returns true' do
+        expect(loader.empty_code_owners?).to eq(true)
+      end
+    end
+
+    context 'when file is empty' do
+      let(:codeowner_content) { '' }
+
+      it 'returns true' do
+        expect(loader.empty_code_owners?).to eq(true)
+      end
+    end
+
+    context 'when file content exists' do
+      it 'returns false' do
+        expect(loader.empty_code_owners?).to eq(false)
       end
     end
   end

ee/spec/presenters/merge_request_approver_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestApproverPresenter do
+  let(:project) { create(:project, :repository) }
+  let(:merge_request) { create(:merge_request, target_project: project, source_project: project) }
+  let(:files) do
+    [
+      double(:file, old_path: 'coo', new_path: nil),
+      double(:file, old_path: 'foo', new_path: 'bar'),
+      double(:file, old_path: nil, new_path: 'baz')
+    ]
+  end
+  let(:approvals_required) { 10 }
+  let(:enable_code_owner_as_approver_suggestion) { true }
+
+  let(:author) { merge_request.author }
+  let(:owner_a) { build(:user) }
+  let(:owner_b) { build(:user) }
+  let(:committer_a) { create(:user) }
+  let(:committer_b) { create(:user) }
+  let(:code_owner_loader) { double(:loader) }
+
+  subject { described_class.new(merge_request) }
+
+  before do
+    diffs = double(:diffs)
+    allow(merge_request).to receive(:diffs).and_return(diffs)
+    allow(diffs).to receive(:diff_files).and_return(files)
+
+    allow(merge_request).to receive(:approvals_required).and_return(approvals_required)
+
+    stub_licensed_features(code_owner_as_approver_suggestion: enable_code_owner_as_approver_suggestion)
+  end
+
+  def expect_code_owner_loader_init
+    expect(Gitlab::CodeOwners::Loader).to receive(:new).with(
+      merge_request.target_project,
+      merge_request.target_branch,
+      %w(coo foo bar baz)
+    ).and_return(code_owner_loader)
+  end
+
+  def expect_code_owners_call(*stub_return_users)
+    expect_code_owner_loader_init
+    expect(code_owner_loader).to receive(:members).and_return(stub_return_users)
+    expect(code_owner_loader).to receive(:non_members).and_return([])
+  end
+
+  def expect_git_log_call(*stub_return_users)
+    analyzer = double(:analyzer)
+
+    expect(Gitlab::AuthorityAnalyzer).to receive(:new).with(
+      merge_request,
+      merge_request.author
+    ).and_return(analyzer)
+
+    expect(analyzer).to receive(:calculate).and_return(stub_return_users)
+  end
+
+  describe '#render' do
+    context 'when code owner exists' do
+      it 'renders code owners' do
+        expect_code_owners_call(owner_a, owner_b)
+        expect(subject).to receive(:render_user).with(owner_a).and_call_original
+        expect(subject).to receive(:render_user).with(owner_b).and_call_original
+
+        subject.render
+      end
+    end
+
+    context 'git log lookup' do
+      context 'when authors are approvers' do
+        before do
+          project.add_developer(committer_a)
+          project.add_developer(committer_b)
+        end
+
+        context 'when the only code owner is skip_user' do
+          it 'displays git log authors instead' do
+            expect_code_owners_call(merge_request.author)
+            expect_git_log_call(committer_a)
+            expect(subject).to receive(:render_user).with(committer_a).and_call_original
+
+            subject.render
+          end
+        end
+
+        context 'when code owners do not exist' do
+          it 'displays git log authors' do
+            expect_code_owners_call
+            expect_git_log_call(committer_a)
+            expect(subject).to receive(:render_user).with(committer_a).and_call_original
+
+            subject.render
+          end
+        end
+
+        context 'approvals_required is low' do
+          let(:approvals_required) { 1 }
+
+          it 'returns top n approvers' do
+            expect_code_owners_call
+            expect_git_log_call(committer_a, committer_b)
+            expect(subject).to receive(:render_user).with(committer_a).and_call_original
+            expect(subject).not_to receive(:render_user).with(committer_b)
+
+            subject.render
+          end
+        end
+      end
+
+      context 'code_owner_as_approver_suggestion disabled' do
+        let(:enable_code_owner_as_approver_suggestion) { false }
+
+        before do
+          project.add_developer(committer_a)
+        end
+
+        it 'displays git log authors' do
+          expect(Gitlab::CodeOwners::Loader).not_to receive(:new)
+          expect_git_log_call(committer_a)
+          expect(subject).to receive(:render_user).with(committer_a).and_call_original
+
+          subject.render
+        end
+      end
+    end
+  end
+
+  describe '#any?' do
+    it 'returns true if any user exists' do
+      expect_code_owners_call(owner_a)
+
+      expect(subject.any?).to eq(true)
+    end
+
+    it 'returns false if no user exists' do
+      expect_code_owners_call
+      expect_git_log_call
+
+      expect(subject.any?).to eq(false)
+    end
+
+    it 'caches loaded users' do
+      expect(subject).to receive(:load_users).once.and_call_original
+
+      subject.any?
+      subject.any?
+    end
+  end
+
+  describe '#render_user' do
+    it 'renders plaintext if user is not an eligible approver' do
+      expect_code_owner_loader_init
+      expect(code_owner_loader).to receive(:members).and_return([])
+      expect(code_owner_loader).to receive(:non_members).and_return([owner_a])
+
+      result = subject.render_user(owner_a)
+
+      expect(result).to start_with('<span')
+      expect(result).to include('has-tooltip')
+    end
+
+    context 'user is an eligible approver' do
+      it 'renders link' do
+        expect_code_owners_call(committer_a)
+
+        result = subject.render_user(committer_a)
+
+        expect(result).to start_with('<a')
+      end
+    end
+  end
+
+  describe '#show_code_owner_tips?' do
+    context 'when code_owner feature enabled and code owner is empty' do
+      before do
+        expect_code_owner_loader_init
+        allow(code_owner_loader).to receive(:empty_code_owners?).and_return(true)
+      end
+
+      it 'returns true' do
+        expect(subject.show_code_owner_tips?).to eq(true)
+      end
+    end
+
+    context 'when code_owner feature enabled and code owner is not empty' do
+      before do
+        expect_code_owner_loader_init
+        allow(code_owner_loader).to receive(:empty_code_owners?).and_return(false)
+      end
+
+      it 'returns false' do
+        expect(subject.show_code_owner_tips?).to eq(false)
+      end
+    end
+
+    context 'when code_owner feature is disabled' do
+      let(:enable_code_owner_as_approver_suggestion) { false }
+
+      it 'returns false' do
+        expect(subject.show_code_owner_tips?).to eq(false)
+      end
+    end
+  end
+end

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

@@ -5,6 +5,7 @@ describe 'shared/issuable/_approvals.html.haml' do
   let(:project) { build(:project) }
   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) }
   let(:form) { double('form') }
 
   before do
@@ -13,8 +14,8 @@ describe 'shared/issuable/_approvals.html.haml' do
     allow(form).to receive(:label)
     allow(form).to receive(:number_field)
     allow(merge_request).to receive(:requires_approve?).and_return(true)
+    allow(MergeRequestApproverPresenter).to receive(:new).and_return(approver_presenter)
     assign(:project, project)
-    assign(:suggested_approvers, [])
   end
 
   context 'has no approvers' do

lib/gitlab/user_extractor.rb

@@ -11,7 +11,7 @@ module Gitlab
     USERNAME_REGEXP = User.reference_pattern
 
     def initialize(text)
-      @text = text
+      @text = text.is_a?(Array) ? text.join(' ') : text
     end
 
     # rubocop: disable CodeReuse/ActiveRecord

spec/lib/gitlab/user_extractor_spec.rb

@@ -38,6 +38,18 @@ describe Gitlab::UserExtractor do
 
       expect(extractor.users).to include(user)
     end
+
+    context 'input as array of strings' do
+      it 'is treated as one string' do
+        extractor = described_class.new(text.lines)
+
+        user_1 = create(:user, username: "USER-1")
+        user_4 = create(:user, username: "USER-4")
+        user_email = create(:user, email: 'user@gitlab.org')
+
+        expect(extractor.users).to contain_exactly(user_1, user_4, user_email)
+      end
+    end
   end
 
   describe '#matches' do

spec/support/shared_examples/update_invalid_issuable.rb

@@ -30,10 +30,6 @@ shared_examples 'update invalid issuable' do |klass|
 
       expect(response).to render_template(:edit)
       expect(assigns[:conflict]).to be_truthy
-
-      if klass == MergeRequest && issuable.requires_approve?
-        expect(assigns[:suggested_approvers]).to be_an(Array)
-      end
     end
 
     it 'renders json error message when format is json' do