Merge branch 'nd/awaiting-users-for-billable-members' into 'master'

Add option to query awaiting users for billable members

See merge request gitlab-org/gitlab!84074

db/migrate/20220404170446_add_index_for_non_requested_non_invited_awaiting_members.rb

+# frozen_string_literal: true
+
+class AddIndexForNonRequestedNonInvitedAwaitingMembers < Gitlab::Database::Migration[1.0]
+  INDEX_NAME = 'index_members_on_non_requested_non_invited_and_state_awaiting'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :members,
+      :source_id,
+      where: '((requested_at IS NULL) AND (invite_token IS NULL) AND (access_level > 5) AND (state = 1))',
+      name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :members, INDEX_NAME
+  end
+end

db/schema_migrations/20220404170446

+f3954f6f741e40abb1ff9595f86f896e653eb14943faccfe2a14520ec583fa9c
\ No newline at end of file

db/structure.sql

@@ -28106,6 +28106,8 @@ CREATE UNIQUE INDEX index_members_on_invite_token ON members USING btree (invite
 
 CREATE INDEX index_members_on_member_namespace_id ON members USING btree (member_namespace_id);
 
+CREATE INDEX index_members_on_non_requested_non_invited_and_state_awaiting ON members USING btree (source_id) WHERE ((requested_at IS NULL) AND (invite_token IS NULL) AND (access_level > 5) AND (state = 1));
+
 CREATE INDEX index_members_on_requested_at ON members USING btree (requested_at);
 
 CREATE INDEX index_members_on_source_id_and_source_type ON members USING btree (source_id, source_type);

doc/api/members.md

@@ -329,11 +329,12 @@ respectively.
 GET /groups/:id/billable_members
 ```
 
-| Attribute | Type | Required | Description                                                                                                  |
-| --------- | ---- | -------- |--------------------------------------------------------------------------------------------------------------|
-| `id`      | integer/string | yes | The ID or [URL-encoded path of the group](index.md#namespaced-path-encoding) owned by the authenticated user |
-| `search`  | string         | no  | A query string to search for group members by name, username, or public email.                               |
-| `sort`    | string         | no  | A query string containing parameters that specify the sort attribute and order. See supported values below.  |
+| Attribute                     | Type            | Required  | Description                                                                                                   |
+| ----------------------------- | --------------- | --------- |-------------------------------------------------------------------------------------------------------------- |
+| `id`                          | integer/string  | yes       | The ID or [URL-encoded path of the group](index.md#namespaced-path-encoding) owned by the authenticated user  |
+| `search`                      | string          | no        | A query string to search for group members by name, username, or public email.                                |
+| `sort`                        | string          | no        | A query string containing parameters that specify the sort attribute and order. See supported values below.   |
+| `include_awaiting_members`    | boolean         | no        | Determines if awaiting members are included.                                                                  |
 
 The supported values for the `sort` attribute are:
 

ee/app/finders/billed_users_finder.rb

 # frozen_string_literal: true
 
 class BilledUsersFinder
-  def initialize(group, search_term: nil, order_by: 'name_asc')
+  def initialize(group, search_term: nil, order_by: 'name_asc', include_awaiting_members: false)
     @group = group
     @search_term = search_term
     @order_by = order_by
+    @include_awaiting_members = include_awaiting_members
   end
 
   def execute
@@ -24,13 +25,19 @@ class BilledUsersFinder
 
   private
 
-  attr_reader :group, :search_term, :order_by
+  attr_reader :group, :search_term, :order_by, :include_awaiting_members
+
+  def user_ids
+    group_billed_user_ids[:user_ids] + awaiting_user_ids
+  end
 
   def group_billed_user_ids
     @group_billed_user_ids ||= group.billed_user_ids
   end
 
-  def user_ids
-    group_billed_user_ids[:user_ids]
+  def awaiting_user_ids
+    return [] unless include_awaiting_members
+
+    group.awaiting_user_ids
   end
 end

ee/app/models/ee/group.rb

@@ -360,6 +360,10 @@ module EE
       exclude_guests?(requested_hosted_plan) ? billed_user_ids_excluding_guests : billed_user_ids_including_guests
     end
 
+    def awaiting_user_ids
+      awaiting_members_without_invites_and_requests.pluck(:id).to_set
+    end
+
     override :supports_events?
     def supports_events?
       feature_available?(:epics)
@@ -710,6 +714,16 @@ module EE
       ::Project.for_group_and_its_subgroups(self).non_archived.without_deleted
     end
 
+    def awaiting_members_without_invites_and_requests
+      groups = self_and_descendants + invited_group_in_groups + invited_groups_in_projects
+      projects = ::Project.where(namespace: self_and_descendants)
+      sources = groups + projects
+
+      members = ::Member.awaiting_without_invites_and_requests.where(source_id: sources)
+
+      users_without_project_bots(members).distinct
+    end
+
     override :_safe_read_repository_read_only_column
     def _safe_read_repository_read_only_column
       ::NamespaceSetting.where(namespace: self).pick(:repository_read_only)

ee/app/models/ee/member.rb

@@ -35,6 +35,12 @@ module EE
         .includes(:user)
         .order(:user_id, :invite_email)
       end
+
+      scope :awaiting_without_invites_and_requests, -> do
+        active
+          .awaiting
+          .non_invite
+      end
     end
 
     override :notification_service

ee/lib/ee/api/members.rb

@@ -117,6 +117,7 @@ module EE
             use :pagination
             optional :search, type: String, desc: 'The exact name of the subscribed member'
             optional :sort, type: String, desc: 'The sorting option', values: Helpers::MembersHelpers.member_sort_options
+            optional :include_awaiting_members, type: Grape::API::Boolean, desc: 'Determines if awaiting members are included', default: false
           end
           get ":id/billable_members" do
             group = find_group!(params[:id])
@@ -127,8 +128,9 @@ module EE
             sorting = params[:sort] || 'id_asc'
 
             result = BilledUsersFinder.new(group,
-                                             search_term: params[:search],
-                                             order_by: sorting).execute
+                                            search_term: params[:search],
+                                            order_by: sorting,
+                                            include_awaiting_members: params[:include_awaiting_members]).execute
 
             present paginate(result[:users]),
                     with: ::EE::API::Entities::BillableMember,

ee/spec/finders/billed_users_finder_spec.rb

@@ -7,90 +7,138 @@ RSpec.describe BilledUsersFinder do
 
   let(:search_term) { nil }
   let(:order_by) { nil }
+  let(:include_awaiting_members) { false }
 
-  describe '#execute' do
-    let_it_be(:maria) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) }
-    let_it_be(:john_smith) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) }
-    let_it_be(:john_doe) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) }
-    let_it_be(:sophie) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) }
+  subject(:execute) { described_class.new(group, search_term: search_term, order_by: order_by, include_awaiting_members: include_awaiting_members).execute }
 
-    subject { described_class.new(group, search_term: search_term, order_by: order_by) }
+  describe '#execute' do
+    context 'without members' do
+      let(:include_awaiting_members) { true }
 
-    context 'when a group does not have any billed users' do
       it 'returns an empty object' do
-        allow(group).to receive(:billed_user_ids).and_return({ user_ids: [] })
-
-        expect(subject.execute).to eq({})
+        expect(execute).to eq({})
       end
     end
 
-    context 'when a search parameter is provided' do
-      let(:search_term) { 'John' }
+    context 'with members' do
+      let_it_be(:maria) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) }
+      let_it_be(:john_smith) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) }
+      let_it_be(:john_doe) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) }
+      let_it_be(:sophie) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) }
+      let_it_be(:alice_awaiting) { create(:group_member, :awaiting, :developer, group: group, user: create(:user, name: 'Alice Waiting'))}
 
-      context 'when a sorting parameter is provided (eg name descending)' do
-        let(:order_by) { 'name_desc' }
+      shared_examples 'with awaiting members' do
+        context 'when awaiting users are included' do
+          let(:include_awaiting_members) { true }
 
-        it 'sorts results accordingly' do
-          expect(subject.execute[:users]).to eq([john_smith, john_doe].map(&:user))
+          it 'includes awaiting users' do
+            expect(execute[:users]).to include(alice_awaiting.user)
+          end
         end
-      end
 
-      context 'when a sorting parameter is not provided' do
-        subject { described_class.new(group, search_term: search_term) }
+        context 'when awaiting users are excluded' do
+          let(:include_awaiting_members) { false }
 
-        it 'sorts expected results in name_asc order' do
-          expect(subject.execute[:users]).to eq([john_doe, john_smith].map(&:user))
+          it 'excludes awaiting users' do
+            expect(execute[:users]).not_to include(alice_awaiting.user)
+          end
         end
       end
-    end
 
-    context 'when a search parameter is not present' do
-      subject { described_class.new(group) }
+      context 'when user is awaiting and active member' do
+        let_it_be(:project) { create(:project, group: group) }
+
+        let(:include_awaiting_members) { true }
 
-      it 'returns expected users in name asc order when a sorting is not provided either' do
-        allow(group).to receive(:billed_user_members).and_return([john_doe, john_smith, sophie, maria])
+        before do
+          create(:project_member, :maintainer, user: alice_awaiting.user, source: project)
+        end
 
-        expect(subject.execute[:users]).to eq([john_doe, john_smith, maria, sophie].map(&:user))
+        it 'is only included once' do
+          expect(execute[:users]).to include(alice_awaiting.user).once
+        end
       end
 
-      context 'and when a sorting parameter is provided (eg name descending)' do
-        let(:order_by) { 'name_desc' }
+      it_behaves_like 'with awaiting members'
+
+      context 'when a search parameter is provided' do
+        let(:search_term) { 'John' }
 
-        subject { described_class.new(group, search_term: search_term, order_by: order_by) }
+        context 'when a sorting parameter is provided (eg name descending)' do
+          let(:order_by) { 'name_desc' }
 
-        it 'sorts results accordingly' do
-          expect(subject.execute[:users]).to eq([sophie, maria, john_smith, john_doe].map(&:user))
+          it 'sorts results accordingly' do
+            expect(execute[:users]).to eq([john_smith, john_doe].map(&:user))
+          end
+        end
+
+        context 'when a sorting parameter is not provided' do
+          subject(:execute) { described_class.new(group, search_term: search_term).execute }
+
+          it 'sorts expected results in name_asc order' do
+            expect(execute[:users]).to eq([john_doe, john_smith].map(&:user))
+          end
         end
-      end
-    end
 
-    context 'with billable group members including shared members' do
-      let_it_be(:shared_with_group_member) { create(:group_member, user: create(:user, name: 'Shared Group User')) }
-      let_it_be(:shared_with_project_member) { create(:group_member, user: create(:user, name: 'Shared Project User')) }
-      let_it_be(:project) { create(:project, group: group) }
+        context 'when searching for an awaiting user' do
+          let(:search_term) { 'Alice' }
 
-      before do
-        create(:group_group_link, shared_group: group, shared_with_group: shared_with_group_member.group)
-        create(:project_group_link, group: shared_with_project_member.group, project: project)
+          it_behaves_like 'with awaiting members'
+        end
       end
 
-      it 'returns a hash of users and user ids' do
-        expect(subject.execute.keys).to eq([
-          :users,
-          :group_member_user_ids,
-          :project_member_user_ids,
-          :shared_group_user_ids,
-          :shared_project_user_ids
-        ])
+      context 'when a search parameter is not present' do
+        subject(:execute) { described_class.new(group, include_awaiting_members: include_awaiting_members).execute }
+
+        it 'returns expected users in name asc order when a sorting is not provided either' do
+          expect(execute[:users]).to eq([john_doe, john_smith, maria, sophie].map(&:user))
+        end
+
+        it_behaves_like 'with awaiting members'
+
+        context 'and when a sorting parameter is provided (eg name descending)' do
+          let(:order_by) { 'name_desc' }
+
+          subject(:execute) { described_class.new(group, search_term: search_term, order_by: order_by, include_awaiting_members: include_awaiting_members).execute }
+
+          it 'sorts results accordingly' do
+            expect(execute[:users]).to eq([sophie, maria, john_smith, john_doe].map(&:user))
+          end
+
+          context 'when awaiting users are included' do
+            let(:include_awaiting_members) { true }
+
+            it 'sorts results accordingly' do
+              expect(execute[:users]).to eq([sophie, maria, john_smith, john_doe, alice_awaiting].map(&:user))
+            end
+          end
+        end
       end
 
-      it 'returns the correct user ids' do
-        result = subject.execute
+      context 'with billable group members including shared members' do
+        let_it_be(:shared_with_group_member) { create(:group_member, user: create(:user, name: 'Shared Group User')) }
+        let_it_be(:shared_with_project_member) { create(:group_member, user: create(:user, name: 'Shared Project User')) }
+        let_it_be(:project) { create(:project, group: group) }
+
+        before do
+          create(:group_group_link, shared_group: group, shared_with_group: shared_with_group_member.group)
+          create(:project_group_link, group: shared_with_project_member.group, project: project)
+        end
+
+        it 'returns a hash of users and user ids' do
+          expect(execute.keys).to eq([
+            :users,
+            :group_member_user_ids,
+            :project_member_user_ids,
+            :shared_group_user_ids,
+            :shared_project_user_ids
+          ])
+        end
 
-        aggregate_failures do
-          expect(result[:group_member_user_ids]).to contain_exactly(*[maria, john_smith, john_doe, sophie].map(&:user_id))
-          expect(result[:shared_group_user_ids]).to contain_exactly(shared_with_group_member.user_id)
-          expect(result[:shared_project_user_ids]).to contain_exactly(shared_with_project_member.user_id)
+        it 'returns the correct user ids', :aggregate_failures do
+          expect(execute[:group_member_user_ids]).to contain_exactly(*[maria, john_smith, john_doe, sophie].map(&:user_id))
+          expect(execute[:shared_group_user_ids]).to contain_exactly(shared_with_group_member.user_id)
+          expect(execute[:shared_project_user_ids]).to contain_exactly(shared_with_project_member.user_id)
         end
       end
     end

ee/spec/models/ee/group_spec.rb

@@ -1168,6 +1168,95 @@ RSpec.describe Group do
     end
   end
 
+  describe '#awaiting_user_ids' do
+    let_it_be(:group, refind: true) { create(:group) }
+    let_it_be(:awaiting_user) { create(:user) }
+    let_it_be(:project_bot) { create(:user, :project_bot) }
+    let_it_be(:project) { create(:project, group: group) }
+    let_it_be(:shared_group, refind: true) { create(:group) }
+    let_it_be(:sub_group) { create(:group, parent: group) }
+
+    subject(:awaiting_user_ids) { group.awaiting_user_ids }
+
+    context 'when awaiting user is member of the group' do
+      before do
+        create(:group_member, :awaiting, user: awaiting_user, source: group)
+        create(:group_member, :awaiting, user: project_bot, source: group)
+      end
+
+      it { is_expected.to match_array([awaiting_user.id]) }
+    end
+
+    context 'when awaiting user is member of a sub-group within the group' do
+      before do
+        create(:group_member, :awaiting, user: awaiting_user, source: sub_group)
+      end
+
+      it { is_expected.to match_array([awaiting_user.id]) }
+    end
+
+    context 'when awaiting user is member of a project in the group' do
+      before do
+        create(:project_member, :awaiting, user: awaiting_user, source: project)
+        create(:project_member, :awaiting, user: project_bot, source: project)
+      end
+
+      it { is_expected.to match_array([awaiting_user.id]) }
+    end
+
+    context 'when other group with awaiting users is member of the group' do
+      let_it_be(:invited_group) { create(:group) }
+
+      before_all do
+        create(:group_member, :awaiting, user: awaiting_user, source: invited_group)
+        create(:group_member, :awaiting, user: project_bot, source: invited_group)
+
+        create(:project_group_link, project: project, group: invited_group)
+      end
+
+      it { is_expected.to match_array([awaiting_user.id]) }
+    end
+
+    context 'when other group with awaiting users is member of a project in the group' do
+      before_all do
+        create(:group_member, :awaiting, user: awaiting_user, source: shared_group)
+        create(:group_member, :awaiting, user: project_bot, source: shared_group)
+        create(:group_group_link, { shared_with_group: shared_group,
+                                    shared_group: group })
+      end
+
+      it { is_expected.to match_array([awaiting_user.id]) }
+    end
+
+    context 'when a user is member multiple times' do
+      before do
+        create(:group_member, :awaiting, :developer, user: awaiting_user, source: group)
+        create(:project_member, :awaiting, :maintainer, user: awaiting_user, source: project)
+        create(:group_member, :awaiting, user: awaiting_user, source: shared_group)
+        create(:group_group_link, { shared_with_group: shared_group,
+                                    shared_group: group })
+      end
+
+      it { is_expected.to match_array([awaiting_user.id]) }
+    end
+
+    context 'when there are multiple awaiting users' do
+      let_it_be(:shared_group_awaiting_user) { create(:user) }
+      let_it_be(:project_awaiting_user) { create(:user) }
+
+      before do
+        create(:group_member, :awaiting, user: awaiting_user, source: group)
+        create(:group_member, :awaiting, user: shared_group_awaiting_user, source: shared_group)
+        create(:project_member, :awaiting, user: project_awaiting_user, source: project)
+
+        create(:group_group_link, { shared_with_group: shared_group,
+                                    shared_group: group })
+      end
+
+      it { is_expected.to match_array([awaiting_user.id, shared_group_awaiting_user.id, project_awaiting_user.id]) }
+    end
+  end
+
   describe '#billable_members_count', :saas do
     let_it_be(:bronze_plan) { create(:bronze_plan) }
     let_it_be(:premium_plan) { create(:premium_plan) }

ee/spec/models/member_spec.rb

@@ -492,4 +492,31 @@ RSpec.describe Member, type: :model do
       )
     end
   end
+
+  describe '.awaiting_without_invites_and_requests' do
+    let_it_be(:awaiting_group_member) { create(:group_member, :awaiting, group: group) }
+    let_it_be(:awaiting_project_member) { create(:project_member, :awaiting, project: project) }
+    let_it_be(:active_group_member) { create(:group_member, group: group) }
+    let_it_be(:invited_member) { create(:group_member, :invited, group: group) }
+    let_it_be(:invited_awaiting_member) { create(:group_member, :invited, :awaiting, group: group) }
+    let_it_be(:accepted_invite_member) { create(:group_member, :invited, group: group).accept_request }
+    let_it_be(:requested_member) { create(:group_member, :access_request, group: group) }
+    let_it_be(:requested_awaiting_member) { create(:group_member, :awaiting, :access_request, group: group) }
+    let_it_be(:accepted_request_member) { create(:group_member, :access_request, group: group).accept_request }
+    let_it_be(:blocked_member) { create(:group_member, :blocked, group: group) }
+
+    subject(:results) { described_class.awaiting_without_invites_and_requests }
+
+    it { is_expected.to include awaiting_group_member }
+    it { is_expected.to include awaiting_project_member }
+
+    it { is_expected.not_to include active_group_member }
+    it { is_expected.not_to include invited_member }
+    it { is_expected.not_to include invited_awaiting_member }
+    it { is_expected.not_to include accepted_invite_member }
+    it { is_expected.not_to include requested_member }
+    it { is_expected.not_to include requested_awaiting_member }
+    it { is_expected.not_to include accepted_request_member }
+    it { is_expected.not_to include blocked_member }
+  end
 end

ee/spec/requests/api/members_spec.rb

@@ -382,6 +382,8 @@ RSpec.describe API::Members do
       end
     end
 
+    let_it_be(:awaiting_user) { create(:group_member, :awaiting, group: group, user: create(:user)).user }
+
     describe 'GET /groups/:id/billable_members' do
       let(:url) { "/groups/#{group.id}/billable_members" }
       let(:params) { {} }
@@ -462,6 +464,16 @@ RSpec.describe API::Members do
             end
           end
         end
+
+        context 'with include_awaiting_members is true' do
+          let(:params) { { include_awaiting_members: true } }
+
+          it 'includes awaiting users' do
+            get_billable_members
+
+            expect_paginated_array_response(*[owner, maintainer, nested_user, awaiting_user, project_user, linked_group_user].map(&:id))
+          end
+        end
       end
 
       context 'with non owner' do