summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
author🙈 jacopo beschi 🙉 <intrip@gmail.com>2018-02-19 17:47:08 +0000
committerRémy Coutable <remy@rymai.me>2018-02-19 17:47:08 +0000
commit80090c8b06859ac23fec4b69c88a0e854ca2db8e (patch)
treed2384ac96dce71a9afc989c8e898a6b713a34f11
parent9f06e3139803f0f20a0150bfe3ab13a06f2e3148 (diff)
downloadgitlab-ce-80090c8b06859ac23fec4b69c88a0e854ca2db8e.tar.gz
Resolve "group request membership mail with too long list of "To:""
-rw-r--r--app/mailers/emails/members.rb11
-rw-r--r--app/services/notification_service.rb17
-rw-r--r--changelogs/unreleased/42274-group-request-membership-long-too.yml5
-rw-r--r--spec/mailers/notify_spec.rb70
-rw-r--r--spec/services/notification_service_spec.rb89
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