summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/helpers/emails_helper.rb16
-rw-r--r--app/mailers/emails/issues.rb33
-rw-r--r--app/mailers/emails/merge_requests.rb37
-rw-r--r--app/mailers/notify.rb2
-rw-r--r--app/models/notification_reason.rb19
-rw-r--r--app/models/notification_recipient.rb36
-rw-r--r--app/services/notification_recipient_service.rb48
-rw-r--r--app/services/notification_service.rb26
-rw-r--r--app/views/layouts/notify.html.haml2
-rw-r--r--app/views/layouts/notify.text.erb2
-rw-r--r--changelogs/unreleased/41532-email-reason.yml5
-rw-r--r--doc/workflow/notifications.md30
-rw-r--r--spec/mailers/notify_spec.rb49
-rw-r--r--spec/services/notification_service_spec.rb62
-rw-r--r--spec/support/email_helpers.rb4
-rw-r--r--spec/workers/new_issue_worker_spec.rb5
-rw-r--r--spec/workers/new_merge_request_worker_spec.rb6
17 files changed, 288 insertions, 94 deletions
diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb
index 878bc9b5c9c..4ddc1dbed49 100644
--- a/app/helpers/emails_helper.rb
+++ b/app/helpers/emails_helper.rb
@@ -80,4 +80,20 @@ module EmailsHelper
'text-align:center'
].join(';')
end
+
+ # "You are receiving this email because #{reason}"
+ def notification_reason_text(reason)
+ string = case reason
+ when NotificationReason::OWN_ACTIVITY
+ 'of your activity'
+ when NotificationReason::ASSIGNED
+ 'you have been assigned an item'
+ when NotificationReason::MENTIONED
+ 'you have been mentioned'
+ else
+ 'of your account'
+ end
+
+ "#{string} on #{Gitlab.config.gitlab.host}"
+ end
end
diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb
index 64ca2d2eacf..b33131becd3 100644
--- a/app/mailers/emails/issues.rb
+++ b/app/mailers/emails/issues.rb
@@ -1,54 +1,54 @@
module Emails
module Issues
- def new_issue_email(recipient_id, issue_id)
+ def new_issue_email(recipient_id, issue_id, reason = nil)
setup_issue_mail(issue_id, recipient_id)
- mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id))
+ mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id, reason))
end
- def new_mention_in_issue_email(recipient_id, issue_id, updated_by_user_id)
+ def new_mention_in_issue_email(recipient_id, issue_id, updated_by_user_id, reason = nil)
setup_issue_mail(issue_id, recipient_id)
- mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason))
end
- def reassigned_issue_email(recipient_id, issue_id, previous_assignee_ids, updated_by_user_id)
+ def reassigned_issue_email(recipient_id, issue_id, previous_assignee_ids, updated_by_user_id, reason = nil)
setup_issue_mail(issue_id, recipient_id)
@previous_assignees = []
@previous_assignees = User.where(id: previous_assignee_ids) if previous_assignee_ids.any?
- mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason))
end
- def closed_issue_email(recipient_id, issue_id, updated_by_user_id)
+ def closed_issue_email(recipient_id, issue_id, updated_by_user_id, reason = nil)
setup_issue_mail(issue_id, recipient_id)
@updated_by = User.find(updated_by_user_id)
- mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason))
end
- def relabeled_issue_email(recipient_id, issue_id, label_names, updated_by_user_id)
+ def relabeled_issue_email(recipient_id, issue_id, label_names, updated_by_user_id, reason = nil)
setup_issue_mail(issue_id, recipient_id)
@label_names = label_names
@labels_url = project_labels_url(@project)
- mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason))
end
- def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
+ def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id, reason = nil)
setup_issue_mail(issue_id, recipient_id)
@issue_status = status
@updated_by = User.find(updated_by_user_id)
- mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason))
end
- def issue_moved_email(recipient, issue, new_issue, updated_by_user)
+ def issue_moved_email(recipient, issue, new_issue, updated_by_user, reason = nil)
setup_issue_mail(issue.id, recipient.id)
@new_issue = new_issue
@new_project = new_issue.project
- mail_answer_thread(issue, issue_thread_options(updated_by_user.id, recipient.id))
+ mail_answer_thread(issue, issue_thread_options(updated_by_user.id, recipient.id, reason))
end
private
@@ -61,11 +61,12 @@ module Emails
@sent_notification = SentNotification.record(@issue, recipient_id, reply_key)
end
- def issue_thread_options(sender_id, recipient_id)
+ def issue_thread_options(sender_id, recipient_id, reason)
{
from: sender(sender_id),
to: recipient(recipient_id),
- subject: subject("#{@issue.title} (##{@issue.iid})")
+ subject: subject("#{@issue.title} (##{@issue.iid})"),
+ 'X-GitLab-NotificationReason' => reason
}
end
end
diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb
index 3626f8ce416..5fe09cea83f 100644
--- a/app/mailers/emails/merge_requests.rb
+++ b/app/mailers/emails/merge_requests.rb
@@ -1,57 +1,57 @@
module Emails
module MergeRequests
- def new_merge_request_email(recipient_id, merge_request_id)
+ def new_merge_request_email(recipient_id, merge_request_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id)
- mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id))
+ mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason))
end
- def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
+ def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id)
- mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
end
- def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id)
+ def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id)
@previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
- mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
end
- def relabeled_merge_request_email(recipient_id, merge_request_id, label_names, updated_by_user_id)
+ def relabeled_merge_request_email(recipient_id, merge_request_id, label_names, updated_by_user_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id)
@label_names = label_names
@labels_url = project_labels_url(@project)
- mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
end
- def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
+ def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id)
@updated_by = User.find(updated_by_user_id)
- mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
end
- def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
+ def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id)
- mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
end
- def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id)
+ def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id)
@mr_status = status
@updated_by = User.find(updated_by_user_id)
- mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+ mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
end
- def resolved_all_discussions_email(recipient_id, merge_request_id, resolved_by_user_id)
+ def resolved_all_discussions_email(recipient_id, merge_request_id, resolved_by_user_id, reason = nil)
setup_merge_request_mail(merge_request_id, recipient_id)
@resolved_by = User.find(resolved_by_user_id)
- mail_answer_thread(@merge_request, merge_request_thread_options(resolved_by_user_id, recipient_id))
+ mail_answer_thread(@merge_request, merge_request_thread_options(resolved_by_user_id, recipient_id, reason))
end
private
@@ -64,11 +64,12 @@ module Emails
@sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
end
- def merge_request_thread_options(sender_id, recipient_id)
+ def merge_request_thread_options(sender_id, recipient_id, reason = nil)
{
from: sender(sender_id),
to: recipient(recipient_id),
- subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})")
+ subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})"),
+ 'X-GitLab-NotificationReason' => reason
}
end
end
diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb
index ec886e993c3..eade0fe278f 100644
--- a/app/mailers/notify.rb
+++ b/app/mailers/notify.rb
@@ -112,6 +112,8 @@ class Notify < BaseMailer
headers["X-GitLab-#{model.class.name}-ID"] = model.id
headers['X-GitLab-Reply-Key'] = reply_key
+ @reason = headers['X-GitLab-NotificationReason']
+
if Gitlab::IncomingEmail.enabled? && @sent_notification
address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key))
address.display_name = @project.name_with_namespace
diff --git a/app/models/notification_reason.rb b/app/models/notification_reason.rb
new file mode 100644
index 00000000000..c3965565022
--- /dev/null
+++ b/app/models/notification_reason.rb
@@ -0,0 +1,19 @@
+# Holds reasons for a notification to have been sent as well as a priority list to select which reason to use
+# above the rest
+class NotificationReason
+ OWN_ACTIVITY = 'own_activity'.freeze
+ ASSIGNED = 'assigned'.freeze
+ MENTIONED = 'mentioned'.freeze
+
+ # Priority list for selecting which reason to return in the notification
+ REASON_PRIORITY = [
+ OWN_ACTIVITY,
+ ASSIGNED,
+ MENTIONED
+ ].freeze
+
+ # returns the priority of a reason as an integer
+ def self.priority(reason)
+ REASON_PRIORITY.index(reason) || REASON_PRIORITY.length + 1
+ end
+end
diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb
index ab5a96209c7..472b348a545 100644
--- a/app/models/notification_recipient.rb
+++ b/app/models/notification_recipient.rb
@@ -1,27 +1,19 @@
class NotificationRecipient
- attr_reader :user, :type
- def initialize(
- user, type,
- custom_action: nil,
- target: nil,
- acting_user: nil,
- project: nil,
- group: nil,
- skip_read_ability: false
- )
-
+ attr_reader :user, :type, :reason
+ def initialize(user, type, **opts)
unless NotificationSetting.levels.key?(type) || type == :subscription
raise ArgumentError, "invalid type: #{type.inspect}"
end
- @custom_action = custom_action
- @acting_user = acting_user
- @target = target
- @project = project || default_project
- @group = group || @project&.group
+ @custom_action = opts[:custom_action]
+ @acting_user = opts[:acting_user]
+ @target = opts[:target]
+ @project = opts[:project] || default_project
+ @group = opts[:group] || @project&.group
@user = user
@type = type
- @skip_read_ability = skip_read_ability
+ @reason = opts[:reason]
+ @skip_read_ability = opts[:skip_read_ability]
end
def notification_setting
@@ -77,9 +69,15 @@ class NotificationRecipient
def own_activity?
return false unless @acting_user
- return false if @acting_user.notified_of_own_activity?
- user == @acting_user
+ if user == @acting_user
+ # if activity was generated by the same user, change reason to :own_activity
+ @reason = NotificationReason::OWN_ACTIVITY
+ # If the user wants to be notified, we must return `false`
+ !@acting_user.notified_of_own_activity?
+ else
+ false
+ end
end
def has_access?
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 3eb8cfcca9b..6835b14648b 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -11,11 +11,11 @@ module NotificationRecipientService
end
def self.build_recipients(*a)
- Builder::Default.new(*a).recipient_users
+ Builder::Default.new(*a).notification_recipients
end
def self.build_new_note_recipients(*a)
- Builder::NewNote.new(*a).recipient_users
+ Builder::NewNote.new(*a).notification_recipients
end
module Builder
@@ -49,25 +49,24 @@ module NotificationRecipientService
@recipients ||= []
end
- def <<(pair)
- users, type = pair
-
+ def add_recipients(users, type, reason)
if users.is_a?(ActiveRecord::Relation)
users = users.includes(:notification_settings)
end
users = Array(users)
users.compact!
- recipients.concat(users.map { |u| make_recipient(u, type) })
+ recipients.concat(users.map { |u| make_recipient(u, type, reason) })
end
def user_scope
User.includes(:notification_settings)
end
- def make_recipient(user, type)
+ def make_recipient(user, type, reason)
NotificationRecipient.new(
user, type,
+ reason: reason,
project: project,
custom_action: custom_action,
target: target,
@@ -75,14 +74,13 @@ module NotificationRecipientService
)
end
- def recipient_users
- @recipient_users ||=
+ def notification_recipients
+ @notification_recipients ||=
begin
build!
filter!
- users = recipients.map(&:user)
- users.uniq!
- users.freeze
+ recipients = self.recipients.sort_by { |r| NotificationReason.priority(r.reason) }.uniq(&:user)
+ recipients.freeze
end
end
@@ -95,13 +93,13 @@ module NotificationRecipientService
def add_participants(user)
return unless target.respond_to?(:participants)
- self << [target.participants(user), :participating]
+ add_recipients(target.participants(user), :participating, nil)
end
def add_mentions(user, target:)
return unless target.respond_to?(:mentioned_users)
- self << [target.mentioned_users(user), :mention]
+ add_recipients(target.mentioned_users(user), :mention, NotificationReason::MENTIONED)
end
# Get project/group users with CUSTOM notification level
@@ -119,11 +117,11 @@ module NotificationRecipientService
global_users_ids = user_ids_with_project_level_global.concat(user_ids_with_group_level_global)
user_ids += user_ids_with_global_level_custom(global_users_ids, custom_action)
- self << [user_scope.where(id: user_ids), :watch]
+ add_recipients(user_scope.where(id: user_ids), :watch, nil)
end
def add_project_watchers
- self << [project_watchers, :watch]
+ add_recipients(project_watchers, :watch, nil)
end
# Get project users with WATCH notification level
@@ -144,7 +142,7 @@ module NotificationRecipientService
def add_subscribed_users
return unless target.respond_to? :subscribers
- self << [target.subscribers(project), :subscription]
+ add_recipients(target.subscribers(project), :subscription, nil)
end
def user_ids_notifiable_on(resource, notification_level = nil)
@@ -195,7 +193,7 @@ module NotificationRecipientService
return unless target.respond_to? :labels
(labels || target.labels).each do |label|
- self << [label.subscribers(project), :subscription]
+ add_recipients(label.subscribers(project), :subscription, nil)
end
end
end
@@ -222,12 +220,12 @@ module NotificationRecipientService
# Re-assign is considered as a mention of the new assignee
case custom_action
when :reassign_merge_request
- self << [previous_assignee, :mention]
- self << [target.assignee, :mention]
+ add_recipients(previous_assignee, :mention, nil)
+ add_recipients(target.assignee, :mention, NotificationReason::ASSIGNED)
when :reassign_issue
previous_assignees = Array(previous_assignee)
- self << [previous_assignees, :mention]
- self << [target.assignees, :mention]
+ add_recipients(previous_assignees, :mention, nil)
+ add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED)
end
add_subscribed_users
@@ -238,6 +236,12 @@ module NotificationRecipientService
# receive them, too.
add_mentions(current_user, target: target)
+ # Add the assigned users, if any
+ assignees = custom_action == :new_issue ? target.assignees : target.assignee
+ # We use the `:participating` notification level in order to match existing legacy behavior as captured
+ # in existing specs (notification_service_spec.rb ~ line 507)
+ add_recipients(assignees, :participating, NotificationReason::ASSIGNED) if assignees
+
add_labels_subscribers
end
end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index be3b4b2ba07..8c84ccfcc92 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -85,10 +85,11 @@ class NotificationService
recipients.each do |recipient|
mailer.send(
:reassigned_issue_email,
- recipient.id,
+ recipient.user.id,
issue.id,
previous_assignee_ids,
- current_user.id
+ current_user.id,
+ recipient.reason
).deliver_later
end
end
@@ -176,7 +177,7 @@ class NotificationService
action: "resolve_all_discussions")
recipients.each do |recipient|
- mailer.resolved_all_discussions_email(recipient.id, merge_request.id, current_user.id).deliver_later
+ mailer.resolved_all_discussions_email(recipient.user.id, merge_request.id, current_user.id, recipient.reason).deliver_later
end
end
@@ -199,7 +200,7 @@ class NotificationService
recipients = NotificationRecipientService.build_new_note_recipients(note)
recipients.each do |recipient|
- mailer.send(notify_method, recipient.id, note.id).deliver_later
+ mailer.send(notify_method, recipient.user.id, note.id).deliver_later
end
end
@@ -299,7 +300,7 @@ class NotificationService
recipients = NotificationRecipientService.build_recipients(issue, current_user, action: 'moved')
recipients.map do |recipient|
- email = mailer.issue_moved_email(recipient, issue, new_issue, current_user)
+ email = mailer.issue_moved_email(recipient.user, issue, new_issue, current_user, recipient.reason)
email.deliver_later
email
end
@@ -339,16 +340,16 @@ class NotificationService
recipients = NotificationRecipientService.build_recipients(target, target.author, action: "new")
recipients.each do |recipient|
- mailer.send(method, recipient.id, target.id).deliver_later
+ mailer.send(method, recipient.user.id, target.id, recipient.reason).deliver_later
end
end
def new_mentions_in_resource_email(target, new_mentioned_users, current_user, method)
recipients = NotificationRecipientService.build_recipients(target, current_user, action: "new")
- recipients = recipients & new_mentioned_users
+ recipients = recipients.select {|r| new_mentioned_users.include?(r.user) }
recipients.each do |recipient|
- mailer.send(method, recipient.id, target.id, current_user.id).deliver_later
+ mailer.send(method, recipient.user.id, target.id, current_user.id, recipient.reason).deliver_later
end
end
@@ -363,7 +364,7 @@ class NotificationService
)
recipients.each do |recipient|
- mailer.send(method, recipient.id, target.id, current_user.id).deliver_later
+ mailer.send(method, recipient.user.id, target.id, current_user.id, recipient.reason).deliver_later
end
end
@@ -381,10 +382,11 @@ class NotificationService
recipients.each do |recipient|
mailer.send(
method,
- recipient.id,
+ recipient.user.id,
target.id,
previous_assignee_id,
- current_user.id
+ current_user.id,
+ recipient.reason
).deliver_later
end
end
@@ -408,7 +410,7 @@ class NotificationService
recipients = NotificationRecipientService.build_recipients(target, current_user, action: "reopen")
recipients.each do |recipient|
- mailer.send(method, recipient.id, target.id, status, current_user.id).deliver_later
+ mailer.send(method, recipient.user.id, target.id, status, current_user.id, recipient.reason).deliver_later
end
end
diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml
index 40bf45cece7..ab8b1271212 100644
--- a/app/views/layouts/notify.html.haml
+++ b/app/views/layouts/notify.html.haml
@@ -20,7 +20,7 @@
#{link_to "View it on GitLab", @target_url}.
%br
-# Don't link the host in the line below, one link in the email is easier to quickly click than two.
- You're receiving this email because of your account on #{Gitlab.config.gitlab.host}.
+ You're receiving this email because #{notification_reason_text(@reason)}.
If you'd like to receive fewer emails, you can
- if @labels_url
adjust your #{link_to 'label subscriptions', @labels_url}.
diff --git a/app/views/layouts/notify.text.erb b/app/views/layouts/notify.text.erb
index b4ce02eead8..de48f548a1b 100644
--- a/app/views/layouts/notify.text.erb
+++ b/app/views/layouts/notify.text.erb
@@ -9,4 +9,4 @@
<% end -%>
<% end -%>
-You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>.
+<%= "You're receiving this email because #{notification_reason_text(@reason)}." %>
diff --git a/changelogs/unreleased/41532-email-reason.yml b/changelogs/unreleased/41532-email-reason.yml
new file mode 100644
index 00000000000..83c28769217
--- /dev/null
+++ b/changelogs/unreleased/41532-email-reason.yml
@@ -0,0 +1,5 @@
+---
+title: Initial work to add notification reason to emails
+merge_request: 16160
+author: Mario de la Ossa
+type: added
diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md
index 3e2e7d0f7b6..123b37abd19 100644
--- a/doc/workflow/notifications.md
+++ b/doc/workflow/notifications.md
@@ -104,3 +104,33 @@ You won't receive notifications for Issues, Merge Requests or Milestones
created by yourself. You will only receive automatic notifications when
somebody else comments or adds changes to the ones that you've created or
mentions you.
+
+### Email Headers
+
+Notification emails include headers that provide extra content about the notification received:
+
+| Header | Description |
+|-----------------------------|-------------------------------------------------------------------------|
+| X-GitLab-Project | The name of the project the notification belongs to |
+| X-GitLab-Project-Id | The ID of the project |
+| X-GitLab-Project-Path | The path of the project |
+| X-GitLab-(Resource)-ID | The ID of the resource the notification is for, where resource is `Issue`, `MergeRequest`, `Commit`, etc|
+| X-GitLab-Discussion-ID | Only in comment emails, the ID of the discussion the comment is from |
+| X-GitLab-Pipeline-Id | Only in pipeline emails, the ID of the pipeline the notification is for |
+| X-GitLab-Reply-Key | A unique token to support reply by email |
+| X-GitLab-NotificationReason | The reason for being notified. "mentioned", "assigned", etc |
+
+#### X-GitLab-NotificationReason
+This header holds the reason for the notification to have been sent out,
+where reason can be `mentioned`, `assigned`, `own_activity`, etc.
+Only one reason is sent out according to its priority:
+- `own_activity`
+- `assigned`
+- `mentioned`
+
+The reason in this header will also be shown in the footer of the notification email. For example an email with the
+reason `assigned` will have this sentence in the footer:
+`"You are receiving this email because you have been assigned an item on {configured GitLab hostname}"`
+
+**Note: Only reasons listed above have been implemented so far**
+Further implementation is [being discussed here](https://gitlab.com/gitlab-org/gitlab-ce/issues/42062)
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index cbc8c67da61..7a8e798e3c9 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -71,6 +71,18 @@ describe Notify do
is_expected.to have_html_escaped_body_text issue.description
end
+ it 'does not add a reason header' do
+ is_expected.not_to have_header('X-GitLab-NotificationReason', /.+/)
+ end
+
+ context 'when sent with a reason' do
+ subject { described_class.new_issue_email(issue.assignees.first.id, issue.id, NotificationReason::ASSIGNED) }
+
+ it 'includes the reason in a header' do
+ is_expected.to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED)
+ end
+ end
+
context 'when enabled email_author_in_body' do
before do
stub_application_setting(email_author_in_body: true)
@@ -108,6 +120,14 @@ describe Notify do
is_expected.to have_body_text(project_issue_path(project, issue))
end
end
+
+ context 'when sent with a reason' do
+ subject { described_class.reassigned_issue_email(recipient.id, issue.id, [previous_assignee.id], current_user.id, NotificationReason::ASSIGNED) }
+
+ it 'includes the reason in a header' do
+ is_expected.to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED)
+ end
+ end
end
describe 'that have been relabeled' do
@@ -226,6 +246,14 @@ describe Notify do
is_expected.to have_html_escaped_body_text merge_request.description
end
+ context 'when sent with a reason' do
+ subject { described_class.new_merge_request_email(merge_request.assignee_id, merge_request.id, NotificationReason::ASSIGNED) }
+
+ it 'includes the reason in a header' do
+ is_expected.to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED)
+ end
+ end
+
context 'when enabled email_author_in_body' do
before do
stub_application_setting(email_author_in_body: true)
@@ -263,6 +291,27 @@ describe Notify do
is_expected.to have_html_escaped_body_text(assignee.name)
end
end
+
+ context 'when sent with a reason' do
+ subject { described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, NotificationReason::ASSIGNED) }
+
+ it 'includes the reason in a header' do
+ is_expected.to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED)
+ end
+
+ it 'includes the reason in the footer' do
+ text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(NotificationReason::ASSIGNED)
+ is_expected.to have_body_text(text)
+
+ new_subject = described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, NotificationReason::MENTIONED)
+ text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(NotificationReason::MENTIONED)
+ expect(new_subject).to have_body_text(text)
+
+ new_subject = described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, nil)
+ text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(nil)
+ expect(new_subject).to have_body_text(text)
+ end
+ end
end
describe 'that have been relabeled' do
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 43e2643f709..5c59455e3e1 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -1,6 +1,8 @@
require 'spec_helper'
describe NotificationService, :mailer do
+ include EmailSpec::Matchers
+
let(:notification) { described_class.new }
let(:assignee) { create(:user) }
@@ -31,6 +33,14 @@ describe NotificationService, :mailer do
send_notifications(@u_disabled)
should_not_email_anyone
end
+
+ it 'sends the proper notification reason header' do
+ send_notifications(@u_watcher)
+ should_only_email(@u_watcher)
+ email = find_email_for(@u_watcher)
+
+ expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::MENTIONED)
+ end
end
# Next shared examples are intended to test notifications of "participants"
@@ -511,12 +521,35 @@ describe NotificationService, :mailer do
should_not_email(issue.assignees.first)
end
+ it 'properly prioritizes notification reason' do
+ # have assignee be both assigned and mentioned
+ issue.update_attribute(:description, "/cc #{assignee.to_reference} #{@u_mentioned.to_reference}")
+
+ notification.new_issue(issue, @u_disabled)
+
+ email = find_email_for(assignee)
+ expect(email).to have_header('X-GitLab-NotificationReason', 'assigned')
+
+ email = find_email_for(@u_mentioned)
+ expect(email).to have_header('X-GitLab-NotificationReason', 'mentioned')
+ end
+
+ it 'adds "assigned" reason for assignees if any' do
+ notification.new_issue(issue, @u_disabled)
+
+ email = find_email_for(assignee)
+
+ expect(email).to have_header('X-GitLab-NotificationReason', 'assigned')
+ end
+
it "emails any mentioned users with the mention level" do
issue.description = @u_mentioned.to_reference
notification.new_issue(issue, @u_disabled)
- should_email(@u_mentioned)
+ email = find_email_for(@u_mentioned)
+ expect(email).not_to be_nil
+ expect(email).to have_header('X-GitLab-NotificationReason', 'mentioned')
end
it "emails the author if they've opted into notifications about their activity" do
@@ -620,6 +653,14 @@ describe NotificationService, :mailer do
should_not_email(@u_lazy_participant)
end
+ it 'adds "assigned" reason for new assignee' do
+ notification.reassigned_issue(issue, @u_disabled, [assignee])
+
+ email = find_email_for(assignee)
+
+ expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED)
+ end
+
it 'emails previous assignee even if he has the "on mention" notif level' do
issue.assignees = [@u_mentioned]
notification.reassigned_issue(issue, @u_disabled, [@u_watcher])
@@ -910,6 +951,14 @@ describe NotificationService, :mailer do
should_not_email(@u_lazy_participant)
end
+ it 'adds "assigned" reason for assignee, if any' do
+ notification.new_merge_request(merge_request, @u_disabled)
+
+ email = find_email_for(merge_request.assignee)
+
+ expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED)
+ end
+
it "emails any mentioned users with the mention level" do
merge_request.description = @u_mentioned.to_reference
@@ -924,6 +973,9 @@ describe NotificationService, :mailer do
notification.new_merge_request(merge_request, merge_request.author)
should_email(merge_request.author)
+
+ email = find_email_for(merge_request.author)
+ expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::OWN_ACTIVITY)
end
it "doesn't email the author if they haven't opted into notifications about their activity" do
@@ -1009,6 +1061,14 @@ describe NotificationService, :mailer do
should_not_email(@u_lazy_participant)
end
+ it 'adds "assigned" reason for new assignee' do
+ notification.reassigned_merge_request(merge_request, merge_request.author)
+
+ email = find_email_for(merge_request.assignee)
+
+ expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED)
+ end
+
it_behaves_like 'participating notifications' do
let(:participant) { create(:user, username: 'user-participant') }
let(:issuable) { merge_request }
diff --git a/spec/support/email_helpers.rb b/spec/support/email_helpers.rb
index b39052923dd..1fb8252459f 100644
--- a/spec/support/email_helpers.rb
+++ b/spec/support/email_helpers.rb
@@ -30,4 +30,8 @@ module EmailHelpers
def email_recipients(kind: :to)
ActionMailer::Base.deliveries.flat_map(&kind)
end
+
+ def find_email_for(user)
+ ActionMailer::Base.deliveries.find { |d| d.to.include?(user.notification_email) }
+ end
end
diff --git a/spec/workers/new_issue_worker_spec.rb b/spec/workers/new_issue_worker_spec.rb
index 4e15ccc534b..baa8ddb59e5 100644
--- a/spec/workers/new_issue_worker_spec.rb
+++ b/spec/workers/new_issue_worker_spec.rb
@@ -44,8 +44,9 @@ describe NewIssueWorker do
expect { worker.perform(issue.id, user.id) }.to change { Event.count }.from(0).to(1)
end
- it 'creates a notification for the assignee' do
- expect(Notify).to receive(:new_issue_email).with(mentioned.id, issue.id).and_return(double(deliver_later: true))
+ it 'creates a notification for the mentioned user' do
+ expect(Notify).to receive(:new_issue_email).with(mentioned.id, issue.id, NotificationReason::MENTIONED)
+ .and_return(double(deliver_later: true))
worker.perform(issue.id, user.id)
end
diff --git a/spec/workers/new_merge_request_worker_spec.rb b/spec/workers/new_merge_request_worker_spec.rb
index 9e0cbde45b1..c3f29a40d58 100644
--- a/spec/workers/new_merge_request_worker_spec.rb
+++ b/spec/workers/new_merge_request_worker_spec.rb
@@ -46,8 +46,10 @@ describe NewMergeRequestWorker do
expect { worker.perform(merge_request.id, user.id) }.to change { Event.count }.from(0).to(1)
end
- it 'creates a notification for the assignee' do
- expect(Notify).to receive(:new_merge_request_email).with(mentioned.id, merge_request.id).and_return(double(deliver_later: true))
+ it 'creates a notification for the mentioned user' do
+ expect(Notify).to receive(:new_merge_request_email)
+ .with(mentioned.id, merge_request.id, NotificationReason::MENTIONED)
+ .and_return(double(deliver_later: true))
worker.perform(merge_request.id, user.id)
end
View Lorry Controller Status