From 1706860d39b2e58158fb7a4a40d3dafaab55166f Mon Sep 17 00:00:00 2001 From: smriti Date: Tue, 4 Jun 2024 22:34:21 +0530 Subject: [PATCH 01/10] Changes for project owners to receive email notifications Currently on token expiration mails are sent to only project maintainers. With this change even project owners will receive notification for upcoming token expiration Changelog: changed --- app/mailers/emails/profile.rb | 2 +- app/models/concerns/has_user_type.rb | 4 ++-- app/models/project.rb | 8 ++++++++ app/services/notification_service.rb | 2 +- locale/gitlab.pot | 4 ++-- spec/models/concerns/has_user_type_spec.rb | 20 ++++++++++++++------ spec/models/project_spec.rb | 18 ++++++++++++++++++ spec/services/notification_service_spec.rb | 10 +++++----- 8 files changed, 51 insertions(+), 17 deletions(-) diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 56f884bcbc2938..e35eccb7b66015 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -71,7 +71,7 @@ def bot_resource_access_token_about_to_expire_email(recipient, resource, token_n @reason_text = _('You are receiving this email because you are an Owner of the Group.') else @target_url = project_settings_access_tokens_url(resource) - @reason_text = _('You are receiving this email because you are a Maintainer of the Project.') + @reason_text = _('You are receiving this email because you are either an Owner or Maintainer of the Project.') end mail_with_locale( diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index 6c272921351cba..09e02384deb2be 100644 --- a/app/models/concerns/has_user_type.rb +++ b/app/models/concerns/has_user_type.rb @@ -82,13 +82,13 @@ def resource_bot_resource projects&.first || groups&.first end - def resource_bot_owners + def resource_bot_notification_recipients return [] unless project_bot? resource = resource_bot_resource return [] unless resource - return resource.maintainers if resource.is_a?(Project) + return resource.owners_and_maintainers if resource.is_a?(Project) resource .owners diff --git a/app/models/project.rb b/app/models/project.rb index 4065039d09a71c..e8972e914d2b00 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -378,6 +378,14 @@ def self.integration_association_name(name) through: :project_members, source: :user + has_many :owners_and_maintainers, + -> do + allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") + .where(members: { access_level: [Gitlab::Access::OWNER, Gitlab::Access::MAINTAINER] }) + end, + through: :project_members, + source: :user + has_many :project_callouts, class_name: 'Users::ProjectCallout', foreign_key: :project_id has_many :deploy_keys_projects, inverse_of: :project diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index d33db5c7b34ca8..39fd1445fada95 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -76,7 +76,7 @@ def new_gpg_key(gpg_key) end def bot_resource_access_token_about_to_expire(bot_user, token_name) - recipients = bot_user.resource_bot_owners.select { |owner| owner.can?(:receive_notifications) } + recipients = bot_user.resource_bot_notification_recipients.select { |user| user.can?(:receive_notifications) } resource = bot_user.resource_bot_resource recipients.each do |recipient| diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7853466836552d..60d2dd6de22339 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -60283,10 +60283,10 @@ msgstr "" msgid "You are on a read-only GitLab instance." msgstr "" -msgid "You are receiving this email because you are a Maintainer of the Project." +msgid "You are receiving this email because you are an Owner of the Group." msgstr "" -msgid "You are receiving this email because you are an Owner of the Group." +msgid "You are receiving this email because you are either an Owner or Maintainer of the Project." msgstr "" msgid "You are receiving this message because you are a GitLab administrator for %{url}." diff --git a/spec/models/concerns/has_user_type_spec.rb b/spec/models/concerns/has_user_type_spec.rb index e9f8e0a487cbbd..124375978653e7 100644 --- a/spec/models/concerns/has_user_type_spec.rb +++ b/spec/models/concerns/has_user_type_spec.rb @@ -152,7 +152,7 @@ end end - describe 'resource_bot_owners' do + describe 'resource_bot_notification_recipients' do it 'returns nil when user is not a project bot' do expect(human.resource_bot_resource).to be_nil end @@ -161,10 +161,10 @@ let(:user1) { create(:user) } let(:user2) { create(:user) } - subject(:owners) { project_bot.resource_bot_owners } + subject(:notification_recipients) { project_bot.resource_bot_notification_recipients } it 'returns an empty array when there is no owning resource' do - expect(owners).to match_array([]) + expect(notification_recipients).to match_array([]) end it 'returns group owners when owned by a group' do @@ -172,15 +172,23 @@ group.add_developer(project_bot) group.add_owner(user1) - expect(owners).to match_array([user1]) + expect(notification_recipients).to match_array([user1]) end - it 'returns project maintainers when owned by a project' do + it 'returns project owners and maintainers when owned by a project' do project = create(:project) project.add_developer(project_bot) project.add_maintainer(user2) - expect(owners).to match_array([user2]) + expect(notification_recipients).to match_array([project.owner, user2]) + end + + it 'does not returns any other role than owner or maintainer' do + project = create(:project) + project.add_developer(project_bot) + project.add_maintainer(user2) + + expect(notification_recipients).not_to include(project_bot) end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 37783346a27891..ebab9edf41d3d7 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -32,6 +32,7 @@ it { is_expected.to belong_to(:pool_repository) } it { is_expected.to have_many(:users) } it { is_expected.to have_many(:maintainers).through(:project_members).source(:user).conditions(members: { access_level: Gitlab::Access::MAINTAINER }) } + it { is_expected.to have_many(:owners_and_maintainers).through(:project_members).source(:user).conditions(members: { access_level: Gitlab::Access::MAINTAINER }) } it { is_expected.to have_many(:events) } it { is_expected.to have_many(:merge_requests) } it { is_expected.to have_many(:merge_request_metrics).class_name('MergeRequest::Metrics') } @@ -255,6 +256,23 @@ end end + describe 'owners_and_maintainers association' do + let_it_be(:project) { create(:project) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:developer) { create(:user) } + + before do + project.add_maintainer(maintainer) + project.add_developer(developer) + project.add_reporter(reporter) + end + + it 'returns only maintainers and owners' do + expect(project.owners_and_maintainers).to match_array([maintainer, project.owner]) + end + end + context 'after initialized' do it "has a project_feature" do expect(described_class.new.project_feature).to be_present diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index ad740c1b7dcd48..69004c9e17f037 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -382,6 +382,7 @@ let_it_be(:owner1) { create(:user) } let_it_be(:owner2) { create(:user) } + let_it_be(:maintainer) { create(:user) } subject(:notification_service) do notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token.name]) @@ -419,21 +420,20 @@ let(:project) { create(:project) } before do - project.add_maintainer(owner1) - project.add_maintainer(owner2) + project.add_maintainer(maintainer) project.add_reporter(project_bot) end - it 'sends emails to the group owners' do + it 'sends emails to the project maintainers and owners' do expect { notification_service }.to( have_enqueued_email( - owner1, + maintainer, project_bot.resource_bot_resource, [expiring_token.name], mail: "bot_resource_access_token_about_to_expire_email" ).and( have_enqueued_email( - owner2, + project.owner, project_bot.resource_bot_resource, [expiring_token.name], mail: "bot_resource_access_token_about_to_expire_email" -- GitLab From 6536e2087613191552e9bab11266cc6c750ba33b Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 5 Jun 2024 13:54:11 +0530 Subject: [PATCH 02/10] Fixed rspec failure --- spec/mailers/emails/profile_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index 92fd507c493472..d3c2fd2074d6be 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -224,7 +224,7 @@ it_behaves_like 'resource about to expire email' it 'includes the email reason' do - is_expected.to have_body_text _('You are receiving this email because you are a Maintainer of the Project.') + is_expected.to have_body_text _('You are receiving this email because you are either an Owner or Maintainer of the Project.') end end end -- GitLab From b6bfc61491a4759eb10d3e5bc431c89d0e848864 Mon Sep 17 00:00:00 2001 From: smriti Date: Fri, 7 Jun 2024 14:30:03 +0530 Subject: [PATCH 03/10] Renamed method and added spec --- app/models/concerns/has_user_type.rb | 2 +- app/models/project.rb | 4 +- app/services/notification_service.rb | 2 +- spec/models/concerns/has_user_type_spec.rb | 4 +- spec/services/notification_service_spec.rb | 43 ++++++++++++++++++++++ 5 files changed, 49 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index 09e02384deb2be..90820781829893 100644 --- a/app/models/concerns/has_user_type.rb +++ b/app/models/concerns/has_user_type.rb @@ -82,7 +82,7 @@ def resource_bot_resource projects&.first || groups&.first end - def resource_bot_notification_recipients + def resource_bot_owners_and_maintainers return [] unless project_bot? resource = resource_bot_resource diff --git a/app/models/project.rb b/app/models/project.rb index e8972e914d2b00..0e1c0d1c58937e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -370,6 +370,7 @@ def self.integration_association_name(name) has_many :users, -> { allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") }, through: :project_members + has_many :maintainers, -> do allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") @@ -380,8 +381,7 @@ def self.integration_association_name(name) has_many :owners_and_maintainers, -> do - allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") - .where(members: { access_level: [Gitlab::Access::OWNER, Gitlab::Access::MAINTAINER] }) + where(members: { access_level: [Gitlab::Access::OWNER, Gitlab::Access::MAINTAINER] }) end, through: :project_members, source: :user diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 39fd1445fada95..e1154452f5325e 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -76,7 +76,7 @@ def new_gpg_key(gpg_key) end def bot_resource_access_token_about_to_expire(bot_user, token_name) - recipients = bot_user.resource_bot_notification_recipients.select { |user| user.can?(:receive_notifications) } + recipients = bot_user.resource_bot_owners_and_maintainers.select { |user| user.can?(:receive_notifications) } resource = bot_user.resource_bot_resource recipients.each do |recipient| diff --git a/spec/models/concerns/has_user_type_spec.rb b/spec/models/concerns/has_user_type_spec.rb index 124375978653e7..a378d5f635a3f8 100644 --- a/spec/models/concerns/has_user_type_spec.rb +++ b/spec/models/concerns/has_user_type_spec.rb @@ -152,7 +152,7 @@ end end - describe 'resource_bot_notification_recipients' do + describe 'resource_bot_owners_and_maintainers' do it 'returns nil when user is not a project bot' do expect(human.resource_bot_resource).to be_nil end @@ -161,7 +161,7 @@ let(:user1) { create(:user) } let(:user2) { create(:user) } - subject(:notification_recipients) { project_bot.resource_bot_notification_recipients } + subject(:notification_recipients) { project_bot.resource_bot_owners_and_maintainers } it 'returns an empty array when there is no owning resource' do expect(notification_recipients).to match_array([]) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 69004c9e17f037..49dfd5852cfc81 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -383,6 +383,8 @@ let_it_be(:owner1) { create(:user) } let_it_be(:owner2) { create(:user) } let_it_be(:maintainer) { create(:user) } + let_it_be(:inherited_user) { create(:user, username: 'Bob', name: 'Bob') } + let_it_be(:parent_group) { create(:group) } subject(:notification_service) do notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token.name]) @@ -414,6 +416,47 @@ ) ) end + + context 'when group has inherited members' do + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:group) { create(:group, parent: parent_group) } + let_it_be(:group_owner) { create(:user) } + let_it_be(:parent_group_owner) { create(:user) } + let_it_be(:expiring_token_1) { create(:personal_access_token, user: group_owner, expires_at: 5.days.from_now) } + let_it_be(:expiring_token_2) { create(:personal_access_token, user: parent_group_owner, expires_at: 5.days.from_now) } + + subject(:notification_service) do + notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token_1.name, expiring_token_2.name]) + end + + before do + parent_group.add_owner(parent_group_owner) + group.add_owner(group_owner) + group.add_reporter(project_bot) + end + + it 'does not send email to inherited members' do + expect { notification_service }.not_to( + have_enqueued_email( + parent_group_owner, + project_bot.resource_bot_resource, + [expiring_token_1.name, expiring_token_2.name], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + end + + it 'sends email to direct members' do + expect { notification_service }.to( + have_enqueued_email( + group_owner, + project_bot.resource_bot_resource, + [expiring_token_1.name, expiring_token_2.name], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + end + end end context 'when the resource is a project' do -- GitLab From a9eef2d6822d52cb95c05fcf570455e159fc60e9 Mon Sep 17 00:00:00 2001 From: smriti Date: Mon, 10 Jun 2024 13:10:37 +0530 Subject: [PATCH 04/10] Used MembersFinder to get list of users --- app/models/concerns/has_user_type.rb | 5 ++++- app/models/project.rb | 8 -------- spec/models/project_spec.rb | 18 ------------------ 3 files changed, 4 insertions(+), 27 deletions(-) diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index 90820781829893..fce1e49c9a597a 100644 --- a/app/models/concerns/has_user_type.rb +++ b/app/models/concerns/has_user_type.rb @@ -88,7 +88,10 @@ def resource_bot_owners_and_maintainers resource = resource_bot_resource return [] unless resource - return resource.owners_and_maintainers if resource.is_a?(Project) + if resource.is_a?(Project) + return MembersFinder.new(resource, nil, params: { owners_and_maintainers: true }) + .execute(include_relations: [:direct]).map(&:user) + end resource .owners diff --git a/app/models/project.rb b/app/models/project.rb index 0e1c0d1c58937e..4065039d09a71c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -370,7 +370,6 @@ def self.integration_association_name(name) has_many :users, -> { allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") }, through: :project_members - has_many :maintainers, -> do allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") @@ -379,13 +378,6 @@ def self.integration_association_name(name) through: :project_members, source: :user - has_many :owners_and_maintainers, - -> do - where(members: { access_level: [Gitlab::Access::OWNER, Gitlab::Access::MAINTAINER] }) - end, - through: :project_members, - source: :user - has_many :project_callouts, class_name: 'Users::ProjectCallout', foreign_key: :project_id has_many :deploy_keys_projects, inverse_of: :project diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index ebab9edf41d3d7..37783346a27891 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -32,7 +32,6 @@ it { is_expected.to belong_to(:pool_repository) } it { is_expected.to have_many(:users) } it { is_expected.to have_many(:maintainers).through(:project_members).source(:user).conditions(members: { access_level: Gitlab::Access::MAINTAINER }) } - it { is_expected.to have_many(:owners_and_maintainers).through(:project_members).source(:user).conditions(members: { access_level: Gitlab::Access::MAINTAINER }) } it { is_expected.to have_many(:events) } it { is_expected.to have_many(:merge_requests) } it { is_expected.to have_many(:merge_request_metrics).class_name('MergeRequest::Metrics') } @@ -256,23 +255,6 @@ end end - describe 'owners_and_maintainers association' do - let_it_be(:project) { create(:project) } - let_it_be(:maintainer) { create(:user) } - let_it_be(:reporter) { create(:user) } - let_it_be(:developer) { create(:user) } - - before do - project.add_maintainer(maintainer) - project.add_developer(developer) - project.add_reporter(reporter) - end - - it 'returns only maintainers and owners' do - expect(project.owners_and_maintainers).to match_array([maintainer, project.owner]) - end - end - context 'after initialized' do it "has a project_feature" do expect(described_class.new.project_feature).to be_present -- GitLab From d5ab9dfeb8b5df2ec1788a495f96887a4cc88bee Mon Sep 17 00:00:00 2001 From: smriti Date: Tue, 11 Jun 2024 12:24:47 +0530 Subject: [PATCH 05/10] Docs update --- doc/security/token_overview.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/security/token_overview.md b/doc/security/token_overview.md index b987cf9fe806f0..3691ffa2a1dad0 100644 --- a/doc/security/token_overview.md +++ b/doc/security/token_overview.md @@ -59,7 +59,7 @@ You can use the [project access tokens API](../api/project_access_tokens.md) to programmatically take action, such as [rotating a project access token](../api/project_access_tokens.md#rotate-a-project-access-token). -Project maintainers with a direct membership receive an email when project access tokens are 7 days or less from expiration. Inherited members do not receive an email. +Project Owners and Maintainers with a direct membership receive an email when project access tokens are seven days or less from expiration. Inherited members do not receive an email. ## Group access tokens -- GitLab From 1e3a3ce25b9ba9184180505e3182b15db3f24333 Mon Sep 17 00:00:00 2001 From: Smriti Garg Date: Tue, 11 Jun 2024 08:02:49 +0000 Subject: [PATCH 06/10] Doc suggestions applied Missed spec added --- app/mailers/emails/profile.rb | 2 +- locale/gitlab.pot | 4 +- spec/mailers/emails/profile_spec.rb | 2 +- spec/models/concerns/has_user_type_spec.rb | 10 +-- spec/services/notification_service_spec.rb | 72 +++++++++++++++------- 5 files changed, 60 insertions(+), 30 deletions(-) diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index e35eccb7b66015..8a0d1ea2638b6f 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -71,7 +71,7 @@ def bot_resource_access_token_about_to_expire_email(recipient, resource, token_n @reason_text = _('You are receiving this email because you are an Owner of the Group.') else @target_url = project_settings_access_tokens_url(resource) - @reason_text = _('You are receiving this email because you are either an Owner or Maintainer of the Project.') + @reason_text = _('You are receiving this email because you are either an Owner or Maintainer of the project.') end mail_with_locale( diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 60d2dd6de22339..0debdfc37d82c3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -60283,10 +60283,10 @@ msgstr "" msgid "You are on a read-only GitLab instance." msgstr "" -msgid "You are receiving this email because you are an Owner of the Group." +msgid "You are receiving this email because you are an Owner of the group." msgstr "" -msgid "You are receiving this email because you are either an Owner or Maintainer of the Project." +msgid "You are receiving this email because you are either an Owner or Maintainer of the project." msgstr "" msgid "You are receiving this message because you are a GitLab administrator for %{url}." diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb index d3c2fd2074d6be..0b70a7693ae2be 100644 --- a/spec/mailers/emails/profile_spec.rb +++ b/spec/mailers/emails/profile_spec.rb @@ -224,7 +224,7 @@ it_behaves_like 'resource about to expire email' it 'includes the email reason' do - is_expected.to have_body_text _('You are receiving this email because you are either an Owner or Maintainer of the Project.') + is_expected.to have_body_text _('You are receiving this email because you are either an Owner or Maintainer of the project.') end end end diff --git a/spec/models/concerns/has_user_type_spec.rb b/spec/models/concerns/has_user_type_spec.rb index a378d5f635a3f8..630212aa8580d3 100644 --- a/spec/models/concerns/has_user_type_spec.rb +++ b/spec/models/concerns/has_user_type_spec.rb @@ -161,10 +161,10 @@ let(:user1) { create(:user) } let(:user2) { create(:user) } - subject(:notification_recipients) { project_bot.resource_bot_owners_and_maintainers } + subject(:owners_and_maintainers) { project_bot.resource_bot_owners_and_maintainers } it 'returns an empty array when there is no owning resource' do - expect(notification_recipients).to match_array([]) + expect(owners_and_maintainers).to match_array([]) end it 'returns group owners when owned by a group' do @@ -172,7 +172,7 @@ group.add_developer(project_bot) group.add_owner(user1) - expect(notification_recipients).to match_array([user1]) + expect(owners_and_maintainers).to match_array([user1]) end it 'returns project owners and maintainers when owned by a project' do @@ -180,7 +180,7 @@ project.add_developer(project_bot) project.add_maintainer(user2) - expect(notification_recipients).to match_array([project.owner, user2]) + expect(owners_and_maintainers).to match_array([project.owner, user2]) end it 'does not returns any other role than owner or maintainer' do @@ -188,7 +188,7 @@ project.add_developer(project_bot) project.add_maintainer(user2) - expect(notification_recipients).not_to include(project_bot) + expect(owners_and_maintainers).not_to include(project_bot) end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 49dfd5852cfc81..92d2c3c2c06bb4 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -383,16 +383,14 @@ let_it_be(:owner1) { create(:user) } let_it_be(:owner2) { create(:user) } let_it_be(:maintainer) { create(:user) } - let_it_be(:inherited_user) { create(:user, username: 'Bob', name: 'Bob') } let_it_be(:parent_group) { create(:group) } + let(:group) { create(:group) } subject(:notification_service) do notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token.name]) end context 'when the resource is a group' do - let(:group) { create(:group) } - before do group.add_owner(owner1) group.add_owner(owner2) @@ -417,39 +415,37 @@ ) end + it 'does not send an email to group maintainer' do + expect { notification_service }.not_to( + have_enqueued_email( + maintainer, + project_bot.resource_bot_resource, + [expiring_token.name], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + end + context 'when group has inherited members' do let_it_be(:project_bot) { create(:user, :project_bot) } let_it_be(:group) { create(:group, parent: parent_group) } - let_it_be(:group_owner) { create(:user) } - let_it_be(:parent_group_owner) { create(:user) } - let_it_be(:expiring_token_1) { create(:personal_access_token, user: group_owner, expires_at: 5.days.from_now) } - let_it_be(:expiring_token_2) { create(:personal_access_token, user: parent_group_owner, expires_at: 5.days.from_now) } + let_it_be(:expiring_token_1) { create(:personal_access_token, user: owner1, expires_at: 5.days.from_now) } + let_it_be(:expiring_token_2) { create(:personal_access_token, user: owner2, expires_at: 5.days.from_now) } subject(:notification_service) do notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token_1.name, expiring_token_2.name]) end before do - parent_group.add_owner(parent_group_owner) - group.add_owner(group_owner) + parent_group.add_owner(owner2) + group.add_owner(owner1) group.add_reporter(project_bot) end - it 'does not send email to inherited members' do - expect { notification_service }.not_to( - have_enqueued_email( - parent_group_owner, - project_bot.resource_bot_resource, - [expiring_token_1.name, expiring_token_2.name], - mail: "bot_resource_access_token_about_to_expire_email" - ) - ) - end - it 'sends email to direct members' do expect { notification_service }.to( have_enqueued_email( - group_owner, + owner1, project_bot.resource_bot_resource, [expiring_token_1.name, expiring_token_2.name], mail: "bot_resource_access_token_about_to_expire_email" @@ -484,6 +480,40 @@ ) ) end + + context 'when project has inherited members' do + let(:group) { create(:group) } + let(:project) { create(:project, namespace: group) } + + before do + project.add_reporter(project_bot) + project.add_maintainer(maintainer) + project.save! + + group.add_owner(owner1) + group.save! + end + + it 'does not send email to inherited members' do + expect { notification_service }.to( + have_enqueued_email( + maintainer, + project_bot.resource_bot_resource, + [expiring_token.name], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + + expect { notification_service }.not_to( + have_enqueued_email( + owner1, + project_bot.resource_bot_resource, + [expiring_token.name], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) + end + end end end -- GitLab From 91689e2f314e92224fedc15bc166d51c2befe7c2 Mon Sep 17 00:00:00 2001 From: smriti Date: Tue, 11 Jun 2024 15:49:12 +0530 Subject: [PATCH 07/10] Rspec failures resolved Removing non relevant code Removing non relevant code --- app/models/concerns/has_user_type.rb | 5 +---- app/models/project.rb | 8 ++++++++ locale/gitlab.pot | 2 +- spec/models/project_spec.rb | 18 ++++++++++++++++++ 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/app/models/concerns/has_user_type.rb b/app/models/concerns/has_user_type.rb index fce1e49c9a597a..90820781829893 100644 --- a/app/models/concerns/has_user_type.rb +++ b/app/models/concerns/has_user_type.rb @@ -88,10 +88,7 @@ def resource_bot_owners_and_maintainers resource = resource_bot_resource return [] unless resource - if resource.is_a?(Project) - return MembersFinder.new(resource, nil, params: { owners_and_maintainers: true }) - .execute(include_relations: [:direct]).map(&:user) - end + return resource.owners_and_maintainers if resource.is_a?(Project) resource .owners diff --git a/app/models/project.rb b/app/models/project.rb index 4065039d09a71c..0e1c0d1c58937e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -370,6 +370,7 @@ def self.integration_association_name(name) has_many :users, -> { allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") }, through: :project_members + has_many :maintainers, -> do allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/422405") @@ -378,6 +379,13 @@ def self.integration_association_name(name) through: :project_members, source: :user + has_many :owners_and_maintainers, + -> do + where(members: { access_level: [Gitlab::Access::OWNER, Gitlab::Access::MAINTAINER] }) + end, + through: :project_members, + source: :user + has_many :project_callouts, class_name: 'Users::ProjectCallout', foreign_key: :project_id has_many :deploy_keys_projects, inverse_of: :project diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0debdfc37d82c3..b81f959069b124 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -60283,7 +60283,7 @@ msgstr "" msgid "You are on a read-only GitLab instance." msgstr "" -msgid "You are receiving this email because you are an Owner of the group." +msgid "You are receiving this email because you are an Owner of the Group." msgstr "" msgid "You are receiving this email because you are either an Owner or Maintainer of the project." diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 37783346a27891..8e0093b97ad8a4 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -32,6 +32,7 @@ it { is_expected.to belong_to(:pool_repository) } it { is_expected.to have_many(:users) } it { is_expected.to have_many(:maintainers).through(:project_members).source(:user).conditions(members: { access_level: Gitlab::Access::MAINTAINER }) } + it { is_expected.to have_many(:owners_and_maintainers).through(:project_members).source(:user).conditions(members: { access_level: Gitlab::Access::MAINTAINER }) } it { is_expected.to have_many(:events) } it { is_expected.to have_many(:merge_requests) } it { is_expected.to have_many(:merge_request_metrics).class_name('MergeRequest::Metrics') } @@ -261,6 +262,23 @@ end end + describe 'owners_and_maintainers association' do + let_it_be(:project) { create(:project) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:developer) { create(:user) } + + before do + project.add_maintainer(maintainer) + project.add_developer(developer) + project.add_reporter(reporter) + end + + it 'returns only maintainers and owners' do + expect(project.owners_and_maintainers).to match_array([maintainer, project.owner]) + end + end + context 'when deleting project' do # using delete rather than destroy due to `delete` skipping AR hooks/callbacks # so it's ensured to work at the DB level. Uses AFTER DELETE trigger. -- GitLab From f99164f1686076d96ee5eb4882d3df34effec5b1 Mon Sep 17 00:00:00 2001 From: smriti Date: Tue, 11 Jun 2024 18:23:43 +0530 Subject: [PATCH 08/10] Added all models config --- spec/lib/gitlab/import_export/all_models.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 6380424da88d2b..2a7d7ca59e1d34 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -648,6 +648,7 @@ project: - project_repository - users - maintainers +- owners_and_maintainers - requesters - namespace_members - namespace_requesters -- GitLab From 76c7b9118f598b8554e26f46e6ada908c4836f23 Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 12 Jun 2024 14:44:30 +0530 Subject: [PATCH 09/10] Spec files restructured --- spec/services/notification_service_spec.rb | 30 ++++++++++++---------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 92d2c3c2c06bb4..8163693e60399c 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -384,7 +384,7 @@ let_it_be(:owner2) { create(:user) } let_it_be(:maintainer) { create(:user) } let_it_be(:parent_group) { create(:group) } - let(:group) { create(:group) } + let(:group) { create(:group, parent: parent_group) } subject(:notification_service) do notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token.name]) @@ -395,6 +395,7 @@ group.add_owner(owner1) group.add_owner(owner2) group.add_reporter(project_bot) + group.add_maintainer(maintainer) end it 'sends emails to the group owners' do @@ -428,9 +429,8 @@ context 'when group has inherited members' do let_it_be(:project_bot) { create(:user, :project_bot) } - let_it_be(:group) { create(:group, parent: parent_group) } - let_it_be(:expiring_token_1) { create(:personal_access_token, user: owner1, expires_at: 5.days.from_now) } - let_it_be(:expiring_token_2) { create(:personal_access_token, user: owner2, expires_at: 5.days.from_now) } + let_it_be(:expiring_token_1) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } + let_it_be(:expiring_token_2) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } subject(:notification_service) do notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token_1.name, expiring_token_2.name]) @@ -442,7 +442,7 @@ group.add_reporter(project_bot) end - it 'sends email to direct members' do + it 'does not send email to inherited members' do expect { notification_service }.to( have_enqueued_email( owner1, @@ -451,6 +451,15 @@ mail: "bot_resource_access_token_about_to_expire_email" ) ) + + expect { notification_service }.not_to( + have_enqueued_email( + owner2, + project_bot.resource_bot_resource, + [expiring_token_1.name, expiring_token_2.name], + mail: "bot_resource_access_token_about_to_expire_email" + ) + ) end end end @@ -482,16 +491,9 @@ end context 'when project has inherited members' do - let(:group) { create(:group) } - let(:project) { create(:project, namespace: group) } - before do - project.add_reporter(project_bot) - project.add_maintainer(maintainer) + project.namespace = group project.save! - - group.add_owner(owner1) - group.save! end it 'does not send email to inherited members' do @@ -506,7 +508,7 @@ expect { notification_service }.not_to( have_enqueued_email( - owner1, + project.owner, project_bot.resource_bot_resource, [expiring_token.name], mail: "bot_resource_access_token_about_to_expire_email" -- GitLab From bf4329c1b1c6341b392f34d6acd137165a193e67 Mon Sep 17 00:00:00 2001 From: smriti Date: Thu, 13 Jun 2024 09:12:45 +0530 Subject: [PATCH 10/10] Rspec refactoring --- spec/services/notification_service_spec.rb | 40 +++++++++++----------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 8163693e60399c..2dabd2c9bbf66e 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -378,20 +378,20 @@ describe '#resource_access_token_about_to_expire' do let_it_be(:project_bot) { create(:user, :project_bot) } - let_it_be(:expiring_token) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } + let_it_be(:expiring_token) { "Expiring Token" } let_it_be(:owner1) { create(:user) } let_it_be(:owner2) { create(:user) } let_it_be(:maintainer) { create(:user) } let_it_be(:parent_group) { create(:group) } - let(:group) { create(:group, parent: parent_group) } + let_it_be(:group) { create(:group, parent: parent_group) } subject(:notification_service) do - notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token.name]) + notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token]) end context 'when the resource is a group' do - before do + before_all do group.add_owner(owner1) group.add_owner(owner2) group.add_reporter(project_bot) @@ -403,13 +403,13 @@ have_enqueued_email( owner1, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ).and( have_enqueued_email( owner2, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -421,7 +421,7 @@ have_enqueued_email( maintainer, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -429,14 +429,14 @@ context 'when group has inherited members' do let_it_be(:project_bot) { create(:user, :project_bot) } - let_it_be(:expiring_token_1) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } - let_it_be(:expiring_token_2) { create(:personal_access_token, user: project_bot, expires_at: 5.days.from_now) } + let_it_be(:expiring_token_1) { "Expiring Token 1" } + let_it_be(:expiring_token_2) { "Expirigin Token 2" } subject(:notification_service) do - notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token_1.name, expiring_token_2.name]) + notification.bot_resource_access_token_about_to_expire(project_bot, [expiring_token_1, expiring_token_2]) end - before do + before_all do parent_group.add_owner(owner2) group.add_owner(owner1) group.add_reporter(project_bot) @@ -447,7 +447,7 @@ have_enqueued_email( owner1, project_bot.resource_bot_resource, - [expiring_token_1.name, expiring_token_2.name], + [expiring_token_1, expiring_token_2], mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -456,7 +456,7 @@ have_enqueued_email( owner2, project_bot.resource_bot_resource, - [expiring_token_1.name, expiring_token_2.name], + [expiring_token_1, expiring_token_2], mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -465,9 +465,9 @@ end context 'when the resource is a project' do - let(:project) { create(:project) } + let_it_be(:project) { create(:project) } - before do + before_all do project.add_maintainer(maintainer) project.add_reporter(project_bot) end @@ -477,13 +477,13 @@ have_enqueued_email( maintainer, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ).and( have_enqueued_email( project.owner, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -491,7 +491,7 @@ end context 'when project has inherited members' do - before do + before_all do project.namespace = group project.save! end @@ -501,7 +501,7 @@ have_enqueued_email( maintainer, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ) ) @@ -510,7 +510,7 @@ have_enqueued_email( project.owner, project_bot.resource_bot_resource, - [expiring_token.name], + [expiring_token], mail: "bot_resource_access_token_about_to_expire_email" ) ) -- GitLab