Commit 583246df authored by Stan Hu's avatar Stan Hu Committed by Douglas Barbosa Alexandre

Limit project moved e-mails to maintainers/owners

Previously the "Project was moved" email notifications were sent every
member in the project. This served a useful purpose when we did not have
project redirects, so there was no way to discover that a project had
actually moved. However, these e-mails generate a significant amount of
noise, especially with projects with large number of team members.

To cut down on these notifications, we restrict the e-mails to
project/group maintainers and owners. Later we can introduce custom
notification settings to disable this.

Part of https://gitlab.com/gitlab-org/gitlab/-/issues/30371
parent 1047ae11
......@@ -29,7 +29,12 @@ class MembersFinder
def find_members(include_relations)
project_members = project.project_members
project_members = project_members.non_invite unless can?(current_user, :admin_project, project)
if params[:active_without_invites_and_requests].present?
project_members = project_members.active_without_invites_and_requests
else
project_members = project_members.non_invite unless can?(current_user, :admin_project, project)
end
return project_members if include_relations == [:direct]
......@@ -44,6 +49,7 @@ class MembersFinder
def filter_members(members)
members = members.search(params[:search]) if params[:search].present?
members = members.sort_by_attribute(params[:sort]) if params[:sort].present?
members = members.owners_and_maintainers if params[:owners_and_maintainers].present?
members
end
......
......@@ -86,6 +86,7 @@ class Member < ApplicationRecord
scope :owners, -> { active.where(access_level: OWNER) }
scope :owners_and_maintainers, -> { active.where(access_level: [OWNER, MAINTAINER]) }
scope :with_user, -> (user) { where(user: user) }
scope :preload_user_and_notification_settings, -> { preload(user: :notification_settings) }
scope :with_source_id, ->(source_id) { where(source_id: source_id) }
scope :including_source, -> { includes(:source) }
......
......@@ -424,7 +424,7 @@ class NotificationService
end
def project_was_moved(project, old_path_with_namespace)
recipients = project.private? ? project.team.members_in_project_and_ancestors : project.team.members
recipients = project_moved_recipients(project)
recipients = notifiable_users(recipients, :mention, project: project)
recipients.each do |recipient|
......@@ -705,6 +705,14 @@ class NotificationService
recipients
end
def project_moved_recipients(project)
finder = MembersFinder.new(project, nil, params: {
active_without_invites_and_requests: true,
owners_and_maintainers: true
})
finder.execute.preload_user_and_notification_settings.map(&:user)
end
def project_maintainers_recipients(target, action:)
NotificationRecipients::BuildService.build_project_maintainers_recipients(target, action: action)
end
......
---
title: Limit project moved e-mails to maintainers/owners
merge_request: 36665
author:
type: other
......@@ -10,16 +10,39 @@ RSpec.describe MembersFinder, '#execute' do
let_it_be(:user2) { create(:user) }
let_it_be(:user3) { create(:user) }
let_it_be(:user4) { create(:user) }
let_it_be(:blocked_user) { create(:user, :blocked) }
it 'returns members for project and parent groups' do
nested_group.request_access(user1)
member1 = group.add_maintainer(user2)
member2 = nested_group.add_maintainer(user3)
member3 = project.add_maintainer(user4)
blocked_member = project.add_maintainer(blocked_user)
result = described_class.new(project, user2).execute
expect(result).to contain_exactly(member1, member2, member3)
expect(result).to contain_exactly(member1, member2, member3, blocked_member)
end
it 'returns owners and maintainers' do
member1 = group.add_owner(user1)
group.add_developer(user2)
member3 = project.add_maintainer(user3)
project.add_developer(user4)
result = described_class.new(project, user2, params: { owners_and_maintainers: true }).execute
expect(result).to contain_exactly(member1, member3)
end
it 'returns active users and excludes invited users' do
member1 = project.add_maintainer(user2)
create(:project_member, :invited, project: project, invite_email: create(:user).email)
project.add_maintainer(blocked_user)
result = described_class.new(project, user2, params: { active_without_invites_and_requests: true }).execute
expect(result).to contain_exactly(member1)
end
it 'includes only non-invite members if user do not have amdin permissions on project' do
......
......@@ -2054,16 +2054,54 @@ RSpec.describe NotificationService, :mailer do
end
describe '#project_was_moved' do
it 'notifies the expected users' do
notification.project_was_moved(project, "gitlab/gitlab")
context 'with users at both project and group level' do
let(:maintainer) { create(:user) }
let(:developer) { create(:user) }
let(:group_owner) { create(:user) }
let(:group_maintainer) { create(:user) }
let(:group_developer) { create(:user) }
let(:blocked_user) { create(:user, :blocked) }
let(:invited_user) { create(:user) }
should_email(@u_watcher)
should_email(@u_participating)
should_email(@u_lazy_participant)
should_email(@u_custom_global)
should_not_email(@u_guest_watcher)
should_not_email(@u_guest_custom)
should_not_email(@u_disabled)
let!(:group) do
create(:group, :public) do |group|
project.group = group
project.save!
group.add_owner(group_owner)
group.add_maintainer(group_maintainer)
group.add_developer(group_developer)
# This is to check for dupes
group.add_maintainer(maintainer)
group.add_maintainer(blocked_user)
end
end
before do
project.add_maintainer(maintainer)
project.add_developer(developer)
project.add_maintainer(blocked_user)
reset_delivered_emails!
end
it 'notifies the expected users' do
notification.project_was_moved(project, "gitlab/gitlab")
should_email(@u_watcher)
should_email(@u_participating)
should_email(@u_lazy_participant)
should_email(@u_custom_global)
should_not_email(@u_guest_watcher)
should_not_email(@u_guest_custom)
should_not_email(@u_disabled)
should_email(maintainer)
should_email(group_owner)
should_email(group_maintainer)
should_not_email(group_developer)
should_not_email(developer)
should_not_email(blocked_user)
end
end
it_behaves_like 'project emails are disabled' 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