Merge branch 'ccr/51520_change_milestone_email' into 'master'

Add email for milestone change

Closes #51520

See merge request gitlab-org/gitlab-ce!22279

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)
 

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)
 

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

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}"
 

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

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,

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)

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")
 

app/views/notify/changed_milestone_issue_email.html.haml

+%p
+  Milestone changed to
+  %strong= link_to(@milestone.name, @milestone_url)

app/views/notify/changed_milestone_issue_email.text.erb

+Milestone changed to <%= @milestone.name %> ( <%= @milestone_url %> )

app/views/notify/changed_milestone_merge_request_email.html.haml

+%p
+  Milestone changed to
+  %strong= link_to(@milestone.name, @milestone_url)

app/views/notify/changed_milestone_merge_request_email.text.erb

+Milestone changed to <%= @milestone.name %> ( <%= @milestone_url %> )

app/views/notify/removed_milestone_issue_email.html.haml

+%p
+  Milestone removed

app/views/notify/removed_milestone_issue_email.text.erb

+Milestone removed

app/views/notify/removed_milestone_merge_request_email.html.haml

+%p
+  Milestone removed

app/views/notify/removed_milestone_merge_request_email.text.erb

+Milestone removed

changelogs/unreleased/ccr-51520_change_milestone_email.yml

+---
+title: Add email for milestone change
+merge_request: 22279
+author:
+type: added

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 |

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

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

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)