From 80090c8b06859ac23fec4b69c88a0e854ca2db8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=99=88=20=20jacopo=20beschi=20=F0=9F=99=89?= Date: Mon, 19 Feb 2018 17:47:08 +0000 Subject: [PATCH] Resolve "group request membership mail with too long list of "To:"" --- app/mailers/emails/members.rb | 11 +-- app/services/notification_service.rb | 17 +++- ...2274-group-request-membership-long-too.yml | 5 ++ spec/mailers/notify_spec.rb | 70 +++++---------- spec/services/notification_service_spec.rb | 89 ++++++++++++++----- 5 files changed, 114 insertions(+), 78 deletions(-) create mode 100644 changelogs/unreleased/42274-group-request-membership-long-too.yml 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