Commit 0e5ac6f9 authored by Robert Speicher's avatar Robert Speicher Committed by Ruben Davila

Merge branch '2451-fix-mentions-in-issue-updates' into 'master'

Send notification emails when users are newly mentioned in issue or MR edits

## What does this MR do?

Introduces "new mention in issue" and "new mention in MR" email notifications.  Editing a Mentionable title or description and adding a mention to a user who was not previously mentioned will now send them a notification email, following usual permissions for doing so.

## Why was this MR needed?

Issues & MRs may be edited to include mentions to new people. We don't currently send out email notifications of these edits to anyone, although they do create TODOs. This brings email notifications into parity with TODOs.

## What are the relevant issue numbers?

Closes #2451

See merge request !5800
parent 863f77b6
...@@ -33,6 +33,7 @@ v 8.11.0 (unreleased) ...@@ -33,6 +33,7 @@ v 8.11.0 (unreleased)
- Fix awardable button mutuality loading spinners (ClemMakesApps) - Fix awardable button mutuality loading spinners (ClemMakesApps)
- Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable - Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
- Optimize maximum user access level lookup in loading of notes - Optimize maximum user access level lookup in loading of notes
- Send notification emails to users newly mentioned in issue and MR edits !5800
- Add "No one can push" as an option for protected branches. !5081 - Add "No one can push" as an option for protected branches. !5081
- Improve performance of AutolinkFilter#text_parse by using XPath - Improve performance of AutolinkFilter#text_parse by using XPath
- Add experimental Redis Sentinel support !1877 - Add experimental Redis Sentinel support !1877
......
...@@ -6,6 +6,11 @@ module Emails ...@@ -6,6 +6,11 @@ module Emails
mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id)) mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id))
end end
def new_mention_in_issue_email(recipient_id, issue_id, updated_by_user_id)
setup_issue_mail(issue_id, recipient_id)
mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
end
def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id) def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
setup_issue_mail(issue_id, recipient_id) setup_issue_mail(issue_id, recipient_id)
......
...@@ -6,6 +6,11 @@ module Emails ...@@ -6,6 +6,11 @@ module Emails
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))
end end
def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
setup_merge_request_mail(merge_request_id, recipient_id)
mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
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)
setup_merge_request_mail(merge_request_id, recipient_id) setup_merge_request_mail(merge_request_id, recipient_id)
......
...@@ -104,11 +104,12 @@ class IssuableBaseService < BaseService ...@@ -104,11 +104,12 @@ class IssuableBaseService < BaseService
change_subscription(issuable) change_subscription(issuable)
filter_params filter_params
old_labels = issuable.labels.to_a old_labels = issuable.labels.to_a
old_mentioned_users = issuable.mentioned_users.to_a
if params.present? && update_issuable(issuable, params) if params.present? && update_issuable(issuable, params)
issuable.reset_events_cache issuable.reset_events_cache
handle_common_system_notes(issuable, old_labels: old_labels) handle_common_system_notes(issuable, old_labels: old_labels)
handle_changes(issuable, old_labels: old_labels) handle_changes(issuable, old_labels: old_labels, old_mentioned_users: old_mentioned_users)
issuable.create_new_cross_references!(current_user) issuable.create_new_cross_references!(current_user)
execute_hooks(issuable, 'update') execute_hooks(issuable, 'update')
end end
......
...@@ -4,7 +4,7 @@ module Issues ...@@ -4,7 +4,7 @@ module Issues
update(issue) update(issue)
end end
def handle_changes(issue, old_labels: []) def handle_changes(issue, old_labels: [], old_mentioned_users: [])
if has_changes?(issue, old_labels: old_labels) if has_changes?(issue, old_labels: old_labels)
todo_service.mark_pending_todos_as_done(issue, current_user) todo_service.mark_pending_todos_as_done(issue, current_user)
end end
...@@ -32,6 +32,11 @@ module Issues ...@@ -32,6 +32,11 @@ module Issues
if added_labels.present? if added_labels.present?
notification_service.relabeled_issue(issue, added_labels, current_user) notification_service.relabeled_issue(issue, added_labels, current_user)
end end
added_mentions = issue.mentioned_users - old_mentioned_users
if added_mentions.present?
notification_service.new_mentions_in_issue(issue, added_mentions, current_user)
end
end end
def reopen_service def reopen_service
......
...@@ -16,7 +16,7 @@ module MergeRequests ...@@ -16,7 +16,7 @@ module MergeRequests
update(merge_request) update(merge_request)
end end
def handle_changes(merge_request, old_labels: []) def handle_changes(merge_request, old_labels: [], old_mentioned_users: [])
if has_changes?(merge_request, old_labels: old_labels) if has_changes?(merge_request, old_labels: old_labels)
todo_service.mark_pending_todos_as_done(merge_request, current_user) todo_service.mark_pending_todos_as_done(merge_request, current_user)
end end
...@@ -55,6 +55,15 @@ module MergeRequests ...@@ -55,6 +55,15 @@ module MergeRequests
current_user current_user
) )
end end
added_mentions = merge_request.mentioned_users - old_mentioned_users
if added_mentions.present?
notification_service.new_mentions_in_merge_request(
merge_request,
added_mentions,
current_user
)
end
end end
def reopen_service def reopen_service
......
...@@ -35,6 +35,20 @@ class NotificationService ...@@ -35,6 +35,20 @@ class NotificationService
new_resource_email(issue, issue.project, :new_issue_email) new_resource_email(issue, issue.project, :new_issue_email)
end end
# When issue text is updated, we should send an email to:
#
# * newly mentioned project team members with notification level higher than Participating
#
def new_mentions_in_issue(issue, new_mentioned_users, current_user)
new_mentions_in_resource_email(
issue,
issue.project,
new_mentioned_users,
current_user,
:new_mention_in_issue_email
)
end
# When we close an issue we should send an email to: # When we close an issue we should send an email to:
# #
# * issue author if their notification level is not Disabled # * issue author if their notification level is not Disabled
...@@ -75,6 +89,20 @@ class NotificationService ...@@ -75,6 +89,20 @@ class NotificationService
new_resource_email(merge_request, merge_request.target_project, :new_merge_request_email) new_resource_email(merge_request, merge_request.target_project, :new_merge_request_email)
end end
# When merge request text is updated, we should send an email to:
#
# * newly mentioned project team members with notification level higher than Participating
#
def new_mentions_in_merge_request(merge_request, new_mentioned_users, current_user)
new_mentions_in_resource_email(
merge_request,
merge_request.target_project,
new_mentioned_users,
current_user,
:new_mention_in_merge_request_email
)
end
# When we reassign a merge_request we should send an email to: # When we reassign a merge_request we should send an email to:
# #
# * merge_request old assignee if their notification level is not Disabled # * merge_request old assignee if their notification level is not Disabled
...@@ -177,7 +205,7 @@ class NotificationService ...@@ -177,7 +205,7 @@ class NotificationService
# build notify method like 'note_commit_email' # build notify method like 'note_commit_email'
notify_method = "note_#{note.noteable_type.underscore}_email".to_sym notify_method = "note_#{note.noteable_type.underscore}_email".to_sym
recipients.each do |recipient| recipients.each do |recipient|
mailer.send(notify_method, recipient.id, note.id).deliver_later mailer.send(notify_method, recipient.id, note.id).deliver_later
end end
...@@ -471,6 +499,15 @@ class NotificationService ...@@ -471,6 +499,15 @@ class NotificationService
end end
end end
def new_mentions_in_resource_email(target, project, new_mentioned_users, current_user, method)
recipients = build_recipients(target, project, current_user, action: "new")
recipients = recipients & new_mentioned_users
recipients.each do |recipient|
mailer.send(method, recipient.id, target.id, current_user.id).deliver_later
end
end
def close_resource_email(target, project, current_user, method) def close_resource_email(target, project, current_user, method)
action = method == :merged_merge_request_email ? "merge" : "close" action = method == :merged_merge_request_email ? "merge" : "close"
recipients = build_recipients(target, project, current_user, action: action) recipients = build_recipients(target, project, current_user, action: action)
......
%p
You have been mentioned in an issue.
- if current_application_settings.email_author_in_body
%div
#{link_to @issue.author_name, user_url(@issue.author)} wrote:
-if @issue.description
= markdown(@issue.description, pipeline: :email, author: @issue.author)
- if @issue.assignee_id.present?
%p
Assignee: #{@issue.assignee_name}
You have been mentioned in an issue.
Issue <%= @issue.iid %>: <%= url_for(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue)) %>
Author: <%= @issue.author_name %>
Assignee: <%= @issue.assignee_name %>
<%= @issue.description %>
%p
You have been mentioned in Merge Request #{@merge_request.to_reference}
- if current_application_settings.email_author_in_body
%div
#{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote:
%p.details
!= merge_path_description(@merge_request, '&rarr;')
- if @merge_request.assignee_id.present?
%p
Assignee: #{@merge_request.author_name} &rarr; #{@merge_request.assignee_name}
-if @merge_request.description
= markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)
You have been mentioned in Merge Request <%= @merge_request.to_reference %>
<%= url_for(namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request)) %>
<%= merge_path_description(@merge_request, 'to') %>
Author: <%= @merge_request.author_name %>
Assignee: <%= @merge_request.assignee_name %>
<%= @merge_request.description %>
...@@ -67,7 +67,7 @@ In all of the below cases, the notification will be sent to: ...@@ -67,7 +67,7 @@ In all of the below cases, the notification will be sent to:
- Participants: - Participants:
- the author and assignee of the issue/merge request - the author and assignee of the issue/merge request
- authors of comments on the issue/merge request - authors of comments on the issue/merge request
- anyone mentioned by `@username` in the issue/merge request description - anyone mentioned by `@username` in the issue/merge request title or description
- anyone mentioned by `@username` in any of the comments on the issue/merge request - anyone mentioned by `@username` in any of the comments on the issue/merge request
...with notification level "Participating" or higher ...with notification level "Participating" or higher
...@@ -89,6 +89,11 @@ In all of the below cases, the notification will be sent to: ...@@ -89,6 +89,11 @@ In all of the below cases, the notification will be sent to:
| Merge merge request | | | Merge merge request | |
| New comment | The above, plus anyone mentioned by `@username` in the comment, with notification level "Mention" or higher | | New comment | The above, plus anyone mentioned by `@username` in the comment, with notification level "Mention" or higher |
In addition, if the title or description of an Issue or Merge Request is
changed, notifications will be sent to any **new** mentions by `@username` as
if they had been mentioned in the original text.
You won't receive notifications for Issues, Merge Requests or Milestones You won't receive notifications for Issues, Merge Requests or Milestones
created by yourself. You will only receive automatic notifications when created by yourself. You will only receive automatic notifications when
somebody else comments or adds changes to the ones that you've created or somebody else comments or adds changes to the ones that you've created or
......
...@@ -319,5 +319,10 @@ describe Issues::UpdateService, services: true do ...@@ -319,5 +319,10 @@ describe Issues::UpdateService, services: true do
end end
end end
end end
context 'updating mentions' do
let(:mentionable) { issue }
include_examples 'updating mentions', Issues::UpdateService
end
end end
end end
...@@ -226,6 +226,11 @@ describe MergeRequests::UpdateService, services: true do ...@@ -226,6 +226,11 @@ describe MergeRequests::UpdateService, services: true do
end end
end end
context 'updating mentions' do
let(:mentionable) { merge_request }
include_examples 'updating mentions', MergeRequests::UpdateService
end
context 'when MergeRequest has tasks' do context 'when MergeRequest has tasks' do
before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) } before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
......
...@@ -9,6 +9,28 @@ describe NotificationService, services: true do ...@@ -9,6 +9,28 @@ describe NotificationService, services: true do
end end
end end
shared_examples 'notifications for new mentions' do
def send_notifications(*new_mentions)
reset_delivered_emails!
notification.send(notification_method, mentionable, new_mentions, @u_disabled)
end
it 'sends no emails when no new mentions are present' do
send_notifications
expect(ActionMailer::Base.deliveries).to be_empty
end
it 'emails new mentions with a watch level higher than participant' do
send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global)
should_only_email(@u_watcher, @u_participant_mentioned, @u_custom_global)
end
it 'does not email new mentions with a watch level equal to or less than participant' do
send_notifications(@u_participating, @u_mentioned)
expect(ActionMailer::Base.deliveries).to be_empty
end
end
describe 'Keys' do describe 'Keys' do
describe '#new_key' do describe '#new_key' do
let!(:key) { create(:personal_key) } let!(:key) { create(:personal_key) }
...@@ -399,6 +421,13 @@ describe NotificationService, services: true do ...@@ -399,6 +421,13 @@ describe NotificationService, services: true do
end end
end end
describe '#new_mentions_in_issue' do
let(:notification_method) { :new_mentions_in_issue }
let(:mentionable) { issue }
include_examples 'notifications for new mentions'
end
describe '#reassigned_issue' do describe '#reassigned_issue' do
before do before do
update_custom_notification(:reassign_issue, @u_guest_custom, project) update_custom_notification(:reassign_issue, @u_guest_custom, project)
...@@ -700,6 +729,8 @@ describe NotificationService, services: true do ...@@ -700,6 +729,8 @@ describe NotificationService, services: true do
before do before do
build_team(merge_request.target_project) build_team(merge_request.target_project)
add_users_with_subscription(merge_request.target_project, merge_request) add_users_with_subscription(merge_request.target_project, merge_request)
update_custom_notification(:new_merge_request, @u_guest_custom, project)
update_custom_notification(:new_merge_request, @u_custom_global)
ActionMailer::Base.deliveries.clear ActionMailer::Base.deliveries.clear
end end
...@@ -763,6 +794,13 @@ describe NotificationService, services: true do ...@@ -763,6 +794,13 @@ describe NotificationService, services: true do
end end
end end
describe '#new_mentions_in_merge_request' do
let(:notification_method) { :new_mentions_in_merge_request }
let(:mentionable) { merge_request }
include_examples 'notifications for new mentions'
end
describe '#reassigned_merge_request' do describe '#reassigned_merge_request' do
before do before do
update_custom_notification(:reassign_merge_request, @u_guest_custom, project) update_custom_notification(:reassign_merge_request, @u_guest_custom, project)
......
...@@ -3,6 +3,16 @@ module EmailHelpers ...@@ -3,6 +3,16 @@ module EmailHelpers
ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1 ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1
end end
def reset_delivered_emails!
ActionMailer::Base.deliveries.clear
end
def should_only_email(*users)
users.each {|user| should_email(user) }
recipients = ActionMailer::Base.deliveries.flat_map(&:to)
expect(recipients.count).to eq(users.count)
end
def should_email(user) def should_email(user)
expect(sent_to_user?(user)).to be_truthy expect(sent_to_user?(user)).to be_truthy
end end
......
RSpec.shared_examples 'updating mentions' do |service_class|
let(:mentioned_user) { create(:user) }
let(:service_class) { service_class }
before { project.team << [mentioned_user, :developer] }
def update_mentionable(opts)
reset_delivered_emails!
perform_enqueued_jobs do
service_class.new(project, user, opts).execute(mentionable)
end
mentionable.reload
end
context 'in title' do
before { update_mentionable(title: mentioned_user.to_reference) }
it 'emails only the newly-mentioned user' do
should_only_email(mentioned_user)
end
end
context 'in description' do
before { update_mentionable(description: mentioned_user.to_reference) }
it 'emails only the newly-mentioned user' do
should_only_email(mentioned_user)
end
end
end
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