diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb index 878bc9b5c9c26f18c3361e9c7c77356bc8e8fd97..4ddc1dbed49b30836f3484d844111b07c1520d8c 100644 --- a/app/helpers/emails_helper.rb +++ b/app/helpers/emails_helper.rb @@ -80,4 +80,20 @@ def email_default_heading(text) '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 64ca2d2eacf95d293126bee57026a3c8ba47eb5f..b33131becd35ad22076f3bb88934d4580b7dcb1b 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 @@ def setup_issue_mail(issue_id, recipient_id) @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 3626f8ce416131dd2a2716c98757c142cdc172c5..5fe09cea83f407d03836c79449c9abb5ad86bd1b 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 @@ def setup_merge_request_mail(merge_request_id, recipient_id) @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 ec886e993c34e222ff16cd39428a2b58470ce876..eade0fe278fab37477aed66b54cb63c5eacfceae 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -112,6 +112,8 @@ def mail_thread(model, headers = {}) 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 0000000000000000000000000000000000000000..c39655650224d98b3d164313710c482368b056ce --- /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 ab5a96209c7cd3ae8db1e5ab2303402b10aed05b..472b348a545fe399cad03d276cd46397a8808651 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 @@ def unsubscribed? 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 3eb8cfcca9be5d581eb1e289e71ed3039aff64fc..6835b14648b1aa3810c03886265c61a053215a54 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -11,11 +11,11 @@ def self.notifiable?(user, *args) 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 @@ def recipients @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 @@ def make_recipient(user, type) ) 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 @@ def custom_action 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 @@ def add_custom_notifications 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 @@ def project_watchers 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 @@ def add_labels_subscribers(labels: nil) 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 @@ def build! # 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 @@ def build! # 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 be3b4b2ba0759151c63064345a2a00643860df65..8c84ccfcc925405c19c68a6c681448142399a027 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -85,10 +85,11 @@ def reassigned_issue(issue, current_user, previous_assignees = []) 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 @@ def resolve_all_discussions(merge_request, current_user) 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 @@ def new_note(note) 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 @@ def issue_moved(issue, new_issue, current_user) 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 @@ def new_resource_email(target, method) 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 @@ def close_resource_email(target, current_user, method, skip_current_user: true) ) 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 @@ def reassign_resource_email(target, current_user, method) 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 @@ def reopen_resource_email(target, current_user, method, status) 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 40bf45cece7ac1de2314dbbf56dd419fad700a15..ab8b1271212fb7bf93a48aad898d8a0bb18d0098 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 b4ce02eead839bd2254193c6bab88c8cf4d99020..de48f548a1b8f91af7c0cfb5a7bccff546d11704 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 0000000000000000000000000000000000000000..83c28769217cd22a6b3e998dea9d55dc74a5402c --- /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 3e2e7d0f7b6f74b0de163e3e359fd6c250e7aafb..123b37abd19659bc6c50870b123706b5e8818137 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 cbc8c67da61b68bc309882bacf093e62921acf69..7a8e798e3c938473b3e8ec787fc407346c67cb90 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -71,6 +71,18 @@ def have_referable_subject(referable, reply: false) 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 @@ def have_referable_subject(referable, reply: false) 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 @@ def have_referable_subject(referable, reply: false) 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 @@ def have_referable_subject(referable, reply: false) 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 43e2643f709a654b90005fb307d9af8cd585618d..5c59455e3e156228956afe9e31dfa04cc423cf86 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 @@ def send_notifications(*new_mentions) 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 @@ def send_notifications(*new_mentions) 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 @@ def send_notifications(*new_mentions) 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 @@ def send_notifications(*new_mentions) 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 @@ def send_notifications(*new_mentions) 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 @@ def send_notifications(*new_mentions) 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 b39052923ddbab58397e1c65600e1b23c5d50be5..1fb8252459f1dd66ffa0756575fb05086974d378 100644 --- a/spec/support/email_helpers.rb +++ b/spec/support/email_helpers.rb @@ -30,4 +30,8 @@ def should_not_email_anyone 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 4e15ccc534b59382a5793d3d902a4f759602f172..baa8ddb59e58dbd5c3c18511c9edea8fa06014b8 100644 --- a/spec/workers/new_issue_worker_spec.rb +++ b/spec/workers/new_issue_worker_spec.rb @@ -44,8 +44,9 @@ 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 9e0cbde45b13dc29e1967fef02252da1ba42d944..c3f29a40d5887759c01175a0adb49a45bf0e276c 100644 --- a/spec/workers/new_merge_request_worker_spec.rb +++ b/spec/workers/new_merge_request_worker_spec.rb @@ -46,8 +46,10 @@ 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