Filter approvers from hidden groups out of suggested approvers

ee/app/models/approval_state.rb

@@ -120,13 +120,7 @@ class ApprovalState
     rules.concat(code_owner_rules) if code_owner
     rules.concat(report_approver_rules) if report_approver
 
-    users = rules.flat_map(&target)
-    users.uniq!
-
-    users -= approved_approvers if unactioned
-
-    users = self.class.filter_author(users, merge_request)
-    self.class.filter_committers(users, merge_request)
+    filter_approvers(rules.flat_map(&target), unactioned: unactioned)
   end
 
   # approvers_left
@@ -134,6 +128,16 @@ class ApprovalState
     strong_memoize(:unactioned_approvers) { approvers - approved_approvers }
   end
 
+  # TODO order by relevance
+  def suggested_approvers(current_user:)
+    # Ignore approvers from rules containing hidden groups
+    rules = wrapped_approval_rules.reject do |rule|
+      ApprovalRules::GroupFinder.new(rule, current_user).contains_hidden_groups?
+    end
+
+    filter_approvers(rules.flat_map(&:approvers), unactioned: true)
+  end
+
   def can_approve?(user)
     return false unless user
     return true if unactioned_approvers.include?(user)
@@ -175,6 +179,14 @@ class ApprovalState
 
   private
 
+  def filter_approvers(approvers, unactioned:)
+    approvers = approvers.uniq
+    approvers -= approved_approvers if unactioned
+    approvers = self.class.filter_author(approvers, merge_request)
+
+    self.class.filter_committers(approvers, merge_request)
+  end
+
   def has_regular_rule_with_approvers?
     regular_rules.any? { |rule| rule.approvers.present? }
   end

ee/lib/ee/api/entities.rb

@@ -464,8 +464,7 @@ module EE
         end
 
         expose :suggested_approvers, using: ::API::Entities::UserBasic do |approval_state, options|
-          # TODO order by relevance
-          approval_state.unactioned_approvers
+          approval_state.suggested_approvers(current_user: options[:current_user])
         end
 
         # @deprecated, reads from first regular rule instead

ee/spec/models/approval_state_spec.rb

@@ -904,6 +904,41 @@ describe ApprovalState do
         end
       end
     end
+
+    describe '#suggested_approvers' do
+      let(:user) { create(:user) }
+      let(:public_group) { create(:group, :public) }
+      let(:private_group) { create(:group, :private) }
+
+      let!(:private_user) { create(:group_member, group: private_group).user }
+      let!(:public_user) { create(:group_member, group: public_group).user }
+      let!(:rule1) { create_rule(groups: [private_group], users: []) }
+      let!(:rule2) { create_rule(groups: [public_group], users: []) }
+
+      subject { merge_request.approval_state.suggested_approvers(current_user: user) }
+
+      context 'user cannot see private group' do
+        it 'shows public users' do
+          is_expected.to contain_exactly(public_user)
+        end
+
+        it 'does not show users who have already approved' do
+          create(:approval, merge_request: merge_request, user: public_user)
+
+          is_expected.to be_empty
+        end
+      end
+
+      context 'user can see private group' do
+        before do
+          create(:group_member, group: private_group, user: user)
+        end
+
+        it 'shows private users' do
+          is_expected.to contain_exactly(public_user, private_user, user)
+        end
+      end
+    end
   end
 
   context 'when only a single rule is allowed' do