Skip to content

G
gitlab-ce
Commit f45f1fb9 authored by Jan Provaznik's avatar Jan Provaznik
Browse files
Options

Merge branch 'notification_rec_service_refactor' into 'master' 

Refactor NotificationRecipientService

See merge request gitlab-org/gitlab!26394
parents a0c00397 50176a33
Hide whitespace changes
Inline Side-by-side
Showing with 404 additions and 322 deletions
app/models/member.rb
View file @ f45f1fb9
......@@ -374,7 +374,7 @@ class Member < ApplicationRecord
# always notify when there isn't a user yet
return true if user.blank?
NotificationRecipientService.notifiable?(user, type, notifiable_options.merge(opts))
NotificationRecipients::BuildService.notifiable?(user, type, notifiable_options.merge(opts))
end
# rubocop: enable CodeReuse/ServiceClass
......
app/services/notification_recipients/build_service.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
#
# Used by NotificationService to determine who should receive notification
#
module NotificationRecipients
module BuildService
def self.notifiable_users(users, *args)
users.compact.map { |u| NotificationRecipient.new(u, *args) }.select(&:notifiable?).map(&:user)
end
def self.notifiable?(user, *args)
NotificationRecipient.new(user, *args).notifiable?
end
def self.build_recipients(*args)
Builder::Default.new(*args).notification_recipients
end
def self.build_new_note_recipients(*args)
Builder::NewNote.new(*args).notification_recipients
end
def self.build_merge_request_unmergeable_recipients(*args)
Builder::MergeRequestUnmergeable.new(*args).notification_recipients
end
def self.build_project_maintainers_recipients(*args)
Builder::ProjectMaintainers.new(*args).notification_recipients
end
def self.build_new_release_recipients(*args)
Builder::NewRelease.new(*args).notification_recipients
end
end
end
NotificationRecipients::BuildService.prepend_if_ee('EE::NotificationRecipients::BuildService')
app/services/notification_recipient_service.rb → app/services/notification_recipients/builder/base.rb
View file @ f45f1fb9
# frozen_string_literal: true
#
# Used by NotificationService to determine who should receive notification
#
module NotificationRecipientService
def self.notifiable_users(users, *args)
users.compact.map { |u| NotificationRecipient.new(u, *args) }.select(&:notifiable?).map(&:user)
end
def self.notifiable?(user, *args)
NotificationRecipient.new(user, *args).notifiable?
end
def self.build_recipients(*args)
Builder::Default.new(*args).notification_recipients
end
def self.build_new_note_recipients(*args)
Builder::NewNote.new(*args).notification_recipients
end
def self.build_merge_request_unmergeable_recipients(*args)
Builder::MergeRequestUnmergeable.new(*args).notification_recipients
end
def self.build_project_maintainers_recipients(*args)
Builder::ProjectMaintainers.new(*args).notification_recipients
end
def self.build_new_release_recipients(*args)
Builder::NewRelease.new(*args).notification_recipients
end
module NotificationRecipients
module Builder
class Base
def initialize(*)
......@@ -244,186 +213,5 @@ module NotificationRecipientService
end
end
end
class Default < Base
MENTION_TYPE_ACTIONS = [:new_issue, :new_merge_request].freeze
attr_reader :target
attr_reader :current_user
attr_reader :action
attr_reader :previous_assignees
attr_reader :skip_current_user
def initialize(target, current_user, action:, custom_action: nil, previous_assignees: nil, skip_current_user: true)
@target = target
@current_user = current_user
@action = action
@custom_action = custom_action
@previous_assignees = previous_assignees
@skip_current_user = skip_current_user
end
def add_watchers
add_project_watchers
end
def build!
add_participants(current_user)
add_watchers
add_custom_notifications
# Re-assign is considered as a mention of the new assignee
case custom_action
when :reassign_merge_request, :reassign_issue
add_recipients(previous_assignees, :mention, nil)
add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED)
end
add_subscribed_users
if self.class.mention_type_actions.include?(custom_action)
# These will all be participants as well, but adding with the :mention
# type ensures that users with the mention notification level will
# receive them, too.
add_mentions(current_user, target: target)
# We use the `:participating` notification level in order to match existing legacy behavior as captured
# in existing specs (notification_service_spec.rb ~ line 507)
if target.is_a?(Issuable)
add_recipients(target.assignees, :participating, NotificationReason::ASSIGNED)
end
add_labels_subscribers
end
end
def acting_user
current_user if skip_current_user
end
# Build event key to search on custom notification level
# Check NotificationSetting.email_events
def custom_action
@custom_action ||= "#{action}_#{target.class.model_name.name.underscore}".to_sym
end
def self.mention_type_actions
MENTION_TYPE_ACTIONS.dup
end
end
class NewNote < Base
attr_reader :note
def initialize(note)
@note = note
end
def target
note.noteable
end
# NOTE: may be nil, in the case of a PersonalSnippet
#
# (this is okay because NotificationRecipient is written
# to handle nil projects)
def project
note.project
end
def group
if note.for_project_noteable?
project.group
else
target.try(:group)
end
end
def build!
# Add all users participating in the thread (author, assignee, comment authors)
add_participants(note.author)
add_mentions(note.author, target: note)
if note.for_project_noteable?
# Merge project watchers
add_project_watchers
else
add_group_watchers
end
add_custom_notifications
add_subscribed_users
end
def custom_action
:new_note
end
def acting_user
note.author
end
end
class NewRelease < Base
attr_reader :target
def initialize(target)
@target = target
end
def build!
add_recipients(target.project.authorized_users, :custom, nil)
end
def custom_action
:new_release
end
def acting_user
target.author
end
end
class MergeRequestUnmergeable < Base
attr_reader :target
def initialize(merge_request)
@target = merge_request
end
def build!
target.merge_participants.each do |user|
add_recipients(user, :participating, nil)
end
end
def custom_action
:unmergeable_merge_request
end
def acting_user
nil
end
end
class ProjectMaintainers < Base
attr_reader :target
def initialize(target, action:)
@target = target
@action = action
end
def build!
return [] unless project
add_recipients(project.team.maintainers, :mention, nil)
end
def acting_user
nil
end
end
end
end
NotificationRecipientService::Builder::Default.prepend_if_ee('EE::NotificationRecipientBuilders::Default') # rubocop: disable Cop/InjectEnterpriseEditionModule
NotificationRecipientService.prepend_if_ee('EE::NotificationRecipientService')
app/services/notification_recipients/builder/default.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
module NotificationRecipients
module Builder
class Default < Base
MENTION_TYPE_ACTIONS = [:new_issue, :new_merge_request].freeze
attr_reader :target
attr_reader :current_user
attr_reader :action
attr_reader :previous_assignees
attr_reader :skip_current_user
def initialize(target, current_user, action:, custom_action: nil, previous_assignees: nil, skip_current_user: true)
@target = target
@current_user = current_user
@action = action
@custom_action = custom_action
@previous_assignees = previous_assignees
@skip_current_user = skip_current_user
end
def add_watchers
add_project_watchers
end
def build!
add_participants(current_user)
add_watchers
add_custom_notifications
# Re-assign is considered as a mention of the new assignee
case custom_action
when :reassign_merge_request, :reassign_issue
add_recipients(previous_assignees, :mention, nil)
add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED)
end
add_subscribed_users
if self.class.mention_type_actions.include?(custom_action)
# These will all be participants as well, but adding with the :mention
# type ensures that users with the mention notification level will
# receive them, too.
add_mentions(current_user, target: target)
# We use the `:participating` notification level in order to match existing legacy behavior as captured
# in existing specs (notification_service_spec.rb ~ line 507)
if target.is_a?(Issuable)
add_recipients(target.assignees, :participating, NotificationReason::ASSIGNED)
end
add_labels_subscribers
end
end
def acting_user
current_user if skip_current_user
end
# Build event key to search on custom notification level
# Check NotificationSetting.email_events
def custom_action
@custom_action ||= "#{action}_#{target.class.model_name.name.underscore}".to_sym
end
def self.mention_type_actions
MENTION_TYPE_ACTIONS.dup
end
end
end
end
NotificationRecipients::Builder::Default.prepend_if_ee('EE::NotificationRecipients::Builder::Default')
app/services/notification_recipients/builder/merge_request_unmergeable.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
module NotificationRecipients
module Builder
class MergeRequestUnmergeable < Base
attr_reader :target
def initialize(merge_request)
@target = merge_request
end
def build!
target.merge_participants.each do |user|
add_recipients(user, :participating, nil)
end
end
def custom_action
:unmergeable_merge_request
end
def acting_user
nil
end
end
end
end
app/services/notification_recipients/builder/new_note.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
module NotificationRecipients
module Builder
class NewNote < Base
attr_reader :note
def initialize(note)
@note = note
end
def target
note.noteable
end
# NOTE: may be nil, in the case of a PersonalSnippet
#
# (this is okay because NotificationRecipient is written
# to handle nil projects)
def project
note.project
end
def group
if note.for_project_noteable?
project.group
else
target.try(:group)
end
end
def build!
# Add all users participating in the thread (author, assignee, comment authors)
add_participants(note.author)
add_mentions(note.author, target: note)
if note.for_project_noteable?
# Merge project watchers
add_project_watchers
else
add_group_watchers
end
add_custom_notifications
add_subscribed_users
end
def custom_action
:new_note
end
def acting_user
note.author
end
end
end
end
app/services/notification_recipients/builder/new_release.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
module NotificationRecipients
module Builder
class NewRelease < Base
attr_reader :target
def initialize(target)
@target = target
end
def build!
add_recipients(target.project.authorized_users, :custom, nil)
end
def custom_action
:new_release
end
def acting_user
target.author
end
end
end
end
app/services/notification_recipients/builder/project_maintainers.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
module NotificationRecipients
module Builder
class ProjectMaintainers < Base
attr_reader :target
def initialize(target, action:)
@target = target
@action = action
end
def build!
return [] unless project
add_recipients(project.team.maintainers, :mention, nil)
end
def acting_user
nil
end
end
end
end
app/services/notification_service.rb
View file @ f45f1fb9
......@@ -108,7 +108,7 @@ class NotificationService
# * users with custom level checked with "reassign issue"
#
def reassigned_issue(issue, current_user, previous_assignees = [])
recipients = NotificationRecipientService.build_recipients(
recipients = NotificationRecipients::BuildService.build_recipients(
issue,
current_user,
action: "reassign",
......@@ -161,7 +161,7 @@ class NotificationService
def push_to_merge_request(merge_request, current_user, new_commits: [], existing_commits: [])
new_commits = new_commits.map { |c| { short_id: c.short_id, title: c.title } }
existing_commits = existing_commits.map { |c| { short_id: c.short_id, title: c.title } }
recipients = NotificationRecipientService.build_recipients(merge_request, current_user, action: "push_to")
recipients = NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: "push_to")
recipients.each do |recipient|
mailer.send(:push_to_merge_request_email, recipient.user.id, merge_request.id, current_user.id, recipient.reason, new_commits: new_commits, existing_commits: existing_commits).deliver_later
......@@ -197,7 +197,7 @@ class NotificationService
# * users with custom level checked with "reassign merge request"
#
def reassigned_merge_request(merge_request, current_user, previous_assignees = [])
recipients = NotificationRecipientService.build_recipients(
recipients = NotificationRecipients::BuildService.build_recipients(
merge_request,
current_user,
action: "reassign",
......@@ -260,7 +260,7 @@ class NotificationService
end
def resolve_all_discussions(merge_request, current_user)
recipients = NotificationRecipientService.build_recipients(
recipients = NotificationRecipients::BuildService.build_recipients(
merge_request,
current_user,
action: "resolve_all_discussions")
......@@ -291,7 +291,7 @@ class NotificationService
def send_new_note_notifications(note)
notify_method = "note_#{note.noteable_ability_name}_email".to_sym
recipients = NotificationRecipientService.build_new_note_recipients(note)
recipients = NotificationRecipients::BuildService.build_new_note_recipients(note)
recipients.each do |recipient|
mailer.send(notify_method, recipient.user.id, note.id, recipient.reason).deliver_later
end
......@@ -299,7 +299,7 @@ class NotificationService
# Notify users when a new release is created
def send_new_release_notifications(release)
recipients = NotificationRecipientService.build_new_release_recipients(release)
recipients = NotificationRecipients::BuildService.build_new_release_recipients(release)
recipients.each do |recipient|
mailer.new_release_email(recipient.user.id, release, recipient.reason).deliver_later
......@@ -413,7 +413,7 @@ class NotificationService
end
def issue_moved(issue, new_issue, current_user)
recipients = NotificationRecipientService.build_recipients(issue, current_user, action: 'moved')
recipients = NotificationRecipients::BuildService.build_recipients(issue, current_user, action: 'moved')
recipients.map do |recipient|
email = mailer.issue_moved_email(recipient.user, issue, new_issue, current_user, recipient.reason)
......@@ -490,7 +490,7 @@ class NotificationService
end
def issue_due(issue)
recipients = NotificationRecipientService.build_recipients(
recipients = NotificationRecipients::BuildService.build_recipients(
issue,
issue.author,
action: 'due',
......@@ -526,7 +526,7 @@ class NotificationService
protected
def new_resource_email(target, method)
recipients = NotificationRecipientService.build_recipients(target, target.author, action: "new")
recipients = NotificationRecipients::BuildService.build_recipients(target, target.author, action: "new")
recipients.each do |recipient|
mailer.send(method, recipient.user.id, target.id, recipient.reason).deliver_later
......@@ -534,7 +534,7 @@ class NotificationService
end
def new_mentions_in_resource_email(target, new_mentioned_users, current_user, method)
recipients = NotificationRecipientService.build_recipients(target, current_user, action: "new")
recipients = NotificationRecipients::BuildService.build_recipients(target, current_user, action: "new")
recipients = recipients.select {|r| new_mentioned_users.include?(r.user) }
recipients.each do |recipient|
......@@ -545,7 +545,7 @@ class NotificationService
def close_resource_email(target, current_user, method, skip_current_user: true, closed_via: nil)
action = method == :merged_merge_request_email ? "merge" : "close"
recipients = NotificationRecipientService.build_recipients(
recipients = NotificationRecipients::BuildService.build_recipients(
target,
current_user,
action: action,
......@@ -573,7 +573,7 @@ class NotificationService
end
def removed_milestone_resource_email(target, current_user, method)
recipients = NotificationRecipientService.build_recipients(
recipients = NotificationRecipients::BuildService.build_recipients(
target,
current_user,
action: 'removed_milestone'
......@@ -585,7 +585,7 @@ class NotificationService
end
def changed_milestone_resource_email(target, milestone, current_user, method)
recipients = NotificationRecipientService.build_recipients(
recipients = NotificationRecipients::BuildService.build_recipients(
target,
current_user,
action: 'changed_milestone'
......@@ -597,7 +597,7 @@ class NotificationService
end
def reopen_resource_email(target, current_user, method, status)
recipients = NotificationRecipientService.build_recipients(target, current_user, action: "reopen")
recipients = NotificationRecipients::BuildService.build_recipients(target, current_user, action: "reopen")
recipients.each do |recipient|
mailer.send(method, recipient.user.id, target.id, status, current_user.id, recipient.reason).deliver_later
......@@ -605,7 +605,7 @@ class NotificationService
end
def merge_request_unmergeable_email(merge_request)
recipients = NotificationRecipientService.build_merge_request_unmergeable_recipients(merge_request)
recipients = NotificationRecipients::BuildService.build_merge_request_unmergeable_recipients(merge_request)
recipients.each do |recipient|
mailer.merge_request_unmergeable_email(recipient.user.id, merge_request.id).deliver_later
......@@ -619,15 +619,15 @@ class NotificationService
private
def project_maintainers_recipients(target, action:)
NotificationRecipientService.build_project_maintainers_recipients(target, action: action)
NotificationRecipients::BuildService.build_project_maintainers_recipients(target, action: action)
end
def notifiable?(*args)
NotificationRecipientService.notifiable?(*args)
NotificationRecipients::BuildService.notifiable?(*args)
end
def notifiable_users(*args)
NotificationRecipientService.notifiable_users(*args)
NotificationRecipients::BuildService.notifiable_users(*args)
end
def deliver_access_request_email(recipient, member)
......
ee/app/services/ee/notification_recipient_builders/default.rb deleted 100644 → 0
View file @ a0c00397
# frozen_string_literal: true
module EE
module NotificationRecipientBuilders
module Default
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
class_methods do
extend ::Gitlab::Utils::Override
override :mention_type_actions
def mention_type_actions
super.append(:new_epic)
end
end
override :add_watchers
def add_watchers
if project
super
else # for group level targets
add_group_watchers
end
end
end
end
end
ee/app/services/ee/notification_recipient_service.rb deleted 100644 → 0
View file @ a0c00397
# frozen_string_literal: true
module EE
module NotificationRecipientService
extend ActiveSupport::Concern
class_methods do
def build_new_review_recipients(*args)
NotificationRecipientService::Builder::NewReview.new(*args).notification_recipients
end
end
prepended do
module Builder
class NewReview < ::NotificationRecipientService::Builder::Base
attr_reader :review
def initialize(review)
@review = review
end
def target
review.merge_request
end
def project
review.project
end
def group
project.group
end
def build!
add_participants(review.author)
add_mentions(review.author, target: review)
add_project_watchers
add_custom_notifications
add_subscribed_users
end
# A new review is a batch of new notes
# therefore new_note subscribers should also
# receive incoming new reviews
def custom_action
:new_note
end
def acting_user
review.author
end
end
end
end
end
end
ee/app/services/ee/notification_recipients/build_service.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
module EE
module NotificationRecipients
module BuildService
extend ActiveSupport::Concern
class_methods do
def build_new_review_recipients(*args)
NotificationRecipients::Builder::NewReview.new(*args).notification_recipients
end
end
end
end
end
ee/app/services/ee/notification_recipients/builder/default.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
module EE
module NotificationRecipients
module Builder
module Default
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
class_methods do
extend ::Gitlab::Utils::Override
override :mention_type_actions
def mention_type_actions
super.append(:new_epic)
end
end
override :add_watchers
def add_watchers
if project
super
else # for group level targets
add_group_watchers
end
end
end
end
end
end
ee/app/services/ee/notification_recipients/builder/new_review.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
module EE
module NotificationRecipients
module Builder
class NewReview < ::NotificationRecipients::Builder::Base
attr_reader :review
def initialize(review)
@review = review
end
def target
review.merge_request
end
def project
review.project
end
def group
project.group
end
def build!
add_participants(review.author)
add_mentions(review.author, target: review)
add_project_watchers
add_custom_notifications
add_subscribed_users
end
# A new review is a batch of new notes
# therefore new_note subscribers should also
# receive incoming new reviews
def custom_action
:new_note
end
def acting_user
review.author
end
end
end
end
end
ee/app/services/ee/notification_service.rb
View file @ f45f1fb9
......@@ -82,7 +82,7 @@ module EE
end
def send_new_review_notification(review)
recipients = ::NotificationRecipientService.build_new_review_recipients(review)
recipients = ::NotificationRecipients::BuildService.build_new_review_recipients(review)
recipients.each do |recipient|
mailer.new_review_email(recipient.user.id, review.id).deliver_later
......@@ -96,7 +96,7 @@ module EE
end
def approve_mr_email(merge_request, project, current_user)
recipients = ::NotificationRecipientService.build_recipients(merge_request, current_user, action: 'approve')
recipients = ::NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: 'approve')
recipients.each do |recipient|
mailer.approved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later
......@@ -104,7 +104,7 @@ module EE
end
def unapprove_mr_email(merge_request, project, current_user)
recipients = ::NotificationRecipientService.build_recipients(merge_request, current_user, action: 'unapprove')
recipients = ::NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: 'unapprove')
recipients.each do |recipient|
mailer.unapproved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later
......@@ -129,7 +129,7 @@ module EE
def epic_status_change_email(target, current_user, status)
action = status == 'reopened' ? 'reopen' : 'close'
recipients = ::NotificationRecipientService.build_recipients(
recipients = ::NotificationRecipients::BuildService.build_recipients(
target,
current_user,
action: action
......
ee/spec/services/ee/notification_recipient_service_spec.rb → ee/spec/services/ee/notification_recipients/build_service_spec.rb
View file @ f45f1fb9
......@@ -2,7 +2,7 @@
require 'spec_helper'
describe NotificationRecipientService do
describe NotificationRecipients::BuildService do
subject(:service) { described_class }
let(:project) { create(:project, :public) }
......
spec/services/notification_recipient_service_spec.rb → spec/services/notification_recipients/build_service_spec.rb
View file @ f45f1fb9
......@@ -2,7 +2,7 @@
require 'spec_helper'
describe NotificationRecipientService do
describe NotificationRecipients::BuildService do
let(:service) { described_class }
let(:assignee) { create(:user) }
let(:project) { create(:project, :public) }
......
spec/services/notification_recipients/builder/default_spec.rb 0 → 100644
View file @ f45f1fb9
# frozen_string_literal: true
require 'spec_helper'
describe NotificationRecipients::Builder::Default do
describe '#build!' do
let_it_be(:group) { create(:group, :public) }
let_it_be(:project) { create(:project, :public, group: group).tap { |p| p.add_developer(project_watcher) } }
let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:current_user) { create(:user) }
let_it_be(:other_user) { create(:user) }
let_it_be(:participant) { create(:user) }
let_it_be(:group_watcher) { create(:user) }
let_it_be(:project_watcher) { create(:user) }
let_it_be(:notification_setting_project_w) { create(:notification_setting, source: project, user: project_watcher, level: 2) }
let_it_be(:notification_setting_group_w) { create(:notification_setting, source: group, user: group_watcher, level: 2) }
subject { described_class.new(issue, current_user, action: :new).tap { |s| s.build! } }
context 'participants and project watchers' do
before do
expect(issue).to receive(:participants).and_return([participant, current_user])
end
it 'adds all participants and watchers' do
expect(subject.recipients.map(&:user)).to include(participant, project_watcher, group_watcher)
expect(subject.recipients.map(&:user)).not_to include(other_user)
end
end
context 'subscribers' do
it 'adds all subscribers' do
subscriber = create(:user)
non_subscriber = create(:user)
create(:subscription, project: project, user: subscriber, subscribable: issue, subscribed: true)
create(:subscription, project: project, user: non_subscriber, subscribable: issue, subscribed: false)
expect(subject.recipients.map(&:user)).to include(subscriber)
end
end
end
end
spec/services/notification_service_spec.rb
View file @ f45f1fb9
......@@ -710,7 +710,7 @@ describe NotificationService, :mailer do
user_3 = create(:user)
recipient_1 = NotificationRecipient.new(user_1, :custom, custom_action: :new_release)
recipient_2 = NotificationRecipient.new(user_2, :custom, custom_action: :new_release)
allow(NotificationRecipientService).to receive(:build_new_release_recipients).and_return([recipient_1, recipient_2])
allow(NotificationRecipients::BuildService).to receive(:build_new_release_recipients).and_return([recipient_1, recipient_2])
release
......
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7