Merge branch '1012-remove-codeowner-suggestion' into 'master'

Remove code owner from approver suggestion

Closes #1012

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

ee/app/presenters/merge_request_approver_presenter.rb

@@ -30,11 +30,7 @@ class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple
   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
+    link_to user.name, '#', id: dom_id(user)
   end
 
   def show_code_owner_tips?
@@ -44,68 +40,28 @@ class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple
   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?
+    @users ||= users_from_git_log_authors
   end
 
   def code_owner_enabled?
     strong_memoize(:code_owner_enabled) do
-      merge_request.project.feature_available?(:code_owner_as_approver_suggestion)
+      merge_request.project.feature_available?(:code_owners)
     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
+  def users_from_git_log_authors
+    if merge_request.approvals_required > 0
+      ::Gitlab::AuthorityAnalyzer.new(merge_request, skip_user).calculate.first(merge_request.approvals_required)
+    else
+      []
     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
+      merge_request.modified_paths
     )
   end
 end

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

@@ -85,4 +85,4 @@
         .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.
+          to automatically add approvers based on file paths and file types.

ee/spec/presenters/merge_request_approver_presenter_spec.rb

@@ -5,19 +5,11 @@ 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(:file_paths) { %w{readme.md} }
   let(:approvals_required) { 10 }
-  let(:enable_code_owner_as_approver_suggestion) { true }
+  let(:enable_code_owner) { 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) }
@@ -25,29 +17,20 @@ describe MergeRequestApproverPresenter do
   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(:modified_paths).and_return(file_paths)
     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)
+    stub_licensed_features(code_owners: enable_code_owner)
   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)
+      file_paths
     ).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)
 
@@ -60,91 +43,46 @@ describe MergeRequestApproverPresenter do
   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
+    before do
+      project.add_developer(committer_a)
+      project.add_developer(committer_b)
     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
+    it 'displays committers' do
+      expect_git_log_call(committer_a)
+      expect(subject).to receive(:render_user).with(committer_a).and_call_original
 
-      context 'code_owner_as_approver_suggestion disabled' do
-        let(:enable_code_owner_as_approver_suggestion) { false }
+      subject.render
+    end
 
-        before do
-          project.add_developer(committer_a)
-        end
+    context 'approvals_required is low' do
+      let(:approvals_required) { 1 }
 
-        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
+      it 'returns the top n committers' do
+        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
+        subject.render
       end
     end
   end
 
   describe '#any?' do
     it 'returns true if any user exists' do
-      expect_code_owners_call(owner_a)
+      expect_git_log_call(committer_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
+      expect(subject).to receive(:users_from_git_log_authors).once.and_call_original
 
       subject.any?
       subject.any?
@@ -152,25 +90,10 @@ describe MergeRequestApproverPresenter do
   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)
+    it 'renders link' do
+      result = subject.render_user(committer_a)
 
-        result = subject.render_user(committer_a)
-
-        expect(result).to start_with('<a')
-      end
+      expect(result).to start_with('<a')
     end
   end
 
@@ -198,7 +121,7 @@ describe MergeRequestApproverPresenter do
     end
 
     context 'when code_owner feature is disabled' do
-      let(:enable_code_owner_as_approver_suggestion) { false }
+      let(:enable_code_owner) { false }
 
       it 'returns false' do
         expect(subject.show_code_owner_tips?).to eq(false)