Remove the N+1 in Profiles::NotificationsController

In order to do this we have to load all the Groups a user has access to
in memory and then load all the NotificationSettings as well so we can
match them up in memory and look up the tree to create "inherited"
NotificationSettings.

app/controllers/profiles/notifications_controller.rb

 # frozen_string_literal: true
 
 class Profiles::NotificationsController < Profiles::ApplicationController
-  NOTIFICATIONS_PER_PAGE = 10
-
   # rubocop: disable CodeReuse/ActiveRecord
   def show
     @user = current_user
     @user_groups = user_groups
-    @group_notifications = user_groups.map { |group| current_user.notification_settings_for(group, inherit: true) }
+    @group_notifications = UserGroupNotificationSettingsFinder.new(current_user, user_groups).execute
 
     @project_notifications = current_user.notification_settings.for_projects.order(:id)
                              .preload_source_route
@@ -35,6 +33,6 @@ class Profiles::NotificationsController < Profiles::ApplicationController
   private
 
   def user_groups
-    GroupsFinder.new(current_user, all_available: false).execute.order_name_asc.page(params[:page]).per(NOTIFICATIONS_PER_PAGE)
+    GroupsFinder.new(current_user, all_available: false).execute.order_name_asc.page(params[:page])
   end
 end

app/finders/user_group_notification_settings_finder.rb

+# frozen_string_literal: true
+
+class UserGroupNotificationSettingsFinder
+  def initialize(user, groups)
+    @user = user
+    @groups = groups
+  end
+
+  def execute
+    groups_with_ancestors = Gitlab::ObjectHierarchy.new(groups).base_and_ancestors
+
+    @loaded_groups_with_ancestors = groups_with_ancestors.index_by(&:id)
+    @loaded_notification_settings = user.notification_settings_for_groups(groups_with_ancestors).preload_source_route.index_by(&:source_id)
+
+    groups.map do |group|
+      find_notification_setting_for(group)
+    end
+  end
+
+  private
+
+  attr_reader :user, :groups, :loaded_groups_with_ancestors, :loaded_notification_settings
+
+  def find_notification_setting_for(group)
+    return loaded_notification_settings[group.id] if loaded_notification_settings[group.id]
+    return user.notification_settings.build(source: group) if group.parent_id.nil?
+
+    parent_setting = loaded_notification_settings[group.parent_id]
+
+    if should_copy?(parent_setting)
+      user.notification_settings.build(source: group) do |ns|
+        ns.assign_attributes(parent_setting.slice(*NotificationSetting.allowed_fields))
+      end
+    else
+      find_notification_setting_for(loaded_groups_with_ancestors[group.parent_id])
+    end
+  end
+
+  def should_copy?(parent_setting)
+    return false unless parent_setting
+
+    parent_setting.level != NotificationSetting.levels[:global] || parent_setting.notification_email.present?
+  end
+end

app/models/user.rb

@@ -1461,6 +1461,11 @@ class User < ApplicationRecord
     end
   end
 
+  def notification_settings_for_groups(groups)
+    ids = groups.is_a?(ActiveRecord::Relation) ? groups.select(:id) : groups.map(&:id)
+    notification_settings.for_groups.where(source_id: ids)
+  end
+
   # Lazy load global notification setting
   # Initializes User setting with Participating level if setting not persisted
   def global_notification_setting

spec/controllers/profiles/notifications_controller_spec.rb

@@ -37,7 +37,7 @@ RSpec.describe Profiles::NotificationsController do
         expect(assigns(:group_notifications).map(&:source_id)).to include(subgroup.id)
       end
 
-      it 'has an N+1 (but should not)' do
+      it 'does not have an N+1' do
         sign_in(user)
 
         control = ActiveRecord::QueryRecorder.new do
@@ -46,10 +46,9 @@ RSpec.describe Profiles::NotificationsController do
 
         create_list(:group, 2, parent: group)
 
-        # We currently have an N + 1, switch to `not_to` once fixed
         expect do
           get :show
-        end.to exceed_query_limit(control)
+        end.not_to exceed_query_limit(control)
       end
     end
 
@@ -62,7 +61,7 @@ RSpec.describe Profiles::NotificationsController do
       before do
         group.add_developer(user)
         sign_in(user)
-        stub_const('Profiles::NotificationsController::NOTIFICATIONS_PER_PAGE', notifications_per_page)
+        allow(Kaminari.config).to receive(:default_per_page).and_return(notifications_per_page)
       end
 
       it 'paginates the groups' do

spec/finders/user_group_notification_settings_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe UserGroupNotificationSettingsFinder do
+  let_it_be(:user) { create(:user) }
+
+  subject { described_class.new(user, Group.where(id: groups.map(&:id))).execute }
+
+  def attributes(&proc)
+    subject.map(&proc).uniq
+  end
+
+  context 'when the groups have no existing notification settings' do
+    context 'when the groups have no ancestors' do
+      let_it_be(:groups) { create_list(:group, 3) }
+
+      it 'will be a default Global notification setting', :aggregate_failures do
+        expect(subject.count).to eq(3)
+        expect(attributes(&:notification_email)).to eq([nil])
+        expect(attributes(&:level)).to eq(['global'])
+      end
+    end
+
+    context 'when the groups have ancestors' do
+      context 'when an ancestor has a level other than Global' do
+        let_it_be(:ancestor_a) { create(:group) }
+        let_it_be(:group_a) { create(:group, parent: ancestor_a) }
+        let_it_be(:ancestor_b) { create(:group) }
+        let_it_be(:group_b) { create(:group, parent: ancestor_b) }
+        let_it_be(:email) { create(:email, :confirmed, email: 'ancestor@example.com', user: user) }
+
+        let_it_be(:groups) { [group_a, group_b] }
+
+        before do
+          create(:notification_setting, user: user, source: ancestor_a, level: 'participating', notification_email: email.email)
+          create(:notification_setting, user: user, source: ancestor_b, level: 'participating', notification_email: email.email)
+        end
+
+        it 'has the same level set' do
+          expect(attributes(&:level)).to eq(['participating'])
+        end
+
+        it 'has the same email set' do
+          expect(attributes(&:notification_email)).to eq(['ancestor@example.com'])
+        end
+
+        it 'only returns the two queried groups' do
+          expect(subject.count).to eq(2)
+        end
+      end
+
+      context 'when an ancestor has a Global level but has an email set' do
+        let_it_be(:grand_ancestor) { create(:group) }
+        let_it_be(:ancestor) { create(:group, parent: grand_ancestor) }
+        let_it_be(:group) { create(:group, parent: ancestor) }
+        let_it_be(:ancestor_email) { create(:email, :confirmed, email: 'ancestor@example.com', user: user) }
+        let_it_be(:grand_email) { create(:email, :confirmed, email: 'grand@example.com', user: user) }
+
+        let_it_be(:groups) { [group] }
+
+        before do
+          create(:notification_setting, user: user, source: grand_ancestor, level: 'participating', notification_email: grand_email.email)
+          create(:notification_setting, user: user, source: ancestor, level: 'global', notification_email: ancestor_email.email)
+        end
+
+        it 'has the same email and level set', :aggregate_failures do
+          expect(subject.count).to eq(1)
+          expect(attributes(&:level)).to eq(['global'])
+          expect(attributes(&:notification_email)).to eq(['ancestor@example.com'])
+        end
+      end
+
+      it 'does not cause an N+1', :aggregate_failures do
+        parent = create(:group)
+        child = create(:group, parent: parent)
+
+        control = ActiveRecord::QueryRecorder.new do
+          described_class.new(user, Group.where(id: child.id)).execute
+        end
+
+        other_parent = create(:group)
+        other_children = create_list(:group, 2, parent: other_parent)
+
+        result = nil
+
+        expect do
+          result = described_class.new(user, Group.where(id: other_children.append(child).map(&:id))).execute
+        end.not_to exceed_query_limit(control)
+
+        expect(result.count).to eq(3)
+      end
+    end
+  end
+end

spec/models/user_spec.rb

@@ -4491,6 +4491,44 @@ RSpec.describe User do
     end
   end
 
+  describe '#notification_settings_for_groups' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:groups) { create_list(:group, 2) }
+
+    subject { user.notification_settings_for_groups(arg) }
+
+    before do
+      groups.each do |group|
+        group.add_maintainer(user)
+      end
+    end
+
+    shared_examples_for 'notification_settings_for_groups method' do
+      it 'returns NotificationSetting objects for provided groups', :aggregate_failures do
+        expect(subject.count).to eq(groups.count)
+        expect(subject.map(&:source_id)).to match_array(groups.map(&:id))
+      end
+    end
+
+    context 'when given an ActiveRecord relationship' do
+      let_it_be(:arg) { Group.where(id: groups.map(&:id)) }
+
+      it_behaves_like 'notification_settings_for_groups method'
+
+      it 'uses #select to maintain lazy querying behavior' do
+        expect(arg).to receive(:select).and_call_original
+
+        subject
+      end
+    end
+
+    context 'when given an Array of Groups' do
+      let_it_be(:arg) { groups }
+
+      it_behaves_like 'notification_settings_for_groups method'
+    end
+  end
+
   describe '#notification_email_for' do
     let(:user) { create(:user) }
     let(:group) { create(:group) }

spec/requests/profiles/notifications_controller_spec.rb

@@ -25,8 +25,7 @@ RSpec.describe 'view user notifications' do
   end
 
   describe 'GET /profile/notifications' do
-    # To be fixed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40457
-    it 'has an N+1 due to an additional groups (with no parent group) - but should not' do
+    it 'does not have an N+1 due to an additional groups (with no parent group)' do
       get_profile_notifications
 
       control = ActiveRecord::QueryRecorder.new do
@@ -37,7 +36,7 @@ RSpec.describe 'view user notifications' do
 
       expect do
         get_profile_notifications
-      end.to exceed_query_limit(control)
+      end.not_to exceed_query_limit(control)
     end
   end
 end