Merge branch '226988-notify-email_participants' into 'master'

Notify email_participants instead of external_author

See merge request gitlab-org/gitlab!51023

app/mailers/emails/service_desk.rb

@@ -17,18 +17,18 @@ module Emails
         send_from_user_email: false,
         sender_name: @project.service_desk_setting&.outgoing_name
       )
-      options = service_desk_options(email_sender, 'thank_you')
+      options = service_desk_options(email_sender, 'thank_you', @issue.external_author)
                   .merge(subject: "Re: #{subject_base}")
 
       mail_new_thread(@issue, options)
     end
 
-    def service_desk_new_note_email(issue_id, note_id)
+    def service_desk_new_note_email(issue_id, note_id, recipient)
       @note = Note.find(note_id)
       setup_service_desk_mail(issue_id)
 
       email_sender = sender(@note.author_id)
-      options = service_desk_options(email_sender, 'new_note')
+      options = service_desk_options(email_sender, 'new_note', recipient)
                   .merge(subject: subject_base)
 
       mail_answer_thread(@issue, options)
@@ -44,10 +44,10 @@ module Emails
       @sent_notification = SentNotification.record(@issue, @support_bot.id, reply_key)
     end
 
-    def service_desk_options(email_sender, email_type)
+    def service_desk_options(email_sender, email_type, recipient)
       {
         from: email_sender,
-        to: @issue.external_author
+        to: recipient
       }.tap do |options|
         next unless template_body = template_content(email_type)
 

app/mailers/previews/notify_preview.rb

@@ -181,7 +181,7 @@ class NotifyPreview < ActionMailer::Preview
     cleanup do
       note = create_note(noteable_type: 'Issue', noteable_id: issue.id, note: 'Issue note content')
 
-      Notify.service_desk_new_note_email(issue.id, note.id).message
+      Notify.service_desk_new_note_email(issue.id, note.id, 'someone@gitlab.com').message
     end
   end
 

app/models/issue.rb

@@ -438,7 +438,11 @@ class Issue < ApplicationRecord
     issue_type_supports?(:assignee)
   end
 
-  def email_participants_downcase
+  def email_participants_emails
+    issue_email_participants.pluck(:email)
+  end
+
+  def email_participants_emails_downcase
     issue_email_participants.pluck(IssueEmailParticipant.arel_table[:email].lower)
   end
 

app/services/notification_service.rb

@@ -369,18 +369,19 @@ class NotificationService
   end
 
   def send_service_desk_notification(note)
-    return unless Gitlab::ServiceDesk.supported?
     return unless note.noteable_type == 'Issue'
 
     issue = note.noteable
-    support_bot = User.support_bot
+    recipients = issue.email_participants_emails
+
+    return unless recipients.any?
 
-    return unless issue.external_author.present?
-    return unless issue.project.service_desk_enabled?
-    return if note.author == support_bot
-    return unless issue.subscribed?(support_bot, issue.project)
+    support_bot = User.support_bot
+    recipients.delete(issue.external_author) if note.author == support_bot
 
-    mailer.service_desk_new_note_email(issue.id, note.id).deliver_later
+    recipients.each do |recipient|
+      mailer.service_desk_new_note_email(issue.id, note.id, recipient).deliver_later
+    end
   end
 
   # Notify users when a new release is created

changelogs/unreleased/226988-notify-email_participants-instead-of-external_author.yml

+---
+title: Notify issue email participants instead of external author
+merge_request: 51023
+author: Lee Tickett @leetickett
+type: other

lib/gitlab/quick_actions/issue_actions.rb

@@ -246,7 +246,7 @@ module Gitlab
         command :invite_email do |emails = ""|
           MAX_NUMBER_OF_EMAILS = 6
 
-          existing_emails = quick_action_target.email_participants_downcase
+          existing_emails = quick_action_target.email_participants_emails_downcase
           emails_to_add = emails.split(' ').index_by { |email| [email.downcase, email] }.except(*existing_emails).each_value.first(MAX_NUMBER_OF_EMAILS)
           added_emails = []
 

spec/mailers/emails/service_desk_spec.rb

@@ -13,8 +13,13 @@ RSpec.describe Emails::ServiceDesk do
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project) }
   let_it_be(:issue) { create(:issue, project: project) }
+  let_it_be(:email) { 'someone@gitlab.com' }
   let(:template) { double(content: template_content) }
 
+  before_all do
+    issue.issue_email_participants.create!(email: email)
+  end
+
   before do
     stub_const('ServiceEmailClass', Class.new(ApplicationMailer))
 
@@ -72,6 +77,10 @@ RSpec.describe Emails::ServiceDesk do
     let(:template_content) { 'custom text' }
     let(:issue) { create(:issue, project: project)}
 
+    before do
+      issue.issue_email_participants.create!(email: email)
+    end
+
     context 'when a template is in the repository' do
       let(:project) { create(:project, :custom_repo, files: { ".gitlab/service_desk_templates/#{template_key}.md" => template_content }) }
 
@@ -151,7 +160,7 @@ RSpec.describe Emails::ServiceDesk do
     let_it_be(:note) { create(:note_on_issue, noteable: issue, project: project) }
     let_it_be(:default_text) { note.note }
 
-    subject { ServiceEmailClass.service_desk_new_note_email(issue.id, note.id) }
+    subject { ServiceEmailClass.service_desk_new_note_email(issue.id, note.id, email) }
 
     it_behaves_like 'read template from repository', 'new_note'
 

spec/mailers/notify_spec.rb

@@ -1218,6 +1218,7 @@ RSpec.describe Notify do
     context 'for service desk issues' do
       before do
         issue.update!(external_author: 'service.desk@example.com')
+        issue.issue_email_participants.create!(email: 'service.desk@example.com')
       end
 
       def expect_sender(username)
@@ -1266,7 +1267,7 @@ RSpec.describe Notify do
       describe 'new note email' do
         let_it_be(:first_note) { create(:discussion_note_on_issue, note: 'Hello world') }
 
-        subject { described_class.service_desk_new_note_email(issue.id, first_note.id) }
+        subject { described_class.service_desk_new_note_email(issue.id, first_note.id, 'service.desk@example.com') }
 
         it_behaves_like 'an unsubscribeable thread'
 

spec/models/issue_spec.rb

@@ -1259,11 +1259,22 @@ RSpec.describe Issue do
     end
   end
 
+  describe '#email_participants_emails' do
+    let_it_be(:issue) { create(:issue) }
+
+    it 'returns a list of emails' do
+      participant1 = issue.issue_email_participants.create(email: 'a@gitlab.com')
+      participant2 = issue.issue_email_participants.create(email: 'b@gitlab.com')
+
+      expect(issue.email_participants_emails).to contain_exactly(participant1.email, participant2.email)
+    end
+  end
+
   describe '#email_participants_downcase' do
     it 'returns a list of emails with all uppercase letters replaced with their lowercase counterparts' do
       participant = create(:issue_email_participant, email: 'SomEoNe@ExamPLe.com')
 
-      expect(participant.issue.email_participants_downcase).to match([participant.email.downcase])
+      expect(participant.issue.email_participants_emails_downcase).to match([participant.email.downcase])
     end
   end
 end

spec/services/notification_service_spec.rb

@@ -315,17 +315,17 @@ RSpec.describe NotificationService, :mailer do
   describe 'Notes' do
     context 'issue note' do
       let_it_be(:project) { create(:project, :private) }
-      let_it_be(:issue) { create(:issue, project: project, assignees: [assignee]) }
+      let_it_be_with_reload(:issue) { create(:issue, project: project, assignees: [assignee]) }
       let_it_be(:mentioned_issue) { create(:issue, assignees: issue.assignees) }
       let_it_be_with_reload(:author) { create(:user) }
       let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') }
 
       subject { notification.new_note(note) }
 
-      context 'on service desk issue' do
+      context 'issue_email_participants' do
         before do
           allow(Notify).to receive(:service_desk_new_note_email)
-                             .with(Integer, Integer).and_return(mailer)
+                             .with(Integer, Integer, String).and_return(mailer)
 
           allow(::Gitlab::IncomingEmail).to receive(:enabled?) { true }
           allow(::Gitlab::IncomingEmail).to receive(:supports_wildcard?) { true }
@@ -336,7 +336,7 @@ RSpec.describe NotificationService, :mailer do
 
         def should_email!
           expect(Notify).to receive(:service_desk_new_note_email)
-            .with(issue.id, note.id)
+            .with(issue.id, note.id, issue.external_author)
         end
 
         def should_not_email!
@@ -365,33 +365,19 @@ RSpec.describe NotificationService, :mailer do
         let(:project) { issue.project }
         let(:note) { create(:note, noteable: issue, project: project) }
 
-        context 'a non-service-desk issue' do
+        context 'do not exist' do
           it_should_not_email!
         end
 
-        context 'a service-desk issue' do
+        context 'do exist' do
+          let!(:issue_email_participant) { issue.issue_email_participants.create!(email: 'service.desk@example.com') }
+
           before do
             issue.update!(external_author: 'service.desk@example.com')
             project.update!(service_desk_enabled: true)
           end
 
           it_should_email!
-
-          context 'where the project has disabled the feature' do
-            before do
-              project.update!(service_desk_enabled: false)
-            end
-
-            it_should_not_email!
-          end
-
-          context 'when the support bot has unsubscribed' do
-            before do
-              issue.unsubscribe(User.support_bot, project)
-            end
-
-            it_should_not_email!
-          end
         end
       end