From e60999ec5c789f1e4e91dd5c71f7c28348164b03 Mon Sep 17 00:00:00 2001
From: Felipe Artur <felipefac@gmail.com>
Date: Wed, 1 Jun 2016 18:24:38 -0300
Subject: [PATCH] Improve notification settings event keys and add some specs
---
app/models/notification_setting.rb | 34 ++++++------
app/services/notification_service.rb | 62 ++++++++++++----------
spec/services/notification_service_spec.rb | 56 ++++++++++++++++---
3 files changed, 102 insertions(+), 50 deletions(-)
diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb
index be4fcbd8467..3f8c5b2caa9 100644
--- a/app/models/notification_setting.rb
+++ b/app/models/notification_setting.rb
@@ -15,23 +15,21 @@ class NotificationSetting < ActiveRecord::Base
scope :for_groups, -> { where(source_type: 'Namespace') }
scope :for_projects, -> { where(source_type: 'Project') }
- serialize :events
-
EMAIL_EVENTS = [
- :new_issue_email,
- :new_note_email,
- :closed_issue_email,
- :reassigned_issue_email,
- :relabeled_issue_email,
- :new_merge_request_email,
- :reassigned_merge_request_email,
- :relabeled_merge_request_email,
- :closed_merge_request_email,
- :issue_status_changed_email,
- :merged_merge_request_email,
- :merge_request_status_email
+ :new_note,
+ :new_issue,
+ :reopen_issue,
+ :closed_issue,
+ :reassign_issue,
+ :new_merge_request,
+ :reopen_merge_request,
+ :close_merge_request,
+ :reassign_merge_request,
+ :merge_merge_request
]
+ store :events, accessors: EMAIL_EVENTS, coder: JSON
+
before_save :set_events
def self.find_or_create_for(source)
@@ -44,7 +42,13 @@ class NotificationSetting < ActiveRecord::Base
setting
end
+ # Set all event attributes as true when level is not custom
def set_events
- self.events = EMAIL_EVENTS if level == "watch"
+ # Level is a ENUM cannot compare to symbol
+ return if level == "custom"
+
+ EMAIL_EVENTS.each do |event|
+ self.send("#{event}=", true)
+ end
end
end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 768f513c195..38cc00f1a05 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -153,6 +153,9 @@ class NotificationService
# Merge project watchers
recipients = add_project_watchers(recipients, note.project)
+ # Merge project with custom notification
+ recipients = add_project_custom_notifications(recipients, note.project, :new_note)
+
# Reject users with Mention notification level, except those mentioned in _this_ note.
recipients = reject_mention_users(recipients - mentioned_users, note.project)
recipients = recipients + mentioned_users
@@ -248,21 +251,22 @@ class NotificationService
protected
+ # Get project/group users with CUSTOM notification level
def add_project_custom_notifications(recipients, project, action)
user_ids = []
- user_ids += project_member_notification(project, :custom, action)
- user_ids += group_member_notification(project, :custom, action)
+ user_ids += notification_settings_for(project, :custom, action)
+ user_ids += notification_settings_for(project.group, :custom, action)
recipients.concat(User.find(user_ids))
end
# Get project users with WATCH notification level
def project_watchers(project)
- project_members = project_member_notification(project)
+ project_members = notification_settings_for(project)
- users_with_project_level_global = project_member_notification(project, :global)
- users_with_group_level_global = group_member_notification(project, :global)
+ users_with_project_level_global = notification_settings_for(project, :global)
+ users_with_group_level_global = notification_settings_for(project, :global)
users = users_with_global_level_watch([users_with_project_level_global, users_with_group_level_global].flatten.uniq)
users_with_project_setting = select_project_member_setting(project, users_with_project_level_global, users)
@@ -271,23 +275,15 @@ class NotificationService
User.where(id: users_with_project_setting.concat(users_with_group_setting).uniq).to_a
end
- def project_member_notification(project, notification_level=nil, action=nil)
- if notification_level
- settings = project.notification_settings.where(level: NotificationSetting.levels[notification_level])
- settings = settings.where(events: action.to_yaml) if action.present?
- settings.pluck(:user_id)
- else
- project.notification_settings.pluck(:user_id)
- end
- end
+ def notification_settings_for(resource, notification_level = nil, action = nil)
+ return [] unless resource
- def group_member_notification(project, notification_level, action=nil)
- if project.group
- settings = project.group.notification_settings.where(level: NotificationSetting.levels[notification_level])
- settings = settings.where(events: action.to_yaml) if action.present?
- settings.pluck(:user_id)
+ if notification_level
+ settings = resource.notification_settings.where(level: NotificationSetting.levels[notification_level])
+ settings = settings.select { |setting| setting.events[action] } if action.present?
+ settings.map(&:user_id)
else
- []
+ resource.notification_settings.pluck(:user_id)
end
end
@@ -301,7 +297,7 @@ class NotificationService
# Build a list of users based on project notifcation settings
def select_project_member_setting(project, global_setting, users_global_level_watch)
- users = project_member_notification(project, :watch)
+ users = notification_settings_for(project, :watch)
# If project setting is global, add to watch list if global setting is watch
global_setting.each do |user_id|
@@ -315,7 +311,7 @@ class NotificationService
# Build a list of users based on group notification settings
def select_group_member_setting(project, project_members, global_setting, users_global_level_watch)
- uids = group_member_notification(project, :watch)
+ uids = notification_settings_for(project, :watch)
# Group setting is watch, add to users list if user is not project member
users = []
@@ -421,7 +417,7 @@ class NotificationService
end
def new_resource_email(target, project, method)
- recipients = build_recipients(target, project, target.author, action: method)
+ recipients = build_recipients(target, project, target.author, action: "new")
recipients.each do |recipient|
mailer.send(method, recipient.id, target.id).deliver_later
@@ -429,7 +425,8 @@ class NotificationService
end
def close_resource_email(target, project, current_user, method)
- recipients = build_recipients(target, project, current_user, action: method)
+ action = method == :merged_merge_request_email ? "merge" : "close"
+ recipients = build_recipients(target, project, current_user, action: action)
recipients.each do |recipient|
mailer.send(method, recipient.id, target.id, current_user.id).deliver_later
@@ -440,7 +437,7 @@ class NotificationService
previous_assignee_id = previous_record(target, 'assignee_id')
previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
- recipients = build_recipients(target, project, current_user, action: method, previous_assignee: previous_assignee)
+ recipients = build_recipients(target, project, current_user, action: "reassign", previous_assignee: previous_assignee)
recipients.each do |recipient|
mailer.send(
@@ -463,7 +460,7 @@ class NotificationService
end
def reopen_resource_email(target, project, current_user, method, status)
- recipients = build_recipients(target, project, current_user, action: method)
+ recipients = build_recipients(target, project, current_user, action: "reopen")
recipients.each do |recipient|
mailer.send(method, recipient.id, target.id, status, current_user.id).deliver_later
@@ -471,9 +468,11 @@ class NotificationService
end
def build_recipients(target, project, current_user, action: nil, previous_assignee: nil)
+ custom_action = build_custom_key(action, target)
+
recipients = target.participants(current_user)
recipients = add_project_watchers(recipients, project)
- recipients = add_project_custom_notifications(recipients, project, action)
+ recipients = add_project_custom_notifications(recipients, project, custom_action)
recipients = reject_mention_users(recipients, project)
recipients = recipients.uniq
@@ -481,7 +480,7 @@ class NotificationService
# Re-assign is considered as a mention of the new assignee so we add the
# new assignee to the list of recipients after we rejected users with
# the "on mention" notification level
- if [:reassigned_merge_request_email, :reassigned_issue_email].include?(action)
+ if [:reassign_merge_request, :reassign_issue].include?(custom_action)
recipients << previous_assignee if previous_assignee
recipients << target.assignee
end
@@ -489,7 +488,7 @@ class NotificationService
recipients = reject_muted_users(recipients, project)
recipients = add_subscribed_users(recipients, target)
- if [:new_issue_email, :new_merge_request_email].include?(action)
+ if [:new_issue, :new_merge_request].include?(custom_action)
recipients = add_labels_subscribers(recipients, target)
end
@@ -519,4 +518,9 @@ class NotificationService
end
end
end
+
+ # Builds key to be used if user has custom notification setting
+ def build_custom_key(action, object)
+ "#{action}_#{object.class.name.underscore}".to_sym
+ end
end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index b99e02ba678..49eaeabb8f4 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -46,6 +46,7 @@ describe NotificationService, services: true do
project.team << [issue.assignee, :master]
project.team << [note.author, :master]
create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy')
+ update_custom_notification(:new_note)
end
describe :new_note do
@@ -66,6 +67,7 @@ describe NotificationService, services: true do
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@subscribed_participant)
+ should_not_email(@u_guest_custom)
should_not_email(@u_guest_watcher)
should_not_email(note.author)
should_not_email(@u_participating)
@@ -116,6 +118,7 @@ describe NotificationService, services: true do
should_email(note.noteable.author)
should_email(note.noteable.assignee)
should_email(@u_mentioned)
+ should_not_email(@u_guest_custom)
should_not_email(@u_guest_watcher)
should_not_email(@u_watcher)
should_not_email(note.author)
@@ -238,13 +241,14 @@ describe NotificationService, services: true do
build_team(note.project)
ActionMailer::Base.deliveries.clear
allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer)
+ update_custom_notification(:new_note)
end
describe '#new_note, #perform_enqueued_jobs' do
it do
notification.new_note(note)
-
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_committer)
should_email(@u_watcher)
should_not_email(@u_mentioned)
@@ -285,6 +289,7 @@ describe NotificationService, services: true do
build_team(issue.project)
add_users_with_subscription(issue.project, issue)
ActionMailer::Base.deliveries.clear
+ update_custom_notification(:new_issue)
end
describe '#new_issue' do
@@ -294,6 +299,7 @@ describe NotificationService, services: true do
should_email(issue.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_not_email(@u_mentioned)
should_not_email(@u_participating)
@@ -349,12 +355,15 @@ describe NotificationService, services: true do
end
describe '#reassigned_issue' do
+ before { update_custom_notification(:reassign_issue) }
+
it 'emails new assignee' do
notification.reassigned_issue(issue, @u_disabled)
should_email(issue.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(@unsubscriber)
@@ -371,6 +380,7 @@ describe NotificationService, services: true do
should_email(@u_mentioned)
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(@unsubscriber)
@@ -387,6 +397,7 @@ describe NotificationService, services: true do
should_email(issue.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(@unsubscriber)
@@ -403,6 +414,7 @@ describe NotificationService, services: true do
should_email(issue.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(@unsubscriber)
@@ -418,6 +430,7 @@ describe NotificationService, services: true do
expect(issue.assignee).to be @u_mentioned
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_not_email(issue.assignee)
@@ -518,6 +531,8 @@ describe NotificationService, services: true do
end
describe '#close_issue' do
+ before { update_custom_notification(:close_issue) }
+
it 'should sent email to issue assignee and issue author' do
notification.close_issue(issue, @u_disabled)
@@ -525,6 +540,7 @@ describe NotificationService, services: true do
should_email(issue.author)
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_email(@watcher_and_subscriber)
@@ -564,6 +580,8 @@ describe NotificationService, services: true do
end
describe '#reopen_issue' do
+ before { update_custom_notification(:reopen_issue) }
+
it 'should send email to issue assignee and issue author' do
notification.reopen_issue(issue, @u_disabled)
@@ -571,6 +589,7 @@ describe NotificationService, services: true do
should_email(issue.author)
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_email(@watcher_and_subscriber)
@@ -620,6 +639,8 @@ describe NotificationService, services: true do
end
describe '#new_merge_request' do
+ before { update_custom_notification(:new_merge_request) }
+
it do
notification.new_merge_request(merge_request, @u_disabled)
@@ -628,6 +649,7 @@ describe NotificationService, services: true do
should_email(@watcher_and_subscriber)
should_email(@u_participant_mentioned)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
@@ -674,6 +696,8 @@ describe NotificationService, services: true do
end
describe '#reassigned_merge_request' do
+ before { update_custom_notification(:reassign_merge_request) }
+
it do
notification.reassigned_merge_request(merge_request, merge_request.author)
@@ -683,6 +707,7 @@ describe NotificationService, services: true do
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
@@ -750,12 +775,15 @@ describe NotificationService, services: true do
end
describe '#closed_merge_request' do
+ before { update_custom_notification(:close_merge_request) }
+
it do
notification.close_mr(merge_request, @u_disabled)
should_email(merge_request.assignee)
should_email(@u_watcher)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_email(@watcher_and_subscriber)
@@ -796,6 +824,8 @@ describe NotificationService, services: true do
end
describe '#merged_merge_request' do
+ before { update_custom_notification(:merge_merge_request) }
+
it do
notification.merge_mr(merge_request, @u_disabled)
@@ -805,6 +835,7 @@ describe NotificationService, services: true do
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
@@ -842,6 +873,8 @@ describe NotificationService, services: true do
end
describe '#reopen_merge_request' do
+ before { update_custom_notification(:reopen_merge_request) }
+
it do
notification.reopen_mr(merge_request, @u_disabled)
@@ -851,6 +884,7 @@ describe NotificationService, services: true do
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@u_guest_watcher)
+ should_email(@u_guest_custom)
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
@@ -904,6 +938,7 @@ describe NotificationService, services: true do
should_email(@u_participating)
should_email(@u_lazy_participant)
should_not_email(@u_guest_watcher)
+ should_not_email(@u_guest_custom)
should_not_email(@u_disabled)
end
end
@@ -924,7 +959,8 @@ describe NotificationService, services: true do
# It should be treated with a :participating notification_level
@u_lazy_participant = create(:user, username: 'lazy-participant')
- create_guest_watcher
+ @u_guest_watcher = create_user_with_notification(:watch, 'guest_watching')
+ @u_guest_custom = create_user_with_notification(:custom, 'guest_custom')
project.team << [@u_watcher, :master]
project.team << [@u_participating, :master]
@@ -944,10 +980,18 @@ describe NotificationService, services: true do
user
end
- def create_guest_watcher
- @u_guest_watcher = create(:user, username: 'guest_watching')
- setting = @u_guest_watcher.notification_settings_for(project)
- setting.level = :watch
+ def create_user_with_notification(level, username)
+ user = create(:user, username: username)
+ setting = user.notification_settings_for(project)
+ setting.level = level
+ setting.save
+
+ user
+ end
+
+ def update_custom_notification(event)
+ setting = @u_guest_custom.notification_settings_for(project)
+ setting.events[event] = true
setting.save
end
--
GitLab