Resolve "group request membership mail with too long list of "To:""

5 files changed, 114 insertions, 78 deletions

diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb
index d76c61c369f..75cf56a51f2 100644
--- a/app/mailers/emails/members.rb
+++ b/app/mailers/emails/members.rb
@@ -7,18 +7,11 @@ module Emails
       helper_method :member_source, :member
     end
 
-    def member_access_requested_email(member_source_type, member_id)
+    def member_access_requested_email(member_source_type, member_id, recipient_notification_email)
       @member_source_type = member_source_type
       @member_id = member_id
 
-      admins = member_source.members.owners_and_masters.pluck(:notification_email)
-      # A project in a group can have no explicit owners/masters, in that case
-      # we fallbacks to the group's owners/masters.
-      if admins.empty? && member_source.respond_to?(:group) && member_source.group
-        admins = member_source.group.members.owners_and_masters.pluck(:notification_email)
-      end
-
-      mail(to: admins,
+      mail(to: recipient_notification_email,
            subject: subject("Request to join the #{member_source.human_name} #{member_source.model_name.singular}"))
     end
 
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 8c84ccfcc92..56e941d90ff 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -208,7 +208,12 @@ class NotificationService
   def new_access_request(member)
     return true unless member.notifiable?(:subscription)
 
-    mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later
+    recipients = member.source.members.owners_and_masters
+    if fallback_to_group_owners_masters?(recipients, member)
+      recipients = member.source.group.members.owners_and_masters
+    end
+
+    recipients.each { |recipient| deliver_access_request_email(recipient, member) }
   end
 
   def decline_access_request(member)
@@ -435,4 +440,14 @@ class NotificationService
   def notifiable_users(*args)
     NotificationRecipientService.notifiable_users(*args)
   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
+  end
+
+  def fallback_to_group_owners_masters?(recipients, member)
+    return false if recipients.present?
+
+    member.source.respond_to?(:group) && member.source.group
+  end
 end
diff --git a/changelogs/unreleased/42274-group-request-membership-long-too.yml b/changelogs/unreleased/42274-group-request-membership-long-too.yml
new file mode 100644
index 00000000000..03efedba638
--- /dev/null
+++ b/changelogs/unreleased/42274-group-request-membership-long-too.yml
@@ -0,0 +1,5 @@
+---
+title: Fix long list of recipients on group request membership email
+merge_request: 17121
+author: Jacopo Beschi @jacopo-beschi
+type: fixed
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 59eda025108..bcbb9287199 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -463,59 +463,30 @@ describe Notify do
     end
 
     describe 'project access requested' do
-      context 'for a project in a user namespace' do
-        let(:project) do
-          create(:project, :public, :access_requestable) do |project|
-            project.add_master(project.owner, current_user: project.owner)
-          end
-        end
-
-        let(:project_member) 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) }
-
-        it_behaves_like 'an email sent from GitLab'
-        it_behaves_like 'it should not have Gmail Actions links'
-        it_behaves_like "a user cannot unsubscribe through footer link"
-
-        it 'contains all the useful information' do
-          to_emails = subject.header[:to].addrs
-          expect(to_emails.size).to eq(1)
-          expect(to_emails[0].address).to eq(project.members.owners_and_masters.first.user.notification_email)
-
-          is_expected.to have_subject "Request to join the #{project.name_with_namespace} project"
-          is_expected.to have_html_escaped_body_text project.name_with_namespace
-          is_expected.to have_body_text project_project_members_url(project)
-          is_expected.to have_body_text project_member.human_access
+      let(:project) do
+        create(:project, :public, :access_requestable) do |project|
+          project.add_master(project.owner)
         end
       end
 
-      context 'for a project in a group' do
-        let(:group_owner) { create(:user) }
-        let(:group) { create(:group).tap { |g| g.add_owner(group_owner) } }
-        let(:project) { create(:project, :public, :access_requestable, namespace: group) }
-        let(:project_member) 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) }
+      let(:project_member) 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) }
 
-        it_behaves_like 'an email sent from GitLab'
-        it_behaves_like 'it should not have Gmail Actions links'
-        it_behaves_like "a user cannot unsubscribe through footer link"
+      it_behaves_like 'an email sent from GitLab'
+      it_behaves_like 'it should not have Gmail Actions links'
+      it_behaves_like "a user cannot unsubscribe through footer link"
 
-        it 'contains all the useful information' do
-          to_emails = subject.header[:to].addrs
-          expect(to_emails.size).to eq(1)
-          expect(to_emails[0].address).to eq(group.members.owners_and_masters.first.user.notification_email)
+      it 'contains all the useful information' do
+        to_emails = subject.header[:to].addrs.map(&:address)
+        expect(to_emails).to eq([recipient.notification_email])
 
-          is_expected.to have_subject "Request to join the #{project.name_with_namespace} project"
-          is_expected.to have_html_escaped_body_text project.name_with_namespace
-          is_expected.to have_body_text project_project_members_url(project)
-          is_expected.to have_body_text project_member.human_access
-        end
+        is_expected.to have_subject "Request to join the #{project.name_with_namespace} project"
+        is_expected.to have_html_escaped_body_text project.name_with_namespace
+        is_expected.to have_body_text project_project_members_url(project)
+        is_expected.to have_body_text project_member.human_access
       end
     end
 
@@ -959,13 +930,16 @@ 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) }
+      subject { described_class.member_access_requested_email('group', group_member.id, recipient.notification_email) }
 
       it_behaves_like 'an email sent from GitLab'
       it_behaves_like 'it should not have Gmail Actions links'
       it_behaves_like "a user cannot unsubscribe through footer link"
 
       it 'contains all the useful information' do
+        to_emails = subject.header[:to].addrs.map(&:address)
+        expect(to_emails).to eq([recipient.notification_email])
+
         is_expected.to have_subject "Request to join the #{group.name} group"
         is_expected.to have_html_escaped_body_text group.name
         is_expected.to have_body_text group_group_members_url(group)
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 35eb84e5e88..836ffb7cea0 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -1307,6 +1307,33 @@ describe NotificationService, :mailer do
   end
 
   describe 'GroupMember' do
+    let(:added_user) { create(:user) }
+
+    describe '#new_access_request' do
+      let(:master) { create(:user) }
+      let(:owner) { create(:user) }
+      let(:developer) { create(:user) }
+      let!(:group) do
+        create(:group, :public, :access_requestable) do |group|
+          group.add_owner(owner)
+          group.add_master(master)
+          group.add_developer(developer)
+        end
+      end
+
+      before do
+        reset_delivered_emails!
+      end
+
+      it 'sends notification to group owners_and_masters' do
+        group.request_access(added_user)
+
+        should_email(owner)
+        should_email(master)
+        should_not_email(developer)
+      end
+    end
+
     describe '#decline_group_invite' do
       let(:creator) { create(:user) }
       let(:group) { create(:group) }
@@ -1328,18 +1355,9 @@ describe NotificationService, :mailer do
 
     describe '#new_group_member' do
       let(:group) { create(:group) }
-      let(:added_user) { create(:user) }
-
-      def create_member!
-        GroupMember.create(
-          group: group,
-          user: added_user,
-          access_level: Gitlab::Access::GUEST
-        )
-      end
 
       it 'sends a notification' do
-        create_member!
+        group.add_guest(added_user)
         should_only_email(added_user)
       end
 
@@ -1349,7 +1367,7 @@ describe NotificationService, :mailer do
         end
 
         it 'does not send a notification' do
-          create_member!
+          group.add_guest(added_user)
           should_not_email_anyone
         end
       end
@@ -1357,8 +1375,42 @@ describe NotificationService, :mailer do
   end
 
   describe 'ProjectMember' do
+    let(:project) { create(:project) }
+    set(:added_user) { create(:user) }
+
+    describe '#new_access_request' do
+      context 'for a project in a user namespace' do
+        let(:project) do
+          create(:project, :public, :access_requestable) do |project|
+            project.add_master(project.owner)
+          end
+        end
+
+        it 'sends notification to project owners_and_masters' do
+          project.request_access(added_user)
+
+          should_only_email(project.owner)
+        end
+      end
+
+      context 'for a project in a group' do
+        let(:group_owner) { create(:user) }
+        let(:group) { create(:group).tap { |g| g.add_owner(group_owner) } }
+        let!(:project) { create(:project, :public, :access_requestable, namespace: group) }
+
+        before do
+          reset_delivered_emails!
+        end
+
+        it 'sends notification to group owners_and_masters' do
+          project.request_access(added_user)
+
+          should_only_email(group_owner)
+        end
+      end
+    end
+
     describe '#decline_group_invite' do
-      let(:project) { create(:project) }
       let(:member) { create(:user) }
 
       before do
@@ -1375,19 +1427,12 @@ describe NotificationService, :mailer do
     end
 
     describe '#new_project_member' do
-      let(:project) { create(:project) }
-      let(:added_user) { create(:user) }
-
-      def create_member!
-        create(:project_member, user: added_user, project: project)
-      end
-
       it do
         create_member!
         should_only_email(added_user)
       end
 
-      describe 'when notifications are disabled' do
+      context 'when notifications are disabled' do
         before do
           create_global_setting_for(added_user, :disabled)
         end
@@ -1398,6 +1443,10 @@ describe NotificationService, :mailer do
         end
       end
     end
+
+    def create_member!
+      create(:project_member, user: added_user, project: project)
+    end
   end
 
   context 'guest user in private project' do