From 944d1efde878b91e72f42fa5742c4531b3792cd1 Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Fri, 21 Jan 2022 16:01:53 +0100 Subject: [PATCH 01/10] Permanent dismissal for the alert banner When the Sears Count Alert is dismissed, the choice will be remembered untill the next time seats are updated --- .../javascripts/persistent_user_callout.js | 4 +- .../javascripts/persistent_user_callouts.js | 1 + app/helpers/users/group_callouts_helper.rb | 1 + app/models/member.rb | 9 ++ app/models/users/group_callout.rb | 3 +- ee/app/helpers/seats_count_alert_helper.rb | 21 ++- ee/app/models/ee/group.rb | 8 ++ .../header/_seats_count_alert.html.haml | 6 +- .../seats_count_alert_spec.rb | 132 ++++++++++++++++++ ee/spec/models/ee/group_spec.rb | 29 ++++ spec/frontend/persistent_user_callout_spec.js | 8 +- 11 files changed, 213 insertions(+), 9 deletions(-) create mode 100644 ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb diff --git a/app/assets/javascripts/persistent_user_callout.js b/app/assets/javascripts/persistent_user_callout.js index bc83844b8b9c77..b003302ec8e5cb 100644 --- a/app/assets/javascripts/persistent_user_callout.js +++ b/app/assets/javascripts/persistent_user_callout.js @@ -7,10 +7,11 @@ const DEFERRED_LINK_CLASS = 'deferred-link'; export default class PersistentUserCallout { constructor(container, options = container.dataset) { - const { dismissEndpoint, featureId, deferLinks } = options; + const { dismissEndpoint, featureId, groupId, deferLinks } = options; this.container = container; this.dismissEndpoint = dismissEndpoint; this.featureId = featureId; + this.groupId = groupId; this.deferLinks = parseBoolean(deferLinks); this.init(); @@ -52,6 +53,7 @@ export default class PersistentUserCallout { axios .post(this.dismissEndpoint, { feature_name: this.featureId, + group_id: this.groupId, }) .then(() => { this.container.remove(); diff --git a/app/assets/javascripts/persistent_user_callouts.js b/app/assets/javascripts/persistent_user_callouts.js index a7f8704b559778..337c204c36ab3d 100644 --- a/app/assets/javascripts/persistent_user_callouts.js +++ b/app/assets/javascripts/persistent_user_callouts.js @@ -10,6 +10,7 @@ const PERSISTENT_USER_CALLOUTS = [ '.js-new-user-signups-cap-reached', '.js-eoa-bronze-plan-banner', '.js-security-newsletter-callout', + '.js-approaching-seats-count-threshold', ]; const initCallouts = () => { diff --git a/app/helpers/users/group_callouts_helper.rb b/app/helpers/users/group_callouts_helper.rb index b66c7f9f8218ce..0aa4eb89499a2a 100644 --- a/app/helpers/users/group_callouts_helper.rb +++ b/app/helpers/users/group_callouts_helper.rb @@ -3,6 +3,7 @@ module Users module GroupCalloutsHelper INVITE_MEMBERS_BANNER = 'invite_members_banner' + APPROACHING_SEAT_COUNT_THRESHOLD = 'approaching_seat_count_threshold' def show_invite_banner?(group) Ability.allowed?(current_user, :admin_group, group) && diff --git a/app/models/member.rb b/app/models/member.rb index 1c1b603b4c741a..d875d766556d00 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -127,6 +127,15 @@ class Member < ApplicationRecord .reorder(nil) end + scope :in_hierarchy_active_non_guest, ->(source) do + ::Member + .in_hierarchy(source) + .active + .order(:created_at) + .non_guests + .non_invite + end + scope :without_invites_and_requests, -> do non_request .non_invite diff --git a/app/models/users/group_callout.rb b/app/models/users/group_callout.rb index da9b95fd718bbc..faa5130e6ec2a8 100644 --- a/app/models/users/group_callout.rb +++ b/app/models/users/group_callout.rb @@ -9,7 +9,8 @@ class GroupCallout < ApplicationRecord belongs_to :group enum feature_name: { - invite_members_banner: 1 + invite_members_banner: 1, + approaching_seat_count_threshold: 2 # EE-only } validates :group, presence: true diff --git a/ee/app/helpers/seats_count_alert_helper.rb b/ee/app/helpers/seats_count_alert_helper.rb index 33b42eeb7d67d2..c088cbb2a530c5 100644 --- a/ee/app/helpers/seats_count_alert_helper.rb +++ b/ee/app/helpers/seats_count_alert_helper.rb @@ -26,9 +26,8 @@ def seats_usage_link end def show_seats_count_alert? - return false unless root_namespace&.group_namespace? - return false unless root_namespace&.has_owner?(current_user) - return false unless current_subscription + return false unless ::Gitlab.dev_env_or_com? && group_with_owner? && current_subscription + return false if user_dismissed_alert? !!@display_seats_count_alert end @@ -39,6 +38,22 @@ def total_seats_count private + def user_dismissed_alert? + current_user.dismissed_callout_for_group?( + feature_name: Users::GroupCalloutsHelper::APPROACHING_SEAT_COUNT_THRESHOLD, + group: root_namespace, + ignore_dismissal_earlier_than: last_member_added_at + ) + end + + def last_member_added_at + root_namespace&.last_billed_user_created_at + end + + def group_with_owner? + root_namespace&.group_namespace? && root_namespace&.has_owner?(current_user) + end + def root_namespace @project&.root_ancestor || @group&.root_ancestor end diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index e2f93bf583b121..d5e099699cbb7c 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -462,6 +462,10 @@ def access_level_roles levels.merge(::Gitlab::Access::MINIMAL_ACCESS_HASH) end + def last_billed_user_created_at + billed_group_and_projects_members.last.created_at + end + override :users_count def users_count return all_group_members.count if minimal_access_role_allowed? @@ -610,6 +614,10 @@ def billed_user_ids_including_guests end end + def billed_group_and_projects_members + ::Member.in_hierarchy_active_non_guest(self) + end + # Members belonging directly to Group or its subgroups def billed_group_users(non_guests: false) members = ::GroupMember.active_without_invites_and_requests.where( diff --git a/ee/app/views/layouts/header/_seats_count_alert.html.haml b/ee/app/views/layouts/header/_seats_count_alert.html.haml index 3bd36b43a296d1..9229f9b95474af 100644 --- a/ee/app/views/layouts/header/_seats_count_alert.html.haml +++ b/ee/app/views/layouts/header/_seats_count_alert.html.haml @@ -1,8 +1,10 @@ - return unless show_seats_count_alert? .container.container-limited.pt-3 - .gl-alert.gl-alert-info{ role: 'alert' } + .gl-alert.gl-alert-info.js-approaching-seats-count-threshold{ role: 'alert', data: { dismiss_endpoint: group_callouts_path, + feature_id: Users::GroupCalloutsHelper::APPROACHING_SEAT_COUNT_THRESHOLD, + group_id: root_namespace.id } } = sprite_icon('information-o', size: 16, css_class: 'gl-icon gl-alert-icon') - %button.js-close.gl-alert-dismiss{ type: 'button', 'aria-label' => _('Dismiss') } + %button.js-close.gl-alert-dismiss{ type: 'button', 'aria-label' => _('Dismiss'), data: { testid: 'approaching-seats-count-threshold-alert-dismiss' } } = sprite_icon('close', size: 16, css_class: 'gl-icon') .gl-alert-body %h4.gl-alert-title= _('%{group_name} is approaching the limit of available seats') % { group_name: group_name } diff --git a/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb b/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb new file mode 100644 index 00000000000000..3965e7b7b2803c --- /dev/null +++ b/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb @@ -0,0 +1,132 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Display approaching seats count threshold alert', :saas, :js do + let_it_be(:user) { create(:user) } + + shared_examples_for 'a hidden alert' do + it 'does not show the alert' do + visit visit_path + + expect(page).not_to have_content("#{group.name} is approaching the limit of available seats") + expect(page).not_to have_link('View seat usage', href: usage_quotas_path(group, anchor: 'seats-quota-tab')) + end + end + + shared_examples_for 'a visible alert' do + it 'shows the alert' do + visit visit_path + + expect(page).to have_content("#{group.name} is approaching the limit of available seats") + expect(page).to have_content("Your subscription has #{gitlab_subscription.seats - gitlab_subscription.seats_in_use} out of #{gitlab_subscription.seats} seats remaining. Even if you reach the number of seats in your subscription, you can continue to add users, and GitLab will bill you for the overage.") + expect(page).to have_link('View seat usage', href: usage_quotas_path(group, anchor: 'seats-quota-tab')) + end + end + + shared_examples_for 'a dismissed alert' do + context 'when alert was dismissed' do + before do + visit visit_path + + find('body.page-initialised [data-testid="approaching-seats-count-threshold-alert-dismiss"]').click + end + + it_behaves_like 'a hidden alert' + end + end + + context 'when visiting a group' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group) { create(:group, parent: group) } + let_it_be(:visit_path) { group_path(group) } + + context 'when not reached user count threshold' do + context 'when regular user is logged out' do + it_behaves_like 'a hidden alert' + end + + context 'when a member is logged in' do + before do + group.add_developer(user) + sign_in(user) + end + + it_behaves_like 'a hidden alert' + end + + context 'when a owner is logged in' do + before do + group.add_owner(user) + sign_in(user) + end + + it_behaves_like 'a hidden alert' + end + end + + context 'with the show conditions are met' do + let_it_be(:gitlab_subscription) do + create(:gitlab_subscription, namespace: group, plan_code: Plan::ULTIMATE, seats: 15, seats_in_use: 1) + end + + xexample 'when a owner is logged in' do + before do + group.add_owner(user) + sign_in(user) + end + + it_behaves_like 'a visible alert' + + it_behaves_like 'a dismissed alert' + end + end + end + + context 'when visiting a project' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, namespace: group) } + let_it_be(:visit_path) { project_path(project) } + + context 'when not reached user count threshold' do + context 'when regular user is logged out' do + it_behaves_like 'a hidden alert' + end + + context 'when a member is logged in' do + before do + group.add_developer(user) + sign_in(user) + end + + it_behaves_like 'a hidden alert' + end + + context 'when a owner is logged in' do + before do + group.add_owner(user) + sign_in(user) + end + + it_behaves_like 'a hidden alert' + end + end + + context 'with the show conditions are met' do + let_it_be(:gitlab_subscription) do + create(:gitlab_subscription, namespace: group, plan_code: Plan::ULTIMATE, seats: 15, seats_in_use: 1) + end + + xexample 'when a owner is logged in' do + before do + group.add_owner(user) + sign_in(user) + end + + it_behaves_like 'a visible alert' + + it_behaves_like 'a dismissed alert' + end + end + end +end diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index 899ef6f185f7b4..f2b2b452112931 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -823,6 +823,35 @@ end end + describe '#last_billed_user_created_at' do + subject { group.last_billed_user_created_at } + + let(:group) { create(:group) } + let(:user) { create(:user) } + + context 'without billed users' do + it { is_expected.to be nil } + end + + context 'with guest users' do + before do + create(:group_member, :guest, user: user, source: group) + end + + it { is_expected.to be nil } + end + + context 'with billed users' do + before do + create(:group_member, user: create(:user), source: group) + create(:group_member, user: create(:user), source: group) + create(:group_member, user: user, source: group) + end + + it { is_expected.to eq user.created_at } + end + end + describe '#saml_discovery_token' do it 'returns existing tokens' do group = create(:group, saml_discovery_token: 'existing') diff --git a/spec/frontend/persistent_user_callout_spec.js b/spec/frontend/persistent_user_callout_spec.js index 1db255106ed1ea..4633602de262ee 100644 --- a/spec/frontend/persistent_user_callout_spec.js +++ b/spec/frontend/persistent_user_callout_spec.js @@ -10,6 +10,7 @@ jest.mock('~/flash'); describe('PersistentUserCallout', () => { const dismissEndpoint = '/dismiss'; const featureName = 'feature'; + const groupId = '5'; function createFixture() { const fixture = document.createElement('div'); @@ -18,6 +19,7 @@ describe('PersistentUserCallout', () => { class="container" data-dismiss-endpoint="${dismissEndpoint}" data-feature-id="${featureName}" + data-group-id="${groupId}" > @@ -86,7 +88,9 @@ describe('PersistentUserCallout', () => { return waitForPromises().then(() => { expect(persistentUserCallout.container.remove).toHaveBeenCalled(); - expect(mockAxios.history.post[0].data).toBe(JSON.stringify({ feature_name: featureName })); + expect(mockAxios.history.post[0].data).toBe( + JSON.stringify({ feature_name: featureName, group_id: groupId }), + ); }); }); @@ -191,8 +195,8 @@ describe('PersistentUserCallout', () => { return waitForPromises().then(() => { expect(window.location.assign).toBeCalledWith(href); - expect(mockAxios.history.post[0].data).toBe(JSON.stringify({ feature_name: featureName })); expect(persistentUserCallout.container.remove).not.toHaveBeenCalled(); + expect(mockAxios.history.post[0].data).toBe(JSON.stringify({ feature_name: featureName })); }); }); -- GitLab From e3e5da86e6e0d85b07410817a7cc2917bb0d5269 Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Mon, 31 Jan 2022 18:36:45 +0100 Subject: [PATCH 02/10] Fixup: restructure tests and mode to non-EE model --- ee/app/models/ee/group.rb | 2 +- .../seats_count_alert_spec.rb | 105 ++++++++---------- ee/spec/models/ee/group_spec.rb | 9 +- 3 files changed, 52 insertions(+), 64 deletions(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index d5e099699cbb7c..c8a1fa1cb05bcd 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -463,7 +463,7 @@ def access_level_roles end def last_billed_user_created_at - billed_group_and_projects_members.last.created_at + billed_group_and_projects_members.last&.created_at end override :users_count diff --git a/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb b/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb index 3965e7b7b2803c..c6fc8b2fd49d26 100644 --- a/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb +++ b/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb @@ -36,92 +36,77 @@ end end - context 'when visiting a group' do + context 'when conditions not met' do let_it_be(:group) { create(:group) } - let_it_be(:sub_group) { create(:group, parent: group) } let_it_be(:visit_path) { group_path(group) } - context 'when not reached user count threshold' do - context 'when regular user is logged out' do - it_behaves_like 'a hidden alert' + context 'when logged out' do + it_behaves_like 'a hidden alert' + end + + context 'when logged in owner' do + before do + group.add_owner(user) + sign_in(user) end - context 'when a member is logged in' do - before do - group.add_developer(user) - sign_in(user) - end + it_behaves_like 'a hidden alert' + end + end - it_behaves_like 'a hidden alert' - end + xexample 'when show condition met' do + let_it_be(:group) { create(:group) } + let_it_be(:visit_path) { group_path(group) } + let_it_be(:gitlab_subscription) do + create(:gitlab_subscription, namespace: group, plan_code: Plan::ULTIMATE, seats: 15, seats_in_use: 14) + end - context 'when a owner is logged in' do - before do - group.add_owner(user) - sign_in(user) - end + context 'when logged out' do + it_behaves_like 'a hidden alert' + end - it_behaves_like 'a hidden alert' + context 'when logged in as developer' do + before do + group.add_developer(user) + sign_in(user) end + + it_behaves_like 'a hidden alert' end - context 'with the show conditions are met' do - let_it_be(:gitlab_subscription) do - create(:gitlab_subscription, namespace: group, plan_code: Plan::ULTIMATE, seats: 15, seats_in_use: 1) + context 'when logged in as owner' do + before do + group.add_owner(user) + sign_in(user) end - xexample 'when a owner is logged in' do - before do - group.add_owner(user) - sign_in(user) - end - + context 'when on group page' do it_behaves_like 'a visible alert' it_behaves_like 'a dismissed alert' end - end - end - - context 'when visiting a project' do - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, namespace: group) } - let_it_be(:visit_path) { project_path(project) } - context 'when not reached user count threshold' do - context 'when regular user is logged out' do - it_behaves_like 'a hidden alert' - end + context 'when on project page' do + let_it_be(:project) { create(:project, namespace: group) } + let_it_be(:visit_path) { project_path(project) } - context 'when a member is logged in' do - before do - group.add_developer(user) - sign_in(user) - end + it_behaves_like 'a visible alert' - it_behaves_like 'a hidden alert' + it_behaves_like 'a dismissed alert' end - context 'when a owner is logged in' do - before do - group.add_owner(user) - sign_in(user) - end + context 'when on project issue page' do + let_it_be(:project) { create(:project, namespace: group) } + let_it_be(:visit_path) { project_issues_path(project) } - it_behaves_like 'a hidden alert' - end - end + it_behaves_like 'a visible alert' - context 'with the show conditions are met' do - let_it_be(:gitlab_subscription) do - create(:gitlab_subscription, namespace: group, plan_code: Plan::ULTIMATE, seats: 15, seats_in_use: 1) + it_behaves_like 'a dismissed alert' end - xexample 'when a owner is logged in' do - before do - group.add_owner(user) - sign_in(user) - end + context 'when on sub-group page' do + let_it_be(:sub_group) { create(:group, parent: group) } + let_it_be(:visit_path) { group_path(sub_group) } it_behaves_like 'a visible alert' diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index f2b2b452112931..769193f109ca3a 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -824,7 +824,7 @@ end describe '#last_billed_user_created_at' do - subject { group.last_billed_user_created_at } + subject(:last_billed) { group.last_billed_user_created_at } let(:group) { create(:group) } let(:user) { create(:user) } @@ -845,10 +845,13 @@ before do create(:group_member, user: create(:user), source: group) create(:group_member, user: create(:user), source: group) - create(:group_member, user: user, source: group) end - it { is_expected.to eq user.created_at } + it 'returns the last added billed member' do + member = create(:group_member, user: user, source: group) + + expect(last_billed).to eq member.created_at + end end end -- GitLab From 5e056a0ad9b4e9d3fc71ce5c1b0ff6f4a7f4b275 Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Tue, 1 Feb 2022 18:57:56 +0100 Subject: [PATCH 03/10] Fixup: remove non necessary tests --- .../seats_count_alert_spec.rb | 61 ------------------- ee/spec/models/ee/group_spec.rb | 6 +- 2 files changed, 3 insertions(+), 64 deletions(-) diff --git a/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb b/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb index c6fc8b2fd49d26..d10a89a43d35f3 100644 --- a/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb +++ b/ee/spec/features/gitlab_subscriptions/seats_count_alert_spec.rb @@ -53,65 +53,4 @@ it_behaves_like 'a hidden alert' end end - - xexample 'when show condition met' do - let_it_be(:group) { create(:group) } - let_it_be(:visit_path) { group_path(group) } - let_it_be(:gitlab_subscription) do - create(:gitlab_subscription, namespace: group, plan_code: Plan::ULTIMATE, seats: 15, seats_in_use: 14) - end - - context 'when logged out' do - it_behaves_like 'a hidden alert' - end - - context 'when logged in as developer' do - before do - group.add_developer(user) - sign_in(user) - end - - it_behaves_like 'a hidden alert' - end - - context 'when logged in as owner' do - before do - group.add_owner(user) - sign_in(user) - end - - context 'when on group page' do - it_behaves_like 'a visible alert' - - it_behaves_like 'a dismissed alert' - end - - context 'when on project page' do - let_it_be(:project) { create(:project, namespace: group) } - let_it_be(:visit_path) { project_path(project) } - - it_behaves_like 'a visible alert' - - it_behaves_like 'a dismissed alert' - end - - context 'when on project issue page' do - let_it_be(:project) { create(:project, namespace: group) } - let_it_be(:visit_path) { project_issues_path(project) } - - it_behaves_like 'a visible alert' - - it_behaves_like 'a dismissed alert' - end - - context 'when on sub-group page' do - let_it_be(:sub_group) { create(:group, parent: group) } - let_it_be(:visit_path) { group_path(sub_group) } - - it_behaves_like 'a visible alert' - - it_behaves_like 'a dismissed alert' - end - end - end end diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index 769193f109ca3a..beca7e667f05f6 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -843,12 +843,12 @@ context 'with billed users' do before do - create(:group_member, user: create(:user), source: group) - create(:group_member, user: create(:user), source: group) + create(:group_member, user: create(:user), source: group, created_at: '2022-03-16') + create(:group_member, user: create(:user), source: group, created_at: '2022-04-19') end it 'returns the last added billed member' do - member = create(:group_member, user: user, source: group) + member = create(:group_member, user: user, source: group, created_at: '2022-07-02') expect(last_billed).to eq member.created_at end -- GitLab From 6ad126671b57bd5cb5e5bd82f1b54781ffef2a9a Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Wed, 2 Feb 2022 07:08:02 +0100 Subject: [PATCH 04/10] Fixup: use better matcher --- ee/spec/models/ee/group_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index beca7e667f05f6..7665356612dff8 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -850,7 +850,7 @@ it 'returns the last added billed member' do member = create(:group_member, user: user, source: group, created_at: '2022-07-02') - expect(last_billed).to eq member.created_at + expect(last_billed).to be_like_time(member.created_at) end end end -- GitLab From 68b404e4010de353d5cbfd149b7c33ac32844bf6 Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Wed, 2 Feb 2022 20:54:27 +0100 Subject: [PATCH 05/10] Fixup: move order clause --- app/models/member.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/member.rb b/app/models/member.rb index d875d766556d00..92f97d2ddecc1b 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -131,9 +131,9 @@ class Member < ApplicationRecord ::Member .in_hierarchy(source) .active - .order(:created_at) .non_guests .non_invite + .order(:created_at) end scope :without_invites_and_requests, -> do -- GitLab From f796013fbea5ae7e2a72da097ca21e46256b48e4 Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Wed, 2 Feb 2022 21:24:45 +0100 Subject: [PATCH 06/10] Fixup: add test for introduced scope --- spec/models/member_spec.rb | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 0f67531382d1e2..c4ac8cffcdd68a 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -459,6 +459,35 @@ it { is_expected.not_to include @member_with_minimal_access } end + describe '.in_hierarchy_active_non_guest' do + let(:root_ancestor) { create(:group) } + let(:project) { create(:project, group: root_ancestor) } + let(:subgroup) { create(:group, parent: root_ancestor) } + let(:subgroup_project) { create(:project, group: subgroup) } + + let!(:root_ancestor_member) { create(:group_member, :owner, group: root_ancestor) } + let!(:project_member) { create(:project_member, project: project) } + let!(:subgroup_member) { create(:group_member, group: subgroup) } + let!(:subgroup_project_member) { create(:project_member, project: subgroup_project) } + let!(:guest_project_member) { create(:project_member, :guest, project: subgroup_project) } + let!(:project_invited_member) { create(:project_member, :invited, :developer, project: project) } + + let(:hierarchy_members) do + [ + root_ancestor_member, + project_member, + subgroup_member, + subgroup_project_member + ] + end + + subject { described_class.in_hierarchy_active_non_guest(subgroup_project) } + + it { is_expected.to contain_exactly(*hierarchy_members) } + it { is_expected.not_to include guest_project_member } + it { is_expected.not_to include project_invited_member } + end + describe '.without_invites_and_requests' do subject { described_class.without_invites_and_requests.to_a } -- GitLab From 63a1535e236a7e575ac902e581e480effb72b4ce Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Wed, 2 Feb 2022 21:38:06 +0100 Subject: [PATCH 07/10] Fixup: make test more resilient --- ee/spec/models/ee/group_spec.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index 7665356612dff8..84dd65d9ad177c 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -842,15 +842,16 @@ end context 'with billed users' do + let_it_be(:expected_time) { Time.new(2022, 4, 19, 00, 00, 00, '+00:00') } + before do + create(:group_member, user: create(:user), source: group, created_at: expected_time) + create(:group_member, :guest, user: user, source: group, created_at: '2022-07-02') create(:group_member, user: create(:user), source: group, created_at: '2022-03-16') - create(:group_member, user: create(:user), source: group, created_at: '2022-04-19') end it 'returns the last added billed member' do - member = create(:group_member, user: user, source: group, created_at: '2022-07-02') - - expect(last_billed).to be_like_time(member.created_at) + expect(last_billed).to be_like_time(expected_time) end end end -- GitLab From 65e3f28b3cde4de31963a983d1ead898bf1adbc0 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Thu, 3 Feb 2022 06:12:19 +0000 Subject: [PATCH 08/10] Fixup: improve element lookup --- ee/app/models/ee/group.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index c8a1fa1cb05bcd..77c6d17fa9d201 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -463,7 +463,7 @@ def access_level_roles end def last_billed_user_created_at - billed_group_and_projects_members.last&.created_at + billed_group_and_projects_members.reverse_order.limit(1).pluck(:created_at) end override :users_count -- GitLab From c81fa9ce44459d9111ee0dc74137a21964de3dfa Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Thu, 3 Feb 2022 09:19:33 +0100 Subject: [PATCH 09/10] Fixup: take first element --- ee/app/models/ee/group.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 77c6d17fa9d201..0a1e4e132983aa 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -463,7 +463,7 @@ def access_level_roles end def last_billed_user_created_at - billed_group_and_projects_members.reverse_order.limit(1).pluck(:created_at) + billed_group_and_projects_members.reverse_order.limit(1).pluck(:created_at).first end override :users_count -- GitLab From 9b4c9ebdd4c57805c7ac58f84edf820850d8df94 Mon Sep 17 00:00:00 2001 From: Angelo Gulina Date: Thu, 3 Feb 2022 17:23:30 +0100 Subject: [PATCH 10/10] Fixup: remove unnecessary scope --- app/models/member.rb | 9 --------- ee/app/models/ee/group.rb | 7 ++++++- spec/models/member_spec.rb | 29 ----------------------------- 3 files changed, 6 insertions(+), 39 deletions(-) diff --git a/app/models/member.rb b/app/models/member.rb index 92f97d2ddecc1b..1c1b603b4c741a 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -127,15 +127,6 @@ class Member < ApplicationRecord .reorder(nil) end - scope :in_hierarchy_active_non_guest, ->(source) do - ::Member - .in_hierarchy(source) - .active - .non_guests - .non_invite - .order(:created_at) - end - scope :without_invites_and_requests, -> do non_request .non_invite diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 0a1e4e132983aa..ae0d98ee5a1754 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -615,7 +615,12 @@ def billed_user_ids_including_guests end def billed_group_and_projects_members - ::Member.in_hierarchy_active_non_guest(self) + ::Member + .in_hierarchy(self) + .active + .non_guests + .non_invite + .order(:created_at) end # Members belonging directly to Group or its subgroups diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index c4ac8cffcdd68a..0f67531382d1e2 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -459,35 +459,6 @@ it { is_expected.not_to include @member_with_minimal_access } end - describe '.in_hierarchy_active_non_guest' do - let(:root_ancestor) { create(:group) } - let(:project) { create(:project, group: root_ancestor) } - let(:subgroup) { create(:group, parent: root_ancestor) } - let(:subgroup_project) { create(:project, group: subgroup) } - - let!(:root_ancestor_member) { create(:group_member, :owner, group: root_ancestor) } - let!(:project_member) { create(:project_member, project: project) } - let!(:subgroup_member) { create(:group_member, group: subgroup) } - let!(:subgroup_project_member) { create(:project_member, project: subgroup_project) } - let!(:guest_project_member) { create(:project_member, :guest, project: subgroup_project) } - let!(:project_invited_member) { create(:project_member, :invited, :developer, project: project) } - - let(:hierarchy_members) do - [ - root_ancestor_member, - project_member, - subgroup_member, - subgroup_project_member - ] - end - - subject { described_class.in_hierarchy_active_non_guest(subgroup_project) } - - it { is_expected.to contain_exactly(*hierarchy_members) } - it { is_expected.not_to include guest_project_member } - it { is_expected.not_to include project_invited_member } - end - describe '.without_invites_and_requests' do subject { described_class.without_invites_and_requests.to_a } -- GitLab