diff options
-rw-r--r-- | app/models/group.rb | 20 | ||||
-rw-r--r-- | app/models/notification_recipient.rb | 29 | ||||
-rw-r--r-- | changelogs/unreleased/issue_44230.yml | 5 | ||||
-rw-r--r-- | doc/workflow/notifications.md | 9 | ||||
-rw-r--r-- | spec/models/group_spec.rb | 24 | ||||
-rw-r--r-- | spec/models/notification_recipient_spec.rb | 44 | ||||
-rw-r--r-- | spec/services/notification_service_spec.rb | 188 |
7 files changed, 286 insertions, 33 deletions
diff --git a/app/models/group.rb b/app/models/group.rb index aeb50fb9bb7..9c171de7fc3 100644 --- a/app/models/group.rb +++ b/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 = diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index 79458bd048a..1a03dd9df56 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -1,4 +1,6 @@ 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 diff --git a/changelogs/unreleased/issue_44230.yml b/changelogs/unreleased/issue_44230.yml new file mode 100644 index 00000000000..2c6dba6c0fb --- /dev/null +++ b/changelogs/unreleased/issue_44230.yml @@ -0,0 +1,5 @@ +--- +title: Apply notification settings level of groups to all child objects +merge_request: +author: +type: changed diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index fc592b99860..edb0c6bdc30 100644 --- a/doc/workflow/notifications.md +++ b/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. diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 8f1f4938065..9fe1186a8c9 100644 --- a/spec/models/group_spec.rb +++ b/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) } diff --git a/spec/models/notification_recipient_spec.rb b/spec/models/notification_recipient_spec.rb index eda0e1da835..13fe47799ed 100644 --- a/spec/models/notification_recipient_spec.rb +++ b/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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 831678b949d..0eadc83bfe3 100644 --- a/spec/services/notification_service_spec.rb +++ b/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,34 +253,56 @@ 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 - it do - notification.new_note(note) + shared_examples 'new note notifications' do + it do + notification.new_note(note) + + should_email(note.noteable.author) + should_email(note.noteable.assignees.first) + should_email(@u_mentioned) + should_email(@u_custom_global) + should_not_email(@u_guest_custom) + should_not_email(@u_guest_watcher) + should_not_email(@u_watcher) + should_not_email(note.author) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + end + end - should_email(note.noteable.author) - should_email(note.noteable.assignees.first) - should_email(@u_mentioned) - should_email(@u_custom_global) - should_not_email(@u_guest_custom) - should_not_email(@u_guest_watcher) - should_not_email(@u_watcher) - should_not_email(note.author) - should_not_email(@u_participating) - should_not_email(@u_disabled) - should_not_email(@u_lazy_participant) + 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 @@ -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 |