Merge branch 'remove_duplicate_user_ids_from_notification_settings_query' into 'master'

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

See merge request gitlab-org/gitlab!57688

app/models/notification_setting.rb

 # frozen_string_literal: true
 
 class NotificationSetting < ApplicationRecord
+  include FromUnion
+
   enum level: { global: 3, watch: 2, participating: 1, mention: 4, disabled: 0, custom: 5 }
 
   default_value_for :level, NotificationSetting.levels[:global]

app/services/notification_recipients/builder/base.rb

@@ -100,6 +100,8 @@ module NotificationRecipients
       # Get project/group users with CUSTOM notification level
       # rubocop: disable CodeReuse/ActiveRecord
       def add_custom_notifications
+        return new_add_custom_notifications if Feature.enabled?(:notification_setting_recipient_refactor, project)
+
         user_ids = []
 
         # Users with a notification setting on group or project
@@ -115,6 +117,48 @@ module NotificationRecipients
 
         add_recipients(user_scope.where(id: user_ids), :custom, nil)
       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
 
       def add_project_watchers

config/feature_flags/development/notification_setting_recipient_refactor.yml

+---
+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

spec/services/notification_recipients/builder/default_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe NotificationRecipients::Builder::Default do
   describe '#build!' do
     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(:issue)   { create(:issue, project: project) }
+    let_it_be(:target)  { create(:issue, project: project) }
 
     let_it_be(:current_user)    { create(:user) }
     let_it_be(:other_user)      { create(:user) }
@@ -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_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
       before do
-        expect(issue).to receive(:participants).and_return([participant, current_user])
+        expect(target).to receive(:participants).and_return([participant, current_user])
       end
 
       it 'adds all participants and watchers' do
@@ -34,11 +34,147 @@ RSpec.describe NotificationRecipients::Builder::Default do
       it 'adds all subscribers' do
         subscriber = create(:user)
         non_subscriber = create(:user)
-        create(:subscription, project: project, user: subscriber, subscribable: issue, subscribed: true)
-        create(:subscription, project: project, user: non_subscriber, subscribable: issue, subscribed: false)
+        create(:subscription, project: project, user: subscriber, subscribable: target, subscribed: true)
+        create(:subscription, project: project, user: non_subscriber, subscribable: target, subscribed: false)
 
         expect(subject.recipients.map(&:user)).to include(subscriber)
       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