Merge branch '29477-notification-settings-display-all-groups' into 'master'

Show all groups user belongs to in Notification settings

See merge request gitlab-org/gitlab!17303

app/controllers/notification_settings_controller.rb

@@ -50,8 +50,6 @@ class NotificationSettingsController < ApplicationController
   end
 
   def notification_setting_params_for(source)
-    allowed_fields = NotificationSetting.email_events(source).dup
-    allowed_fields << :level
-    params.require(:notification_setting).permit(allowed_fields)
+    params.require(:notification_setting).permit(NotificationSetting.allowed_fields(source))
   end
 end

app/controllers/profiles/groups_controller.rb

@@ -5,7 +5,7 @@ class Profiles::GroupsController < Profiles::ApplicationController
 
   def update
     group = find_routable!(Group, params[:id])
-    notification_setting = current_user.notification_settings.find_by(source: group) # rubocop: disable CodeReuse/ActiveRecord
+    notification_setting = current_user.notification_settings_for(group)
 
     if notification_setting.update(update_params)
       flash[:notice] = "Notification settings for #{group.name} saved"

app/controllers/profiles/notifications_controller.rb

@@ -3,9 +3,14 @@
 class Profiles::NotificationsController < Profiles::ApplicationController
   # rubocop: disable CodeReuse/ActiveRecord
   def show
-    @user                        = current_user
-    @group_notifications         = current_user.notification_settings.for_groups.order(:id)
-    @project_notifications       = current_user.notification_settings.for_projects.order(:id)
+    @user = current_user
+    @group_notifications = current_user.notification_settings.for_groups.order(:id)
+    @group_notifications += GroupsFinder.new(
+      current_user,
+      all_available: false,
+      exclude_group_ids: @group_notifications.select(:source_id)
+    ).execute.map { |group| current_user.notification_settings_for(group, inherit: true) }
+    @project_notifications = current_user.notification_settings.for_projects.order(:id)
     @global_notification_setting = current_user.global_notification_setting
   end
   # rubocop: enable CodeReuse/ActiveRecord

app/models/notification_setting.rb

@@ -47,6 +47,10 @@ class NotificationSetting < ApplicationRecord
     EMAIL_EVENTS
   end
 
+  def self.allowed_fields(source = nil)
+    NotificationSetting.email_events(source).dup + %i(level notification_email)
+  end
+
   def email_events
     self.class.email_events(source)
   end

app/models/user.rb

@@ -1317,14 +1317,27 @@ class User < ApplicationRecord
     notification_group&.notification_email_for(self) || notification_email
   end
 
-  def notification_settings_for(source)
+  def notification_settings_for(source, inherit: false)
     if notification_settings.loaded?
       notification_settings.find do |notification|
         notification.source_type == source.class.base_class.name &&
           notification.source_id == source.id
       end
     else
-      notification_settings.find_or_initialize_by(source: source)
+      notification_settings.find_or_initialize_by(source: source) do |ns|
+        next unless source.is_a?(Group) && inherit
+
+        # If we're here it means we're trying to create a NotificationSetting for a group that doesn't have one.
+        # Find the closest parent with a notification_setting that's not Global level, or that has an email set.
+        ancestor_ns = source
+                        .notification_settings(hierarchy_order: :asc)
+                        .where(user: self)
+                        .find_by('level != ? OR notification_email IS NOT NULL', NotificationSetting.levels[:global])
+        # Use it to seed the settings
+        ns.assign_attributes(ancestor_ns&.slice(*NotificationSetting.allowed_fields))
+        ns.source = source
+        ns.user = self
+      end
     end
   end
 

app/views/profiles/notifications/_group_settings.html.haml

@@ -12,5 +12,5 @@
     = render 'shared/notifications/button', notification_setting: setting, emails_disabled: emails_disabled
 
   .table-section.section-30
-    = form_for @user.notification_settings.find { |ns| ns.source == group }, url: profile_notifications_group_path(group), method: :put, html: { class: 'update-notifications' } do |f|
+    = form_for setting, url: profile_notifications_group_path(group), method: :put, html: { class: 'update-notifications' } do |f|
       = f.select :notification_email, @user.all_emails, { include_blank: 'Global notification email' }, class: 'select2 js-group-notification-email'

changelogs/unreleased/29477-notification-settings-display-all-groups.yml

+---
+title: Show all groups user belongs to in Notification settings
+merge_request: 17303
+author:
+type: fixed

spec/controllers/profiles/notifications_controller_spec.rb

@@ -20,6 +20,38 @@ describe Profiles::NotificationsController do
 
       expect(response).to render_template :show
     end
+
+    context 'with groups that do not have notification preferences' do
+      set(:group) { create(:group) }
+      set(:subgroup) { create(:group, parent: group) }
+
+      before do
+        group.add_developer(user)
+      end
+
+      it 'still shows up in the list' do
+        sign_in(user)
+
+        get :show
+
+        expect(assigns(:group_notifications).map(&:source_id)).to include(subgroup.id)
+      end
+
+      it 'has an N+1 (but should not)' do
+        sign_in(user)
+
+        control = ActiveRecord::QueryRecorder.new do
+          get :show
+        end
+
+        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
+    end
   end
 
   describe 'POST update' do

spec/models/user_spec.rb

@@ -3735,6 +3735,80 @@ describe User do
     end
   end
 
+  describe '#notification_settings_for' do
+    let(:user) { create(:user) }
+    let(:source) { nil }
+
+    subject { user.notification_settings_for(source) }
+
+    context 'when source is nil' do
+      it 'returns a blank global notification settings object' do
+        expect(subject.source).to eq(nil)
+        expect(subject.notification_email).to eq(nil)
+        expect(subject.level).to eq('global')
+      end
+    end
+
+    context 'when source is a Group' do
+      let(:group) { create(:group) }
+
+      subject { user.notification_settings_for(group, inherit: true) }
+
+      context 'when group has no existing notification settings' do
+        context 'when group has no ancestors' do
+          it 'will be a default Global notification setting' do
+            expect(subject.notification_email).to eq(nil)
+            expect(subject.level).to eq('global')
+          end
+        end
+
+        context 'when group has ancestors' do
+          context 'when an ancestor has a level other than Global' do
+            let(:ancestor) { create(:group) }
+            let(:group) { create(:group, parent: ancestor) }
+
+            before do
+              create(:notification_setting, user: user, source: ancestor, level: 'participating', notification_email: 'ancestor@example.com')
+            end
+
+            it 'has the same level set' do
+              expect(subject.level).to eq('participating')
+            end
+
+            it 'has the same email set' do
+              expect(subject.notification_email).to eq('ancestor@example.com')
+            end
+
+            context 'when inherit is false' do
+              subject { user.notification_settings_for(group) }
+
+              it 'does not inherit settings' do
+                expect(subject.notification_email).to eq(nil)
+                expect(subject.level).to eq('global')
+              end
+            end
+          end
+
+          context 'when an ancestor has a Global level but has an email set' do
+            let(:grand_ancestor) { create(:group) }
+            let(:ancestor) { create(:group, parent: grand_ancestor) }
+            let(:group) { create(:group, parent: ancestor) }
+
+            before do
+              create(:notification_setting, user: user, source: grand_ancestor, level: 'participating', notification_email: 'grand@example.com')
+              create(:notification_setting, user: user, source: ancestor, level: 'global', notification_email: 'ancestor@example.com')
+            end
+
+            it 'has the same email set' do
+              expect(subject.level).to eq('global')
+              expect(subject.notification_email).to eq('ancestor@example.com')
+            end
+          end
+        end
+      end
+    end
+  end
+
   describe '#notification_email_for' do
     let(:user) { create(:user) }
     let(:group) { create(:group) }