Merge branch '218003-iteration_changes_emails' into 'master'

Send email notifications on Issue Iteration change

See merge request gitlab-org/gitlab!32817

app/helpers/milestones_helper.rb → app/helpers/timeboxes_helper.rb

 # frozen_string_literal: true
 
-module MilestonesHelper
+module TimeboxesHelper
   include EntityDateHelper
   include Gitlab::Utils::StrongMemoize
 
@@ -209,23 +209,24 @@ module MilestonesHelper
     end
   end
 
-  def milestone_date_range(milestone)
-    if milestone.start_date && milestone.due_date
-      "#{milestone.start_date.to_s(:medium)}–#{milestone.due_date.to_s(:medium)}"
-    elsif milestone.due_date
-      if milestone.due_date.past?
-        _("expired on %{milestone_due_date}") % { milestone_due_date: milestone.due_date.strftime('%b %-d, %Y') }
+  def timebox_date_range(timebox)
+    if timebox.start_date && timebox.due_date
+      "#{timebox.start_date.to_s(:medium)}–#{timebox.due_date.to_s(:medium)}"
+    elsif timebox.due_date
+      if timebox.due_date.past?
+        _("expired on %{timebox_due_date}") % { timebox_due_date: timebox.due_date.to_s(:medium) }
       else
-        _("expires on %{milestone_due_date}") % { milestone_due_date: milestone.due_date.strftime('%b %-d, %Y') }
+        _("expires on %{timebox_due_date}") % { timebox_due_date: timebox.due_date.to_s(:medium) }
       end
-    elsif milestone.start_date
-      if milestone.start_date.past?
-        _("started on %{milestone_start_date}") % { milestone_start_date: milestone.start_date.strftime('%b %-d, %Y') }
+    elsif timebox.start_date
+      if timebox.start_date.past?
+        _("started on %{timebox_start_date}") % { timebox_start_date: timebox.start_date.to_s(:medium) }
       else
-        _("starts on %{milestone_start_date}") % { milestone_start_date: milestone.start_date.strftime('%b %-d, %Y') }
+        _("starts on %{timebox_start_date}") % { timebox_start_date: timebox.start_date.to_s(:medium) }
       end
     end
   end
+  alias_method :milestone_date_range, :timebox_date_range
 
   def milestone_tab_path(milestone, tab)
     if milestone.global_milestone?
@@ -306,4 +307,4 @@ module MilestonesHelper
   end
 end
 
-MilestonesHelper.prepend_if_ee('EE::MilestonesHelper')
+TimeboxesHelper.prepend_if_ee('EE::TimeboxesHelper')

app/mailers/emails/issues.rb

@@ -126,3 +126,5 @@ module Emails
     end
   end
 end
+
+Emails::Issues.prepend_if_ee('EE::Emails::Issues')

app/mailers/notify.rb

@@ -19,7 +19,7 @@ class Notify < ApplicationMailer
   include Emails::Releases
   include Emails::Groups
 
-  helper MilestonesHelper
+  helper TimeboxesHelper
   helper MergeRequestsHelper
   helper DiffHelper
   helper BlobHelper

ee/app/helpers/ee/milestones_helper.rb → ee/app/helpers/ee/timeboxes_helper.rb

 # frozen_string_literal: true
 
 module EE
-  module MilestonesHelper
+  module TimeboxesHelper
     def burndown_chart(milestone)
       if milestone.supports_burndown_charts?
         issues = milestone.issues_visible_to_user(current_user)

ee/app/mailers/ee/emails/issues.rb

+# frozen_string_literal: true
+
+module EE
+  module Emails
+    module Issues
+      def removed_iteration_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_iteration_issue_email(recipient_id, issue_id, iteration, updated_by_user_id, reason = nil)
+        setup_issue_mail(issue_id, recipient_id)
+
+        @iteration = iteration
+        @iteration_url = iteration_url(@iteration)
+        mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason).merge({
+          template_name: 'changed_iteration_email'
+        }))
+      end
+    end
+  end
+end

ee/app/services/ee/issues/update_service.rb

@@ -21,8 +21,29 @@ module EE
         result
       end
 
+      override :handle_changes
+      def handle_changes(issue, _options)
+        super
+
+        handle_iteration_change(issue)
+      end
+
       private
 
+      def handle_iteration_change(issue)
+        return unless issue.previous_changes.include?('sprint_id')
+
+        send_iteration_change_notification(issue)
+      end
+
+      def send_iteration_change_notification(issue)
+        if issue.iteration.nil?
+          notification_service.async.removed_iteration_issue(issue, current_user)
+        else
+          notification_service.async.changed_iteration_issue(issue, issue.iteration, current_user)
+        end
+      end
+
       def handle_promotion(issue)
         return unless params.delete(:promote_to_epic)
 

ee/app/services/ee/notification_service.rb

@@ -61,6 +61,14 @@ module EE
       mailer.project_mirror_user_changed_email(new_mirror_user.id, deleted_user_name, project.id).deliver_later
     end
 
+    def removed_iteration_issue(issue, current_user)
+      removed_iteration_resource_email(issue, current_user)
+    end
+
+    def changed_iteration_issue(issue, new_iteration, current_user)
+      changed_iteration_resource_email(issue, new_iteration, current_user)
+    end
+
     private
 
     def send_new_review_notification(review)
@@ -108,6 +116,30 @@ module EE
       mailer.service_desk_new_note_email(issue.id, note.id).deliver_later
     end
 
+    def removed_iteration_resource_email(target, current_user)
+      recipients = ::NotificationRecipients::BuildService.build_recipients(
+        target,
+        current_user,
+        action: 'removed_iteration'
+      )
+
+      recipients.each do |recipient|
+        mailer.removed_iteration_issue_email(recipient.user.id, target.id, current_user.id).deliver_later
+      end
+    end
+
+    def changed_iteration_resource_email(target, iteration, current_user)
+      recipients = ::NotificationRecipients::BuildService.build_recipients(
+        target,
+        current_user,
+        action: 'changed_iteration'
+      )
+
+      recipients.each do |recipient|
+        mailer.changed_iteration_issue_email(recipient.user.id, target.id, iteration, current_user.id).deliver_later
+      end
+    end
+
     def epic_status_change_email(target, current_user, status)
       action = status == 'reopened' ? 'reopen' : 'close'
 

ee/app/views/notify/changed_iteration_email.html.haml

+%p
+  = _('Iteration changed to')
+  %strong= link_to(@iteration.name, @iteration_url)
+  - if date_range = timebox_date_range(@iteration)
+    = "(#{date_range})"

ee/app/views/notify/changed_iteration_email.text.erb

+<%= _('Iteration changed to') %> <%= @iteration.name %><% if date_range = timebox_date_range(@iteration) %> (<%= date_range %>)<% end %> ( <%= @iteration_url %> )

ee/app/views/notify/removed_iteration_issue_email.html.haml

+%p
+  = _('Iteration removed')

ee/app/views/notify/removed_iteration_issue_email.text.erb

+<%= _('Iteration removed') %>

ee/changelogs/unreleased/218003-iteration_changes_emails.yml

+---
+title: Send email notifications on Issue Iteration change
+merge_request: 32817
+author:
+type: added

ee/spec/mailers/ee/emails/issues_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'email_spec'
+
+describe Emails::Issues do
+  include EmailSpec::Matchers
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, namespace: group) }
+  let_it_be(:issue) { create(:issue, project: project) }
+
+  context 'iterations' do
+    let_it_be(:iteration) { create(:iteration, group: group) }
+
+    describe '#changed_iteration_issue_email' do
+      subject { Notify.changed_iteration_issue_email(user.id, issue.id, iteration, user.id) }
+
+      it 'shows the iteration it was changed to' do
+        expect(subject).to have_body_text 'Iteration changed to'
+        expect(subject).to have_body_text iteration.name
+      end
+    end
+
+    describe '#removed_iteration_issue_email' do
+      subject { Notify.removed_iteration_issue_email(user.id, issue.id, user.id) }
+
+      it 'says iteration was removed' do
+        expect(subject).to have_body_text 'Iteration removed'
+      end
+    end
+  end
+end

ee/spec/services/ee/issues/update_service_spec.rb

@@ -27,6 +27,48 @@ describe Issues::UpdateService do
         end
       end
 
+      context 'updating iteration' do
+        let(:iteration) { create(:iteration, group: group) }
+
+        context 'when issue does not already have an iteration' do
+          it 'calls NotificationService#changed_iteration_issue' do
+            expect_next_instance_of(NotificationService::Async) do |ns|
+              expect(ns).to receive(:changed_iteration_issue)
+            end
+
+            update_issue(iteration: iteration)
+          end
+        end
+
+        context 'when issue already has an iteration' do
+          let(:old_iteration) { create(:iteration, group: group) }
+
+          before do
+            update_issue(iteration: old_iteration)
+          end
+
+          context 'setting to nil' do
+            it 'calls NotificationService#removed_iteration_issue' do
+              expect_next_instance_of(NotificationService::Async) do |ns|
+                expect(ns).to receive(:removed_iteration_issue)
+              end
+
+              update_issue(iteration: nil)
+            end
+          end
+
+          context 'setting to another iteration' do
+            it 'calls NotificationService#changed_iteration_issue' do
+              expect_next_instance_of(NotificationService::Async) do |ns|
+                expect(ns).to receive(:changed_iteration_issue)
+              end
+
+              update_issue(iteration: iteration)
+            end
+          end
+        end
+      end
+
       context 'updating weight' do
         before do
           project.add_maintainer(user)

ee/spec/services/ee/notification_service_spec.rb

@@ -328,6 +328,159 @@ describe EE::NotificationService, :mailer do
     end
   end
 
+  describe 'issues' do
+    let(:group) { create(:group) }
+    let(:project) { create(:project, :public, namespace: group) }
+    let(:assignee) { create(:user) }
+
+    let(:issue) { create :issue, project: project, assignees: [assignee], description: 'cc @participant @unsubscribed_mentioned' }
+
+    let(:notification) { NotificationService.new }
+
+    before do
+      build_group_members(group)
+
+      add_users_with_subscription(group, issue)
+      reset_delivered_emails!
+    end
+
+    around do |example|
+      perform_enqueued_jobs { example.run }
+    end
+
+    shared_examples 'altered iteration notification on issue' do
+      it 'sends the email to the correct people' do
+        should_email(subscriber_to_new_iteration)
+        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
+
+    describe '#removed_iteration_issue' do
+      let(:mailer_method) { :removed_iteration_issue_email }
+
+      context do
+        let(:iteration) { create(:iteration, group: group, issues: [issue]) }
+        let!(:subscriber_to_new_iteration) { create(:user) { |u| issue.toggle_subscription(u, project) } }
+
+        it_behaves_like 'altered iteration notification on issue' do
+          before do
+            notification.removed_iteration_issue(issue, issue.author)
+          end
+        end
+
+        it_behaves_like 'project emails are disabled' do
+          let(:notification_target)  { issue }
+          let(:notification_trigger) { notification.removed_iteration_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(:iteration) { create(:iteration, project: project, issues: [confidential_issue]) }
+
+        it "emails subscribers of the issue's iteration 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_iteration_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_iteration_issue' do
+      let(:mailer_method) { :changed_iteration_issue_email }
+
+      context do
+        let(:new_iteration) { create(:iteration, project: project, issues: [issue]) }
+        let!(:subscriber_to_new_iteration) { create(:user) { |u| issue.toggle_subscription(u, project) } }
+
+        it_behaves_like 'altered iteration notification on issue' do
+          before do
+            notification.changed_iteration_issue(issue, new_iteration, issue.author)
+          end
+        end
+
+        it_behaves_like 'project emails are disabled' do
+          let(:notification_target)  { issue }
+          let(:notification_trigger) { notification.changed_iteration_issue(issue, new_iteration, 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_iteration) { create(:iteration, project: project, issues: [confidential_issue]) }
+
+        it "emails subscribers of the issue's iteration 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_iteration_issue(confidential_issue, new_iteration, @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
+  end
+
   describe 'epics' do
     let_it_be(:group) { create(:group, :private) }
     let_it_be(:epic) { create(:epic, group: group) }

locale/gitlab.pot

@@ -12202,6 +12202,12 @@ msgstr ""
 msgid "It's you"
 msgstr ""
 
+msgid "Iteration changed to"
+msgstr ""
+
+msgid "Iteration removed"
+msgstr ""
+
 msgid "Iterations"
 msgstr ""
 
@@ -26006,10 +26012,10 @@ msgstr ""
 msgid "exceeds the limit of %{bytes} bytes for directory name \"%{dirname}\""
 msgstr ""
 
-msgid "expired on %{milestone_due_date}"
+msgid "expired on %{timebox_due_date}"
 msgstr ""
 
-msgid "expires on %{milestone_due_date}"
+msgid "expires on %{timebox_due_date}"
 msgstr ""
 
 msgid "external_url"
@@ -26784,10 +26790,10 @@ msgstr ""
 msgid "started a discussion on %{design_link}"
 msgstr ""
 
-msgid "started on %{milestone_start_date}"
+msgid "started on %{timebox_start_date}"
 msgstr ""
 
-msgid "starts on %{milestone_start_date}"
+msgid "starts on %{timebox_start_date}"
 msgstr ""
 
 msgid "stuck"

spec/helpers/milestones_helper_spec.rb → spec/helpers/timeboxes_helper_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe MilestonesHelper do
+describe TimeboxesHelper do
   describe '#milestones_filter_dropdown_path' do
     let(:project) { create(:project) }
     let(:project2) { create(:project) }
@@ -39,23 +39,34 @@ describe MilestonesHelper do
     end
   end
 
-  describe "#milestone_date_range" do
-    def result_for(*args)
-      milestone_date_range(build(:milestone, *args))
-    end
-
+  describe "#timebox_date_range" do
     let(:yesterday) { Date.yesterday }
     let(:tomorrow) { yesterday + 2 }
     let(:format) { '%b %-d, %Y' }
     let(:yesterday_formatted) { yesterday.strftime(format) }
     let(:tomorrow_formatted) { tomorrow.strftime(format) }
 
-    it { expect(result_for(due_date: nil, start_date: nil)).to be_nil }
-    it { expect(result_for(due_date: tomorrow)).to eq("expires on #{tomorrow_formatted}") }
-    it { expect(result_for(due_date: yesterday)).to eq("expired on #{yesterday_formatted}") }
-    it { expect(result_for(start_date: tomorrow)).to eq("starts on #{tomorrow_formatted}") }
-    it { expect(result_for(start_date: yesterday)).to eq("started on #{yesterday_formatted}") }
-    it { expect(result_for(start_date: yesterday, due_date: tomorrow)).to eq("#{yesterday_formatted}–#{tomorrow_formatted}") }
+    context 'milestone' do
+      def result_for(*args)
+        timebox_date_range(build(:milestone, *args))
+      end
+
+      it { expect(result_for(due_date: nil, start_date: nil)).to be_nil }
+      it { expect(result_for(due_date: tomorrow)).to eq("expires on #{tomorrow_formatted}") }
+      it { expect(result_for(due_date: yesterday)).to eq("expired on #{yesterday_formatted}") }
+      it { expect(result_for(start_date: tomorrow)).to eq("starts on #{tomorrow_formatted}") }
+      it { expect(result_for(start_date: yesterday)).to eq("started on #{yesterday_formatted}") }
+      it { expect(result_for(start_date: yesterday, due_date: tomorrow)).to eq("#{yesterday_formatted}–#{tomorrow_formatted}") }
+    end
+
+    context 'iteration' do
+      # Iterations always have start and due dates, so only A-B format is expected
+      it 'formats properly' do
+        iteration = build(:iteration, start_date: yesterday, due_date: tomorrow)
+
+        expect(timebox_date_range(iteration)).to eq("#{yesterday_formatted}–#{tomorrow_formatted}")
+      end
+    end
   end
 
   describe '#milestone_counts' do