Merge branch '63568-access-email-notifications-custom-email' into 'master'

Respect alt email when sending group access notifications

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

7 files changed, 45 insertions, 22 deletions

diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb
index 2bfa59774d7..76fa7236ab1 100644
--- a/app/mailers/emails/members.rb
+++ b/app/mailers/emails/members.rb
@@ -9,11 +9,11 @@ module Emails
       helper_method :member_source, :member
     end
 
-    def member_access_requested_email(member_source_type, member_id, recipient_notification_email)
+    def member_access_requested_email(member_source_type, member_id, recipient_id)
       @member_source_type = member_source_type
       @member_id = member_id
 
-      mail(to: recipient_notification_email,
+      mail(to: recipient(recipient_id, notification_group),
            subject: subject("Request to join the #{member_source.human_name} #{member_source.model_name.singular}"))
     end
 
@@ -21,16 +21,15 @@ module Emails
       @member_source_type = member_source_type
       @member_id = member_id
 
-      mail(to: member.user.notification_email,
+      mail(to: recipient(member.user, notification_group),
            subject: subject("Access to the #{member_source.human_name} #{member_source.model_name.singular} was granted"))
     end
 
     def member_access_denied_email(member_source_type, source_id, user_id)
       @member_source_type = member_source_type
       @member_source = member_source_class.find(source_id)
-      requester = User.find(user_id)
 
-      mail(to: requester.notification_email,
+      mail(to: recipient(user_id, notification_group),
            subject: subject("Access to the #{member_source.human_name} #{member_source.model_name.singular} was denied"))
     end
 
@@ -48,7 +47,7 @@ module Emails
       @member_id = member_id
       return unless member.created_by
 
-      mail(to: member.created_by.notification_email,
+      mail(to: recipient(member.created_by, notification_group),
            subject: subject('Invitation accepted'))
     end
 
@@ -59,7 +58,7 @@ module Emails
       @member_source = member_source_class.find(source_id)
       @invite_email = invite_email
 
-      mail(to: recipient(created_by_id, member_source_type == 'Project' ? @member_source.group : @member_source),
+      mail(to: recipient(created_by_id, notification_group),
            subject: subject('Invitation declined'))
     end
 
@@ -71,6 +70,10 @@ module Emails
       @member_source ||= member.source
     end
 
+    def notification_group
+      @member_source_type.casecmp?('project') ? member_source.group : member_source
+    end
+
     private
 
     def member_source_class
diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb
index 8ef20a03541..5d292094a05 100644
--- a/app/mailers/notify.rb
+++ b/app/mailers/notify.rb
@@ -71,14 +71,18 @@ class Notify < BaseMailer
     address.format
   end
 
-  # Look up a User by their ID and return their email address
+  # Look up a User's notification email for a particular context.
+  # Can look up by their ID or can accept a User object.
   #
-  # recipient_id       - User ID
+  # recipient          - User object OR a User ID
   # notification_group - The parent group of the notification
   #
   # Returns a String containing the User's email address.
-  def recipient(recipient_id, notification_group = nil)
-    User.find(recipient_id).notification_email_for(notification_group)
+  def recipient(recipient, notification_group = nil)
+    user = recipient if recipient.is_a?(User)
+    user ||= User.find(recipient)
+
+    user.notification_email_for(notification_group)
   end
 
   # Formats arguments into a String suitable for use as an email subject
diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb
index 80e0a17c312..b3fab930922 100644
--- a/app/mailers/previews/notify_preview.rb
+++ b/app/mailers/previews/notify_preview.rb
@@ -105,11 +105,11 @@ class NotifyPreview < ActionMailer::Preview
   end
 
   def member_access_granted_email
-    Notify.member_access_granted_email('project', user.id).message
+    Notify.member_access_granted_email(member.source_type, member.id).message
   end
 
   def member_access_requested_email
-    Notify.member_access_requested_email('group', user.id, 'some@example.com').message
+    Notify.member_access_requested_email('group', user.id, user.id).message
   end
 
   def member_invite_accepted_email
@@ -183,6 +183,10 @@ class NotifyPreview < ActionMailer::Preview
     @user ||= User.last
   end
 
+  def member
+    @member ||= Member.last
+  end
+
   def create_note(params)
     Notes::CreateService.new(project, user, params).execute
   end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index a55771ed538..21fab22e0d4 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -595,7 +595,7 @@ class NotificationService
   end
 
   def deliver_access_request_email(recipient, member)
-    mailer.member_access_requested_email(member.real_source_type, member.id, recipient.user.notification_email).deliver_later
+    mailer.member_access_requested_email(member.real_source_type, member.id, recipient.user.id).deliver_later
   end
 
   def fallback_to_group_owners_maintainers?(recipients, member)
diff --git a/changelogs/unreleased/63568-access-email-notifications-custom-email.yml b/changelogs/unreleased/63568-access-email-notifications-custom-email.yml
new file mode 100644
index 00000000000..ece6442d7cf
--- /dev/null
+++ b/changelogs/unreleased/63568-access-email-notifications-custom-email.yml
@@ -0,0 +1,5 @@
+---
+title: Respect group notification email when sending group access notifications
+merge_request: 31089
+author:
+type: fixed
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 56bbcc4c306..dcc4b70a382 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -640,7 +640,7 @@ describe Notify do
         project.request_access(user)
         project.requesters.find_by(user_id: user.id)
       end
-      subject { described_class.member_access_requested_email('project', project_member.id, recipient.notification_email) }
+      subject { described_class.member_access_requested_email('project', project_member.id, recipient.id) }
 
       it_behaves_like 'an email sent from GitLab'
       it_behaves_like 'it should not have Gmail Actions links'
@@ -737,9 +737,9 @@ describe Notify do
 
     describe 'project invitation accepted' do
       let(:invited_user) { create(:user, name: 'invited user') }
-      let(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } }
+      let(:recipient) { create(:user).tap { |u| project.add_maintainer(u) } }
       let(:project_member) do
-        invitee = invite_to_project(project, inviter: maintainer)
+        invitee = invite_to_project(project, inviter: recipient)
         invitee.accept_invite!(invited_user)
         invitee
       end
@@ -747,6 +747,7 @@ describe Notify do
       subject { described_class.member_invite_accepted_email('project', project_member.id) }
 
       it_behaves_like 'an email sent from GitLab'
+      it_behaves_like 'an email sent to a user'
       it_behaves_like 'it should not have Gmail Actions links'
       it_behaves_like "a user cannot unsubscribe through footer link"
       it_behaves_like 'appearance header and footer enabled'
@@ -762,16 +763,17 @@ describe Notify do
     end
 
     describe 'project invitation declined' do
-      let(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } }
+      let(:recipient) { create(:user).tap { |u| project.add_maintainer(u) } }
       let(:project_member) do
-        invitee = invite_to_project(project, inviter: maintainer)
+        invitee = invite_to_project(project, inviter: recipient)
         invitee.decline_invite!
         invitee
       end
 
-      subject { described_class.member_invite_declined_email('Project', project.id, project_member.invite_email, maintainer.id) }
+      subject { described_class.member_invite_declined_email('Project', project.id, project_member.invite_email, recipient.id) }
 
       it_behaves_like 'an email sent from GitLab'
+      it_behaves_like 'an email sent to a user'
       it_behaves_like 'it should not have Gmail Actions links'
       it_behaves_like "a user cannot unsubscribe through footer link"
       it_behaves_like 'appearance header and footer enabled'
@@ -1087,9 +1089,10 @@ describe Notify do
         group.request_access(user)
         group.requesters.find_by(user_id: user.id)
       end
-      subject { described_class.member_access_requested_email('group', group_member.id, recipient.notification_email) }
+      subject { described_class.member_access_requested_email('group', group_member.id, recipient.id) }
 
       it_behaves_like 'an email sent from GitLab'
+      it_behaves_like 'an email sent to a user'
       it_behaves_like 'it should not have Gmail Actions links'
       it_behaves_like "a user cannot unsubscribe through footer link"
       it_behaves_like 'appearance header and footer enabled'
@@ -1111,9 +1114,11 @@ describe Notify do
         group.request_access(user)
         group.requesters.find_by(user_id: user.id)
       end
+      let(:recipient) { user }
       subject { described_class.member_access_denied_email('group', group.id, user.id) }
 
       it_behaves_like 'an email sent from GitLab'
+      it_behaves_like 'an email sent to a user'
       it_behaves_like 'it should not have Gmail Actions links'
       it_behaves_like "a user cannot unsubscribe through footer link"
       it_behaves_like 'appearance header and footer enabled'
@@ -1128,10 +1133,12 @@ describe Notify do
 
     describe 'group access changed' do
       let(:group_member) { create(:group_member, group: group, user: user) }
+      let(:recipient) { user }
 
       subject { described_class.member_access_granted_email('group', group_member.id) }
 
       it_behaves_like 'an email sent from GitLab'
+      it_behaves_like 'an email sent to a user'
       it_behaves_like 'it should not have Gmail Actions links'
       it_behaves_like "a user cannot unsubscribe through footer link"
       it_behaves_like 'appearance header and footer enabled'
diff --git a/spec/support/shared_examples/notify_shared_examples.rb b/spec/support/shared_examples/notify_shared_examples.rb
index a537fab4bcd..ca031df000e 100644
--- a/spec/support/shared_examples/notify_shared_examples.rb
+++ b/spec/support/shared_examples/notify_shared_examples.rb
@@ -52,7 +52,7 @@ shared_examples 'an email sent to a user' do
     it 'is sent to user\'s group notification email' do
       group_notification_email = 'user+group@example.com'
 
-      create(:notification_setting, user: recipient, source: project.group, notification_email: group_notification_email)
+      create(:notification_setting, user: recipient, source: group, notification_email: group_notification_email)
 
       expect(subject).to deliver_to(group_notification_email)
     end