diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index e8c800a36cae622f90e8ceef9583818e7a214258..c8819d27da8b77c73db2a1f9625714429a5171f7 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -22,6 +22,8 @@ module ApprovalRuleLike has_and_belongs_to_many :groups, class_name: 'Group', join_table: "#{self.table_name}_groups", after_add: :audit_add, after_remove: :audit_remove + + has_many :group_members, through: :groups has_many :group_users, -> { distinct.allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/417457") }, through: :groups, source: :users @@ -81,6 +83,14 @@ def approvers @approvers ||= filter_inactive_approvers(with_role_approvers) end + def approvers_include_user?(user) + return false if filter_inactive_approvers([user]).empty? + + relation_exists?(users, column: :id, value: user.id) || + relation_exists?(role_approvers, column: :id, value: user.id) || + relation_exists?(group_members, column: :user_id, value: user.id) + end + def code_owner? raise NotImplementedError end @@ -128,6 +138,12 @@ def direct_approvers end end + def relation_exists?(relation, column:, value:) + return relation.exists?({ column => value }) unless relation.loaded? + + relation.detect { |item| item.read_attribute(column) == value } + end + def with_role_approvers if users.loaded? && group_users.loaded? users | group_users | role_approvers diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 536e848525f0415a3dd30e622c2d1e126b965f42..1bc4bf199916ffdea34b2aaec3b0d9704b073529 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -744,11 +744,9 @@ def approvals_before_merge super end - def applicable_approval_rules_for_user(user_id, target_branch = nil) + def applicable_approval_rules_for_user(user, target_branch = nil) visible_approval_rules(target_branch: target_branch).select do |rule| - rule.approvers - .allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/417461") - .pluck(:id).include?(user_id) + rule.approvers_include_user?(user) end end diff --git a/ee/app/serializers/ee/user_entity.rb b/ee/app/serializers/ee/user_entity.rb index 578ae86e922cf5e57ee301e6dc9a596192b4d394..ada28b68131dac518b766a5fc1d16956cc2d0d13 100644 --- a/ee/app/serializers/ee/user_entity.rb +++ b/ee/app/serializers/ee/user_entity.rb @@ -6,7 +6,7 @@ module UserEntity prepended do expose :applicable_approval_rules, if: proc { |_, options| options[:project]&.feature_available?(:merge_request_approvers) && options[:approval_rules] }, using: ::EE::API::Entities::ApprovalRuleShort do |user, options| - options[:project]&.applicable_approval_rules_for_user(user.id, options[:target_branch]) + options[:project]&.applicable_approval_rules_for_user(user, options[:target_branch]) end end end diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index 0c0ee82a0667a776f5cf2c1dfc6d01faad95ba5d..8d228d31c53f3f5df2bbe29268071f1bc44c1083 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -15,20 +15,56 @@ let(:subject_traits) { [] } shared_examples 'approval rule like' do - describe '#approvers' do - let(:group1_user) { create(:user) } - let(:group2_user) { create(:user) } + let(:group1_user) { create(:user) } + let(:group2_user) { create(:user) } + + before do + subject.users << user1 + subject.users << user2 + subject.groups << group1 + subject.groups << group2 + + group1.add_guest(group1_user) + group2.add_guest(group2_user) + end + + describe '#approvers_include_user?' do + let(:rule) { subject.class.find(subject.id) } - before do - subject.users << user1 - subject.users << user2 - subject.groups << group1 - subject.groups << group2 + it 'returns true for a contained user' do + expect(rule.approvers_include_user?(user1)).to be_truthy + end - group1.add_guest(group1_user) - group2.add_guest(group2_user) + it 'returns true for a group user' do + expect(rule.approvers_include_user?(group1_user)).to be_truthy end + it 'returns false for a missing user' do + expect(rule.approvers_include_user?(user3)).to be_falsey + end + + context 'when the user relations are already loaded' do + it 'returns true for a contained user' do + rule.users.to_a + + expect(rule.approvers_include_user?(user1)).to be_truthy + end + + it 'returns true for a group user' do + rule.group_members.to_a + + expect(rule.approvers_include_user?(group1_user)).to be_truthy + end + + it 'returns false for a missing user' do + rule.users.to_a + + expect(rule.approvers_include_user?(user3)).to be_falsey + end + end + end + + describe '#approvers' do shared_examples 'approvers contains the right users' do it 'contains users as direct members and group members' do rule = subject.class.find(subject.id) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 4a6924b43a89d0a90981b146d6769981a04613c7..a5b26acbf6e31509c0fd7735f1914b8001806a8d 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -1060,6 +1060,7 @@ approval_rules: - users - groups - group_users + - group_members - security_orchestration_policy_configuration - protected_branches - approval_merge_request_rule_sources