Commit 34a04dea authored by Doug Stull's avatar Doug Stull

Filter unconnected-to-user members from receiving access request emails

- not valid since no associated user exists.

Changelog: fixed
parent 6db586a8
......@@ -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?
......
......@@ -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
......
......@@ -2421,7 +2421,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)
......
......@@ -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
......
---
title: Filter out unconnected-to-user members from receiving on access request emails
merge_request: 61819
author:
type: fixed
......@@ -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
......
......@@ -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?
......
......@@ -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
......
......@@ -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 }
......
......@@ -6303,23 +6303,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
......
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