Merge branch 'id-groups-in-codeowners' into 'master'

Allow adding groups to CODEOWNERS file

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

app/models/group.rb

@@ -63,6 +63,8 @@ class Group < Namespace
   after_save :update_two_factor_requirement
   after_update :path_changed_hook, if: :saved_change_to_path?
 
+  scope :with_users, -> { includes(:users) }
+
   class << self
     def sort_by_attribute(method)
       if method == 'storage_size_desc'

doc/user/project/code_owners.md

 # Code Owners **[STARTER]**
 
-> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6916)
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6916)
 in [GitLab Starter](https://about.gitlab.com/pricing/) 11.3.
+> - [Support for group namespaces](https://gitlab.com/gitlab-org/gitlab-ce/issues/53182) added in GitLab Starter 12.1.
 
-You can use a `CODEOWNERS` file to specify users that are responsible
-for certain files in a repository.
+You can use a `CODEOWNERS` file to specify users or
+[shared groups](members/share_project_with_groups.md)
+that are responsible for certain files in a repository.
 
 You can choose and add the `CODEOWNERS` file in three places:
 
@@ -25,7 +27,8 @@ the given file.
 
 Files can be specified using the same kind of patterns you would use
 in the `.gitignore` file followed by the `@username` or email of one
-or more users that should be owners of the file.
+or more users or by the `@name` of one or more groups that should
+be owners of the file.
 
 The order in which the paths are defined is significant: the last
 pattern that matches a given path will be used to find the code
@@ -63,6 +66,10 @@ CODEOWNERS @multiple @owners	@tab-separated
 # owner for the LICENSE file
 LICENSE @legal this_does_not_match janedoe@gitlab.com
 
+# Group names can be used to match groups and nested groups to specify
+# them as owners for a file
+README @group @group/with-nested/subgroup
+
 # Ending a path in a `/` will specify the code owners for every file
 # nested in that directory, on any level
 /docs/ @all-docs

ee/app/services/merge_requests/sync_code_owner_approval_rules.rb

@@ -19,6 +19,8 @@ module MergeRequests
         rule ||= create_rule(entry)
 
         rule.users = entry.users
+        rule.groups = entry.groups
+
         rule.save
       end
     end

ee/changelogs/unreleased/id-groups-in-codeowners.yml

+---
+title: Allow adding groups to CODEOWNERS file
+merge_request: 14071
+author:
+type: added

ee/lib/gitlab/code_owners/entry.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module CodeOwners
     class Entry
+      include ::Gitlab::Utils::StrongMemoize
+
       Data = Struct.new(:pattern, :owner_line)
 
       attr_reader :data
@@ -14,12 +16,37 @@ module Gitlab
         @data = Data.new(pattern, owner_line)
       end
 
+      def all_users
+        strong_memoize(:all_users) do
+          group_members = groups.flat_map do |group|
+            raise "CodeOwners for #{group.full_path} not loaded" unless group.users.loaded?
+
+            group.users
+          end
+
+          (group_members + users).uniq
+        end
+      end
+
       def users
         raise "CodeOwners for #{owner_line} not loaded" unless defined?(@users)
 
         @users.to_a
       end
 
+      def groups
+        raise "CodeOwners groups for #{owner_line} not loaded" unless defined?(@groups)
+
+        @groups.to_a
+      end
+
+      def add_matching_groups_from(new_groups)
+        @groups ||= Set.new
+
+        matching_groups = new_groups.select { |u| matching_group?(u) }
+        @groups.merge(matching_groups)
+      end
+
       def add_matching_users_from(new_users)
         @users ||= Set.new
 
@@ -37,19 +64,23 @@ module Gitlab
       private
 
       def extractor
-        @extractor ||= Gitlab::UserExtractor.new(owner_line)
+        @extractor ||= Gitlab::CodeOwners::ReferenceExtractor.new(owner_line)
       end
 
       def emails
         @emails ||= extractor.emails.map(&:downcase)
       end
 
-      def usernames
-        @usernames ||= extractor.usernames.map(&:downcase)
+      def names
+        @names ||= extractor.names.map(&:downcase)
+      end
+
+      def matching_group?(group)
+        names.include?(group.full_path.downcase)
       end
 
       def matching_user?(user)
-        usernames.include?(user.username.downcase) || matching_emails?(user)
+        names.include?(user.username.downcase) || matching_emails?(user)
       end
 
       def matching_emails?(user)

ee/lib/gitlab/code_owners/groups_loader.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module CodeOwners
+    class GroupsLoader
+      def initialize(project, extractor)
+        @project = project
+        @extractor = extractor
+      end
+
+      def load_to(entries)
+        groups = load_groups
+        entries.each do |entry|
+          entry.add_matching_groups_from(groups)
+        end
+      end
+
+      private
+
+      attr_reader :extractor, :project
+
+      def load_groups
+        return Group.none if extractor.names.empty?
+
+        groups = project.invited_groups.where_full_path_in(extractor.names)
+        groups.with_route.with_users
+      end
+    end
+  end
+end

ee/lib/gitlab/code_owners/loader.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module CodeOwners
     class Loader
+      include ::Gitlab::Utils::StrongMemoize
+
       def initialize(project, ref, paths)
         @project, @ref, @paths = project, ref, Array(paths)
       end
@@ -10,11 +12,25 @@ module Gitlab
       def entries
         return [] if empty_code_owners?
 
-        @entries ||= load_entries
+        strong_memoize(:entries) do
+          entries = load_bare_entries_for_paths
+
+          # a single extractor is used here, since usernames and groupnames
+          # share the same pattern. This way we don't need to match it twice.
+          owner_lines = entries.map(&:owner_line)
+          extractor = Gitlab::CodeOwners::ReferenceExtractor.new(owner_lines)
+
+          UsersLoader.new(@project, extractor).load_to(entries)
+          GroupsLoader.new(@project, extractor).load_to(entries)
+
+          entries
+        end
       end
 
       def members
-        @members ||= entries.map(&:users).flatten.uniq
+        strong_memoize(:members) do
+          entries.flat_map(&:all_users).uniq
+        end
       end
 
       def empty_code_owners?
@@ -23,22 +39,12 @@ module Gitlab
 
       private
 
-      def load_entries
-        entries = @paths.map { |path| code_owners_file.entry_for_path(path) }.compact.uniq
-        members = all_members_for_entries(entries)
-
-        entries.each do |entry|
-          entry.add_matching_users_from(members)
+      def load_bare_entries_for_paths
+        entries = @paths.map do |path|
+          code_owners_file.entry_for_path(path)
         end
 
-        entries
-      end
-
-      def all_members_for_entries(entries)
-        owner_lines = entries.map(&:owner_line)
-        all_users = Gitlab::UserExtractor.new(owner_lines).users.with_emails
-
-        @project.members_among(all_users)
+        entries.compact.uniq
       end
 
       def code_owners_file

ee/lib/gitlab/code_owners/reference_extractor.rb

+# frozen_string_literal: true
+
+# This class extracts all references found in a piece
+# it's either @name or email address
+
+module Gitlab
+  module CodeOwners
+    class ReferenceExtractor
+      # Not using `Devise.email_regexp` to filter out any chars that an email
+      # does not end with and not pinning the email to a start of end of a string.
+      EMAIL_REGEXP = /(?<email>([^@\s]+@[^@\s]+(?<!\W)))/.freeze
+      NAME_REGEXP = User.reference_pattern
+
+      def initialize(text)
+        # EE passes an Array to `text` in a few places, so we want to support both
+        # here.
+        @text = Array(text).join(' ')
+      end
+
+      def names
+        matches[:names]
+      end
+
+      def emails
+        matches[:emails]
+      end
+
+      def references
+        return [] if @text.blank?
+
+        @references ||= matches.values.flatten.uniq
+      end
+
+      private
+
+      def matches
+        @matches ||= {
+          emails: @text.scan(EMAIL_REGEXP).flatten.uniq,
+          names: @text.scan(NAME_REGEXP).flatten.uniq
+        }
+      end
+    end
+  end
+end

ee/lib/gitlab/code_owners/users_loader.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module CodeOwners
+    class UsersLoader
+      def initialize(project, extractor)
+        @project = project
+        @extractor = extractor
+      end
+
+      def load_to(entries)
+        members = project.members_among(users)
+        entries.each do |entry|
+          entry.add_matching_users_from(members)
+        end
+      end
+
+      private
+
+      attr_reader :extractor, :project
+
+      def users
+        return User.none if extractor.references.empty?
+
+        relations = []
+        relations << User.by_any_email(extractor.emails) if extractor.emails.any?
+        relations << User.by_username(extractor.names) if extractor.names.any?
+
+        User.from_union(relations).with_emails
+      end
+    end
+  end
+end

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

@@ -2,12 +2,19 @@
 require 'spec_helper'
 
 describe Gitlab::CodeOwners::Entry do
-  subject(:entry) { described_class.new('/**/file', '@user jane@gitlab.org') }
+  subject(:entry) { described_class.new('/**/file', '@user jane@gitlab.org @group @group/nested-group') }
+
   let(:user) { build(:user, username: 'user') }
+  let(:group_user) { create(:user) }
+  let(:group) do
+    group = create(:group, path: 'Group')
+    group.add_developer(group_user)
+    group
+  end
 
   it 'is uniq by the pattern and owner line' do
-    equal_entry = described_class.new('/**/file', '@user jane@gitlab.org')
-    other_entry = described_class.new('/**/other_file', '@user jane@gitlab.org')
+    equal_entry = described_class.new('/**/file', '@user jane@gitlab.org @group @group/nested-group')
+    other_entry = described_class.new('/**/other_file', '@user jane@gitlab.org @group')
 
     expect(equal_entry).to eq(entry)
     expect([entry, equal_entry, other_entry].uniq).to contain_exactly(entry, other_entry)
@@ -25,6 +32,47 @@ describe Gitlab::CodeOwners::Entry do
     end
   end
 
+  describe '#all_users' do
+    it 'raises an error if users have not been loaded for groups' do
+      entry.add_matching_groups_from([group])
+
+      expect { entry.all_users }.to raise_error(/not loaded/)
+    end
+
+    it 'returns users and users from groups' do
+      user.save!
+      group.add_reporter(user)
+
+      entry.add_matching_groups_from(Group.with_users)
+      entry.add_matching_users_from([user])
+
+      expect(entry.all_users).to contain_exactly(user, group_user)
+    end
+  end
+
+  describe '#groups' do
+    it 'raises an error if no groups have been added' do
+      expect { entry.groups }.to raise_error(/not loaded/)
+    end
+
+    it 'returns mentioned groups' do
+      entry.add_matching_groups_from([group])
+
+      expect(entry.groups).to eq([group])
+    end
+  end
+
+  describe '#add_matching_groups_from' do
+    it 'returns only mentioned groups, case-insensitively' do
+      group2 = create(:group, path: 'Group2')
+      nested_group = create(:group, path: 'nested-group', parent: group)
+
+      entry.add_matching_groups_from([group, group2, nested_group])
+
+      expect(entry.groups).to eq([group, nested_group])
+    end
+  end
+
   describe '#add_matching_users_from' do
     it 'does not add the same user twice' do
       2.times { entry.add_matching_users_from([user]) }

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::CodeOwners::GroupsLoader do
+  let(:text) do
+    <<~TXT
+    This is a long text that mentions some groups.
+    @group-1 and @group-doesnt-exist take a walk in the park.
+    There they meet @group-2 and @group-1/with-nested/Group-3
+    TXT
+  end
+
+  let(:extractor) { Gitlab::CodeOwners::ReferenceExtractor.new(text) }
+  let(:project) { create(:project, :public) }
+  let(:entry) { double('Entries') }
+
+  describe '#load_to' do
+    subject(:load_groups) do
+      described_class.new(project, extractor).load_to([entry])
+    end
+
+    before do
+      allow(entry).to receive(:add_matching_groups_from)
+    end
+
+    context 'input has no matching group paths' do
+      let(:text) { 'My test' }
+
+      it 'returns an empty list of groups' do
+        load_groups
+
+        expect(entry).to have_received(:add_matching_groups_from).with([])
+      end
+    end
+
+    context 'nil input' do
+      let(:text) { nil }
+
+      it 'returns an empty relation when nil was passed' do
+        load_groups
+
+        expect(entry).to have_received(:add_matching_groups_from).with([])
+      end
+    end
+
+    context 'input matches group paths' do
+      let(:project) { create(:project, :private) }
+
+      it 'returns the groups case insensitive for names' do
+        group = create(:group, path: "GROUP-1")
+        create(:group, path: "GROUP-2")
+        project.invited_groups << group
+
+        load_groups
+
+        expect(entry).to have_received(:add_matching_groups_from).with([group])
+      end
+    end
+
+    context 'input as array of strings' do
+      let(:text) { super().lines }
+
+      it 'is treated as one string' do
+        group_1 = create(:group, path: 'GROup-1')
+        group_2 = create(:group, path: 'GROUP-2')
+        create(:group, path: 'group-3')
+        project.invited_groups << [group_1, group_2]
+
+        load_groups
+
+        expect(entry).to have_received(:add_matching_groups_from) do |args|
+          expect(args).to contain_exactly(group_2, group_1)
+        end
+      end
+    end
+
+    context 'nested groups' do
+      it 'returns nested groups by mentioned full paths' do
+        group_1 = create(:group, path: 'GROup-1')
+        group_2 = create(:group, path: 'with-nested', parent: group_1)
+        create(:group, path: 'not-invited')
+        nested_group = create(:group, path: 'group-3', parent: group_2)
+        project.invited_groups << [group_1, group_2, nested_group]
+
+        load_groups
+
+        expect(entry).to have_received(:add_matching_groups_from) do |args|
+          expect(args).to contain_exactly(group_1, nested_group)
+        end
+      end
+    end
+  end
+end

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

@@ -8,18 +8,19 @@ describe Gitlab::CodeOwners::Loader do
   set(:project) { create(:project, namespace: group) }
   subject(:loader) { described_class.new(project, 'with-codeowners', paths) }
 
-  let!(:owner_1) { create(:user, username: 'owner-1') }
-  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
+    spec/* @test-owner @test-group @test-group/nested-group
     CODEOWNERS
   end
+
+  let!(:owner_1) { create(:user, username: 'owner-1') }
+  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_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) }
   let(:paths) { 'docs/CODEOWNERS' }
 
@@ -57,18 +58,48 @@ describe Gitlab::CodeOwners::Loader do
     end
 
     context 'for multiple paths' do
+      let(:project) { create(:project, :public, namespace: group) }
       let(:paths) { ['docs/CODEOWNERS', 'spec/loader_spec.rb', 'spec/entry_spec.rb'] }
 
       it 'loads 2 entries' do
-        other_entry = Gitlab::CodeOwners::Entry.new('spec/*', '@test-owner')
+        other_entry = Gitlab::CodeOwners::Entry.new('spec/*', '@test-owner @test-group @test-group/nested-group')
 
         expect(loader.entries).to contain_exactly(expected_entry, other_entry)
       end
 
-      it 'only performs 2 query for users' do
-        # One query for users, one for the emails to later divide them across the
-        # entries
-        expect { loader.entries }.not_to exceed_query_limit(2)
+      it 'only performs 3 query for users and groups' do
+        test_group = create(:group, path: 'test-group')
+        test_group.add_developer(create(:user))
+
+        another_group = create(:group, parent: test_group, path: 'nested-group')
+        another_group.add_developer(create(:user))
+
+        project.invited_groups << [test_group, another_group]
+
+        # One query for users, one for the emails to later divide them across the entries
+        # one for groups with joined routes and users
+        expect { loader.entries }.not_to exceed_query_limit(3)
+      end
+    end
+
+    context 'group as a code owner' do
+      let(:paths) { ['spec/loader_spec.rb'] }
+      let(:expected_entry) { Gitlab::CodeOwners::Entry.new('spec/*', '@test-owner @test-group @test-group/nested-group') }
+
+      it 'loads group members as code owners' do
+        test_group = create(:group, path: 'test-group')
+        project.invited_groups << test_group
+
+        group_user = create(:user)
+
+        test_group.add_developer(group_user)
+        test_group.add_developer(test_owner)
+
+        expect(loader.entries).to contain_exactly(expected_entry)
+        expect(loader.members).to contain_exactly(group_user, test_owner)
+
+        entry = loader.entries.first
+        expect(entry.groups).to contain_exactly(test_group)
       end
     end
 

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::CodeOwners::ReferenceExtractor do
+  let(:text) do
+    <<~TXT
+    This is a long text that mentions some users.
+    @user-1, @user-2 and user@gitlab.org take a walk in the park.
+    There they meet @user-4 that was out with other-user@gitlab.org.
+    @user-1 thought it was late, so went home straight away not to
+    run into some @group @group/nested-on/other-group
+    TXT
+  end
+
+  subject(:extractor) { described_class.new(text) }
+
+  describe '#emails' do
+    it 'includes all mentioned email adresses' do
+      expect(extractor.emails).to contain_exactly('user@gitlab.org', 'other-user@gitlab.org')
+    end
+  end
+
+  describe '#names' do
+    it 'includes all mentioned usernames and groupnames' do
+      expect(extractor.names).to contain_exactly(
+        'user-1', 'user-2', 'user-4', 'group', 'group/nested-on/other-group'
+      )
+    end
+  end
+
+  describe '#references' do
+    it 'includes all user-references once' do
+      expect(extractor.references).to contain_exactly(
+        'user-1', 'user-2', 'user@gitlab.org', 'user-4',
+        'other-user@gitlab.org', 'group', 'group/nested-on/other-group'
+      )
+    end
+  end
+end

spec/lib/gitlab/user_extractor_spec.rb → ee/spec/lib/gitlab/code_owners/users_loader_spec.rb

@@ -2,77 +2,95 @@
 
 require 'spec_helper'
 
-describe Gitlab::UserExtractor do
+describe Gitlab::CodeOwners::UsersLoader do
   let(:text) do
     <<~TXT
-    This is a long texth that mentions some users.
+    This is a long text that mentions some users.
     @user-1, @user-2 and user@gitlab.org take a walk in the park.
     There they meet @user-4 that was out with other-user@gitlab.org.
     @user-1 thought it was late, so went home straight away
     TXT
   end
-  subject(:extractor) { described_class.new(text) }
 
-  describe '#users' do
-    it 'returns an empty relation when nil was passed' do
-      extractor = described_class.new(nil)
+  let(:extractor) { Gitlab::CodeOwners::ReferenceExtractor.new(text) }
+  let(:project) { create(:project) }
+  let(:entry) { double('Entries') }
 
-      expect(extractor.users).to be_empty
-      expect(extractor.users).to be_a(ActiveRecord::Relation)
+  describe '#load_to' do
+    subject(:load_users) do
+      described_class.new(project, extractor).load_to([entry])
+    end
+
+    before do
+      allow(entry).to receive(:add_matching_users_from)
+    end
+
+    context 'input has no matching e-mail or usernames' do
+      let(:text) { 'My test' }
+
+      it 'returns an empty list of users' do
+        load_users
+
+        expect(entry).to have_received(:add_matching_users_from).with([])
+      end
+    end
+
+    context 'nil input' do
+      let(:text) { nil }
+
+      it 'returns an empty relation when nil was passed' do
+        load_users
+
+        expect(entry).to have_received(:add_matching_users_from).with([])
+      end
     end
 
     it 'returns the user case insensitive for usernames' do
       user = create(:user, username: "USER-4")
+      project.add_developer(user)
+
+      load_users
 
-      expect(extractor.users).to include(user)
+      expect(entry).to have_received(:add_matching_users_from).with([user])
     end
 
     it 'returns users by primary email' do
       user = create(:user, email: 'user@gitlab.org')
+      project.add_developer(user)
 
-      expect(extractor.users).to include(user)
+      load_users
+
+      expect(entry).to have_received(:add_matching_users_from).with([user])
     end
 
     it 'returns users by secondary email' do
       user = create(:email, email: 'other-user@gitlab.org').user
+      project.add_developer(user)
+
+      load_users
 
-      expect(extractor.users).to include(user)
+      expect(entry).to have_received(:add_matching_users_from).with([user])
     end
 
     context 'input as array of strings' do
-      it 'is treated as one string' do
-        extractor = described_class.new(text.lines)
+      let(:text) { super().lines }
 
+      it 'is treated as one string' do
         user_1 = create(:user, username: "USER-1")
-        user_4 = create(:user, username: "USER-4")
-        user_email = create(:user, email: 'user@gitlab.org')
+        project.add_guest(user_1)
 
-        expect(extractor.users).to contain_exactly(user_1, user_4, user_email)
-      end
-    end
-  end
-
-  describe '#matches' do
-    it 'includes all mentioned email adresses' do
-      expect(extractor.matches[:emails]).to contain_exactly('user@gitlab.org', 'other-user@gitlab.org')
-    end
+        user_4 = create(:user, username: "USER-4")
+        project.add_reporter(user_4)
 
-    it 'includes all mentioned usernames' do
-      expect(extractor.matches[:usernames]).to contain_exactly('user-1', 'user-2', 'user-4')
-    end
+        user_email = create(:user, email: 'user@gitlab.org')
+        project.add_maintainer(user_email)
 
-    context 'input has no matching e-mail or usernames' do
-      it 'returns an empty list of users' do
-        extractor = described_class.new('My test')
+        load_users
 
-        expect(extractor.users).to be_empty
+        expect(entry).to have_received(:add_matching_users_from) do |args|
+          expect(args).to contain_exactly(user_1, user_4, user_email)
+        end
       end
     end
   end
-
-  describe '#references' do
-    it 'includes all user-references once' do
-      expect(extractor.references).to contain_exactly('user-1', 'user-2', 'user@gitlab.org', 'user-4', 'other-user@gitlab.org')
-    end
-  end
 end

ee/spec/services/merge_requests/sync_code_owner_approval_rules_spec.rb

@@ -6,13 +6,18 @@ describe MergeRequests::SyncCodeOwnerApprovalRules do
   let(:merge_request) { create(:merge_request) }
   let(:rb_owners) { create_list(:user, 2) }
   let(:doc_owners) { create_list(:user, 2) }
-  let(:rb_entry) { build_entry('*.rb', rb_owners) }
-  let(:doc_entry) { build_entry('doc/*', doc_owners) }
+  let(:rb_group_owners) { create_list(:group, 2) }
+  let(:doc_group_owners) { create_list(:group, 2) }
+  let(:rb_entry) { build_entry('*.rb', rb_owners, rb_group_owners) }
+  let(:doc_entry) { build_entry('doc/*', doc_owners, doc_group_owners) }
   let(:entries) { [rb_entry, doc_entry] }
 
-  def build_entry(pattern, users)
-    entry = Gitlab::CodeOwners::Entry.new(pattern, users.map(&:to_reference).join(' '))
+  def build_entry(pattern, users, groups)
+    text = (users + groups).map(&:to_reference).join(' ')
+    entry = Gitlab::CodeOwners::Entry.new(pattern, text)
+
     entry.add_matching_users_from(users)
+    entry.add_matching_groups_from(groups)
 
     entry
   end
@@ -34,6 +39,9 @@ describe MergeRequests::SyncCodeOwnerApprovalRules do
 
       expect(rb_rule.users).to eq(rb_owners)
       expect(doc_rule.users).to eq(doc_owners)
+
+      expect(rb_rule.groups).to match_array(rb_group_owners)
+      expect(doc_rule.groups).to match_array(doc_group_owners)
     end
 
     it 'deletes rules that are not relevant anymore' do
@@ -48,11 +56,13 @@ describe MergeRequests::SyncCodeOwnerApprovalRules do
     it 'updates rules for which the users changed' do
       other_rule = create(:code_owner_rule, merge_request: merge_request, name: '*.rb')
       other_rule.users += doc_owners
+      other_rule.groups += doc_group_owners
       other_rule.save!
 
       service.execute
 
       expect(other_rule.reload.users).to eq(rb_owners)
+      expect(other_rule.reload.groups).to match_array(rb_group_owners)
     end
   end
 end

lib/gitlab/user_extractor.rb

-# frozen_string_literal: true
-
-# This class extracts all users found in a piece of text by the username or the
-# email address
-
-module Gitlab
-  class UserExtractor
-    # Not using `Devise.email_regexp` to filter out any chars that an email
-    # does not end with and not pinning the email to a start of end of a string.
-    EMAIL_REGEXP = /(?<email>([^@\s]+@[^@\s]+(?<!\W)))/.freeze
-    USERNAME_REGEXP = User.reference_pattern
-
-    def initialize(text)
-      # EE passes an Array to `text` in a few places, so we want to support both
-      # here.
-      @text = Array(text).join(' ')
-    end
-
-    def users
-      return User.none unless @text.present?
-      return User.none if references.empty?
-
-      @users ||= User.from_union(union_relations)
-    end
-
-    def usernames
-      matches[:usernames]
-    end
-
-    def emails
-      matches[:emails]
-    end
-
-    def references
-      @references ||= matches.values.flatten
-    end
-
-    def matches
-      @matches ||= {
-        emails: @text.scan(EMAIL_REGEXP).flatten.uniq,
-        usernames: @text.scan(USERNAME_REGEXP).flatten.uniq
-      }
-    end
-
-    private
-
-    def union_relations
-      relations = []
-
-      relations << User.by_any_email(emails) if emails.any?
-      relations << User.by_username(usernames) if usernames.any?
-
-      relations
-    end
-  end
-end