From b943baa42a5365bd6377022223f66db9af58da33 Mon Sep 17 00:00:00 2001 From: Manoj MJ Date: Fri, 30 Aug 2019 21:30:51 +0000 Subject: Limit access request email to 10 most recently active owners/maintainers This change limits the number of emails for new access requests notifications to 10 most recently active owners/maintainers --- app/models/group.rb | 6 + app/models/project.rb | 6 + app/services/notification_service.rb | 15 +- ...3-access-request-emails-limit-to-ten-owners.yml | 5 + doc/user/group/index.md | 7 +- doc/user/project/members/index.md | 11 +- spec/factories/group_members.rb | 4 + spec/factories/project_members.rb | 4 + spec/factories/users.rb | 8 ++ spec/models/group_spec.rb | 19 +++ spec/models/project_spec.rb | 20 +++ spec/services/notification_service_spec.rb | 152 ++++++++++++++++----- .../notification_service_shared_examples.rb | 44 ++++++ 13 files changed, 258 insertions(+), 43 deletions(-) create mode 100644 changelogs/unreleased/28643-access-request-emails-limit-to-ten-owners.yml diff --git a/app/models/group.rb b/app/models/group.rb index 61a4802a6ee..abe93cf3c84 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 @@ -429,6 +431,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 8f568a5b840..a6d203f1e72 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 @@ -2193,6 +2195,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 43721c531e4..86fb7533e70 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 -- cgit v1.2.1