Commit db65d0dc authored by Mark Chao's avatar Mark Chao

Allows handling multiple paths for efficiency

Add `members` because MR approver suggestion
needs to display non-members too.
Rename `users` to `raw_users` for clarity.

Add reader for whether code owner is defined,
which is used to decide whether to display tips.
parent 98d3f894
...@@ -7,7 +7,7 @@ module Gitlab ...@@ -7,7 +7,7 @@ module Gitlab
def self.for_blob(blob) def self.for_blob(blob)
if blob.project.feature_available?(:code_owners) 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 else
User.none # rubocop: disable CodeReuse/ActiveRecord User.none # rubocop: disable CodeReuse/ActiveRecord
end end
......
...@@ -3,17 +3,30 @@ ...@@ -3,17 +3,30 @@
module Gitlab module Gitlab
module CodeOwners module CodeOwners
class Loader class Loader
def initialize(project, ref, path) def initialize(project, ref, paths)
@project, @ref, @path = project, ref, path @project, @ref, @paths = project, ref, Array(paths)
end end
def users def members
return User.none if code_owners_file.empty? # rubocop: disable CodeReuse/ActiveRecord @_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) @_raw_users ||= begin
extracted_users = Gitlab::UserExtractor.new(owners).users 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 end
private private
......
...@@ -12,10 +12,12 @@ describe Gitlab::CodeOwners::Loader do ...@@ -12,10 +12,12 @@ describe Gitlab::CodeOwners::Loader do
let!(:email_owner) { create(:user, username: 'owner-2') } let!(:email_owner) { create(:user, username: 'owner-2') }
let!(:owner_3) { create(:user, username: 'owner-3') } let!(:owner_3) { create(:user, username: 'owner-3') }
let!(:documentation_owner) { create(:user, username: 'documentation-owner') } let!(:documentation_owner) { create(:user, username: 'documentation-owner') }
let!(:test_owner) { create(:user, username: 'test-owner') }
let(:codeowner_content) do let(:codeowner_content) do
<<~CODEOWNERS <<~CODEOWNERS
docs/* @documentation-owner docs/* @documentation-owner
docs/CODEOWNERS @owner-1 owner2@gitlab.org @owner-3 @documentation-owner docs/CODEOWNERS @owner-1 owner2@gitlab.org @owner-3 @documentation-owner
spec/* @test-owner
CODEOWNERS CODEOWNERS
end end
let(:codeowner_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) } let(:codeowner_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) }
...@@ -23,28 +25,68 @@ describe Gitlab::CodeOwners::Loader do ...@@ -23,28 +25,68 @@ describe Gitlab::CodeOwners::Loader do
before do before do
create(:email, user: email_owner, email: 'owner2@gitlab.org') create(:email, user: email_owner, email: 'owner2@gitlab.org')
allow(project.repository).to receive(:code_owners_blob).and_return(codeowner_blob)
end
describe '#non_members' do
before do
project.add_developer(owner_1) project.add_developer(owner_1)
project.add_developer(email_owner) project.add_developer(email_owner)
project.add_developer(documentation_owner) project.add_developer(test_owner)
end
allow(project.repository).to receive(:code_owners_blob).and_return(codeowner_blob) 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 end
describe '#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 it 'returns all existing users that are members of the project' do
expect(loader.users).to contain_exactly(owner_1, email_owner, documentation_owner) expect(loader.members).to contain_exactly(owner_1, email_owner, documentation_owner)
end end
it 'does not return users that are not members of the project' do it 'does not return users that are not members of the project' do
expect(loader.users).not_to include(owner_3) expect(loader.members).not_to include(owner_3)
end end
it 'includes group members of the project' do it 'includes group members of the project' do
group.add_developer(owner_3) group.add_developer(owner_3)
expect(loader.users).to include(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 owners' do
expect(loader.raw_users).to contain_exactly(owner_1, owner_3, email_owner, documentation_owner)
end
end
context 'for multiple paths with code owners' do
let(:path) { ['docs/test.rb', 'spec/spec_helper.rb', 'docs/foo.rb'] }
it 'returns all owners for all paths' do
expect(loader.raw_users).to contain_exactly(documentation_owner, test_owner)
end end
end end
...@@ -52,7 +94,7 @@ describe Gitlab::CodeOwners::Loader do ...@@ -52,7 +94,7 @@ describe Gitlab::CodeOwners::Loader do
let(:path) { 'no-codeowner' } let(:path) { 'no-codeowner' }
it 'returns no users' do it 'returns no users' do
expect(loader.users).to be_empty expect(loader.raw_users).to be_empty
end end
end end
end end
...@@ -61,7 +103,7 @@ describe Gitlab::CodeOwners::Loader do ...@@ -61,7 +103,7 @@ describe Gitlab::CodeOwners::Loader do
let(:codeowner_blob) { nil } let(:codeowner_blob) { nil }
it 'returns no users without failing' do it 'returns no users without failing' do
expect(loader.users).to be_empty expect(loader.raw_users).to be_empty
end end
end end
...@@ -69,7 +111,7 @@ describe Gitlab::CodeOwners::Loader do ...@@ -69,7 +111,7 @@ describe Gitlab::CodeOwners::Loader do
it 'only calls out to the repository once' do it 'only calls out to the repository once' do
expect(project.repository).to receive(:code_owners_blob).once expect(project.repository).to receive(:code_owners_blob).once
2.times { loader.users } 2.times { loader.raw_users }
end end
it 'only processes the file once' do it 'only processes the file once' do
...@@ -77,7 +119,31 @@ describe Gitlab::CodeOwners::Loader do ...@@ -77,7 +119,31 @@ describe Gitlab::CodeOwners::Loader do
expect(code_owners_file).to receive(:get_parsed_data).once.and_call_original 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 end
end end
......
...@@ -11,7 +11,7 @@ module Gitlab ...@@ -11,7 +11,7 @@ module Gitlab
USERNAME_REGEXP = User.reference_pattern USERNAME_REGEXP = User.reference_pattern
def initialize(text) def initialize(text)
@text = text @text = text.is_a?(Array) ? text.join(' ') : text
end end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
......
...@@ -38,6 +38,18 @@ describe Gitlab::UserExtractor do ...@@ -38,6 +38,18 @@ describe Gitlab::UserExtractor do
expect(extractor.users).to include(user) expect(extractor.users).to include(user)
end 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 end
describe '#matches' do describe '#matches' do
......
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