Commit d1e96959 authored by Mark Chao's avatar Mark Chao

Remove code owner from suggestion

As this is automatically done in !7933
parent ebb3c023
......@@ -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
end
def show_code_owner_tips?
......@@ -50,62 +46,25 @@ class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple
@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?
set_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
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
......@@ -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.
......@@ -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,48 +43,22 @@ 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
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)
it 'displays committers' do
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
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)
......@@ -111,40 +68,21 @@ describe MergeRequestApproverPresenter do
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_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,27 +90,12 @@ 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)
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
......@@ -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)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment