Merge branch 'sh-limit-project-moved-emails' into 'master'

Limit project moved e-mails to maintainers/owners

See merge request gitlab-org/gitlab!36665

app/finders/members_finder.rb

@@ -29,7 +29,12 @@ class MembersFinder
 
   def find_members(include_relations)
     project_members = project.project_members
+
+    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
 

app/models/member.rb

@@ -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) }

app/services/notification_service.rb

@@ -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

changelogs/unreleased/sh-limit-project-moved-emails.yml

+---
+title: Limit project moved e-mails to maintainers/owners
+merge_request: 36665
+author:
+type: other

spec/finders/members_finder_spec.rb

@@ -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

spec/services/notification_service_spec.rb

@@ -2054,6 +2054,36 @@ RSpec.describe NotificationService, :mailer do
     end
 
     describe '#project_was_moved' do
+      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) }
+
+        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")
 
@@ -2064,6 +2094,14 @@ RSpec.describe NotificationService, :mailer do
           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