Commit 711041bd authored by David Kim's avatar David Kim Committed by Tiger Watson

Improve notification recipients builder db performance [RUN ALL RSPEC] [RUN AS-IF-FOSS]

parent 8b407f0a
# frozen_string_literal: true # frozen_string_literal: true
class NotificationSetting < ApplicationRecord class NotificationSetting < ApplicationRecord
include FromUnion
enum level: { global: 3, watch: 2, participating: 1, mention: 4, disabled: 0, custom: 5 } enum level: { global: 3, watch: 2, participating: 1, mention: 4, disabled: 0, custom: 5 }
default_value_for :level, NotificationSetting.levels[:global] default_value_for :level, NotificationSetting.levels[:global]
......
...@@ -100,6 +100,8 @@ module NotificationRecipients ...@@ -100,6 +100,8 @@ module NotificationRecipients
# Get project/group users with CUSTOM notification level # Get project/group users with CUSTOM notification level
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def add_custom_notifications def add_custom_notifications
return new_add_custom_notifications if Feature.enabled?(:notification_setting_recipient_refactor, project)
user_ids = [] user_ids = []
# Users with a notification setting on group or project # Users with a notification setting on group or project
...@@ -115,6 +117,48 @@ module NotificationRecipients ...@@ -115,6 +117,48 @@ module NotificationRecipients
add_recipients(user_scope.where(id: user_ids), :custom, nil) add_recipients(user_scope.where(id: user_ids), :custom, nil)
end end
def new_add_custom_notifications
notification_by_sources = related_notification_settings_sources(:custom)
return if notification_by_sources.blank?
user_ids = NotificationSetting.from_union(notification_by_sources).select(:user_id)
add_recipients(user_scope.where(id: user_ids), :custom, nil)
end
def related_notification_settings_sources(level)
sources = [project, group].compact
sources.map do |source|
source
.notification_settings
.where(source_or_global_setting_by_level_query(level)).select(:user_id)
end
end
def global_setting_by_level_query(level)
table = NotificationSetting.arel_table
aliased_table = table.alias
table
.project('true')
.from(aliased_table)
.where(
aliased_table[:user_id].eq(table[:user_id])
.and(aliased_table[:source_id].eq(nil))
.and(aliased_table[:source_type].eq(nil))
.and(aliased_table[:level].eq(level))
).exists
end
def source_or_global_setting_by_level_query(level)
table = NotificationSetting.arel_table
table.grouping(
table[:level].eq(:global).and(global_setting_by_level_query(level))
).or(table[:level].eq(level))
end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
def add_project_watchers def add_project_watchers
......
---
name: notification_setting_recipient_refactor
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57688
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/327303
milestone: '13.11'
type: development
group: group::code review
default_enabled: false
...@@ -6,7 +6,7 @@ RSpec.describe NotificationRecipients::Builder::Default do ...@@ -6,7 +6,7 @@ RSpec.describe NotificationRecipients::Builder::Default do
describe '#build!' do describe '#build!' do
let_it_be(:group) { create(:group, :public) } let_it_be(:group) { create(:group, :public) }
let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } } let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } }
let_it_be(:issue) { create(:issue, project: project) } let_it_be(:target) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) } let_it_be(:current_user) { create(:user) }
let_it_be(:other_user) { create(:user) } let_it_be(:other_user) { create(:user) }
...@@ -17,11 +17,11 @@ RSpec.describe NotificationRecipients::Builder::Default do ...@@ -17,11 +17,11 @@ RSpec.describe NotificationRecipients::Builder::Default do
let_it_be(:notification_setting_project_w) { create(:notification_setting, source: project, user: project_watcher, level: 2) } let_it_be(:notification_setting_project_w) { create(:notification_setting, source: project, user: project_watcher, level: 2) }
let_it_be(:notification_setting_group_w) { create(:notification_setting, source: group, user: group_watcher, level: 2) } let_it_be(:notification_setting_group_w) { create(:notification_setting, source: group, user: group_watcher, level: 2) }
subject { described_class.new(issue, current_user, action: :new).tap { |s| s.build! } } subject { described_class.new(target, current_user, action: :new).tap { |s| s.build! } }
context 'participants and project watchers' do context 'participants and project watchers' do
before do before do
expect(issue).to receive(:participants).and_return([participant, current_user]) expect(target).to receive(:participants).and_return([participant, current_user])
end end
it 'adds all participants and watchers' do it 'adds all participants and watchers' do
...@@ -34,11 +34,147 @@ RSpec.describe NotificationRecipients::Builder::Default do ...@@ -34,11 +34,147 @@ RSpec.describe NotificationRecipients::Builder::Default do
it 'adds all subscribers' do it 'adds all subscribers' do
subscriber = create(:user) subscriber = create(:user)
non_subscriber = create(:user) non_subscriber = create(:user)
create(:subscription, project: project, user: subscriber, subscribable: issue, subscribed: true) create(:subscription, project: project, user: subscriber, subscribable: target, subscribed: true)
create(:subscription, project: project, user: non_subscriber, subscribable: issue, subscribed: false) create(:subscription, project: project, user: non_subscriber, subscribable: target, subscribed: false)
expect(subject.recipients.map(&:user)).to include(subscriber) expect(subject.recipients.map(&:user)).to include(subscriber)
end end
end end
context 'custom notifications' do
shared_examples 'custom notification recipients' do
let_it_be(:custom_notification_user) { create(:user) }
let_it_be(:another_group) { create(:group) }
let_it_be(:another_project) { create(:project, namespace: another_group) }
context 'with project custom notification setting' do
before do
create(:notification_setting, source: project, user: custom_notification_user, level: :custom)
end
it 'adds the user to the recipients' do
expect(subject.recipients.map(&:user)).to include(custom_notification_user)
end
end
context 'with the project custom notification setting in another project' do
before do
create(:notification_setting, source: another_project, user: custom_notification_user, level: :custom)
end
it 'does not add the user to the recipients' do
expect(subject.recipients.map(&:user)).not_to include(custom_notification_user)
end
end
context 'with group custom notification setting' do
before do
create(:notification_setting, source: group, user: custom_notification_user, level: :custom)
end
it 'adds the user to the recipients' do
expect(subject.recipients.map(&:user)).to include(custom_notification_user)
end
end
context 'with the group custom notification setting in another group' do
before do
create(:notification_setting, source: another_group, user: custom_notification_user, level: :custom)
end
it 'does not add the user to the recipients' do
expect(subject.recipients.map(&:user)).not_to include(custom_notification_user)
end
end
context 'with project global custom notification setting' do
before do
create(:notification_setting, source: project, user: custom_notification_user, level: :global)
end
context 'with global custom notification setting' do
before do
create(:notification_setting, source: nil, user: custom_notification_user, level: :custom)
end
it 'adds the user to the recipients' do
expect(subject.recipients.map(&:user)).to include(custom_notification_user)
end
end
context 'without global custom notification setting' do
it 'does not add the user to the recipients' do
expect(subject.recipients.map(&:user)).not_to include(custom_notification_user)
end
end
end
context 'with group global custom notification setting' do
before do
create(:notification_setting, source: group, user: custom_notification_user, level: :global)
end
context 'with global custom notification setting' do
before do
create(:notification_setting, source: nil, user: custom_notification_user, level: :custom)
end
it 'adds the user to the recipients' do
expect(subject.recipients.map(&:user)).to include(custom_notification_user)
end
end
context 'without global custom notification setting' do
it 'does not add the user to the recipients' do
expect(subject.recipients.map(&:user)).not_to include(custom_notification_user)
end
end
end
context 'with group custom notification setting in deeply nested parent group' do
let(:grand_parent_group) { create(:group, :public) }
let(:parent_group) { create(:group, :public, parent: grand_parent_group) }
let(:group) { create(:group, :public, parent: parent_group) }
let(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } }
let(:target) { create(:issue, project: project) }
before do
create(:notification_setting, source: grand_parent_group, user: custom_notification_user, level: :custom)
end
it 'adds the user to the recipients' do
expect(subject.recipients.map(&:user)).to include(custom_notification_user)
end
end
context 'without a project or group' do
let(:target) { create(:snippet) }
before do
create(:notification_setting, source: nil, user: custom_notification_user, level: :custom)
end
it 'does not add the user to the recipients' do
expect(subject.recipients.map(&:user)).not_to include(custom_notification_user)
end
end
end
before do
stub_feature_flags(notification_setting_recipient_refactor: enabled)
end
context 'with notification_setting_recipient_refactor enabled' do
let(:enabled) { true }
it_behaves_like 'custom notification recipients'
end
context 'with notification_setting_recipient_refactor disabled' do
let(:enabled) { false }
it_behaves_like 'custom notification recipients'
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