Commit 311dbdf9 authored by Adam Hegyi's avatar Adam Hegyi Committed by Bob Van Landuyt

Preload user status for mentioning users

This change uses BatchLoader to efficiently preload
UserStatus.availability for several different user arrays.
parent c5995a35
...@@ -8,6 +8,8 @@ module Users ...@@ -8,6 +8,8 @@ module Users
attr_reader :noteable attr_reader :noteable
end end
private
def noteable_owner def noteable_owner
return [] unless noteable && noteable.author.present? return [] unless noteable && noteable.author.present?
...@@ -22,23 +24,29 @@ module Users ...@@ -22,23 +24,29 @@ module Users
end end
def sorted(users) def sorted(users)
users.uniq.to_a.compact.sort_by(&:username).map do |user| users.uniq.to_a.compact.sort_by(&:username).tap do |users|
user_as_hash(user) preload_status(users)
end end
end end
def groups def groups
group_counts = GroupMember current_user.authorized_groups.with_route.sort_by(&:path)
.of_groups(current_user.authorized_groups) end
.non_request
.count_users_by_group_id
current_user.authorized_groups.with_route.sort_by(&:path).map do |group| def render_participants_as_hash(participants)
group_as_hash(group, group_counts) participants.map(&method(:participant_as_hash))
end
end end
private def participant_as_hash(participant)
case participant
when Group
group_as_hash(participant)
when User
user_as_hash(participant)
else
participant
end
end
def user_as_hash(user) def user_as_hash(user)
{ {
...@@ -46,12 +54,11 @@ module Users ...@@ -46,12 +54,11 @@ module Users
username: user.username, username: user.username,
name: user.name, name: user.name,
avatar_url: user.avatar_url, avatar_url: user.avatar_url,
availability: nil availability: lazy_user_availability(user).itself # calling #itself to avoid returning a BatchLoader instance
} }
# Return nil for availability for now due to https://gitlab.com/gitlab-org/gitlab/-/issues/285442
end end
def group_as_hash(group, group_counts) def group_as_hash(group)
{ {
type: group.class.name, type: group.class.name,
username: group.full_path, username: group.full_path,
...@@ -61,5 +68,27 @@ module Users ...@@ -61,5 +68,27 @@ module Users
mentionsDisabled: group.mentions_disabled mentionsDisabled: group.mentions_disabled
} }
end end
def group_counts
@group_counts ||= GroupMember
.of_groups(current_user.authorized_groups)
.non_request
.count_users_by_group_id
end
def preload_status(users)
users.each { |u| lazy_user_availability(u) }
end
def lazy_user_availability(user)
BatchLoader.for(user.id).batch do |user_ids, loader|
user_ids.each_slice(1_000) do |sliced_user_ids|
UserStatus
.select(:user_id, :availability)
.primary_key_in(sliced_user_ids)
.each { |status| loader.call(status.user_id, status.availability) }
end
end
end
end end
end end
...@@ -14,7 +14,7 @@ module Projects ...@@ -14,7 +14,7 @@ module Projects
groups + groups +
project_members project_members
participants.uniq render_participants_as_hash(participants.uniq)
end end
def project_members def project_members
......
...@@ -7,8 +7,14 @@ module Groups ...@@ -7,8 +7,14 @@ module Groups
def execute(noteable) def execute(noteable)
@noteable = noteable @noteable = noteable
participants = noteable_owner + participants_in_noteable + all_members + groups + group_members participants =
participants.uniq noteable_owner +
participants_in_noteable +
all_members +
groups +
group_members
render_participants_as_hash(participants.uniq)
end end
def all_members def all_members
......
...@@ -42,11 +42,11 @@ RSpec.describe Groups::ParticipantsService do ...@@ -42,11 +42,11 @@ RSpec.describe Groups::ParticipantsService do
it 'returns all participants' do it 'returns all participants' do
service = described_class.new(group, user) service = described_class.new(group, user)
service.instance_variable_set(:@noteable, epic) service.instance_variable_set(:@noteable, epic)
result = service.participants_in_noteable result = service.execute(epic)
expected_users = (@users + [user]).map(&method(:user_to_autocompletable)) expected_users = (@users + [user]).map(&method(:user_to_autocompletable))
expect(result).to match_array(expected_users) expect(result).to include(*expected_users)
end end
end end
...@@ -55,6 +55,7 @@ RSpec.describe Groups::ParticipantsService do ...@@ -55,6 +55,7 @@ RSpec.describe Groups::ParticipantsService do
let(:group) { create(:group, parent: parent_group) } let(:group) { create(:group, parent: parent_group) }
let(:subgroup) { create(:group_with_members, parent: group) } let(:subgroup) { create(:group_with_members, parent: group) }
let(:subproject) { create(:project, group: subgroup) } let(:subproject) { create(:project, group: subgroup) }
let(:epic) { create(:epic, group: group, author: user) }
it 'returns all members in parent groups, sub-groups, and sub-projects' do it 'returns all members in parent groups, sub-groups, and sub-projects' do
parent_group.add_developer(create(:user)) parent_group.add_developer(create(:user))
...@@ -63,31 +64,31 @@ RSpec.describe Groups::ParticipantsService do ...@@ -63,31 +64,31 @@ RSpec.describe Groups::ParticipantsService do
service = described_class.new(group, user) service = described_class.new(group, user)
service.instance_variable_set(:@noteable, epic) service.instance_variable_set(:@noteable, epic)
result = service.group_members result = service.execute(epic)
expected_users = (group.self_and_hierarchy.flat_map(&:users) + subproject.users) expected_users = (group.self_and_hierarchy.flat_map(&:users) + subproject.users)
.map(&method(:user_to_autocompletable)) .map(&method(:user_to_autocompletable))
expect(expected_users.count).to eq(5) expect(expected_users.count).to eq(5)
expect(result).to match_array(expected_users) expect(result).to include(*expected_users)
end end
end end
describe '#groups' do describe 'group items' do
describe 'avatar_url' do subject(:group_items) { described_class.new(group, user).execute(epic).select { |hash| hash[:type].eql?('Group') } }
let(:groups) { described_class.new(group, user).groups }
describe 'avatar_url' do
it 'returns a URL for the avatar' do it 'returns a URL for the avatar' do
expect(groups.size).to eq 1 expect(group_items.size).to eq 1
expect(groups.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png") expect(group_items.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png")
end end
it 'returns a relative URL for the avatar' do it 'returns a relative URL for the avatar' do
stub_config_setting(relative_url_root: '/gitlab') stub_config_setting(relative_url_root: '/gitlab')
stub_config_setting(url: Settings.send(:build_gitlab_url)) stub_config_setting(url: Settings.send(:build_gitlab_url))
expect(groups.size).to eq 1 expect(group_items.size).to eq 1
expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png") expect(group_items.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png")
end end
end end
end end
......
...@@ -3,57 +3,90 @@ ...@@ -3,57 +3,90 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Projects::ParticipantsService do RSpec.describe Projects::ParticipantsService do
describe '#groups' do describe '#execute' do
let_it_be(:user) { create(:user) } let(:user) { create(:user) }
let_it_be(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
let(:service) { described_class.new(project, user) } let(:noteable) { create(:issue, project: project) }
it 'avoids N+1 queries' do def run_service
group_1 = create(:group) described_class.new(project, user).execute(noteable)
group_1.add_owner(user) end
service.groups # Run general application warmup queries before do
control_count = ActiveRecord::QueryRecorder.new { service.groups }.count project.add_developer(user)
group_2 = create(:group) run_service # warmup, runs table cache queries and create queries
group_2.add_owner(user) BatchLoader::Executor.clear_current
end
expect { service.groups }.not_to exceed_query_limit(control_count) it 'avoids N+1 UserDetail queries' do
project.add_developer(create(:user))
control_count = ActiveRecord::QueryRecorder.new { run_service.to_a }.count
BatchLoader::Executor.clear_current
project.add_developer(create(:user, status: build(:user_status, availability: :busy)))
expect { run_service.to_a }.not_to exceed_query_limit(control_count)
end end
it 'returns correct user counts for groups' do it 'avoids N+1 groups queries' do
group_1 = create(:group) group_1 = create(:group)
group_1.add_owner(user) group_1.add_owner(user)
group_1.add_owner(create(:user))
control_count = ActiveRecord::QueryRecorder.new { run_service }.count
BatchLoader::Executor.clear_current
group_2 = create(:group) group_2 = create(:group)
group_2.add_owner(user) group_2.add_owner(user)
create(:group_member, :access_request, group: group_2, user: create(:user))
expect(service.groups).to contain_exactly( expect { run_service }.not_to exceed_query_limit(control_count)
a_hash_including(name: group_1.full_name, count: 2),
a_hash_including(name: group_2.full_name, count: 1)
)
end end
describe 'avatar_url' do describe 'group items' do
let(:group) { create(:group, avatar: fixture_file_upload('spec/fixtures/dk.png')) } subject(:group_items) { run_service.select { |hash| hash[:type].eql?('Group') } }
before do describe 'group user counts' do
group.add_owner(user) let(:group_1) { create(:group) }
end let(:group_2) { create(:group) }
before do
group_1.add_owner(user)
group_1.add_owner(create(:user))
it 'returns an url for the avatar' do group_2.add_owner(user)
expect(service.groups.size).to eq 1 create(:group_member, :access_request, group: group_2, user: create(:user))
expect(service.groups.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png") end
it 'returns correct user counts for groups' do
expect(group_items).to contain_exactly(
a_hash_including(name: group_1.full_name, count: 2),
a_hash_including(name: group_2.full_name, count: 1)
)
end
end end
it 'returns an url for the avatar with relative url' do describe 'avatar_url' do
stub_config_setting(relative_url_root: '/gitlab') let(:group) { create(:group, avatar: fixture_file_upload('spec/fixtures/dk.png')) }
stub_config_setting(url: Settings.send(:build_gitlab_url))
expect(service.groups.size).to eq 1 before do
expect(service.groups.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png") group.add_owner(user)
end
it 'returns an url for the avatar' do
expect(group_items.size).to eq 1
expect(group_items.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png")
end
it 'returns an url for the avatar with relative url' do
stub_config_setting(relative_url_root: '/gitlab')
stub_config_setting(url: Settings.send(:build_gitlab_url))
expect(group_items.size).to eq 1
expect(group_items.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png")
end
end end
end 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