Limit access request email to 10 most recently active owners/maintainers28643-access-request-emails-limit-to-ten-owners

This change limits the number of emails
for new access requests notifications to
10 most recently active owners/maintainers

13 files changed, 258 insertions, 43 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 6c868b1d1f0..590791e98e2 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -15,6 +15,8 @@ class Group < Namespace
   include WithUploads
   include Gitlab::Utils::StrongMemoize
 
+  ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10
+
   has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
   alias_method :members, :group_members
   has_many :users, through: :group_members
@@ -427,6 +429,10 @@ class Group < Namespace
     super || ::Gitlab::Access::OWNER_SUBGROUP_ACCESS
   end
 
+  def access_request_approvers_to_be_notified
+    members.owners.order_recent_sign_in.limit(ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT)
+  end
+
   private
 
   def update_two_factor_requirement
diff --git a/app/models/project.rb b/app/models/project.rb
index 66d5286196e..ddd92d51d57 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -55,6 +55,8 @@ class Project < ApplicationRecord
   VALID_MIRROR_PORTS = [22, 80, 443].freeze
   VALID_MIRROR_PROTOCOLS = %w(http https ssh git).freeze
 
+  ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10
+
   SORTING_PREFERENCE_FIELD = :projects_sort
 
   cache_markdown_field :description, pipeline: :description
@@ -2190,6 +2192,10 @@ class Project < ApplicationRecord
     pool_repository.present?
   end
 
+  def access_request_approvers_to_be_notified
+    members.maintainers.order_recent_sign_in.limit(ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT)
+  end
+
   private
 
   def merge_requests_allowing_collaboration(source_branch = nil)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 83710ffce2f..5b8c1288854 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -293,11 +293,16 @@ class NotificationService
   def new_access_request(member)
     return true unless member.notifiable?(:subscription)
 
-    recipients = member.source.members.active_without_invites_and_requests.owners_and_maintainers
-    if fallback_to_group_owners_maintainers?(recipients, member)
-      recipients = member.source.group.members.active_without_invites_and_requests.owners_and_maintainers
+    source = member.source
+
+    recipients = source.access_request_approvers_to_be_notified
+
+    if fallback_to_group_access_request_approvers?(recipients, source)
+      recipients = source.group.access_request_approvers_to_be_notified
     end
 
+    return true if recipients.empty?
+
     recipients.each { |recipient| deliver_access_request_email(recipient, member) }
   end
 
@@ -611,9 +616,9 @@ class NotificationService
     mailer.member_access_requested_email(member.real_source_type, member.id, recipient.user.id).deliver_later
   end
 
-  def fallback_to_group_owners_maintainers?(recipients, member)
+  def fallback_to_group_access_request_approvers?(recipients, source)
     return false if recipients.present?
 
-    member.source.respond_to?(:group) && member.source.group
+    source.respond_to?(:group) && source.group
   end
 end
diff --git a/changelogs/unreleased/28643-access-request-emails-limit-to-ten-owners.yml b/changelogs/unreleased/28643-access-request-emails-limit-to-ten-owners.yml
new file mode 100644
index 00000000000..25e83eaf68e
--- /dev/null
+++ b/changelogs/unreleased/28643-access-request-emails-limit-to-ten-owners.yml
@@ -0,0 +1,5 @@
+---
+title: Limit access request emails to ten most recently active owners or maintainers
+merge_request: 32141
+author:
+type: changed
diff --git a/doc/user/group/index.md b/doc/user/group/index.md
index 864f1a397d5..1e16891aa7c 100644
--- a/doc/user/group/index.md
+++ b/doc/user/group/index.md
@@ -150,8 +150,11 @@ side of your screen.
 
 ![Request access button](img/request_access_button.png)
 
-Group owners and maintainers will be notified of your request and will be able to approve or
-decline it on the members page.
+Once access is requested:
+
+- Up to ten group owners are notified of your request via email.
+  Email is sent to the most recently active group owners.
+- Any group owner can approve or decline your request on the members page.
 
 ![Manage access requests](img/access_requests_management.png)
 
diff --git a/doc/user/project/members/index.md b/doc/user/project/members/index.md
index e343fd45488..21016dca358 100644
--- a/doc/user/project/members/index.md
+++ b/doc/user/project/members/index.md
@@ -79,8 +79,15 @@ side of your screen.
 
 ![Request access button](img/request_access_button.png)
 
-Project owners & maintainers will be notified of your request and will be able to approve or
-decline it on the members page.
+Once access is requested:
+
+- Up to ten project maintainers are notified of your request via email.
+  Email is sent to the most recently active project maintainers.
+- Any project maintainer can approve or decline your request on the members page.
+
+NOTE: **Note:**
+If a project does not have any maintainers, the notification is sent to the
+most recently active owners of the project's group.
 
 ![Manage access requests](img/access_requests_management.png)
 
diff --git a/spec/factories/group_members.rb b/spec/factories/group_members.rb
index 4c875935d82..a93f13395a2 100644
--- a/spec/factories/group_members.rb
+++ b/spec/factories/group_members.rb
@@ -24,5 +24,9 @@ FactoryBot.define do
     trait(:ldap) do
       ldap true
     end
+
+    trait :blocked do
+      after(:build) { |group_member, _| group_member.user.block! }
+    end
   end
 end
diff --git a/spec/factories/project_members.rb b/spec/factories/project_members.rb
index 6dcac0400ca..723fa6058fe 100644
--- a/spec/factories/project_members.rb
+++ b/spec/factories/project_members.rb
@@ -17,5 +17,9 @@ FactoryBot.define do
       invite_token 'xxx'
       invite_email 'email@email.com'
     end
+
+    trait :blocked do
+      after(:build) { |project_member, _| project_member.user.block! }
+    end
   end
 end
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index b2c8bdab013..57e58513529 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -39,6 +39,14 @@ FactoryBot.define do
       avatar { fixture_file_upload('spec/fixtures/dk.png') }
     end
 
+    trait :with_sign_ins do
+      sign_in_count 3
+      current_sign_in_at { Time.now }
+      last_sign_in_at { FFaker::Time.between(10.days.ago, 1.day.ago) }
+      current_sign_in_ip '127.0.0.1'
+      last_sign_in_ip '127.0.0.1'
+    end
+
     trait :two_factor_via_otp do
       before(:create) do |user|
         user.otp_required_for_login = true
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 1c41ceb7deb..796b6917fb2 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -1039,4 +1039,23 @@ describe Group do
         .to eq(Gitlab::Access::MAINTAINER_SUBGROUP_ACCESS)
     end
   end
+
+  describe '#access_request_approvers_to_be_notified' do
+    it 'returns a maximum of ten, active, non_requested owners of the group in recent_sign_in descending order' do
+      group = create(:group, :public)
+
+      users = create_list(:user, 12, :with_sign_ins)
+      active_owners = users.map do |user|
+        create(:group_member, :owner, group: group, user: user)
+      end
+
+      create(:group_member, :owner, :blocked, group: group)
+      create(:group_member, :maintainer, group: group)
+      create(:group_member, :access_request, :owner, group: group)
+
+      active_owners_in_recent_sign_in_desc_order = group.members_and_requesters.where(id: active_owners).order_recent_sign_in.limit(10)
+
+      expect(group.access_request_approvers_to_be_notified).to eq(active_owners_in_recent_sign_in_desc_order)
+    end
+  end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index bd352db2236..bfbcac60fea 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -4991,6 +4991,26 @@ describe Project do
     end
   end
 
+  describe '#access_request_approvers_to_be_notified' do
+    it 'returns a maximum of ten, active, non_requested maintainers of the project in recent_sign_in descending order' do
+      group = create(:group, :public)
+      project = create(:project, group: group)
+
+      users = create_list(:user, 12, :with_sign_ins)
+      active_maintainers = users.map do |user|
+        create(:project_member, :maintainer, user: user)
+      end
+
+      create(:project_member, :maintainer, :blocked, project: project)
+      create(:project_member, :developer, project: project)
+      create(:project_member, :access_request, :maintainer, project: project)
+
+      active_maintainers_in_recent_sign_in_desc_order = project.members_and_requesters.where(id: active_maintainers).order_recent_sign_in.limit(10)
+
+      expect(project.access_request_approvers_to_be_notified).to eq(active_maintainers_in_recent_sign_in_desc_order)
+    end
+  end
+
   def rugged_config
     rugged_repo(project.repository).config
   end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index d925aa2b6c3..ab0e01e27d7 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -1932,31 +1932,39 @@ describe NotificationService, :mailer do
     let(:added_user) { create(:user) }
 
     describe '#new_access_request' do
-      let(:maintainer) { 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_maintainer(maintainer)
-          group.add_developer(developer)
+      context 'recipients' do
+        let(:maintainer) { 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_maintainer(maintainer)
+            group.add_developer(developer)
+          end
         end
-      end
 
-      before do
-        reset_delivered_emails!
-      end
+        before do
+          reset_delivered_emails!
+        end
 
-      it 'sends notification to group owners_and_maintainers' do
-        group.request_access(added_user)
+        it 'sends notification only to group owners' do
+          group.request_access(added_user)
+
+          should_email(owner)
+          should_not_email(maintainer)
+          should_not_email(developer)
+        end
 
-        should_email(owner)
-        should_email(maintainer)
-        should_not_email(developer)
+        it_behaves_like 'group emails are disabled' do
+          let(:notification_target)  { group }
+          let(:notification_trigger) { group.request_access(added_user) }
+        end
       end
 
-      it_behaves_like 'group emails are disabled' do
-        let(:notification_target)  { group }
+      it_behaves_like 'sends notification only to a maximum of ten, most recently active group owners' do
+        let(:group) { create(:group, :public, :access_requestable) }
         let(:notification_trigger) { group.request_access(added_user) }
       end
     end
@@ -2012,20 +2020,36 @@ describe NotificationService, :mailer do
 
     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_maintainer(project.owner)
+        context 'recipients' do
+          let(:developer) { create(:user) }
+          let(:maintainer) { create(:user) }
+
+          let!(:project) do
+            create(:project, :public, :access_requestable) do |project|
+              project.add_developer(developer)
+              project.add_maintainer(maintainer)
+            end
           end
-        end
 
-        it 'sends notification to project owners_and_maintainers' do
-          project.request_access(added_user)
+          before do
+            reset_delivered_emails!
+          end
+
+          it 'sends notification only to project maintainers' do
+            project.request_access(added_user)
+
+            should_email(maintainer)
+            should_not_email(developer)
+          end
 
-          should_only_email(project.owner)
+          it_behaves_like 'project emails are disabled' do
+            let(:notification_target)  { project }
+            let(:notification_trigger) { project.request_access(added_user) }
+          end
         end
 
-        it_behaves_like 'project emails are disabled' do
-          let(:notification_target)  { project }
+        it_behaves_like 'sends notification only to a maximum of ten, most recently active project maintainers' do
+          let(:project) { create(:project, :public, :access_requestable) }
           let(:notification_trigger) { project.request_access(added_user) }
         end
       end
@@ -2033,16 +2057,76 @@ describe NotificationService, :mailer do
       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!
+        context 'when the project has no maintainers' do
+          context 'when the group has at least one owner' do
+            let!(:project) { create(:project, :public, :access_requestable, namespace: group) }
+
+            before do
+              reset_delivered_emails!
+            end
+
+            context 'recipients' do
+              it 'sends notifications to the group owners' do
+                project.request_access(added_user)
+
+                should_only_email(group_owner)
+              end
+            end
+
+            it_behaves_like 'sends notification only to a maximum of ten, most recently active group owners' do
+              let(:group) { create(:group, :public, :access_requestable) }
+              let(:notification_trigger) { project.request_access(added_user) }
+            end
+          end
+
+          context 'when the group does not have any owners' do
+            let(:group) { create(:group) }
+            let!(:project) { create(:project, :public, :access_requestable, namespace: group) }
+
+            context 'recipients' do
+              before do
+                reset_delivered_emails!
+              end
+
+              it 'does not send any notifications' do
+                project.request_access(added_user)
+
+                should_not_email_anyone
+              end
+            end
+          end
         end
 
-        it 'sends notification to group owners_and_maintainers' do
-          project.request_access(added_user)
+        context 'when the project has maintainers' do
+          let(:maintainer) { create(:user) }
+          let(:developer) { create(:user) }
+
+          let!(:project) do
+            create(:project, :public, :access_requestable, namespace: group) do |project|
+              project.add_maintainer(maintainer)
+              project.add_developer(developer)
+            end
+          end
+
+          before do
+            reset_delivered_emails!
+          end
+
+          context 'recipients' do
+            it 'sends notifications only to project maintainers' do
+              project.request_access(added_user)
 
-          should_only_email(group_owner)
+              should_email(maintainer)
+              should_not_email(developer)
+              should_not_email(group_owner)
+            end
+          end
+
+          it_behaves_like 'sends notification only to a maximum of ten, most recently active project maintainers' do
+            let(:project) { create(:project, :public, :access_requestable, namespace: group) }
+            let(:notification_trigger) { project.request_access(added_user) }
+          end
         end
       end
     end
diff --git a/spec/support/shared_examples/services/notification_service_shared_examples.rb b/spec/support/shared_examples/services/notification_service_shared_examples.rb
index dd338ea47c7..ad580b581d6 100644
--- a/spec/support/shared_examples/services/notification_service_shared_examples.rb
+++ b/spec/support/shared_examples/services/notification_service_shared_examples.rb
@@ -52,3 +52,47 @@ shared_examples 'group emails are disabled' do
     should_email_anyone
   end
 end
+
+shared_examples 'sends notification only to a maximum of ten, most recently active group owners' do
+  let(:owners) { create_list(:user, 12, :with_sign_ins) }
+
+  before do
+    owners.each do |owner|
+      group.add_owner(owner)
+    end
+
+    reset_delivered_emails!
+  end
+
+  context 'limit notification emails' do
+    it 'sends notification only to a maximum of ten, most recently active group owners' do
+      ten_most_recently_active_group_owners = owners.sort_by(&:last_sign_in_at).last(10)
+
+      notification_trigger
+
+      should_only_email(*ten_most_recently_active_group_owners)
+    end
+  end
+end
+
+shared_examples 'sends notification only to a maximum of ten, most recently active project maintainers' do
+  let(:maintainers) { create_list(:user, 12, :with_sign_ins) }
+
+  before do
+    maintainers.each do |maintainer|
+      project.add_maintainer(maintainer)
+    end
+
+    reset_delivered_emails!
+  end
+
+  context 'limit notification emails' do
+    it 'sends notification only to a maximum of ten, most recently active project maintainers' do
+      ten_most_recently_active_project_maintainers = maintainers.sort_by(&:last_sign_in_at).last(10)
+
+      notification_trigger
+
+      should_only_email(*ten_most_recently_active_project_maintainers)
+    end
+  end
+end