From f9f496e7f19f574992b9543763490fda6d920e77 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 31 Dec 2020 00:10:12 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/mailers/emails/service_desk.rb | 10 +++---- app/mailers/previews/notify_preview.rb | 2 +- app/models/issue.rb | 4 --- app/services/notification_service.rb | 15 +++++----- ...articipants-instead-of-external_author.yml | 5 ---- spec/mailers/emails/service_desk_spec.rb | 11 +------ spec/mailers/notify_spec.rb | 3 +- spec/models/issue_spec.rb | 11 ------- spec/services/notification_service_spec.rb | 30 ++++++++++++++----- 9 files changed, 37 insertions(+), 54 deletions(-) delete mode 100644 changelogs/unreleased/226988-notify-email_participants-instead-of-external_author.yml diff --git a/app/mailers/emails/service_desk.rb b/app/mailers/emails/service_desk.rb index 66eb2c646a9..4dceff5b7ba 100644 --- a/app/mailers/emails/service_desk.rb +++ b/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', @issue.external_author) + options = service_desk_options(email_sender, 'thank_you') .merge(subject: "Re: #{subject_base}") mail_new_thread(@issue, options) end - def service_desk_new_note_email(issue_id, note_id, recipient) + def service_desk_new_note_email(issue_id, note_id) @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', recipient) + options = service_desk_options(email_sender, 'new_note') .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, recipient) + def service_desk_options(email_sender, email_type) { from: email_sender, - to: recipient + to: @issue.external_author }.tap do |options| next unless template_body = template_content(email_type) diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index 7d7273fc768..c70ac1428cd 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -169,7 +169,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, 'someone@gitlab.com').message + Notify.service_desk_new_note_email(issue.id, note.id).message end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 3ee5ba5a4f4..253f4465cd9 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -434,10 +434,6 @@ class Issue < ApplicationRecord moved_to || duplicated_to end - def email_participants_emails - issue_email_participants.pluck(:email) - end - private def ensure_metrics diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 5d0414e58b8..4ff462191fe 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -347,19 +347,18 @@ class NotificationService end def send_service_desk_notification(note) + return unless Gitlab::ServiceDesk.supported? return unless note.noteable_type == 'Issue' issue = note.noteable - - return unless issue.issue_email_participants.any? - - recipients = issue.email_participants_emails support_bot = User.support_bot - recipients.delete(issue.external_author) if note.author == support_bot - recipients.each do |recipient| - mailer.service_desk_new_note_email(issue.id, note.id, recipient).deliver_later - end + 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) + + mailer.service_desk_new_note_email(issue.id, note.id).deliver_later end # Notify users when a new release is created diff --git a/changelogs/unreleased/226988-notify-email_participants-instead-of-external_author.yml b/changelogs/unreleased/226988-notify-email_participants-instead-of-external_author.yml deleted file mode 100644 index 68c9428a765..00000000000 --- a/changelogs/unreleased/226988-notify-email_participants-instead-of-external_author.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Notify issue_email_participants instead of external_author -merge_request: 48823 -author: Lee Tickett @leetickett -type: other diff --git a/spec/mailers/emails/service_desk_spec.rb b/spec/mailers/emails/service_desk_spec.rb index cb74194020d..7d04b373be6 100644 --- a/spec/mailers/emails/service_desk_spec.rb +++ b/spec/mailers/emails/service_desk_spec.rb @@ -13,13 +13,8 @@ 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)) @@ -77,10 +72,6 @@ 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 }) } @@ -160,7 +151,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, email) } + subject { ServiceEmailClass.service_desk_new_note_email(issue.id, note.id) } it_behaves_like 'read template from repository', 'new_note' diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 645b015bd67..3ebc2fc1e36 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -1285,7 +1285,6 @@ 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) @@ -1334,7 +1333,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, 'service.desk@example.com') } + subject { described_class.service_desk_new_note_email(issue.id, first_note.id) } it_behaves_like 'an unsubscribeable thread' diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 68917dcec77..81f045b4db1 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -1260,15 +1260,4 @@ RSpec.describe Issue do expect { issue.issue_type_supports?(:unkown_feature) }.to raise_error(ArgumentError) 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 end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index e4ad8909785..9431c023850 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -297,17 +297,17 @@ RSpec.describe NotificationService, :mailer do describe 'Notes' do context 'issue note' do let_it_be(:project) { create(:project, :private) } - let_it_be_with_reload(:issue) { create(:issue, project: project, assignees: [assignee]) } + let_it_be(: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 'issue_email_participants' do + context 'on service desk issue' do before do allow(Notify).to receive(:service_desk_new_note_email) - .with(Integer, Integer, String).and_return(mailer) + .with(Integer, Integer).and_return(mailer) allow(::Gitlab::IncomingEmail).to receive(:enabled?) { true } allow(::Gitlab::IncomingEmail).to receive(:supports_wildcard?) { true } @@ -318,7 +318,7 @@ RSpec.describe NotificationService, :mailer do def should_email! expect(Notify).to receive(:service_desk_new_note_email) - .with(issue.id, note.id, issue.external_author) + .with(issue.id, note.id) end def should_not_email! @@ -347,19 +347,33 @@ RSpec.describe NotificationService, :mailer do let(:project) { issue.project } let(:note) { create(:note, noteable: issue, project: project) } - context 'do not exist' do + context 'a non-service-desk issue' do it_should_not_email! end - context 'do exist' do - let!(:issue_email_participant) { issue.issue_email_participants.create!(email: 'service.desk@example.com') } - + context 'a service-desk issue' do 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