Merge branch 'issue_44230' into 'master'

Apply notification settings level of groups to all child objects

Closes #44230

See merge request gitlab-org/gitlab-ce!19191

app/models/group.rb

@@ -11,6 +11,7 @@ class Group < Namespace
   include GroupDescendant
   include TokenAuthenticatable
   include WithUploads
+  include Gitlab::Utils::StrongMemoize
 
   has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
   alias_method :members, :group_members
@@ -26,7 +27,11 @@ class Group < Namespace
   has_many :milestones
   has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :shared_projects, through: :project_group_links, source: :project
+
+  # Overridden on another method
+  # Left here just to be dependent: :destroy
   has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
+
   has_many :labels, class_name: 'GroupLabel'
   has_many :variables, class_name: 'Ci::GroupVariable'
   has_many :custom_attributes, class_name: 'GroupCustomAttribute'
@@ -88,6 +93,15 @@ class Group < Namespace
     end
   end
 
+  # Overrides notification_settings has_many association
+  # This allows to apply notification settings from parent groups
+  # to child groups and projects.
+  def notification_settings
+    source_type = self.class.base_class.name
+
+    NotificationSetting.where(source_type: source_type, source_id: self_and_ancestors_ids)
+  end
+
   def to_reference(_from = nil, full: nil)
     "#{self.class.reference_prefix}#{full_path}"
   end
@@ -225,6 +239,12 @@ class Group < Namespace
     members_with_parents.pluck(:user_id)
   end
 
+  def self_and_ancestors_ids
+    strong_memoize(:self_and_ancestors_ids) do
+      self_and_ancestors.pluck(:id)
+    end
+  end
+
   def members_with_parents
     # Avoids an unnecessary SELECT when the group has no parents
     source_ids =

app/models/notification_recipient.rb

 class NotificationRecipient
+  include Gitlab::Utils::StrongMemoize
+
   attr_reader :user, :type, :reason
   def initialize(user, type, **opts)
     unless NotificationSetting.levels.key?(type) || type == :subscription
@@ -142,10 +144,33 @@ class NotificationRecipient
 
     return project_setting unless project_setting.nil? || project_setting.global?
 
-    group_setting = @group && user.notification_settings_for(@group)
+    group_setting = closest_non_global_group_notification_settting
 
-    return group_setting unless group_setting.nil? || group_setting.global?
+    return group_setting unless group_setting.nil?
 
     user.global_notification_setting
   end
+
+  # Returns the notificaton_setting of the lowest group in hierarchy with non global level
+  def closest_non_global_group_notification_settting
+    return unless @group
+    return if indexed_group_notification_settings.empty?
+
+    notification_setting = nil
+
+    @group.self_and_ancestors_ids.each do |id|
+      notification_setting = indexed_group_notification_settings[id]
+      break if notification_setting
+    end
+
+    notification_setting
+  end
+
+  def indexed_group_notification_settings
+    strong_memoize(:indexed_group_notification_settings) do
+      @group.notification_settings.where(user_id: user.id)
+        .where.not(level: NotificationSetting.levels[:global])
+        .index_by(&:source_id)
+    end
+  end
 end

changelogs/unreleased/issue_44230.yml

+---
+title: Apply notification settings level of groups to all child objects
+merge_request:
+author:
+type: changed

doc/workflow/notifications.md

@@ -34,9 +34,14 @@ anything that is set at Global Settings.
 
 ![notification settings](img/notification_group_settings.png)
 
-Group Settings are taking precedence over Global Settings but are on a level below Project Settings.
+Group Settings are taking precedence over Global Settings but are on a level below Project or Subgroup Settings:
+
+```
+Group < Subgroup < Project
+```
+
 This means that you can set a different level of notifications per group while still being able
-to have a finer level setting per project.
+to have a finer level setting per project or subgroup.
 Organization like this is suitable for users that belong to different groups but don't have the
 same need for being notified for every group they are member of.
 These settings can be configured on group page under the name of the group. It will be the dropdown with the bell icon. They can also be configured on the user profile notifications dropdown.

spec/models/group_spec.rb

@@ -67,6 +67,30 @@ describe Group do
       end
     end
 
+    describe '#notification_settings', :nested_groups do
+      let(:user) { create(:user) }
+      let(:group) { create(:group) }
+      let(:sub_group) { create(:group, parent_id: group.id) }
+
+      before do
+        group.add_developer(user)
+        sub_group.add_developer(user)
+      end
+
+      it 'also gets notification settings from parent groups' do
+        expect(sub_group.notification_settings.size).to eq(2)
+        expect(sub_group.notification_settings).to include(group.notification_settings.first)
+      end
+
+      context 'when sub group is deleted' do
+        it 'does not delete parent notification settings' do
+          expect do
+            sub_group.destroy
+          end.to change { NotificationSetting.count }.by(-1)
+        end
+      end
+    end
+
     describe '#visibility_level_allowed_by_parent' do
       let(:parent) { create(:group, :internal) }
       let(:sub_group) { build(:group, parent_id: parent.id) }

spec/models/notification_recipient_spec.rb

@@ -13,4 +13,48 @@ describe NotificationRecipient do
 
     expect(recipient.has_access?).to be_falsy
   end
+
+  context '#notification_setting' do
+    context 'for child groups', :nested_groups do
+      let!(:moved_group) { create(:group) }
+      let(:group) { create(:group) }
+      let(:sub_group_1) { create(:group, parent: group) }
+      let(:sub_group_2) { create(:group, parent: sub_group_1) }
+      let(:project) { create(:project, namespace: moved_group) }
+
+      before do
+        sub_group_2.add_owner(user)
+        moved_group.add_owner(user)
+        Groups::TransferService.new(moved_group, user).execute(sub_group_2)
+
+        moved_group.reload
+      end
+
+      context 'when notification setting is global' do
+        before do
+          user.notification_settings_for(group).global!
+          user.notification_settings_for(sub_group_1).mention!
+          user.notification_settings_for(sub_group_2).global!
+          user.notification_settings_for(moved_group).global!
+        end
+
+        it 'considers notification setting from the first parent without global setting' do
+          expect(subject.notification_setting.source).to eq(sub_group_1)
+        end
+      end
+
+      context 'when notification setting is not global' do
+        before do
+          user.notification_settings_for(group).global!
+          user.notification_settings_for(sub_group_1).mention!
+          user.notification_settings_for(sub_group_2).watch!
+          user.notification_settings_for(moved_group).disabled!
+        end
+
+        it 'considers notification setting from lowest group member in hierarchy' do
+          expect(subject.notification_setting.source).to eq(moved_group)
+        end
+      end
+    end
+  end
 end

spec/services/notification_service_spec.rb

@@ -59,6 +59,20 @@ describe NotificationService, :mailer do
 
       should_email(participant)
     end
+
+    context 'for subgroups', :nested_groups do
+      before do
+        build_group(project)
+      end
+
+      it 'emails the participant' do
+        create(:note_on_issue, noteable: issuable, project_id: project.id, note: 'anything', author: @pg_participant)
+
+        notification_trigger
+
+        should_email_nested_group_user(@pg_participant)
+      end
+    end
   end
 
   shared_examples 'participating by assignee notification' do
@@ -239,20 +253,19 @@ describe NotificationService, :mailer do
       end
 
       describe 'new note on issue in project that belongs to a group' do
-        let(:group) { create(:group) }
-
         before do
           note.project.namespace_id = group.id
-          note.project.group.add_user(@u_watcher, GroupMember::MASTER)
-          note.project.group.add_user(@u_custom_global, GroupMember::MASTER)
+          group.add_user(@u_watcher, GroupMember::MASTER)
+          group.add_user(@u_custom_global, GroupMember::MASTER)
           note.project.save
 
           @u_watcher.notification_settings_for(note.project).participating!
-          @u_watcher.notification_settings_for(note.project.group).global!
+          @u_watcher.notification_settings_for(group).global!
           update_custom_notification(:new_note, @u_custom_global)
           reset_delivered_emails!
         end
 
+        shared_examples 'new note notifications' do
           it do
             notification.new_note(note)
 
@@ -269,6 +282,29 @@ describe NotificationService, :mailer do
             should_not_email(@u_lazy_participant)
           end
         end
+
+        let(:group) { create(:group) }
+
+        it_behaves_like 'new note notifications'
+
+        context 'which is a subgroup', :nested_groups do
+          let!(:parent) { create(:group) }
+          let!(:group) { create(:group, parent: parent) }
+
+          it_behaves_like 'new note notifications'
+
+          it 'overrides child objects with global level' do
+            user = create(:user)
+            parent.add_developer(user)
+            user.notification_settings_for(parent).watch!
+            reset_delivered_emails!
+
+            notification.new_note(note)
+
+            should_email(user)
+          end
+        end
+      end
     end
 
     context 'confidential issue note' do
@@ -301,6 +337,31 @@ describe NotificationService, :mailer do
         should_email(member)
         should_email(admin)
       end
+
+      context 'on project that belongs to subgroup', :nested_groups do
+        let(:group_reporter) { create(:user) }
+        let(:group_guest) { create(:user) }
+        let(:parent_group) { create(:group) }
+        let(:child_group) { create(:group, parent: parent_group) }
+        let(:project) { create(:project, namespace: child_group) }
+
+        context 'when user is group guest member' do
+          before do
+            parent_group.add_reporter(group_reporter)
+            parent_group.add_guest(group_guest)
+            group_guest.notification_settings_for(parent_group).watch!
+            group_reporter.notification_settings_for(parent_group).watch!
+            reset_delivered_emails!
+          end
+
+          it 'does not email guest user' do
+            notification.new_note(note)
+
+            should_email(group_reporter)
+            should_not_email(group_guest)
+          end
+        end
+      end
     end
 
     context 'issue note mention' do
@@ -311,6 +372,7 @@ describe NotificationService, :mailer do
 
       before do
         build_team(note.project)
+        build_group(note.project)
         note.project.add_master(note.author)
         add_users_with_subscription(note.project, issue)
         reset_delivered_emails!
@@ -336,10 +398,20 @@ describe NotificationService, :mailer do
           should_email(@u_guest_watcher)
           should_email(note.noteable.author)
           should_email(note.noteable.assignees.first)
-          should_not_email(note.author)
+          should_email_nested_group_user(@pg_watcher)
           should_email(@u_mentioned)
-          should_not_email(@u_disabled)
           should_email(@u_not_mentioned)
+          should_not_email(note.author)
+          should_not_email(@u_disabled)
+          should_not_email_nested_group_user(@pg_disabled)
+        end
+
+        it 'notifies parent group members with mention level', :nested_groups do
+          note = create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: "@#{@pg_mention.username}")
+
+          notification.new_note(note)
+
+          should_email_nested_group_user(@pg_mention)
         end
 
         it 'filters out "mentioned in" notes' do
@@ -352,17 +424,18 @@ describe NotificationService, :mailer do
     end
 
     context 'project snippet note' do
-      let(:project) { create(:project, :public) }
+      let!(:project) { create(:project, :public) }
       let(:snippet) { create(:project_snippet, project: project, author: create(:user)) }
-      let(:note) { create(:note_on_project_snippet, noteable: snippet, project_id: snippet.project.id, note: '@all mentioned') }
+      let(:note) { create(:note_on_project_snippet, noteable: snippet, project_id: project.id, note: '@all mentioned') }
 
       before do
-        build_team(note.project)
+        build_team(project)
+        build_group(project)
 
         # make sure these users can read the project snippet!
         project.add_guest(@u_guest_watcher)
         project.add_guest(@u_guest_custom)
-
+        add_member_for_parent_group(@pg_watcher, project)
         note.project.add_master(note.author)
         reset_delivered_emails!
       end
@@ -370,7 +443,6 @@ describe NotificationService, :mailer do
       describe '#new_note' do
         it 'notifies the team members' do
           notification.new_note(note)
-
           # Notify all team members
           note.project.team.members.each do |member|
             # User with disabled notification should not be notified
@@ -449,6 +521,7 @@ describe NotificationService, :mailer do
 
       before do
         build_team(note.project)
+        build_group(project)
         reset_delivered_emails!
         allow(note.noteable).to receive(:author).and_return(@u_committer)
         update_custom_notification(:new_note, @u_guest_custom, resource: project)
@@ -463,11 +536,13 @@ describe NotificationService, :mailer do
           should_email(@u_guest_custom)
           should_email(@u_committer)
           should_email(@u_watcher)
+          should_email_nested_group_user(@pg_watcher)
           should_not_email(@u_mentioned)
           should_not_email(note.author)
           should_not_email(@u_participating)
           should_not_email(@u_disabled)
           should_not_email(@u_lazy_participant)
+          should_not_email_nested_group_user(@pg_disabled)
         end
 
         it do
@@ -478,10 +553,12 @@ describe NotificationService, :mailer do
           should_email(@u_committer)
           should_email(@u_watcher)
           should_email(@u_mentioned)
+          should_email_nested_group_user(@pg_watcher)
           should_not_email(note.author)
           should_not_email(@u_participating)
           should_not_email(@u_disabled)
           should_not_email(@u_lazy_participant)
+          should_not_email_nested_group_user(@pg_disabled)
         end
 
         it do
@@ -548,10 +625,13 @@ describe NotificationService, :mailer do
         should_email(@g_global_watcher)
         should_email(@g_watcher)
         should_email(@unsubscribed_mentioned)
+        should_email_nested_group_user(@pg_watcher)
         should_not_email(@u_mentioned)
         should_not_email(@u_participating)
         should_not_email(@u_disabled)
         should_not_email(@u_lazy_participant)
+        should_not_email_nested_group_user(@pg_disabled)
+        should_not_email_nested_group_user(@pg_mention)
       end
 
       it do
@@ -1922,19 +2002,69 @@ describe NotificationService, :mailer do
   # Users in the project's group but not part of project's team
   # with different notification settings
   def build_group(project)
-    group = create(:group, :public)
-    project.group = group
+    group = create_nested_group
+    project.update(namespace_id: group.id)
 
     # Group member: global=disabled, group=watch
-    @g_watcher = create_user_with_notification(:watch, 'group_watcher', project.group)
+    @g_watcher ||= create_user_with_notification(:watch, 'group_watcher', project.group)
     @g_watcher.notification_settings_for(nil).disabled!
 
     # Group member: global=watch, group=global
-    @g_global_watcher = create_global_setting_for(create(:user), :watch)
+    @g_global_watcher ||= create_global_setting_for(create(:user), :watch)
     group.add_users([@g_watcher, @g_global_watcher], :master)
+
     group
   end
 
+  # Creates a nested group only if supported
+  # to avoid errors on MySQL
+  def create_nested_group
+    if Group.supports_nested_groups?
+      parent_group = create(:group, :public)
+      child_group = create(:group, :public, parent: parent_group)
+
+      # Parent group member: global=disabled, parent_group=watch, child_group=global
+      @pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group)
+      @pg_watcher.notification_settings_for(nil).disabled!
+
+      # Parent group member: global=global, parent_group=disabled, child_group=global
+      @pg_disabled ||= create_user_with_notification(:disabled, 'parent_group_disabled', parent_group)
+      @pg_disabled.notification_settings_for(nil).global!
+
+      # Parent group member: global=global, parent_group=mention, child_group=global
+      @pg_mention ||= create_user_with_notification(:mention, 'parent_group_mention', parent_group)
+      @pg_mention.notification_settings_for(nil).global!
+
+      # Parent group member: global=global, parent_group=participating, child_group=global
+      @pg_participant ||= create_user_with_notification(:participating, 'parent_group_participant', parent_group)
+      @pg_mention.notification_settings_for(nil).global!
+
+      child_group
+    else
+      create(:group, :public)
+    end
+  end
+
+  def add_member_for_parent_group(user, project)
+    return unless Group.supports_nested_groups?
+
+    project.reload
+
+    project.group.parent.add_master(user)
+  end
+
+  def should_email_nested_group_user(user, times: 1, recipients: email_recipients)
+    return unless Group.supports_nested_groups?
+
+    should_email(user, times: 1, recipients: email_recipients)
+  end
+
+  def should_not_email_nested_group_user(user, recipients: email_recipients)
+    return unless Group.supports_nested_groups?
+
+    should_not_email(user, recipients: email_recipients)
+  end
+
   def add_users_with_subscription(project, issuable)
     @subscriber = create :user
     @unsubscriber = create :user