Commit 29fdab0a authored by Mayra Cabrera's avatar Mayra Cabrera

Merge branch '28643-access-request-emails-limit-to-ten-owners' into 'master'

Resolve "Limit access request emails to ten most recently active owners/maintainers"

See merge request gitlab-org/gitlab-ce!32141
parents 663b7bb4 b943baa4
......@@ -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
......
......@@ -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)
......
......@@ -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
---
title: Limit access request emails to ten most recently active owners or maintainers
merge_request: 32141
author:
type: changed
......@@ -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)
......
......@@ -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)
......
......@@ -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
......@@ -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
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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
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