Merge branch 'fix-notes-emails-with-group-settings' into 'master'

Fix notes email with group-level notification email

Closes #63355

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

app/mailers/emails/notes.rb

@@ -60,7 +60,7 @@ module Emails
       # `note_id` is a `Note` when originating in `NotifyPreview`
       @note = note_id.is_a?(Note) ? note_id : Note.find(note_id)
       @project = @note.project
-      @group = @note.noteable.try(:group)
+      @group = @project.try(:group) || @note.noteable.try(:group)
 
       if (@project || @group) && @note.persisted?
         @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key)

changelogs/unreleased/fix-notes-emails-with-group-settings.yml

+---
+title: Fix comment emails not respecting group-level notification email
+merge_request:
+author:
+type: fixed

spec/mailers/emails/pages_domains_spec.rb

@@ -9,7 +9,7 @@ describe Emails::PagesDomains do
   set(:user) { project.creator }
 
   shared_examples 'a pages domain email' do
-    let(:test_recipient) { user }
+    let(:recipient) { user }
 
     it_behaves_like 'an email sent to a user'
     it_behaves_like 'an email sent from GitLab'

spec/mailers/notify_spec.rb

@@ -45,7 +45,7 @@ describe Notify do
 
   context 'for a project' do
     shared_examples 'an assignee email' do
-      let(:test_recipient) { assignee }
+      let(:recipient) { assignee }
 
       it_behaves_like 'an email sent to a user'
 
@@ -55,7 +55,7 @@ describe Notify do
         aggregate_failures do
           expect(sender.display_name).to eq(current_user.name)
           expect(sender.address).to eq(gitlab_sender)
-          expect(subject).to deliver_to(assignee.email)
+          expect(subject).to deliver_to(recipient.notification_email)
         end
       end
     end
@@ -547,12 +547,13 @@ describe Notify do
       let(:host) { Gitlab.config.gitlab.host }
 
       context 'in discussion' do
-        set(:first_note) { create(:discussion_note_on_issue) }
-        set(:second_note) { create(:discussion_note_on_issue, in_reply_to: first_note) }
-        set(:third_note) { create(:discussion_note_on_issue, in_reply_to: second_note) }
+        set(:first_note) { create(:discussion_note_on_issue, project: project) }
+        set(:second_note) { create(:discussion_note_on_issue, in_reply_to: first_note, project: project) }
+        set(:third_note) { create(:discussion_note_on_issue, in_reply_to: second_note, project: project) }
 
         subject { described_class.note_issue_email(recipient.id, third_note.id) }
 
+        it_behaves_like 'an email sent to a user'
         it_behaves_like 'appearance header and footer enabled'
         it_behaves_like 'appearance header and footer not enabled'
 
@@ -572,10 +573,11 @@ describe Notify do
       end
 
       context 'individual issue comments' do
-        set(:note) { create(:note_on_issue) }
+        set(:note) { create(:note_on_issue, project: project) }
 
         subject { described_class.note_issue_email(recipient.id, note.id) }
 
+        it_behaves_like 'an email sent to a user'
         it_behaves_like 'appearance header and footer enabled'
         it_behaves_like 'appearance header and footer not enabled'
 
@@ -604,13 +606,13 @@ describe Notify do
       it_behaves_like 'a user cannot unsubscribe through footer link'
 
       it 'has the correct subject and body' do
-        is_expected.to have_referable_subject(project_snippet, reply: true)
+        is_expected.to have_referable_subject(project_snippet, include_group: true, reply: true)
         is_expected.to have_body_text project_snippet_note.note
       end
     end
 
     describe 'project was moved' do
-      let(:test_recipient) { user }
+      let(:recipient) { user }
       subject { described_class.project_was_moved_email(project.id, user.id, "gitlab/gitlab") }
 
       it_behaves_like 'an email sent to a user'
@@ -811,7 +813,7 @@ describe Notify do
 
         it 'has the correct subject and body' do
           aggregate_failures do
-            is_expected.to have_subject("Re: #{project.name} | #{commit.title} (#{commit.short_id})")
+            is_expected.to have_subject("Re: #{project.name} | #{project.group.name} | #{commit.title} (#{commit.short_id})")
             is_expected.to have_body_text(commit.short_id)
           end
         end
@@ -837,7 +839,7 @@ describe Notify do
 
         it 'has the correct subject and body' do
           aggregate_failures do
-            is_expected.to have_referable_subject(merge_request, reply: true)
+            is_expected.to have_referable_subject(merge_request, include_group: true, reply: true)
             is_expected.to have_body_text note_on_merge_request_path
           end
         end
@@ -863,7 +865,7 @@ describe Notify do
 
         it 'has the correct subject and body' do
           aggregate_failures do
-            is_expected.to have_referable_subject(issue, reply: true)
+            is_expected.to have_referable_subject(issue, include_group: true, reply: true)
             is_expected.to have_body_text(note_on_issue_path)
           end
         end
@@ -929,7 +931,7 @@ describe Notify do
         it_behaves_like 'appearance header and footer not enabled'
 
         it 'has the correct subject' do
-          is_expected.to have_subject "Re: #{project.name} | #{commit.title} (#{commit.short_id})"
+          is_expected.to have_subject "Re: #{project.name} | #{project.group.name} | #{commit.title} (#{commit.short_id})"
         end
 
         it 'contains a link to the commit' do
@@ -957,7 +959,7 @@ describe Notify do
         it_behaves_like 'appearance header and footer not enabled'
 
         it 'has the correct subject' do
-          is_expected.to have_referable_subject(merge_request, reply: true)
+          is_expected.to have_referable_subject(merge_request, include_group: true, reply: true)
         end
 
         it 'contains a link to the merge request note' do
@@ -985,7 +987,7 @@ describe Notify do
         it_behaves_like 'appearance header and footer not enabled'
 
         it 'has the correct subject' do
-          is_expected.to have_referable_subject(issue, reply: true)
+          is_expected.to have_referable_subject(issue, include_group: true, reply: true)
         end
 
         it 'contains a link to the issue note' do

spec/support/helpers/email_helpers.rb

@@ -37,8 +37,19 @@ module EmailHelpers
     ActionMailer::Base.deliveries.find { |d| d.to.include?(user.notification_email) }
   end
 
-  def have_referable_subject(referable, include_project: true, reply: false)
-    prefix = (include_project && referable.project ? "#{referable.project.name} | " : '').freeze
+  def have_referable_subject(referable, include_project: true, include_group: false, reply: false)
+    context = []
+
+    context << referable.project.name if include_project && referable.project
+    context << referable.project.group.name if include_group && referable.project.group
+
+    prefix =
+      if context.any?
+        context.join(' | ') + ' | '
+      else
+        ''
+      end
+
     prefix = "Re: #{prefix}" if reply
 
     suffix = "#{referable.title} (#{referable.to_reference})"

spec/support/shared_examples/notify_shared_examples.rb

@@ -45,18 +45,18 @@ shared_examples 'an email sent to a user' do
   let(:group_notification_email) { 'user+group@example.com' }
 
   it 'is sent to user\'s global notification email address' do
-    expect(subject).to deliver_to(test_recipient.notification_email)
+    expect(subject).to deliver_to(recipient.notification_email)
   end
 
   context 'that is part of a project\'s group' do
     it 'is sent to user\'s group notification email address when set' do
-      create(:notification_setting, user: test_recipient, source: project.group, notification_email: group_notification_email)
+      create(:notification_setting, user: recipient, source: project.group, notification_email: group_notification_email)
       expect(subject).to deliver_to(group_notification_email)
     end
 
     it 'is sent to user\'s global notification email address when no group email set' do
-      create(:notification_setting, user: test_recipient, source: project.group, notification_email: '')
-      expect(subject).to deliver_to(test_recipient.notification_email)
+      create(:notification_setting, user: recipient, source: project.group, notification_email: '')
+      expect(subject).to deliver_to(recipient.notification_email)
     end
   end
 
@@ -67,17 +67,17 @@ shared_examples 'an email sent to a user' do
 
     it 'is sent to user\'s subgroup notification email address when set' do
       # Set top-level group notification email address to make sure it doesn't get selected
-      create(:notification_setting, user: test_recipient, source: group, notification_email: group_notification_email)
+      create(:notification_setting, user: recipient, source: group, notification_email: group_notification_email)
 
       subgroup_notification_email = 'user+subgroup@example.com'
-      create(:notification_setting, user: test_recipient, source: subgroup, notification_email: subgroup_notification_email)
+      create(:notification_setting, user: recipient, source: subgroup, notification_email: subgroup_notification_email)
 
       expect(subject).to deliver_to(subgroup_notification_email)
     end
 
     it 'is sent to user\'s group notification email address when set and subgroup email address not set' do
-      create(:notification_setting, user: test_recipient, source: subgroup, notification_email: '')
-      expect(subject).to deliver_to(test_recipient.notification_email)
+      create(:notification_setting, user: recipient, source: subgroup, notification_email: '')
+      expect(subject).to deliver_to(recipient.notification_email)
     end
   end
 end