Commit 6a831b52 authored by Douwe Maan's avatar Douwe Maan

Merge branch '27374-groups-groupmemberscontroller-create-is-slow-due-to-sql' into 'master'

Optimize Groups::GroupMembersController#create

Closes #27374

See merge request !13825
parents 745bc356 66cfb901
......@@ -16,6 +16,7 @@ class Group < Namespace
source: :user
has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent
has_many :members_and_requesters, as: :source, class_name: 'GroupMember'
has_many :milestones
has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
......
......@@ -126,20 +126,11 @@ class Member < ActiveRecord::Base
find_by(invite_token: invite_token)
end
def add_user(source, user, access_level, current_user: nil, expires_at: nil)
user = retrieve_user(user)
def add_user(source, user, access_level, existing_members: nil, current_user: nil, expires_at: nil)
# `user` can be either a User object, User ID or an email to be invited
member = retrieve_member(source, user, existing_members)
access_level = retrieve_access_level(access_level)
# `user` can be either a User object or an email to be invited
member =
if user.is_a?(User)
source.members.find_by(user_id: user.id) ||
source.requesters.find_by(user_id: user.id) ||
source.members.build(user_id: user.id)
else
source.members.build(invite_email: user)
end
return member unless can_update_member?(current_user, member)
member.attributes = {
......@@ -165,17 +156,15 @@ class Member < ActiveRecord::Base
def add_users(source, users, access_level, current_user: nil, expires_at: nil)
return [] unless users.present?
# Collect all user ids into separate array
# so we can use single sql query to get user objects
user_ids = users.select { |user| user =~ /\A\d+\Z/ }
users = users - user_ids + User.where(id: user_ids)
emails, users, existing_members = parse_users_list(source, users)
self.transaction do
users.map do |user|
(emails + users).map! do |user|
add_user(
source,
user,
access_level,
existing_members: existing_members,
current_user: current_user,
expires_at: expires_at
)
......@@ -189,6 +178,31 @@ class Member < ActiveRecord::Base
private
def parse_users_list(source, list)
emails, user_ids, users = [], [], []
existing_members = {}
list.each do |item|
case item
when User
users << item
when Integer
user_ids << item
when /\A\d+\Z/
user_ids << item.to_i
when Devise.email_regexp
emails << item
end
end
if user_ids.present?
users.concat(User.where(id: user_ids))
existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id)
end
[emails, users, existing_members]
end
# This method is used to find users that have been entered into the "Add members" field.
# These can be the User objects directly, their IDs, their emails, or new emails to be invited.
def retrieve_user(user)
......@@ -197,6 +211,20 @@ class Member < ActiveRecord::Base
User.find_by(id: user) || User.find_by(email: user) || user
end
def retrieve_member(source, user, existing_members)
user = retrieve_user(user)
if user.is_a?(User)
if existing_members
existing_members[user.id] || source.members.build(user_id: user.id)
else
source.members_and_requesters.find_or_initialize_by(user_id: user.id)
end
else
source.members.build(invite_email: user)
end
end
def retrieve_access_level(access_level)
access_levels.fetch(access_level) { access_level.to_i }
end
......
......@@ -144,6 +144,7 @@ class Project < ActiveRecord::Base
has_many :requesters, -> { where.not(requested_at: nil) },
as: :source, class_name: 'ProjectMember', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_many :members_and_requesters, as: :source, class_name: 'ProjectMember'
has_many :deploy_keys_projects
has_many :deploy_keys, through: :deploy_keys_projects
......
......@@ -264,6 +264,7 @@ project:
- statistics
- container_repositories
- uploads
- members_and_requesters
award_emoji:
- awardable
- user
......
......@@ -9,6 +9,7 @@ describe Group do
it { is_expected.to have_many(:users).through(:group_members) }
it { is_expected.to have_many(:owners).through(:group_members) }
it { is_expected.to have_many(:requesters).dependent(:destroy) }
it { is_expected.to have_many(:members_and_requesters) }
it { is_expected.to have_many(:project_group_links).dependent(:destroy) }
it { is_expected.to have_many(:shared_projects).through(:project_group_links) }
it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
......@@ -25,22 +26,8 @@ describe Group do
group.add_developer(developer)
end
describe '#members' do
it 'includes members and exclude requesters' do
member_user_ids = group.members.pluck(:user_id)
expect(member_user_ids).to include(developer.id)
expect(member_user_ids).not_to include(requester.id)
end
end
describe '#requesters' do
it 'does not include requesters' do
requester_user_ids = group.requesters.pluck(:user_id)
expect(requester_user_ids).to include(requester.id)
expect(requester_user_ids).not_to include(developer.id)
end
it_behaves_like 'members and requesters associations' do
let(:namespace) { group }
end
end
end
......
......@@ -409,6 +409,15 @@ describe Member do
expect(members).to be_a Array
expect(members).to be_empty
end
it 'supports differents formats' do
list = ['joe@local.test', admin, user1.id, user2.id.to_s]
members = described_class.add_users(source, list, :master)
expect(members.size).to eq(4)
expect(members.first).to be_invite
end
end
end
end
......
......@@ -74,6 +74,7 @@ describe Project do
it { is_expected.to have_many(:forks).through(:forked_project_links) }
it { is_expected.to have_many(:uploads).dependent(:destroy) }
it { is_expected.to have_many(:pipeline_schedules) }
it { is_expected.to have_many(:members_and_requesters) }
context 'after initialized' do
it "has a project_feature" do
......@@ -90,22 +91,8 @@ describe Project do
project.team << [developer, :developer]
end
describe '#members' do
it 'includes members and exclude requesters' do
member_user_ids = project.members.pluck(:user_id)
expect(member_user_ids).to include(developer.id)
expect(member_user_ids).not_to include(requester.id)
end
end
describe '#requesters' do
it 'does not include requesters' do
requester_user_ids = project.requesters.pluck(:user_id)
expect(requester_user_ids).to include(requester.id)
expect(requester_user_ids).not_to include(developer.id)
end
it_behaves_like 'members and requesters associations' do
let(:namespace) { project }
end
end
......
RSpec.shared_examples 'members and requesters associations' do
describe '#members_and_requesters' do
it 'includes members and requesters' do
member_and_requester_user_ids = namespace.members_and_requesters.pluck(:user_id)
expect(member_and_requester_user_ids).to include(requester.id, developer.id)
end
end
describe '#members' do
it 'includes members and exclude requesters' do
member_user_ids = namespace.members.pluck(:user_id)
expect(member_user_ids).to include(developer.id)
expect(member_user_ids).not_to include(requester.id)
end
end
describe '#requesters' do
it 'does not include requesters' do
requester_user_ids = namespace.requesters.pluck(:user_id)
expect(requester_user_ids).to include(requester.id)
expect(requester_user_ids).not_to include(developer.id)
end
end
end
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