Commit 3666f698 authored by Robert Speicher's avatar Robert Speicher

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
parents 7e6a1777 2797cfbf
......@@ -34,6 +34,7 @@ v 8.11.0 (unreleased)
- Fix awardable button mutuality loading spinners (ClemMakesApps)
- Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
- 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
- Improve performance of AutolinkFilter#text_parse by using XPath
- Add experimental Redis Sentinel support !1877
......
......@@ -6,6 +6,11 @@ module Emails
mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id))
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)
setup_issue_mail(issue_id, recipient_id)
......
......@@ -6,6 +6,11 @@ module Emails
mail_new_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id))
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)
setup_merge_request_mail(merge_request_id, recipient_id)
......
......@@ -104,11 +104,12 @@ class IssuableBaseService < BaseService
change_subscription(issuable)
filter_params
old_labels = issuable.labels.to_a
old_mentioned_users = issuable.mentioned_users.to_a
if params.present? && update_issuable(issuable, params)
issuable.reset_events_cache
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)
execute_hooks(issuable, 'update')
end
......
......@@ -4,7 +4,7 @@ module Issues
update(issue)
end
def handle_changes(issue, old_labels: [])
def handle_changes(issue, old_labels: [], old_mentioned_users: [])
if has_changes?(issue, old_labels: old_labels)
todo_service.mark_pending_todos_as_done(issue, current_user)
end
......@@ -32,6 +32,11 @@ module Issues
if added_labels.present?
notification_service.relabeled_issue(issue, added_labels, current_user)
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
def reopen_service
......
......@@ -16,7 +16,7 @@ module MergeRequests
update(merge_request)
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)
todo_service.mark_pending_todos_as_done(merge_request, current_user)
end
......@@ -55,6 +55,15 @@ module MergeRequests
current_user
)
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
def reopen_service
......
......@@ -35,6 +35,20 @@ class NotificationService
new_resource_email(issue, issue.project, :new_issue_email)
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:
#
# * issue author if their notification level is not Disabled
......@@ -75,6 +89,20 @@ class NotificationService
new_resource_email(merge_request, merge_request.target_project, :new_merge_request_email)
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:
#
# * merge_request old assignee if their notification level is not Disabled
......@@ -471,6 +499,15 @@ class NotificationService
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)
action = method == :merged_merge_request_email ? "merge" : "close"
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:
- Participants:
- the author and assignee of 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
...with notification level "Participating" or higher
......@@ -89,6 +89,11 @@ In all of the below cases, the notification will be sent to:
| Merge merge request | |
| 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
created by yourself. You will only receive automatic notifications when
somebody else comments or adds changes to the ones that you've created or
......
......@@ -319,5 +319,10 @@ describe Issues::UpdateService, services: true do
end
end
end
context 'updating mentions' do
let(:mentionable) { issue }
include_examples 'updating mentions', Issues::UpdateService
end
end
end
......@@ -226,6 +226,11 @@ describe MergeRequests::UpdateService, services: true do
end
end
context 'updating mentions' do
let(:mentionable) { merge_request }
include_examples 'updating mentions', MergeRequests::UpdateService
end
context 'when MergeRequest has tasks' do
before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
......
......@@ -9,6 +9,28 @@ describe NotificationService, services: true do
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 '#new_key' do
let!(:key) { create(:personal_key) }
......@@ -399,6 +421,13 @@ describe NotificationService, services: true do
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
before do
update_custom_notification(:reassign_issue, @u_guest_custom, project)
......@@ -700,6 +729,8 @@ describe NotificationService, services: true do
before do
build_team(merge_request.target_project)
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
end
......@@ -763,6 +794,13 @@ describe NotificationService, services: true do
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
before do
update_custom_notification(:reassign_merge_request, @u_guest_custom, project)
......
......@@ -3,6 +3,16 @@ module EmailHelpers
ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1
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)
expect(sent_to_user?(user)).to be_truthy
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