Show all groups user belongs to in Notification settings

Adds an "inherited group settings" area to the notification settings
profile area so users are able to edit notification settings for groups
they're members of without having to jump through hoops

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

@@ -46,6 +46,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

@@ -3734,6 +3734,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) }