From 59bfa0809822c3dd257748197223809922ab5f80 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 12 Aug 2016 22:54:32 +0100 Subject: [PATCH] Send notification emails when users are newly mentioned in issue edits --- app/mailers/emails/issues.rb | 5 +++ app/services/issuable_base_service.rb | 3 +- app/services/issues/update_service.rb | 7 +++- app/services/notification_service.rb | 24 +++++++++++++- .../new_mention_in_issue_email.html.haml | 9 +++++ .../new_mention_in_issue_email.text.erb | 7 ++++ spec/services/issues/update_service_spec.rb | 33 +++++++++++++++++++ spec/services/notification_service_spec.rb | 26 +++++++++++++++ 8 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 app/views/notify/new_mention_in_issue_email.html.haml create mode 100644 app/views/notify/new_mention_in_issue_email.text.erb diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 6f54c42146c..d64e48f774b 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -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) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 2d96efe1042..b0ea7c905f8 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -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 diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index c7d406cc331..a2111b3806b 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -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 diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index ab6e51209ee..73df572514f 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -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 @@ -177,7 +191,7 @@ class NotificationService # build notify method like 'note_commit_email' notify_method = "note_#{note.noteable_type.underscore}_email".to_sym - + recipients.each do |recipient| mailer.send(notify_method, recipient.id, note.id).deliver_later end @@ -471,6 +485,14 @@ 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) & 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) diff --git a/app/views/notify/new_mention_in_issue_email.html.haml b/app/views/notify/new_mention_in_issue_email.html.haml new file mode 100644 index 00000000000..f42b150c0d6 --- /dev/null +++ b/app/views/notify/new_mention_in_issue_email.html.haml @@ -0,0 +1,9 @@ +- 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} diff --git a/app/views/notify/new_mention_in_issue_email.text.erb b/app/views/notify/new_mention_in_issue_email.text.erb new file mode 100644 index 00000000000..457e94b4800 --- /dev/null +++ b/app/views/notify/new_mention_in_issue_email.text.erb @@ -0,0 +1,7 @@ +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 %> diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 088c3d48bf7..a5e375578ec 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -319,5 +319,38 @@ describe Issues::UpdateService, services: true do end end end + + context 'updated user mentions' do + let(:user4) { create(:user) } + before do + project.team << [user4, :developer] + end + + context "in title" do + before do + perform_enqueued_jobs { update_issue(title: user4.to_reference) } + end + + it "should email only the newly-mentioned user" do + should_not_email(user) + should_not_email(user2) + should_not_email(user3) + should_email(user4) + end + end + + context "in description" do + before do + perform_enqueued_jobs { update_issue(description: user4.to_reference) } + end + + it "should email only the newly-mentioned user" do + should_not_email(user) + should_not_email(user2) + should_not_email(user3) + should_email(user4) + end + end + end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 92b441c28ca..1935451ff12 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -399,6 +399,32 @@ describe NotificationService, services: true do end end + describe '#new_mentions_in_issue' do + def send_notifications(*new_mentions) + ActionMailer::Base.deliveries.clear + notification.new_mentions_in_issue(issue, new_mentions, @u_disabled) + end + + it "should not email anyone unless they are newly mentioned" do + send_notifications() + expect(ActionMailer::Base.deliveries).to eq [] + end + + it "should email new mentions with a watch level higher than participant" do + send_notifications(@u_watcher, @u_participant_mentioned) + + should_email(@u_watcher) + should_email(@u_participant_mentioned) + + expect(ActionMailer::Base.deliveries.count).to eq 2 + end + + it "should 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 eq [] + end + end + describe '#reassigned_issue' do before do update_custom_notification(:reassign_issue, @u_guest_custom, project)