Commit 80090c8b authored by 🙈  jacopo beschi 🙉's avatar 🙈 jacopo beschi 🙉 Committed by Rémy Coutable

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

parent 9f06e313
...@@ -7,18 +7,11 @@ module Emails ...@@ -7,18 +7,11 @@ module Emails
helper_method :member_source, :member helper_method :member_source, :member
end 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_source_type = member_source_type
@member_id = member_id @member_id = member_id
admins = member_source.members.owners_and_masters.pluck(:notification_email) mail(to: recipient_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,
subject: subject("Request to join the #{member_source.human_name} #{member_source.model_name.singular}")) subject: subject("Request to join the #{member_source.human_name} #{member_source.model_name.singular}"))
end end
......
...@@ -208,7 +208,12 @@ class NotificationService ...@@ -208,7 +208,12 @@ class NotificationService
def new_access_request(member) def new_access_request(member)
return true unless member.notifiable?(:subscription) 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 end
def decline_access_request(member) def decline_access_request(member)
...@@ -435,4 +440,14 @@ class NotificationService ...@@ -435,4 +440,14 @@ class NotificationService
def notifiable_users(*args) def notifiable_users(*args)
NotificationRecipientService.notifiable_users(*args) NotificationRecipientService.notifiable_users(*args)
end 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 end
---
title: Fix long list of recipients on group request membership email
merge_request: 17121
author: Jacopo Beschi @jacopo-beschi
type: fixed
...@@ -463,59 +463,30 @@ describe Notify do ...@@ -463,59 +463,30 @@ describe Notify do
end end
describe 'project access requested' do describe 'project access requested' do
context 'for a project in a user namespace' do let(:project) do
let(:project) do create(:project, :public, :access_requestable) do |project|
create(:project, :public, :access_requestable) do |project| project.add_master(project.owner)
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
end end
end end
context 'for a project in a group' do let(:project_member) do
let(:group_owner) { create(:user) } project.request_access(user)
let(:group) { create(:group).tap { |g| g.add_owner(group_owner) } } project.requesters.find_by(user_id: user.id)
let(:project) { create(:project, :public, :access_requestable, namespace: group) } end
let(:project_member) do subject { described_class.member_access_requested_email('project', project_member.id, recipient.notification_email) }
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 'an email sent from GitLab'
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
it 'contains all the useful information' do it 'contains all the useful information' do
to_emails = subject.header[:to].addrs to_emails = subject.header[:to].addrs.map(&:address)
expect(to_emails.size).to eq(1) expect(to_emails).to eq([recipient.notification_email])
expect(to_emails[0].address).to eq(group.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_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_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_project_members_url(project)
is_expected.to have_body_text project_member.human_access is_expected.to have_body_text project_member.human_access
end
end end
end end
...@@ -959,13 +930,16 @@ describe Notify do ...@@ -959,13 +930,16 @@ describe Notify do
group.request_access(user) group.request_access(user)
group.requesters.find_by(user_id: user.id) group.requesters.find_by(user_id: user.id)
end 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 'an email sent from GitLab'
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
it 'contains all the useful information' do 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_subject "Request to join the #{group.name} group"
is_expected.to have_html_escaped_body_text group.name is_expected.to have_html_escaped_body_text group.name
is_expected.to have_body_text group_group_members_url(group) is_expected.to have_body_text group_group_members_url(group)
......
...@@ -1307,6 +1307,33 @@ describe NotificationService, :mailer do ...@@ -1307,6 +1307,33 @@ describe NotificationService, :mailer do
end end
describe 'GroupMember' do 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 describe '#decline_group_invite' do
let(:creator) { create(:user) } let(:creator) { create(:user) }
let(:group) { create(:group) } let(:group) { create(:group) }
...@@ -1328,18 +1355,9 @@ describe NotificationService, :mailer do ...@@ -1328,18 +1355,9 @@ describe NotificationService, :mailer do
describe '#new_group_member' do describe '#new_group_member' do
let(:group) { create(:group) } 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 it 'sends a notification' do
create_member! group.add_guest(added_user)
should_only_email(added_user) should_only_email(added_user)
end end
...@@ -1349,7 +1367,7 @@ describe NotificationService, :mailer do ...@@ -1349,7 +1367,7 @@ describe NotificationService, :mailer do
end end
it 'does not send a notification' do it 'does not send a notification' do
create_member! group.add_guest(added_user)
should_not_email_anyone should_not_email_anyone
end end
end end
...@@ -1357,8 +1375,42 @@ describe NotificationService, :mailer do ...@@ -1357,8 +1375,42 @@ describe NotificationService, :mailer do
end end
describe 'ProjectMember' do 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 describe '#decline_group_invite' do
let(:project) { create(:project) }
let(:member) { create(:user) } let(:member) { create(:user) }
before do before do
...@@ -1375,19 +1427,12 @@ describe NotificationService, :mailer do ...@@ -1375,19 +1427,12 @@ describe NotificationService, :mailer do
end end
describe '#new_project_member' do 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 it do
create_member! create_member!
should_only_email(added_user) should_only_email(added_user)
end end
describe 'when notifications are disabled' do context 'when notifications are disabled' do
before do before do
create_global_setting_for(added_user, :disabled) create_global_setting_for(added_user, :disabled)
end end
...@@ -1398,6 +1443,10 @@ describe NotificationService, :mailer do ...@@ -1398,6 +1443,10 @@ describe NotificationService, :mailer do
end end
end end
end end
def create_member!
create(:project_member, user: added_user, project: project)
end
end end
context 'guest user in private project' do context 'guest user in private project' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment