Commit afe5d7d5 authored by Felipe Artur's avatar Felipe Artur

Apply notification settings level of groups to all child objects

parent 439adb96
......@@ -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
......@@ -220,6 +234,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 =
......
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
---
title: Apply notification settings level of groups to all child objects
merge_request:
author:
type: changed
......@@ -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.
......
......@@ -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) }
......
......@@ -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
This diff is collapsed.
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment