diff options
author | Robert Speicher <robert@gitlab.com> | 2018-11-02 16:29:32 +0000 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2018-11-02 16:29:32 +0000 |
commit | 6f12e3929df987e49427a1355cdf35798f97da65 (patch) | |
tree | ba65cb3e6a04ddd97659782a9e83a5b6749cf4b8 | |
parent | 45bc509352cf8d20fb15d6c638d6fe0335c20b62 (diff) | |
parent | bb6b5653e28cf4f1b7264d318238797b08c74df3 (diff) | |
download | gitlab-ce-6f12e3929df987e49427a1355cdf35798f97da65.tar.gz |
Merge branch 'ccr/51520_change_milestone_email' into 'master'11-5-stable-prepare-rc1
Add email for milestone change
Closes #51520
See merge request gitlab-org/gitlab-ce!22279
21 files changed, 399 insertions, 5 deletions
diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 602e5afe26b..93b51fb1774 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -45,6 +45,20 @@ module Emails mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) end + def removed_milestone_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, reason)) + end + + def changed_milestone_issue_email(recipient_id, issue_id, milestone, updated_by_user_id, reason = nil) + setup_issue_mail(issue_id, recipient_id) + + @milestone = milestone + @milestone_url = milestone_url(@milestone) + 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, reason = nil) setup_issue_mail(issue_id, recipient_id) diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index be085496731..6524d0c2087 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -40,6 +40,20 @@ module Emails mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end + def removed_milestone_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, reason)) + end + + def changed_milestone_merge_request_email(recipient_id, merge_request_id, milestone, updated_by_user_id, reason = nil) + setup_merge_request_mail(merge_request_id, recipient_id) + + @milestone = milestone + @milestone_url = milestone_url(@milestone) + 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, reason = nil) setup_merge_request_mail(merge_request_id, recipient_id) diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index 2f5b5483e9d..e7e8d96eca4 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -68,6 +68,14 @@ class NotifyPreview < ActionMailer::Preview Notify.issue_status_changed_email(user.id, issue.id, 'closed', user.id).message end + def removed_milestone_issue_email + Notify.removed_milestone_issue_email(user.id, issue.id, user.id) + end + + def changed_milestone_issue_email + Notify.changed_milestone_issue_email(user.id, issue.id, milestone, user.id) + end + def closed_merge_request_email Notify.closed_merge_request_email(user.id, issue.id, user.id).message end @@ -80,6 +88,14 @@ class NotifyPreview < ActionMailer::Preview Notify.merged_merge_request_email(user.id, merge_request.id, user.id).message end + def removed_milestone_merge_request_email + Notify.removed_milestone_merge_request_email(user.id, merge_request.id, user.id) + end + + def changed_milestone_merge_request_email + Notify.changed_milestone_merge_request_email(user.id, merge_request.id, milestone, user.id) + end + def member_access_denied_email Notify.member_access_denied_email('project', project.id, user.id).message end @@ -143,6 +159,10 @@ class NotifyPreview < ActionMailer::Preview @merge_request ||= project.merge_requests.first end + def milestone + @milestone ||= issue.milestone + end + def pipeline @pipeline = Ci::Pipeline.last end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 3e8b9f84042..c388913ae65 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -3,6 +3,14 @@ class IssuableBaseService < BaseService private + attr_accessor :params, :skip_milestone_email + + def initialize(project, user = nil, params = {}) + super + + @skip_milestone_email = @params.delete(:skip_milestone_email) + end + def filter_params(issuable) ability_name = :"admin_#{issuable.to_ability_name}" diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index b54b0bf6ef6..fba252b0bae 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -48,6 +48,8 @@ module Issues notification_service.async.relabeled_issue(issue, added_labels, current_user) end + handle_milestone_change(issue) + added_mentions = issue.mentioned_users - old_mentioned_users if added_mentions.present? @@ -91,6 +93,18 @@ module Issues private + def handle_milestone_change(issue) + return if skip_milestone_email + + return unless issue.previous_changes.include?('milestone_id') + + if issue.milestone.nil? + notification_service.async.removed_milestone_issue(issue, current_user) + else + notification_service.async.changed_milestone_issue(issue, issue.milestone, current_user) + end + end + # rubocop: disable CodeReuse/ActiveRecord def get_issue_if_allowed(id, board_group_id = nil) return unless id diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index b112edbce7f..aacaf10d09c 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -58,6 +58,8 @@ module MergeRequests merge_request.mark_as_unchecked end + handle_milestone_change(merge_request) + added_labels = merge_request.labels - old_labels if added_labels.present? notification_service.async.relabeled_merge_request( @@ -105,6 +107,18 @@ module MergeRequests private + def handle_milestone_change(merge_request) + return if skip_milestone_email + + return unless merge_request.previous_changes.include?('milestone_id') + + if merge_request.milestone.nil? + notification_service.async.removed_milestone_merge_request(merge_request, current_user) + else + notification_service.async.changed_milestone_merge_request(merge_request, merge_request.milestone, current_user) + end + end + def create_branch_change_note(issuable, branch_type, old_branch, new_branch) SystemNoteService.change_branch( issuable, issuable.project, current_user, branch_type, diff --git a/app/services/milestones/destroy_service.rb b/app/services/milestones/destroy_service.rb index 7cda802c120..87c7a282081 100644 --- a/app/services/milestones/destroy_service.rb +++ b/app/services/milestones/destroy_service.rb @@ -4,7 +4,7 @@ module Milestones class DestroyService < Milestones::BaseService def execute(milestone) Milestone.transaction do - update_params = { milestone: nil } + update_params = { milestone: nil, skip_milestone_email: true } milestone.issues.each do |issue| Issues::UpdateService.new(parent, current_user, update_params).execute(issue) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 50fa373025b..fb9c18ea75d 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -129,6 +129,14 @@ class NotificationService relabeled_resource_email(issue, added_labels, current_user, :relabeled_issue_email) end + def removed_milestone_issue(issue, current_user) + removed_milestone_resource_email(issue, current_user, :removed_milestone_issue_email) + end + + def changed_milestone_issue(issue, new_milestone, current_user) + changed_milestone_resource_email(issue, new_milestone, current_user, :changed_milestone_issue_email) + end + # When create a merge request we should send an email to: # # * mr author @@ -138,7 +146,6 @@ class NotificationService # * users with custom level checked with "new merge request" # # In EE, approvers of the merge request are also included - # def new_merge_request(merge_request, current_user) new_resource_email(merge_request, :new_merge_request_email) end @@ -208,6 +215,14 @@ class NotificationService relabeled_resource_email(merge_request, added_labels, current_user, :relabeled_merge_request_email) end + def removed_milestone_merge_request(merge_request, current_user) + removed_milestone_resource_email(merge_request, current_user, :removed_milestone_merge_request_email) + end + + def changed_milestone_merge_request(merge_request, new_milestone, current_user) + changed_milestone_resource_email(merge_request, new_milestone, current_user, :changed_milestone_merge_request_email) + end + def close_mr(merge_request, current_user) close_resource_email(merge_request, current_user, :closed_merge_request_email) end @@ -500,6 +515,30 @@ class NotificationService end end + def removed_milestone_resource_email(target, current_user, method) + recipients = NotificationRecipientService.build_recipients( + target, + current_user, + action: 'removed_milestone' + ) + + recipients.each do |recipient| + mailer.send(method, recipient.user.id, target.id, current_user.id).deliver_later + end + end + + def changed_milestone_resource_email(target, milestone, current_user, method) + recipients = NotificationRecipientService.build_recipients( + target, + current_user, + action: 'changed_milestone' + ) + + recipients.each do |recipient| + mailer.send(method, recipient.user.id, target.id, milestone, current_user.id).deliver_later + end + end + def reopen_resource_email(target, current_user, method, status) recipients = NotificationRecipientService.build_recipients(target, current_user, action: "reopen") diff --git a/app/views/notify/changed_milestone_issue_email.html.haml b/app/views/notify/changed_milestone_issue_email.html.haml new file mode 100644 index 00000000000..7d5425fc72d --- /dev/null +++ b/app/views/notify/changed_milestone_issue_email.html.haml @@ -0,0 +1,3 @@ +%p + Milestone changed to + %strong= link_to(@milestone.name, @milestone_url) diff --git a/app/views/notify/changed_milestone_issue_email.text.erb b/app/views/notify/changed_milestone_issue_email.text.erb new file mode 100644 index 00000000000..c5fc0b61518 --- /dev/null +++ b/app/views/notify/changed_milestone_issue_email.text.erb @@ -0,0 +1 @@ +Milestone changed to <%= @milestone.name %> ( <%= @milestone_url %> ) diff --git a/app/views/notify/changed_milestone_merge_request_email.html.haml b/app/views/notify/changed_milestone_merge_request_email.html.haml new file mode 100644 index 00000000000..7d5425fc72d --- /dev/null +++ b/app/views/notify/changed_milestone_merge_request_email.html.haml @@ -0,0 +1,3 @@ +%p + Milestone changed to + %strong= link_to(@milestone.name, @milestone_url) diff --git a/app/views/notify/changed_milestone_merge_request_email.text.erb b/app/views/notify/changed_milestone_merge_request_email.text.erb new file mode 100644 index 00000000000..c5fc0b61518 --- /dev/null +++ b/app/views/notify/changed_milestone_merge_request_email.text.erb @@ -0,0 +1 @@ +Milestone changed to <%= @milestone.name %> ( <%= @milestone_url %> ) diff --git a/app/views/notify/removed_milestone_issue_email.html.haml b/app/views/notify/removed_milestone_issue_email.html.haml new file mode 100644 index 00000000000..7e9205b6491 --- /dev/null +++ b/app/views/notify/removed_milestone_issue_email.html.haml @@ -0,0 +1,2 @@ +%p + Milestone removed diff --git a/app/views/notify/removed_milestone_issue_email.text.erb b/app/views/notify/removed_milestone_issue_email.text.erb new file mode 100644 index 00000000000..0b83ed7a4c5 --- /dev/null +++ b/app/views/notify/removed_milestone_issue_email.text.erb @@ -0,0 +1 @@ +Milestone removed diff --git a/app/views/notify/removed_milestone_merge_request_email.html.haml b/app/views/notify/removed_milestone_merge_request_email.html.haml new file mode 100644 index 00000000000..7e9205b6491 --- /dev/null +++ b/app/views/notify/removed_milestone_merge_request_email.html.haml @@ -0,0 +1,2 @@ +%p + Milestone removed diff --git a/app/views/notify/removed_milestone_merge_request_email.text.erb b/app/views/notify/removed_milestone_merge_request_email.text.erb new file mode 100644 index 00000000000..0b83ed7a4c5 --- /dev/null +++ b/app/views/notify/removed_milestone_merge_request_email.text.erb @@ -0,0 +1 @@ +Milestone removed diff --git a/changelogs/unreleased/ccr-51520_change_milestone_email.yml b/changelogs/unreleased/ccr-51520_change_milestone_email.yml new file mode 100644 index 00000000000..ce4beba2c5f --- /dev/null +++ b/changelogs/unreleased/ccr-51520_change_milestone_email.yml @@ -0,0 +1,5 @@ +--- +title: Add email for milestone change +merge_request: 22279 +author: +type: added diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index 9e41038e02e..c590ac4b0ba 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -92,12 +92,16 @@ In most of the below cases, the notification will be sent to: | Reassign issue | The above, plus the old assignee | | Reopen issue | | | Due issue | Participants and Custom notification level with this event selected | +| Change milestone issue | Subscribers, participants mentioned, and Custom notification level with this event selected | +| Remove milestone issue | Subscribers, participants mentioned, and Custom notification level with this event selected | | New merge request | | | Push to merge request | Participants and Custom notification level with this event selected | | Reassign merge request | The above, plus the old assignee | | Close merge request | | | Reopen merge request | | | Merge merge request | | +| Change milestone merge request | Subscribers, participants mentioned, and Custom notification level with this event selected | +| Remove milestone merge request | Subscribers, participants mentioned, and Custom notification level with this event selected | | New comment | The above, plus anyone mentioned by `@username` in the comment, with notification level "Mention" or higher | | Failed pipeline | The author of the pipeline | | Successful pipeline | The author of the pipeline, if they have the custom notification setting for successful pipelines set | diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 07aa8449a66..bd519e7f077 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -343,7 +343,42 @@ describe Issues::UpdateService, :mailer do end end - context 'when the milestone change' do + context 'when the milestone is removed' do + let!(:non_subscriber) { create(:user) } + + let!(:subscriber) do + create(:user) do |u| + issue.toggle_subscription(u, project) + project.add_developer(u) + end + end + + it_behaves_like 'system notes for milestones' + + it 'sends notifications for subscribers of changed milestone' do + issue.milestone = create(:milestone) + + issue.save + + perform_enqueued_jobs do + update_issue(milestone_id: "") + end + + should_email(subscriber) + should_not_email(non_subscriber) + end + end + + context 'when the milestone is changed' do + let!(:non_subscriber) { create(:user) } + + let!(:subscriber) do + create(:user) do |u| + issue.toggle_subscription(u, project) + project.add_developer(u) + end + end + it 'marks todos as done' do update_issue(milestone: create(:milestone)) @@ -351,6 +386,15 @@ describe Issues::UpdateService, :mailer do end it_behaves_like 'system notes for milestones' + + it 'sends notifications for subscribers of changed milestone' do + perform_enqueued_jobs do + update_issue(milestone: create(:milestone)) + end + + should_email(subscriber) + should_not_email(non_subscriber) + end end context 'when the labels change' do @@ -374,7 +418,7 @@ describe Issues::UpdateService, :mailer do let!(:non_subscriber) { create(:user) } let!(:subscriber) do - create(:user).tap do |u| + create(:user) do |u| label.toggle_subscription(u, project) project.add_developer(u) end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 55dfab81c26..1b599ba11b6 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -315,7 +315,42 @@ describe MergeRequests::UpdateService, :mailer do end end - context 'when the milestone change' do + context 'when the milestone is removed' do + let!(:non_subscriber) { create(:user) } + + let!(:subscriber) do + create(:user) do |u| + merge_request.toggle_subscription(u, project) + project.add_developer(u) + end + end + + it_behaves_like 'system notes for milestones' + + it 'sends notifications for subscribers of changed milestone' do + merge_request.milestone = create(:milestone) + + merge_request.save + + perform_enqueued_jobs do + update_merge_request(milestone_id: "") + end + + should_email(subscriber) + should_not_email(non_subscriber) + end + end + + context 'when the milestone is changed' do + let!(:non_subscriber) { create(:user) } + + let!(:subscriber) do + create(:user) do |u| + merge_request.toggle_subscription(u, project) + project.add_developer(u) + end + end + it 'marks pending todos as done' do update_merge_request({ milestone: create(:milestone) }) @@ -323,6 +358,15 @@ describe MergeRequests::UpdateService, :mailer do end it_behaves_like 'system notes for milestones' + + it 'sends notifications for subscribers of changed milestone' do + perform_enqueued_jobs do + update_merge_request(milestone: create(:milestone)) + end + + should_email(subscriber) + should_not_email(non_subscriber) + end end context 'when the labels change' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 68a361fa882..2d8da7673dc 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -13,6 +13,54 @@ describe NotificationService, :mailer do end end + shared_examples 'altered milestone notification on issue' do + it 'sends the email to the correct people' do + should_email(subscriber_to_new_milestone) + issue.assignees.each do |a| + should_email(a) + end + should_email(@u_watcher) + should_email(@u_guest_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_email(@subscribed_participant) + should_email(@watcher_and_subscriber) + should_not_email(@u_guest_custom) + should_not_email(@u_committer) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_lazy_participant) + should_not_email(issue.author) + should_not_email(@u_disabled) + should_not_email(@u_custom_global) + should_not_email(@u_mentioned) + end + end + + shared_examples 'altered milestone notification on merge request' do + it 'sends the email to the correct people' do + should_email(subscriber_to_new_milestone) + merge_request.assignees.each do |a| + should_email(a) + end + should_email(@u_watcher) + should_email(@u_guest_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_email(@subscribed_participant) + should_email(@watcher_and_subscriber) + should_not_email(@u_guest_custom) + should_not_email(@u_committer) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_lazy_participant) + should_not_email(merge_request.author) + should_not_email(@u_disabled) + should_not_email(@u_custom_global) + should_not_email(@u_mentioned) + end + end + shared_examples 'notifications for new mentions' do it 'sends no emails when no new mentions are present' do send_notifications @@ -952,6 +1000,96 @@ describe NotificationService, :mailer do end end + describe '#removed_milestone_issue' do + it_behaves_like 'altered milestone notification on issue' do + let(:milestone) { create(:milestone, project: project, issues: [issue]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| issue.toggle_subscription(u, project) } } + + before do + notification.removed_milestone_issue(issue, issue.author) + end + end + + context 'confidential issues' do + let(:author) { create(:user) } + let(:assignee) { create(:user) } + let(:non_member) { create(:user) } + let(:member) { create(:user) } + let(:guest) { create(:user) } + let(:admin) { create(:admin) } + let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignees: [assignee]) } + let(:milestone) { create(:milestone, project: project, issues: [confidential_issue]) } + + it "emails subscribers of the issue's milestone that can read the issue" do + project.add_developer(member) + project.add_guest(guest) + + confidential_issue.subscribe(non_member, project) + confidential_issue.subscribe(author, project) + confidential_issue.subscribe(assignee, project) + confidential_issue.subscribe(member, project) + confidential_issue.subscribe(guest, project) + confidential_issue.subscribe(admin, project) + + reset_delivered_emails! + + notification.removed_milestone_issue(confidential_issue, @u_disabled) + + should_not_email(non_member) + should_not_email(guest) + should_email(author) + should_email(assignee) + should_email(member) + should_email(admin) + end + end + end + + describe '#changed_milestone_issue' do + it_behaves_like 'altered milestone notification on issue' do + let(:new_milestone) { create(:milestone, project: project, issues: [issue]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| issue.toggle_subscription(u, project) } } + + before do + notification.changed_milestone_issue(issue, new_milestone, issue.author) + end + end + + context 'confidential issues' do + let(:author) { create(:user) } + let(:assignee) { create(:user) } + let(:non_member) { create(:user) } + let(:member) { create(:user) } + let(:guest) { create(:user) } + let(:admin) { create(:admin) } + let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignees: [assignee]) } + let(:new_milestone) { create(:milestone, project: project, issues: [confidential_issue]) } + + it "emails subscribers of the issue's milestone that can read the issue" do + project.add_developer(member) + project.add_guest(guest) + + confidential_issue.subscribe(non_member, project) + confidential_issue.subscribe(author, project) + confidential_issue.subscribe(assignee, project) + confidential_issue.subscribe(member, project) + confidential_issue.subscribe(guest, project) + confidential_issue.subscribe(admin, project) + + reset_delivered_emails! + + notification.changed_milestone_issue(confidential_issue, new_milestone, @u_disabled) + + should_not_email(non_member) + should_not_email(guest) + should_email(author) + should_email(assignee) + should_email(member) + should_email(admin) + end + end + end + describe '#close_issue' do before do update_custom_notification(:close_issue, @u_guest_custom, resource: project) @@ -1304,6 +1442,28 @@ describe NotificationService, :mailer do end end + describe '#removed_milestone_merge_request' do + it_behaves_like 'altered milestone notification on merge request' do + let(:milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + + before do + notification.removed_milestone_merge_request(merge_request, merge_request.author) + end + end + end + + describe '#changed_milestone_merge_request' do + it_behaves_like 'altered milestone notification on merge request' do + let(:new_milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + + before do + notification.changed_milestone_merge_request(merge_request, new_milestone, merge_request.author) + end + end + end + describe '#merge_request_unmergeable' do it "sends email to merge request author" do notification.merge_request_unmergeable(merge_request) |