Merge branch '328412-filter-out-invited-owners-on-member-access-requests' into 'master'

Filter out non-user members from receiving on access request emails

See merge request gitlab-org/gitlab!61819

app/models/group.rb

@@ -649,7 +649,7 @@ class Group < Namespace
   end
 
   def access_request_approvers_to_be_notified
-    members.owners.order_recent_sign_in.limit(ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT)
+    members.owners.connected_to_user.order_recent_sign_in.limit(ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT)
   end
 
   def supports_events?

app/models/member.rb

@@ -92,11 +92,13 @@ class Member < ApplicationRecord
       .reorder(nil)
   end
 
+  scope :connected_to_user, -> { where.not(user_id: nil) }
+
   # This scope is exclusively used to get the members
   # that can possibly have project_authorization records
   # to projects/groups.
   scope :authorizable, -> do
-    where.not(user_id: nil)
+    connected_to_user
       .non_request
       .non_minimal_access
   end

app/models/project.rb

@@ -2454,7 +2454,7 @@ class Project < ApplicationRecord
   end
 
   def access_request_approvers_to_be_notified
-    members.maintainers.order_recent_sign_in.limit(ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT)
+    members.maintainers.connected_to_user.order_recent_sign_in.limit(ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT)
   end
 
   def pages_lookup_path(trim_prefix: nil, domain: nil)

app/presenters/projects/import_export/project_export_presenter.rb

@@ -31,11 +31,11 @@ module Projects
       def group_members
         return [] unless current_user.can?(:admin_group, project.group)
 
-        # We need `.where.not(user_id: nil)` here otherwise when a group has an
+        # We need `.connected_to_user` here otherwise when a group has an
         # invitee, it would make the following query return 0 rows since a NULL
         # user_id would be present in the subquery
         # See http://stackoverflow.com/questions/129077/not-in-clause-and-null-values
-        non_null_user_ids = project.project_members.where.not(user_id: nil).select(:user_id)
+        non_null_user_ids = project.project_members.connected_to_user.select(:user_id)
         GroupMembersFinder.new(project.group).execute.where.not(user_id: non_null_user_ids)
       end
       # rubocop: enable CodeReuse/ActiveRecord

changelogs/unreleased/328412-filter-out-invited-owners-on-member-access-requests.yml

+---
+title: Filter out unconnected-to-user members from receiving on access request emails
+merge_request: 61819
+author:
+type: fixed

ee/spec/controllers/groups/group_members_controller_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe Groups::GroupMembersController do
 
         control = ActiveRecord::QueryRecorder.new(skip_cached: false) { get :index, params: { group_id: group } }
         create_list(:group_member, 5, group: group, created_by: user)
-        create_list(:group_member, 5, :invited, :developer, group: group, created_by: user)
+        create_list(:group_member, 5, :invited, group: group, created_by: user)
         create_list(:group_member, 5, :access_request, group: group)
         # locally 47 vs 52 GDK vs 57 CI
         unresolved_n_plus_ones = 5 # still have a few queries created by can_update/can_remove that should be reduced

lib/api/helpers/members_helpers.rb

@@ -18,7 +18,7 @@ module API
 
       # rubocop: disable CodeReuse/ActiveRecord
       def retrieve_members(source, params:, deep: false)
-        members = deep ? find_all_members(source) : source_members(source).where.not(user_id: nil)
+        members = deep ? find_all_members(source) : source_members(source).connected_to_user
         members = members.includes(:user)
         members = members.references(:user).merge(User.search(params[:query])) if params[:query].present?
         members = members.where(user_id: params[:user_ids]) if params[:user_ids].present?

spec/models/group_spec.rb

@@ -2245,22 +2245,31 @@ RSpec.describe Group do
   end
 
   describe '#access_request_approvers_to_be_notified' do
-    it 'returns a maximum of ten, active, non_requested owners of the group in recent_sign_in descending order' do
-      group = create(:group, :public)
+    let_it_be(:group) { create(:group, :public) }
 
+    it 'returns a maximum of ten owners of the group in recent_sign_in descending order' do
       users = create_list(:user, 12, :with_sign_ins)
       active_owners = users.map do |user|
         create(:group_member, :owner, group: group, user: user)
       end
 
-      create(:group_member, :owner, :blocked, group: group)
-      create(:group_member, :maintainer, group: group)
-      create(:group_member, :access_request, :owner, group: group)
-
-      active_owners_in_recent_sign_in_desc_order = group.members_and_requesters.where(id: active_owners).order_recent_sign_in.limit(10)
+      active_owners_in_recent_sign_in_desc_order = group.members_and_requesters
+                                                        .id_in(active_owners)
+                                                        .order_recent_sign_in.limit(10)
 
       expect(group.access_request_approvers_to_be_notified).to eq(active_owners_in_recent_sign_in_desc_order)
     end
+
+    it 'returns active, non_invited, non_requested owners of the group' do
+      owner = create(:group_member, :owner, source: group)
+
+      create(:group_member, :maintainer, group: group)
+      create(:group_member, :owner, :invited, group: group)
+      create(:group_member, :owner, :access_request, group: group)
+      create(:group_member, :owner, :blocked, group: group)
+
+      expect(group.access_request_approvers_to_be_notified.to_a).to eq([owner])
+    end
   end
 
   describe '.groups_including_descendants_by' do

spec/models/member_spec.rb

@@ -408,6 +408,20 @@ RSpec.describe Member do
       it { is_expected.not_to include @member_with_minimal_access }
     end
 
+    describe '.connected_to_user' do
+      subject { described_class.connected_to_user.to_a }
+
+      it { is_expected.to include @owner }
+      it { is_expected.to include @maintainer }
+      it { is_expected.to include @accepted_invite_member }
+      it { is_expected.to include @accepted_request_member }
+      it { is_expected.to include @blocked_maintainer }
+      it { is_expected.to include @blocked_developer }
+      it { is_expected.to include @requested_member }
+      it { is_expected.to include @member_with_minimal_access }
+      it { is_expected.not_to include @invited_member }
+    end
+
     describe '.authorizable' do
       subject { described_class.authorizable.to_a }
 

spec/models/project_spec.rb

@@ -6304,23 +6304,31 @@ RSpec.describe Project, factory_default: :keep do
   end
 
   describe '#access_request_approvers_to_be_notified' do
-    it 'returns a maximum of ten, active, non_requested maintainers of the project in recent_sign_in descending order' do
-      group = create(:group, :public)
-      project = create(:project, group: group)
+    let_it_be(:project) { create(:project, group: create(:group, :public)) }
 
+    it 'returns a maximum of ten maintainers of the project in recent_sign_in descending order' do
       users = create_list(:user, 12, :with_sign_ins)
       active_maintainers = users.map do |user|
-        create(:project_member, :maintainer, user: user)
+        create(:project_member, :maintainer, user: user, project: project)
       end
 
-      create(:project_member, :maintainer, :blocked, project: project)
-      create(:project_member, :developer, project: project)
-      create(:project_member, :access_request, :maintainer, project: project)
-
-      active_maintainers_in_recent_sign_in_desc_order = project.members_and_requesters.where(id: active_maintainers).order_recent_sign_in.limit(10)
+      active_maintainers_in_recent_sign_in_desc_order = project.members_and_requesters
+                                                               .id_in(active_maintainers)
+                                                               .order_recent_sign_in.limit(10)
 
       expect(project.access_request_approvers_to_be_notified).to eq(active_maintainers_in_recent_sign_in_desc_order)
     end
+
+    it 'returns active, non_invited, non_requested maintainers of the project' do
+      maintainer = create(:project_member, :maintainer, source: project)
+
+      create(:project_member, :developer, project: project)
+      create(:project_member, :maintainer, :invited, project: project)
+      create(:project_member, :maintainer, :access_request, project: project)
+      create(:project_member, :maintainer, :blocked, project: project)
+
+      expect(project.access_request_approvers_to_be_notified.to_a).to eq([maintainer])
+    end
   end
 
   describe '#pages_lookup_path' do